Puzzled about VFS sysctl OIDs -- signed vs. unsigned

[Matthew Fleming]

On Thu, Mar 3, 2011 at 2:03 PM, Brandon Gooch
<jamesbrandongooch at gmail.com> wrote:
> On Thu, Mar 3, 2011 at 3:34 PM, Matthew Fleming <mdf356 at gmail.com> wrote:
>> On Thu, Mar 3, 2011 at 1:03 PM, Brandon Gooch
>> <jamesbrandongooch at gmail.com> wrote:
>>> On Thu, Mar 3, 2011 at 11:49 AM, David Wolfskill <david at catwhisker.org> wrote:
>>>> I'm using a little shell script to capture selected sysctl OID
>>>> values periodically, in an attempt to get a better idea how the
>>>> resources of a system are being used during a long-running (usually
>>>> measured in hours), mission-critical workload.
>>>>
>>>> In the process of testing this, I've seen some of the VFS sysctl
>>>> OIDs (in particular) report negative values ... when the description
>>>> looks to me as if the OID in question is intended to be a monotonically
>>>> increasing counter.
>>>>
>>>> For example:
>>>>
>>>>> sysctl -d vfs.getnewbufcalls
>>>> vfs.getnewbufcalls: Number of calls to getnewbuf
>>>>> sysctl vfs.getnewbufcalls
>>>> vfs.getnewbufcalls: -348909432
>>>>
>>>> Examining sys/kern/vfs_bio.c, the definition of vfs.getnewbufcalls
>>>> appears to be:
>>>>
>>>> ...
>>>> static int getnewbufcalls;
>>>> SYSCTL_INT(_vfs, OID_AUTO, getnewbufcalls, CTLFLAG_RW, &getnewbufcalls, 0,
>>>>   "Number of calls to getnewbuf");
>>>> ...
>>>>
>>>> Many of the other OIDs defined nearby are also SYSCTL_INT (or
>>>> SYSCTL_LONG), vs. SYSCTL_UINT (or SYSCTL_ULONG), and the corresponding
>>>> variables are defined as static int (or static long) vs. static u_int
>>>> (or static u_long).
>>>>
>>>> Is this both correct and reasonable?  If so, how should I interpret such
>>>> negative values?
>>>>
>>>> [GSoC project, anyone?]
>>>>
>>>> Thanks!
>>>>
>>>> Peace,
>>>> david
>>>
>>> The following initiative may factor heavily into any decision to
>>> change sysctl declarations at this point:
>>>
>>> http://www.freebsd.org/news/status/report-2010-10-2010-12.html#SYSCTL-Type-Safety
>>
>> The intent of the type-safety is to make sure that the types assumed
>> for the kernel's sysctl handler match the type of the variable.  This
>> project won't fix issues where a signed type is being used and the
>> value wraps (at least, I presume that's what happened in this case?)
>>
>> Thanks,
>> matthew
>
> Yes, it's wrapping. I wonder, would an audit of the SYCTL_* types be
> of general use to FreeBSD? I haven't ran into these issues myself, but
> I can see where this could become more of a problem with the OS in
> general as larger, heavier loads are placed on general-purpose type
> systems; where FreeBSD has been used in a product, I assume that the
> companies engineers, such as those at Isilon, have applied local
> patches where necessary. Y'know, I think this could be a good GSoC
> project after all...

Yes and no. :-)

The problem with changing the types is that it can be an ABI change.
See the function sysctl_bufspace() in vfs_bio.c which stands on its
head to output an int size if that's what the caller expects.

This problem is mitigated (or made worse, depending on your POV) by a
commit I plan to make when $WORK is slightly less insane, to add a new
sysctl handler that will copy out 4 bytes if that's what the caller
expects and there's no overflow, or even perhaps if there is, even if
the kernel type is 8 bytes.  The upside of this new handler is that,
like bufspace, it preserves ABI for applications that thought they
knew the size.  The downside is that (1) there's lots of design
options, like copying out 4 bytes even if this truncates the data
versus reporting the error, and (2) such a change means that broken
applications don't know they're broken, if the FreeBSD type changes.

I hope this explains the issues.  All that said, personally I'm in
favor of having the kernel have the right types, and fixing broken
apps as they're found.  This isn't the most friendly stance for
third-party vendors, though.

Cheers,
matthew