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

On Wed, Jun 15, 2011 at 09:41:28AM +0400, Sergey Kandaurov wrote:
> 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).
This patch accomodates the userland to the changed ABI. Why it was
changed at all ? I would argue that keeping the stable ABI there is
more important then using a 'clean' type.

At least, the stable branches usermode is broken on the current kernel.