Re: [PATCH v2] Add x86 32 bit vDSO time function support

[Nathan Lynch <Nathan_Lynch@codesourcery.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.