Re: svn commit: r214611 - head/sys/kern

On 15 June 2011 06:20, David Xu <davidxu_at_freebsd.org> wrote:
> On 2011/06/14 20:02, Sergey Kandaurov wrote:
>> On 1 November 2010 03:42, David Xu <davidxu_at_freebsd.org> wrote:
>>> Author: davidxu
>>> Date: Mon Nov  1 00:42:25 2010
>>> New Revision: 214611
>>> URL: http://svn.freebsd.org/changeset/base/214611
>>>
>>> Log:
>>>  Use integer for size of cpuset, as it won't be bigger than INT_MAX,
>>>  This is requested by bge.
>>>  Also move the sysctl into file kern_cpuset.c, because it should
>>>  always be there, it is independent of thread scheduler.
>>
>> Hi.
>> This breaks for me fetching a cpusetsize value with sysconf(3) interface,
>> as after this change sysconf(3) consumers expect a long return type, while
>> sysctl kern.sched.cpusetsize has switched from long to int type in kernel.
>> That makes for me sizeof(cpusize_t) from 8 to incorrect 34359738376.
>>
>> In particular, kvm_getpcpu(3) uses sysconf(3) to fetch cpusetsize on
>> live kernel. That gives me a broken result:
>> kvm_open: kcpusetsize: 8
>> pcpu[0] = 0x801072300
>> kvm_open: kcpusetsize: 34359738376
>> pcpu[1] = 0xffffffffffffffff
>> kvm_open: kcpusetsize: 8
>> pcpu[2] = 0x801072600
>> kvm_open: kcpusetsize: 34359738376
>> pcpu[3] = 0xffffffffffffffff
>>
>> This small test indicates that that's due to int->long type conversion:
>>         long lvalue;
>>         size_t len;
>>
>>         len = sizeof(lvalue);
>>         if (sysctlbyname("kern.sched.cpusetsize", &lvalue, &len, NULL, 0) < 0)
>>                 err(1, "sysctlbyname");
>>         printf("sysctl: %ld\n", lvalue);
>>         printf("sysctl: %d -- explicitly casted to (int)\n", (int)lvalue);
>>         printf("sysconf: %ld\n", sysconf(_SC_CPUSET_SIZE));
>>         printf("sysconf: %d -- explicitly casted to (int)\n",
>> (int)sysconf(_SC_CPUSET_SIZE));
>>
>> That prints:
>> sysctl: 34359738376
>> sysctl: 8 -- explicitly casted to (int)
>> sysconf: 34359738376
>> sysconf: 8 -- explicitly casted to (int)
>>
>> The other way to solve this other than reverting is to "fix" all cpusetsize
>> consumers in userland. Now sysconf() saves long returned value to int:
>>
>> Index: lib/libkvm/kvm_pcpu.c
>> ===================================================================
>> --- lib/libkvm/kvm_pcpu.c       (revision 223073)
>> +++ lib/libkvm/kvm_pcpu.c       (working copy)
>> _at__at_ -120,7 +120,7 _at__at_
>>  void *
>>  kvm_getpcpu(kvm_t *kd, int cpu)
>>  {
>> -       long kcpusetsize;
>> +       int kcpusetsize;
>>         ssize_t nbytes;
>>         uintptr_t readptr;
>>         char *buf;
>>
>> So, after applying the above change all is ok:
>> kvm_open: kcpusetsize: 8
>> pcpu[0] = 0x801072300
>> kvm_open: kcpusetsize: 8
>> pcpu[1] = 0x801072600
>> kvm_open: kcpusetsize: 8
>> pcpu[2] = 0x801072900
>> kvm_open: kcpusetsize: 8
>> pcpu[3] = 0x801072c00
>>
>>
> Try this patch, I think it should fix it.
>
> Index: lib/libc/gen/sysconf.c
> ===================================================================
> --- lib/libc/gen/sysconf.c      (revision 221356)
> +++ lib/libc/gen/sysconf.c      (working copy)
> _at__at_ -599,11 +599,11 _at__at_
>
>  #ifdef _SC_CPUSET_SIZE
>        case _SC_CPUSET_SIZE:
> -               len = sizeof(lvalue);
> -               if (sysctlbyname("kern.sched.cpusetsize", &lvalue, &len, NULL,
> +               len = sizeof(value);
> +               if (sysctlbyname("kern.sched.cpusetsize", &value, &len, NULL,
>                    0) == -1)
>                        return (-1);
> -               return (lvalue);
> +               return ((long)(value));
>  #endif
>
>        default:
>

Great, thanks! Look good for me.
Nitpicking:
 return ((long)value); should be enough (extra parenthesis).

-- 
wbr,
pluknet