This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Add x86 32 bit vDSO time function support
- From: Nathan Lynch <Nathan_Lynch at codesourcery dot com>
- To: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Mon, 3 Nov 2014 17:51:50 -0600
- Subject: Re: [PATCH v2] Add x86 32 bit vDSO time function support
- Authentication-results: sourceware.org; auth=none
- References: <5436D48C dot 2090509 at linux dot vnet dot ibm dot com> <5436F50C dot 9070002 at codesourcery dot com> <5457E5D5 dot 6000103 at linux dot vnet dot ibm dot com>
On 11/03/2014 02:30 PM, Adhemerval Zanella wrote:
> On 09-10-2014 17:50, Nathan Lynch wrote:
>> On 10/09/2014 01:31 PM, Adhemerval Zanella wrote:
>>
>>> +#ifdef SHARED
>>> +# define SYSCALL_GETTIME(id, tp) \
>>> + ({ long int (*f) (clockid_t, struct timespec *) = __vdso_clock_gettime; \
>>> + long int v_ret; \
>>> + PTR_DEMANGLE (f); \
>>> + v_ret = (*f) (id, tp); \
>>> + if (INTERNAL_SYSCALL_ERROR_P (v_ret, )) { \
>>> + __set_errno (INTERNAL_SYSCALL_ERRNO (v_ret, )); \
>>> + v_ret = -1; \
>>> + } \
>>> + v_ret; })
>> Does introducing the dispatch through function pointer here cause a
>> measurable performance regression on i386 kernels which lack the VDSO?
>> If so, is that a concern?
>>
>> When I've tried this approach on ARM, it appears to do so (around 5%
>> slowdown).
>
> Using a simple benchmark (in attachments) the difference in such scenarios is not
> as drastic as ARM it seems:
>
> kernel: Linux birita 3.13.0-39
> CPU: Intel(R) Core(TM)2 CPU 6600 @ 2.40GHz
>
> EGLIBC 2.19-0ubuntu6.3: 1415.12 cycles
> GLIBC 2.20 master: 1421.66 cycles
OK.
>>> +# define INTERNAL_GETTIME(id, tp) \
>>> + ({ long int (*f) (clockid_t, struct timespec *) = __vdso_clock_gettime; \
>>> + PTR_DEMANGLE (f); \
>>> + (*f) (id, tp); })
>>> +#endif
>> I'm probably missing something, but I am failing to see the need for an
>> INTERNAL_GETTIME definition in
>> sysdeps/unix/sysv/linux/x86/clock_gettime.c. I know this patch is
>> merely moving existing code, but sysdeps/unix/sysv/linux/clock_gettime.c
>> does not use INTERNAL_GETTIME, and neither does
>> sysdeps/unix/clock_gettime.c.
>>
>> INTERNAL_GETTIME is needed for timespec_get, but I am not seeing the
>> need to duplicate it for clock_gettime.
>
> i386 does not define HAVE_CLOCK_GETTIME_VSYSCALL and thus:
>
> sysdeps/unix/sysv/linux/clock_gettime.c:
>
> 26: # define INTERNAL_VSYSCALL INTERNAL_SYSCALL
>
> and then if INTERNAL_GETTIME is not defined, it will as:
>
> 37 #ifndef INTERNAL_GETTIME
> 38 # define INTERNAL_GETTIME(id, tp) \
> 39 INTERNAL_VSYSCALL (clock_gettime, err, 2, id, tp)
> 40 #endif
>
> And without proper set the PTR_DEMANGLE is not called either.
I think my point might be better expressed as: why is INTERNAL_GETTIME
defined in linux/clock_gettime.c at all? It is not used in the
clock_gettime implementation.
The only use in glibc of a macro by that name is in
linux/timespec_get.c, and the arch can override it in its own
timespec_get.c (as x86_64 currently does).
> With PREPARE_VERSION_KNOWN fixes, is it ok to commit?
I have no objection, FWIW.