Re: [Patch] Use VDSO interface for gettimeofday on aarch64

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

On 16/05/18 00:07, Steve Ellcey wrote:
> Here is an updated version of the gettimeofday vdso patch.  I tried to
> address as many of the questions as I could.  I added two tests,
> tst-gettimeofday and tst-gettimeofday2.  tst-gettimeofday2 just
> includes tst-gettimeofday but is tested with 'LD_BIND_NOW=1'.
> 
> I think defining __gettimeofday to be __redirect___gettimeofday and
> including sys/time.h is the best way to handle things.  Even
> if we did a explicit definition of __redirect__gettimeofday we
> would need to include sys/time.h to get the timeval and timezone
> structures and if we didn't redefine __gettimeofday when doing
> that the definition of __gettimeofday in the header file would
> conflict with the one defined by libc_ifunc_hidden.  This is how
> x86 and powerpc handle it in their gettimeofday functions and also
> how aarch64 handles the definition/redefinition of memcpy in
> sysdeps/aarch64/multiarch/memcpy.c.
> 
> I am not sure how to handle the static linked binary issue you raised,
> I have been copying what x86/powerpc did and if they or some other
> platform has solved this then I would be interested in how to
> do it.  Since PR 19767 is still open I assume it hasn't been fixed
> anywhere yet and I would rather not try to deal with it in this
> patch.
> 
> I changed __gettimeofday_syscall to __gettimeofday_vsyscall and
> I got rid of the redefinition of libc_hidden_def by just calling
> __hidden_ver1 directly (in the shared case).
> 

This is OK.

> There do seem to be places where libc calls gettimeofday (nis/nis_call.c,
> login/logwtmp.c, resolv/gai_suspend.c, others).  Most of them call
> __gettimeofday but some just call gettimeofday.  I am not sure what
> if anything needs to be done with these calls, they don't seem to
> have changed when x86 or powerpc made their gettimeofday/vdso changes..
> 

What i wanted to know/document is that internal libc.so
calls don't go via the ifunc resolver, but call the
vsyscall and this is the only reason why it should remain
a vsyscall instead of a syscall as far as i can see
(otherwise if ifunc already checked the vdso then there
would be no point doing that in vsyscall too)

The other thing that would be nice to document is that
why this change is safe for gettimeofday but not clock_gettime.
(former does not have to set errno other than EFAULT but that
case never works with vdso anyway, so the gettimeofday vdso
function is a complete implementation, while clock_gettime
has to deal with errno after the vdso call)

> Steve Ellcey
> sellcey@cavium.com
> 
> 
> This is what I would use for a commit message:
> 
> 
> aarch64: Use an ifunc/VDSO to implement gettimeofday in shared glibc.
> 
> This patch uses an ifunc to implement gettimeofday in the shared libc.

please add here something like

"This is faster compared to the vsyscall mechanism that has to
check a global pointer, demangle it and call it indirectly when
the VDSO is present. Resolving the gettimeofday symbol directly
to the VDSO code is safe because there are no failures that the
libc has to handle by setting errno like in a generic vsyscall."

> If the kernel supports the VDSO interface we use it, otherwise we use
> the old method of a vsyscall.  The static version of gettimeofday
> continues to use a syscall.
> 
> 
> 2018-05-15  Steve Ellcey<sellcey@caviumnetworks.com>
> 
> 	* sysdeps/unix/sysv/linux/aarch64/gettimeofday.c: New file.
> 	* posix/tst-gettimeofday.c: New file.
> 	* posix/tst-gettimeofday2.c: New file.
> 	* posix/Makefile (tests): Add new tests to list.
> 

> --- a/posix/tst-gettimeofday.c
> +++ b/posix/tst-gettimeofday.c
...
> +
> +#include <errno.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/time.h>
> +#include <time.h>
> +
> +
> +/* Test that nanosleep() does sleep.  */

this seems to be from the nanosleep test,
i think that can be reused instead of copied, just
add a comment that it is used for gettimeofday testing
too and in the testcase that is run with BIND_NOW
add a comment that gettimeofday may be ifunc resolved
on targets with VDSO.

(a better test is probably checking if &gettimeofday
is indeed in vdso and not in libc.so, although that is
tricky: it may point to the plt in the main executable,
so the test has to be a shared lib, then bindnow is not
needed, but the test would be target specific)

> +static int
> +do_test (void)
> +{
> +  /* Current time.  */
> +  struct timeval tv1;
> +  TEMP_FAILURE_RETRY (gettimeofday (&tv1, NULL));
> +
> +  /* Sleep for one second to make sure time changes.  */
> +  TEMP_FAILURE_RETRY (sleep (1));
> +
...

> +#ifdef SHARED
> +
> +# define __gettimeofday __redirect___gettimeofday
> +# include <sys/time.h>
> +# undef __gettimeofday
> +# define HAVE_VSYSCALL
> +# include <dl-vdso.h>
> +# include <sysdep-vdso.h>
> +

i'd add a comment here:

/* Used as a fallback in the ifunc resolver if VDSO is not available
    and for libc.so internal __gettimeofday calls.  */

> +static int
> +__gettimeofday_vsyscall (struct timeval *tv, struct timezone *tz)
> +{
> +  return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);
> +}
> +
> +/* PREPARE_VERSION will need an __LP64__ ifdef when ILP32 support
> +   goes in.  See _libc_vdso_platform_setup in
> +   sysdeps/unix/sysv/linux/aarch64/init-first.c.  */
> +
> +# undef INIT_ARCH
> +# define INIT_ARCH() \
> +	   PREPARE_VERSION (linux_version, "LINUX_2.6.39", 123718537); \
> +	   void *vdso_gettimeofday = \
> +	     _dl_vdso_vsym ("__kernel_gettimeofday", &linux_version);
> +
> +libc_ifunc_hidden (__redirect___gettimeofday, __gettimeofday,
> +                    vdso_gettimeofday ?: (void *) __gettimeofday_vsyscall)
> +
> +__hidden_ver1 (__gettimeofday_vsyscall, __GI___gettimeofday,
> +	       __gettimeofday_vsyscall);
> +
> +#else
> +
> +# include <sys/time.h>
> +# include <sysdep.h>
> +int
> +__gettimeofday (struct timeval *tv, struct timezone *tz)
> +{
> +  return INLINE_SYSCALL (gettimeofday, 2, tv, tz);
> +}
> +libc_hidden_def (__gettimeofday)
> +
> +#endif
> +
> +weak_alias (__gettimeofday, gettimeofday)
> +libc_hidden_weak (gettimeofday)
>