Date: Sat, 30 Oct 2010 06:58:01 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Pawel Jakub Dawidek <pjd@freebsd.org>, David Xu <davidxu@freebsd.org> Subject: Re: svn commit: r214510 - in head: include lib/libc/gen sys/kern Message-ID: <AANLkTikEVjbLOP15ub=Lh2mDAP88gniM8MO%2B%2Btk5CUZ0@mail.gmail.com> In-Reply-To: <20101030203424.M1007@besplex.bde.org> References: <201010291331.o9TDVAtm027022@svn.freebsd.org> <20101029222159.GA2160@garage.freebsd.pl> <20101030203424.M1007@besplex.bde.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
On Sat, Oct 30, 2010 at 3:08 AM, Bruce Evans <brde@optusnet.com.au> wrote:
> On Sat, 30 Oct 2010, Pawel Jakub Dawidek wrote:
>
>> On Fri, Oct 29, 2010 at 01:31:10PM +0000, David Xu wrote:
>>>
>>> Log:
>>> Add sysctl kern.sched.cpusetsize to export the size of kernel cpuset,
>>> also add sysconf() key _SC_CPUSET_SIZE to get sysctl value.
>>>
>>> Submitted by: gcooper
>>
>> [...]
>>>
>>> +#ifdef _SC_CPUSET_SIZE
>>> + case _SC_CPUSET_SIZE:
>>> + len = sizeof(lvalue);
>>> + if (sysctlbyname("kern.sched.cpusetsize", &lvalue, &len,
>>> NULL,
>>> + 0) == -1)
>>> + return (-1);
>>> + return (lvalue);
>>> +#endif
>>
>> [...]
>>>
>>> +static size_t _kern_cpuset_size = sizeof(cpuset_t);
>
> No need for this or its bogus type, since it is a small compile-time
> constant value.
>
>> [...]
>>>
>>> +/*
>>> + * Return the size of cpuset_t at the kernel level
>>> + *
>>> + * XXX (gcooper): replace ULONG with SIZE once CTLTYPE_SIZE is
>>> implemented.
>>> + */
>>> +SYSCTL_ULONG(_kern_sched, OID_AUTO, cpusetsize, CTLFLAG_RD,
>>> + &_kern_cpuset_size, 0, "Kernel-level cpuset_t struct size");
>>> +
>
> Just use:
>
> SYSCTL_INT(_kern_sched, OID_AUTO, cpusetsize, CTLFLAG_RD,
> 0, sizeof(cpuset_t), "sizeof(cpuset_t) in the kernel");
>
> the same as for all debugging sizeofs in kern_mib.c. (I also changed the
> style of the message to be more like the ones there. (sysctl -ad | greo
> sizeof on ref9-i386 shows only 1 other inconsistency: the banal description
> is missing for debug.sizeof.namecache.))
Yeah... it was silly of me to do it that way, in hindsight :(...
>> Because it is used via sysconf(3), I don't think it should be converted
>> to CTLTYPE_SIZE at all. I even think it would be safer to make
>> _kern_cpuset_size a long (sysconf's lvalue is long) and (just for
>> consistency) use SYSCTL_LONG().
>>
>> Also note, that on i386 long is 32bit and on amd64 long is 64bit, so
>> 32bit process running on 64bit system won't be able to read this sysctl.
>> Or do we detect 32bit processes on 64bit systems and convert such types
>> in the kernel?
>
> Just use int. 16-bit ints are only large enough for 8*32767 CPUs, but
> 32-bit ints are large enough for 8*2147482647 CPUs, which should be
> enough for anyone. Also use 'int value' instead of 'long lvalue' in
> sysconf.c.
That won't really do much good, otherwise it would oppose the POSIX
API definition :/...
> Using u_long is also bogus. Because the value is returned by sysconf(3),
> u_long won't actually work if it is actually needed, because if the
> value exceeds LONG_MAX then `return (lvalue)' will blindly overflow.
> But the value is very far from exeeding even INT_MAX, so there is no
> problem. (Overflow would actually occur earlier, via the type pun of
> using sysctl for a u_long variable to read the variable into a plain
> long. It is unclear that this type pun is safe even when there is no
> overflow.)
>
> The kernel isn't going to be having any data structures larger than
> 2147482647 bytes any time soon, so 32-bit ints are plenty large enough
> for returning the size of any data structure in the kernel. However,
> 16-bit ints wouldn't be. Careful code might use size_t to avoid
> assuming anything about sizeof(int), but with 32-bit ints this mainly
> gives bloat when size_t is 64 bits.
Ok.
Thanks!
-Garrett
[-- Attachment #2 --]
Index: sys/kern/sched_ule.c
===================================================================
--- sys/kern/sched_ule.c (revision 214554)
+++ sys/kern/sched_ule.c (working copy)
@@ -2712,8 +2712,6 @@
sbuf_delete(topo);
return (err);
}
-
-static size_t _kern_cpuset_size = sizeof(cpuset_t);
#endif
SYSCTL_NODE(_kern, OID_AUTO, sched, CTLFLAG_RW, 0, "Scheduler");
@@ -2751,13 +2749,9 @@
CTLFLAG_RD, NULL, 0, sysctl_kern_sched_topology_spec, "A",
"XML dump of detected CPU topology");
-/*
- * Return the size of cpuset_t at the kernel level
- *
- * XXX (gcooper): replace ULONG with SIZE once CTLTYPE_SIZE is implemented.
- */
-SYSCTL_ULONG(_kern_sched, OID_AUTO, cpusetsize, CTLFLAG_RD,
- &_kern_cpuset_size, 0, "Kernel-level cpuset_t struct size");
+/* Return the size of cpuset_t at the kernel level */
+SYSCTL_INT(_kern_sched, OID_AUTO, cpusetsize, CTLFLAG_RD,
+ 0, sizeof(cpuset_t), "sizeof(cpuset_t)");
#endif
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikEVjbLOP15ub=Lh2mDAP88gniM8MO%2B%2Btk5CUZ0>