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

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

On 10/05/18 19:00, Steve Ellcey wrote:
> 
> This is a Aarch64 version of gettimeofday that uses the VDSO interface
> when it is available.  I did a test with 100000000 gettimeofday calls
> on a T88 and the time went from 7.1 seconds to 5.5 seconds.   I also
> ran the glibc testsuite and I did not get any regressions.
> 
> OK to checkin?
> 
> Steve Ellcey
> sellcey@cavium.com
> 
> 
> 2018-05-10  Steve Ellcey  <sellcey@caviumnetworks.com>
> 
> 	* sysdeps/unix/sysv/linux/aarch64/gettimeofday.c: New file.

thanks, it looks reasonable approach, but the commit
message should be fixed to indicate that this is a new
VDSO mechanism (using ifunc) and why the old mechanism
is still needed.

please test with LD_BIND_NOW=1 too (this applies whenever
ifuncs are involved, since they may behave differently
when resolved lazily vs at load time and i don't see
such test in glibc currently, a simple helloworld.c with
gettimeofday usage is enough i think, it would be even
better to add something like that to the test system)

> +
> +#ifdef SHARED
> +

note that static linked binaries do a real syscall now,
this should be solved since users who really care about
performance want to use static linked binaries, this is
https://sourceware.org/bugzilla/show_bug.cgi?id=19767

(i think if !SHARED then global __vdso pointers can be
initialized while the process is still single threaded
using custom elf symbol lookup code and then current
VSYSCALL mechanism should work)

> +# define __gettimeofday __redirect___gettimeofday
> +# include <sys/time.h>
> +# undef __gettimeofday

is this necessary?
can we write out the declarations here?
such macro redirection looks fragile to me.

> +# define HAVE_VSYSCALL
> +# include <dl-vdso.h>
> +# include <sysdep-vdso.h>
> +
> +static int
> +__gettimeofday_syscall (struct timeval *tv, struct timezone *tz)
> +{
> +  return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);
> +}
> +

i'd call it __gettimeofday_vsyscall if you use VSYSCALL.

is there a way _dl_vdso_vsym fails in the ifunc resolver
but succeeds in VDSO_SETUP during _init?

are there cases when __gettimeofday_syscall is called directly
instead of via ifunc dispatch? (e.g. libc internal calls)

vdso mechanisms are getting confusing, adding new mechanism is ok,
but then either old ones should be cleaned up or comments added
there clarifying which mechanism is used when (so the questions
above are easy to answer).

> +/* 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_syscall)
> +

this may do a vdso symbol look up whenever a dso is loaded
that references gettimeofday (or when it's called in case of
lazy binding) we could do the lookup only once at early init
and use that in the ifunc resolver, but currently VDSO_SETUP
runs after libc.so is relocated so i don't have a better idea.

note that clock_gettime could use the same mechanism on aarch64
if we introduced a new abi symbol: __clock_gettime_noerrno
and the public time.h had something like

#define clock_gettime(id,ts) \
   ( __id <= 6U \
    ? __clock_gettime_noerrno (__id, __ts) \
    : clock_gettime (__id, __ts) )

there might be better ways, not sure if glibc is happy with
such hacks in public headers, but it's worth considering if
you see significant performance difference.

> +# undef libc_hidden_def
> +# define libc_hidden_def(name)                               \
> +  __hidden_ver1 (__gettimeofday_syscall, __GI___gettimeofday,  \
> +               __gettimeofday_syscall);

i'd use a new macro with different name here, e.g.

#define hidden_vsyscall(name) __hidden_ver1 (name##_syscall,...)

(or just write out explicitly what you want for SHARED vs !SHARED
case separately.)

does this mean internally in libc.so gettimeofday uses the
existing VSYSCALL mechanism, but e.g. another dso like
libpthread.so goes via ifunc?

> +
> +#else
> +
> +# include <sys/time.h>
> +# include <sysdep.h>
> +int
> +__gettimeofday (struct timeval *tv, struct timezone *tz)
> +{
> +  return INLINE_SYSCALL (gettimeofday, 2, tv, tz);
> +}
> +#endif
> +
> +libc_hidden_def (__gettimeofday)
> +weak_alias (__gettimeofday, gettimeofday)
> +libc_hidden_weak (gettimeofday)
>