Date: Wed, 24 Jan 2007 05:10:50 -0800 From: Luigi Rizzo <rizzo@icir.org> To: Randall Stewart <rrs@cisco.com> Cc: freebsd-net <freebsd-net@freebsd.org> Subject: Re: mbuf patch with sysctl suggestions too Message-ID: <20070124051050.A56064@xorpc.icir.org> In-Reply-To: <45B679F3.3080407@cisco.com>; from rrs@cisco.com on Tue, Jan 23, 2007 at 04:11:15PM -0500 References: <45B679F3.3080407@cisco.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jan 23, 2007 at 04:11:15PM -0500, Randall Stewart wrote:
> Hi all:
>
> Here is iteration 2 of the mbuf patch with limits I
> proposed.
>
> Also note the changes for sysctl stuff that Lugi suggested.
> Please let me know what you think :-)
...
> + newnmbjclusters = nmbjumbop;
> + error = sysctl_int_checked(oidp, &newnmbjclusters, nmbjumbop,
> + SYSCTL_NO_LIMIT, req);
A few things here:
- i don't see much of a point in defining SYSCTL_NO_LIMIT;
UINT32_MAX would do perfectly there, and i think it is easier
to understand than SYSCTL_NO_LIMIT (which looks like a flag).
- here and in other places you do not allow decresaing the value
(by putting min = nmbjumbop etc.), and i am not sure why.
I understand a reasonable lower bound, but i guess the worst
that can happen, when you reduce the limit to something above
the current allocation, is that nothing is allocated until
you go again below the limit, right ?
- given that you don't use the new value if error != 0, perhaps
you can save the assignment newnmbjclusters = nmbjumbop;
And below:
> +/*
> + * Handle an int, unsigned, but limited
> + * between min and max (unsigned)
> + * Two cases:
> + * a variable: point arg1 at it.
> + * a constant: pass it in arg2.
> + *
> + */
> +
> +extern int nmbjumbo9;
> +
> +int
> +sysctl_int_checked(struct sysctl_oid *oidp, void *val, uint32_t min, uint32_t max, struct sysctl_req *req)
> +{
the comment refers to something else and should be fixed e.g.
Handle an unsigned int variable with bound checking.
Returns 0 (and updates *val) if min <= v <= max;
returns EINVAL otherwhise (in which case *val is unchanged)
cheers
luigi
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070124051050.A56064>