Date: Wed, 15 Jun 2011 09:41:28 +0400 From: Sergey Kandaurov <pluknet@freebsd.org> To: David Xu <davidxu@freebsd.org> Cc: Attilio Rao <attilio@freebsd.org>, FreeBSD Current <freebsd-current@freebsd.org>, Bruce Evans <bde@freebsd.org> Subject: Re: svn commit: r214611 - head/sys/kern Message-ID: <BANLkTim_4HYekFgBWXp75qDoVOf7dnjEyw@mail.gmail.com> In-Reply-To: <4DF816E4.1080003@freebsd.org> References: <201011010042.oA10gPeg016326@svn.freebsd.org> <BANLkTi=TL4Yz5oR88qqorBvouWm0aYYmiQ@mail.gmail.com> <4DF816E4.1080003@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 15 June 2011 06:20, David Xu <davidxu@freebsd.org> wrote: > On 2011/06/14 20:02, Sergey Kandaurov wrote: >> On 1 November 2010 03:42, David Xu <davidxu@freebsd.org> wrote: >>> Author: davidxu >>> Date: Mon Nov =A01 00:42:25 2010 >>> New Revision: 214611 >>> URL: http://svn.freebsd.org/changeset/base/214611 >>> >>> Log: >>> =A0Use integer for size of cpuset, as it won't be bigger than INT_MAX, >>> =A0This is requested by bge. >>> =A0Also move the sysctl into file kern_cpuset.c, because it should >>> =A0always 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, whi= le >> sysctl kern.sched.cpusetsize has switched from long to int type in kerne= l. >> 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] =3D 0x801072300 >> kvm_open: kcpusetsize: 34359738376 >> pcpu[1] =3D 0xffffffffffffffff >> kvm_open: kcpusetsize: 8 >> pcpu[2] =3D 0x801072600 >> kvm_open: kcpusetsize: 34359738376 >> pcpu[3] =3D 0xffffffffffffffff >> >> This small test indicates that that's due to int->long type conversion: >> =A0 =A0 =A0 =A0 long lvalue; >> =A0 =A0 =A0 =A0 size_t len; >> >> =A0 =A0 =A0 =A0 len =3D sizeof(lvalue); >> =A0 =A0 =A0 =A0 if (sysctlbyname("kern.sched.cpusetsize", &lvalue, &len,= NULL, 0) < 0) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 err(1, "sysctlbyname"); >> =A0 =A0 =A0 =A0 printf("sysctl: %ld\n", lvalue); >> =A0 =A0 =A0 =A0 printf("sysctl: %d -- explicitly casted to (int)\n", (in= t)lvalue); >> =A0 =A0 =A0 =A0 printf("sysconf: %ld\n", sysconf(_SC_CPUSET_SIZE)); >> =A0 =A0 =A0 =A0 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 cpusets= ize >> consumers in userland. Now sysconf() saves long returned value to int: >> >> Index: lib/libkvm/kvm_pcpu.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- lib/libkvm/kvm_pcpu.c =A0 =A0 =A0 (revision 223073) >> +++ lib/libkvm/kvm_pcpu.c =A0 =A0 =A0 (working copy) >> @@ -120,7 +120,7 @@ >> =A0void * >> =A0kvm_getpcpu(kvm_t *kd, int cpu) >> =A0{ >> - =A0 =A0 =A0 long kcpusetsize; >> + =A0 =A0 =A0 int kcpusetsize; >> =A0 =A0 =A0 =A0 ssize_t nbytes; >> =A0 =A0 =A0 =A0 uintptr_t readptr; >> =A0 =A0 =A0 =A0 char *buf; >> >> So, after applying the above change all is ok: >> kvm_open: kcpusetsize: 8 >> pcpu[0] =3D 0x801072300 >> kvm_open: kcpusetsize: 8 >> pcpu[1] =3D 0x801072600 >> kvm_open: kcpusetsize: 8 >> pcpu[2] =3D 0x801072900 >> kvm_open: kcpusetsize: 8 >> pcpu[3] =3D 0x801072c00 >> >> > Try this patch, I think it should fix it. > > Index: lib/libc/gen/sysconf.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- lib/libc/gen/sysconf.c =A0 =A0 =A0(revision 221356) > +++ lib/libc/gen/sysconf.c =A0 =A0 =A0(working copy) > @@ -599,11 +599,11 @@ > > =A0#ifdef _SC_CPUSET_SIZE > =A0 =A0 =A0 =A0case _SC_CPUSET_SIZE: > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 len =3D sizeof(lvalue); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (sysctlbyname("kern.sched.cpusetsize", &= lvalue, &len, NULL, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 len =3D sizeof(value); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (sysctlbyname("kern.sched.cpusetsize", &= value, &len, NULL, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A00) =3D=3D -1) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (-1); > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return (lvalue); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ((long)(value)); > =A0#endif > > =A0 =A0 =A0 =A0default: > Great, thanks! Look good for me. Nitpicking: return ((long)value); should be enough (extra parenthesis). --=20 wbr, pluknet
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTim_4HYekFgBWXp75qDoVOf7dnjEyw>