Date: Wed, 24 Jan 2007 08:46:02 -0500 From: Randall Stewart <rrs@cisco.com> To: Luigi Rizzo <rizzo@icir.org> Cc: freebsd-net <freebsd-net@freebsd.org> Subject: Re: mbuf patch with sysctl suggestions too Message-ID: <45B7631A.3070001@cisco.com> In-Reply-To: <20070124051050.A56064@xorpc.icir.org> References: <45B679F3.3080407@cisco.com> <20070124051050.A56064@xorpc.icir.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Luigi Rizzo wrote:
> 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).
>
ok
> - 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 ?
Well.. no I believe someone (was in Lin) mentioned that
you can get a live-lock if you allow a reduction.. and
thus the mbuf clusters were NOT allowed to be reduced..
>
> - given that you don't use the new value if error != 0, perhaps
> you can save the assignment newnmbjclusters = nmbjumbop;
>
ok
> 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)
>
Cool.. but as I said, Andre wants me to wait on this.. so
I will :-)
R
--
Randall Stewart
NSSTG - Cisco Systems Inc.
803-345-0369 <or> 803-317-4952 (cell)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?45B7631A.3070001>