Date: Fri, 29 Jun 2012 07:11:16 +0800 From: David Xu <listlog2011@gmail.com> To: Kostik Belousov <kostikbel@gmail.com> Cc: Attilio Rao <attilio@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, davidxu@freebsd.org Subject: Re: svn commit: r237660 - head/lib/libc/gen Message-ID: <4FECE494.5020005@gmail.com> In-Reply-To: <CAFKVNxBM-Gqj7xwPsjz6LbN0j9MuB6fFPjSwimGipd61WKYDcA@mail.gmail.com> References: <201206272032.q5RKWjvt031174@svn.freebsd.org> <4FEBB8C9.8070006@gmail.com> <CAJ-FndBdpP4hFPyqHaU6GucXeGi3WT4d%2Bpu-9hCNEpAPZAZQgg@mail.gmail.com> <4FEBC0A2.3010708@gmail.com> <CAJ-FndD9Hc_KvHe8vC3usVDZh7MYb51gtXsHgmins8dBC3mu_w@mail.gmail.com> <4FEBC70F.40408@gmail.com> <20120628075301.GS2337@deviant.kiev.zoral.com.ua> <4FEC1AAB.6070408@gmail.com> <4FEC557F.4080807@gmail.com> <CAFKVNxBM-Gqj7xwPsjz6LbN0j9MuB6fFPjSwimGipd61WKYDcA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2012/06/28 21:52, Kostik Belousov wrote: > On Thu, Jun 28, 2012 at 4:00 PM, David Xu <listlog2011@gmail.com> wrote: >> On 2012/6/28 16:49, David Xu wrote: >>> On 2012/6/28 15:53, Konstantin Belousov wrote: >>>> On Thu, Jun 28, 2012 at 10:53:03AM +0800, David Xu wrote: >>>>> On 2012/6/28 10:32, Attilio Rao wrote: >>>>>> 2012/6/28, David Xu<listlog2011@gmail.com>: >>>>>>> On 2012/6/28 10:21, Attilio Rao wrote: >>>>>>>> 2012/6/28, David Xu<listlog2011@gmail.com>: >>>>>>>>> On 2012/6/28 4:32, Konstantin Belousov wrote: >>>>>>>>>> Author: kib >>>>>>>>>> Date: Wed Jun 27 20:32:45 2012 >>>>>>>>>> New Revision: 237660 >>>>>>>>>> URL: http://svn.freebsd.org/changeset/base/237660 >>>>>>>>>> >>>>>>>>>> Log: >>>>>>>>>> Optimize the handling of SC_NPROCESSORS_CONF, by using auxv >>>>>>>>>> AT_NCPU >>>>>>>>>> value if present. >>>>>>>>>> >>>>>>>>>> MFC after: 1 week >>>>>>>>>> >>>>>>>>>> Modified: >>>>>>>>>> head/lib/libc/gen/sysconf.c >>>>>>>>>> >>>>>>>>>> Modified: head/lib/libc/gen/sysconf.c >>>>>>>>>> >>>>>>>>>> ============================================================================== >>>>>>>>>> --- head/lib/libc/gen/sysconf.c Wed Jun 27 20:24:25 2012 >>>>>>>>>> (r237659) >>>>>>>>>> +++ head/lib/libc/gen/sysconf.c Wed Jun 27 20:32:45 2012 >>>>>>>>>> (r237660) >>>>>>>>>> @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$"); >>>>>>>>>> #include<sys/resource.h> >>>>>>>>>> #include<sys/socket.h> >>>>>>>>>> >>>>>>>>>> +#include<elf.h> >>>>>>>>>> #include<errno.h> >>>>>>>>>> #include<limits.h> >>>>>>>>>> #include<paths.h> >>>>>>>>>> @@ -51,6 +52,7 @@ __FBSDID("$FreeBSD$"); >>>>>>>>>> >>>>>>>>>> #include "../stdlib/atexit.h" >>>>>>>>>> #include "tzfile.h" /* from >>>>>>>>>> ../../../contrib/tzcode/stdtime */ >>>>>>>>>> +#include "libc_private.h" >>>>>>>>>> >>>>>>>>>> #define _PATH_ZONEINFO TZDIR /* from tzfile.h */ >>>>>>>>>> >>>>>>>>>> @@ -585,6 +587,8 @@ yesno: >>>>>>>>>> >>>>>>>>>> case _SC_NPROCESSORS_CONF: >>>>>>>>>> case _SC_NPROCESSORS_ONLN: >>>>>>>>>> + if (_elf_aux_info(AT_NCPUS,&value, sizeof(value)) == 0) >>>>>>>>>> + return ((long)value); >>>>>>>>>> mib[0] = CTL_HW; >>>>>>>>>> mib[1] = HW_NCPU; >>>>>>>>>> break; >>>>>>>>>> >>>>>>>>> Will this make controlling the number of CPU online or CPU hotplug >>>>>>>>> be impossible on FreeBSD ? >>>>>>>> If I think about hotplug CPUs I can think of other 1000 >>>>>>>> problems/races/bad situations to be fixed before this one, really. >>>>>>> These are problems only in kernel, but kib's change is about ABI >>>>>>> between userland and kernel, I hope we don't introduce an ABI which >>>>>>> is not extendable road stone. >>>>>> I'm not entirely sure I see the ABI breakage here. >>>>> It is not breakage, it is the ABI thinks number of online cpu is fixed, >>>>> obviously, it is not the case in future unless FreeBSD won't support >>>>> dynamic number of online cpus. >>>>> >>>>> >>>>>> If the AT_NCPUS >>>>>> becames unconvenient and not correct at some point we can just fix >>>>>> sysconf() to not look into the aux vector anymoe. >>>>> If you already know this will be a problem, why do you introduce it >>>>> and later need to fix it ? >>>>> >>>>>> Please note that >>>>>> AT_NCPUS is already exported nowadays. I think this is instead a >>>>>> clever optimization to avoid the sysctl() (usual way to retrieve the >>>>>> number of CPUs). >>>>> But why don't you cache it in libc ? following code is enough: >>>>> >>>>> static int online_cpu; >>>>> if (online_cpu == 0) >>>>> online_cpu = sysctl >>>>> return online_cpu; >>>>> >>>> Thread did evolved somewhat while I was AFK. >>>> >>>> First, please note that the ABI which I designed there is fixable: >>>> if kernel does not export AT_NCPUS at all, then auxv correctly handles >>>> the situation returning an error, and libc falls back to sysctl(2). >>> >>> Do we really want to bypass sysctl and instead passing all info via auxv >>> vector ? >>> I found the sysconf() is a bunch of switch-case, which is already slow, >>> before >>> _SC_NPROCESSES_ONLN, it has already a quite number of case branches, >>> and in your code, it calls _elf_aux_info() which also has some >>> switch-cases branch, >>> if you cache smp_cpus in libc, the call for _elf_aux_info is not needed, >>> and you >>> don't need code in kernel to passing it either, in any case, the code to >>> call >>> sysctl is still needed, so why don't we just use sysctl instead and cache >>> the result in libc ? this at least can generate small code and a bit >>> faster after >>> first call to sysconf(_SC_NPROCESSES_ONLN). >> >> And as a side note, I think we should not put non-critical code into >> fork/exec >> path, these two functions are rather critical path for any UNIX like >> system, >> anything slowing down these two functions will affect overall performance, >> so we should not waste cpu cycle trying to push data to user mode via auxv >> or other ways and yet the data is not used by user code in most time, >> such as the number of online cpu. And in rtld-elf or libc, we should not >> waste >> too much time before executing main(). > My motivation for extending auxv vector and to develop auxv.c was > exactly to shave around dozen > of syscalls from the application startup sequence. If you look at the > ktrace output of the binary startup > on RELENG_8 libc, you should note exactly the sysctls to request > ncpus, pagesizes, canary and so on. > In RELENG_9 and HEAD, there are no sysctls in the trace, because the > data is already pre-filled by > kernel for libc consumption. > The sysconf(3) commit you are commenting on was caused by jemalloc(3) > initialization starting using > _sysconf(3) to query ncpu (for older version, which used sysctl > directly, I direct auxv call). So HEAD > has temporary +1 sysctl syscall done on app startup, now it should be > back to zero. This is a bug of jemalloc, in case __isthreaded is zero, it should not call sysctl to get number of cpu. I think it has already bypassed pthread_mutex_lock/unlock if __isthreaded is zero. > > In other words, 'we should not put non-critical code into fork/exec > path' exactly contradicts with > proposal to remove auxv, since exec would need to call sysctls to > fetch the same data. >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FECE494.5020005>