svn commit: r237660 - head/lib/libc/gen

Thu Jun 28 23:11:25 UTC 2012

On 2012/06/28 21:52, Kostik Belousov wrote:
> On Thu, Jun 28, 2012 at 4:00 PM, David Xu <listlog2011 at 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 at gmail.com>:
>>>>>>> On 2012/6/28 10:21, Attilio Rao wrote:
>>>>>>>> 2012/6/28, David Xu<listlog2011 at 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.
>