Use VDSO interface for gettimeofday on aarch64

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.

Hi,

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.

Yay!

[...]
> --- a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
> @@ -0,0 +1,69 @@
[...]
> +/* 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); \

Can this be added as a PREPARE_VERSION_KNOWN?

> +	   void *vdso_gettimeofday = \
> +	     _dl_vdso_vsym ("__kernel_gettimeofday", &linux_version);

I was surprised to see LINUX_2.6.39 here, but that's really what the
symbol version exported from Linux is called.  It was actually
introduced in v3.7-rc1~178^2~15.  I then suspected it was for
consistency with arm, but arm uses LINUX_2.6.  Maybe that's just how
long it took for the patch to be merged?

Not related to your patch, just an issue I was curious about on the
way.

The rest looks good.

With or without the PREPARE_VERSION_KNOWN change mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

On 05/10/2018 11:30 PM, 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?

Can you please add a benchmark in benchtests that measures this?  I 
think you could walk the ifuncs and compare with the fallback similar to 
what we do with string functions.

Also, do you see an impact of this on any standard benchmarks such as 
phoronix/openbenchmarking or SPEC?

Thanks,
Siddhesh

> Steve Ellcey
> sellcey@cavium.com
> 
> 
> 2018-05-10  Steve Ellcey  <sellcey@caviumnetworks.com>
> 
> 	* sysdeps/unix/sysv/linux/aarch64/gettimeofday.c: New file.
> 
> 
> diff --git a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
> index e69de29..973b6ff 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
> @@ -0,0 +1,69 @@
> +/* Copyright (C) 2018 Free Software Foundation, Inc.
> +
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Get the current time of day and timezone information,
> +   putting it into *tv and *tz.  If tz is null, *tz is not filled.
> +   Returns 0 on success, -1 on errors.  */
> +
> +#ifdef SHARED
> +
> +# define __gettimeofday __redirect___gettimeofday
> +# include <sys/time.h>
> +# undef __gettimeofday
> +# 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);
> +}
> +
> +/* 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)
> +
> +# undef libc_hidden_def
> +# define libc_hidden_def(name)                               \
> +  __hidden_ver1 (__gettimeofday_syscall, __GI___gettimeofday,  \
> +               __gettimeofday_syscall);
> +
> +#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)
>

On Thu, May 10, 2018 at 9:05 PM, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
> On 05/10/2018 11:30 PM, 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?
>
>
> Can you please add a benchmark in benchtests that measures this?  I think
> you could walk the ifuncs and compare with the fallback similar to what we
> do with string functions.

That does not measure the difference here.  We were already using the
VDSO before; just differently.  Before we had a PLT that calls into
the libc and would do then an check on the VDSO and then (another)
indirect call (to the VDSO).  After we just have an PLT (indirect)
call into the VDSO directly.  Basically saving the check and an extra
indirect call (the indirect call can be costly if not predicted
correctly).

Walking the ifuncs won't benchmark the difference that is gotten here.

>
> Also, do you see an impact of this on any standard benchmarks such as
> phoronix/openbenchmarking or SPEC?

You will see the biggest benefit from Apache Bench and running Apache
as the web server (maybe any other webserver which calls gettimeofday
a lot as you are producing a date for each socket connection).   And
yes this is considered a standard benchmark when it comes to CDN.

Thanks,
Andrew


>
> Thanks,
> Siddhesh
>
>
>> Steve Ellcey
>> sellcey@cavium.com
>>
>>
>> 2018-05-10  Steve Ellcey  <sellcey@caviumnetworks.com>
>>
>>         * sysdeps/unix/sysv/linux/aarch64/gettimeofday.c: New file.
>>
>>
>> diff --git a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>> b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>> index e69de29..973b6ff 100644
>> --- a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>> +++ b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>> @@ -0,0 +1,69 @@
>> +/* Copyright (C) 2018 Free Software Foundation, Inc.
>> +
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public License as
>> +   published by the Free Software Foundation; either version 2.1 of the
>> +   License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +/* Get the current time of day and timezone information,
>> +   putting it into *tv and *tz.  If tz is null, *tz is not filled.
>> +   Returns 0 on success, -1 on errors.  */
>> +
>> +#ifdef SHARED
>> +
>> +# define __gettimeofday __redirect___gettimeofday
>> +# include <sys/time.h>
>> +# undef __gettimeofday
>> +# 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);
>> +}
>> +
>> +/* 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)
>> +
>> +# undef libc_hidden_def
>> +# define libc_hidden_def(name)                               \
>> +  __hidden_ver1 (__gettimeofday_syscall, __GI___gettimeofday,  \
>> +               __gettimeofday_syscall);
>> +
>> +#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)
>>
>

On 05/11/2018 10:13 AM, Andrew Pinski wrote:
> That does not measure the difference here.  We were already using the
> VDSO before; just differently.  Before we had a PLT that calls into
> the libc and would do then an check on the VDSO and then (another)
> indirect call (to the VDSO).  After we just have an PLT (indirect)
> call into the VDSO directly.  Basically saving the check and an extra
> indirect call (the indirect call can be costly if not predicted
> correctly).

Ah OK, got it.

> Walking the ifuncs won't benchmark the difference that is gotten here.
> 
> You will see the biggest benefit from Apache Bench and running Apache
> as the web server (maybe any other webserver which calls gettimeofday
> a lot as you are producing a date for each socket connection).   And
> yes this is considered a standard benchmark when it comes to CDN.

OK, thanks.  Looks good to me too then.  The benchmark would still be 
nice to have (let me know if y'all don't intend to write it so that I 
can put it in my list of things to do), but not a blocker for this patch.

Siddhesh

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)
>

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).

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..

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.
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.
diff --git a/posix/Makefile b/posix/Makefile
index e9730ee..2cd3a44 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -95,7 +95,8 @@ tests		:= test-errno tstgetopt testfnm runtests runptests \
 		   tst-posix_spawn-fd tst-posix_spawn-setsid \
 		   tst-posix_fadvise tst-posix_fadvise64 \
 		   tst-sysconf-empty-chroot tst-glob_symlinks tst-fexecve \
-		   tst-glob-tilde test-ssize-max
+		   tst-glob-tilde test-ssize-max \
+		   tst-gettimeofday tst-gettimeofday2
 tests-internal	:= bug-regex5 bug-regex20 bug-regex33 \
 		   tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
 		   tst-glob_lstat_compat
@@ -257,6 +258,8 @@ tst-boost-ARGS = BOOST.tests
 bug-glob1-ARGS = "$(objpfx)"
 tst-execvp3-ARGS = --test-dir=$(objpfx)
 
+tst-gettimeofday2-ENV = LD_BIND_NOW=1
+
 testcases.h: TESTS TESTS2C.sed
 	LC_ALL=C sed -f TESTS2C.sed < $< > $@T
 	mv -f $@T $@
diff --git a/posix/tst-gettimeofday.c b/posix/tst-gettimeofday.c
index e69de29..703c1ce 100644
--- a/posix/tst-gettimeofday.c
+++ b/posix/tst-gettimeofday.c
@@ -0,0 +1,55 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/time.h>
+#include <time.h>
+
+
+/* Test that nanosleep() does sleep.  */
+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));
+
+  /* At least one second must have passed.  */
+  struct timeval tv2;
+  TEMP_FAILURE_RETRY (gettimeofday (&tv2, NULL));
+
+  tv2.tv_sec -= tv1.tv_sec;
+  tv2.tv_usec -= tv1.tv_usec;
+  if (tv2.tv_usec < 0)
+    --tv2.tv_sec;
+
+  if (tv2.tv_sec < 1)
+    {
+      puts ("sleep didn't sleep long enough or gettimeofday is broken");
+      return 1;
+    }
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/posix/tst-gettimeofday2.c b/posix/tst-gettimeofday2.c
index e69de29..6c08761 100644
--- a/posix/tst-gettimeofday2.c
+++ b/posix/tst-gettimeofday2.c
@@ -0,0 +1 @@
+#include "tst-gettimeofday.c"
diff --git a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
index e69de29..5b6ba70 100644
--- a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
+++ b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
@@ -0,0 +1,68 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Get the current time of day and timezone information,
+   putting it into *tv and *tz.  If tz is null, *tz is not filled.
+   Returns 0 on success, -1 on errors.  */
+
+#ifdef SHARED
+
+# define __gettimeofday __redirect___gettimeofday
+# include <sys/time.h>
+# undef __gettimeofday
+# define HAVE_VSYSCALL
+# include <dl-vdso.h>
+# include <sysdep-vdso.h>
+
+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)

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)
>

On 16/05/2018 07:44, Szabolcs Nagy wrote:
> 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)

As I put previously keep in mind that different that x86 and powerpc 
implementations, where the vDSO symbol does not fail; the arm64 vDSO 
implements a syscall fallback in case of underlying hardware requires 
an out-of-line counter access (arch_timer_enable_workaround).

Using a ifunc accessors to call vDSO directly will result in a slight
different semantic since generic implementation (kernel/time/time.c)
might return EFAULT in some cases (which won't be handled by ifunc
implementation).  This should not be an issue since POSIX [1] defines
no error code should reserved for the symbol, but it might trigger
some test in LTP.

> 
>> 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)
>>
>

On 16/05/18 12:51, Adhemerval Zanella wrote:
> On 16/05/2018 07:44, Szabolcs Nagy wrote:
>> 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)
> 
> As I put previously keep in mind that different that x86 and powerpc
> implementations, where the vDSO symbol does not fail; the arm64 vDSO
> implements a syscall fallback in case of underlying hardware requires
> an out-of-line counter access (arch_timer_enable_workaround).
> 
> Using a ifunc accessors to call vDSO directly will result in a slight
> different semantic since generic implementation (kernel/time/time.c)
> might return EFAULT in some cases (which won't be handled by ifunc
> implementation).  This should not be an issue since POSIX [1] defines
> no error code should reserved for the symbol, but it might trigger
> some test in LTP.
> 

sorry i missed this comment,

if the vdso can fail with -EFAULT then this optimization is not valid
(gettimeofday must either return 0 or -1).

if userspace can test whatever the kernel is testing there, that
could be added to the ifunc resolver and only use vdso if we know
it does not fail (i don't know if this is possible).

On 16/05/18 13:03, Szabolcs Nagy wrote:
> On 16/05/18 12:51, Adhemerval Zanella wrote:
>> On 16/05/2018 07:44, Szabolcs Nagy wrote:
>>> 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)
>>
>> As I put previously keep in mind that different that x86 and powerpc
>> implementations, where the vDSO symbol does not fail; the arm64 vDSO
>> implements a syscall fallback in case of underlying hardware requires
>> an out-of-line counter access (arch_timer_enable_workaround).
>>
>> Using a ifunc accessors to call vDSO directly will result in a slight
>> different semantic since generic implementation (kernel/time/time.c)
>> might return EFAULT in some cases (which won't be handled by ifunc
>> implementation).  This should not be an issue since POSIX [1] defines
>> no error code should reserved for the symbol, but it might trigger
>> some test in LTP.
>>
> 
> sorry i missed this comment,
> 
> if the vdso can fail with -EFAULT then this optimization is not valid
> (gettimeofday must either return 0 or -1).
> 
> if userspace can test whatever the kernel is testing there, that
> could be added to the ifunc resolver and only use vdso if we know
> it does not fail (i don't know if this is possible).
> 

hm in posix EFAULT is not a requirement, it's probably
undefined behaviour if gettimeofday is called with
wrong pointers. (this is borderline since linux man
pages document the EFAULT, but obviously that cannot
work for vdso anyway)

then the optimization is ok, but we definitely need
to document why we think directly going to the vdso
is safe.

On 16/05/2018 09:12, Szabolcs Nagy wrote:
> On 16/05/18 13:03, Szabolcs Nagy wrote:
>> On 16/05/18 12:51, Adhemerval Zanella wrote:
>>> On 16/05/2018 07:44, Szabolcs Nagy wrote:
>>>> 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)
>>>
>>> As I put previously keep in mind that different that x86 and powerpc
>>> implementations, where the vDSO symbol does not fail; the arm64 vDSO
>>> implements a syscall fallback in case of underlying hardware requires
>>> an out-of-line counter access (arch_timer_enable_workaround).
>>>
>>> Using a ifunc accessors to call vDSO directly will result in a slight
>>> different semantic since generic implementation (kernel/time/time.c)
>>> might return EFAULT in some cases (which won't be handled by ifunc
>>> implementation).  This should not be an issue since POSIX [1] defines
>>> no error code should reserved for the symbol, but it might trigger
>>> some test in LTP.
>>>
>>
>> sorry i missed this comment,
>>
>> if the vdso can fail with -EFAULT then this optimization is not valid
>> (gettimeofday must either return 0 or -1).
>>
>> if userspace can test whatever the kernel is testing there, that
>> could be added to the ifunc resolver and only use vdso if we know
>> it does not fail (i don't know if this is possible).
>>
> 
> hm in posix EFAULT is not a requirement, it's probably
> undefined behaviour if gettimeofday is called with
> wrong pointers. (this is borderline since linux man
> pages document the EFAULT, but obviously that cannot
> work for vdso anyway)
> 
> then the optimization is ok, but we definitely need
> to document why we think directly going to the vdso
> is safe.

I think the optimization is fine as well, we had previous discussion about some
syscall semantics returning EFAULT [1] (which also lead to LTP test fixes [2]),
and the generic idea was since is UB to rely on EFAULT return code (for instance
a call to 'gettimeofday (0, 0)') it is fine to glibc also change semantic if it
finds suitable.

[1] https://sourceware.org/ml/libc-alpha/2017-10/msg00830.html
[2] https://github.com/linux-test-project/ltp/commit/259db6fed55f88ab32a0875e66803eee44d298be

On Wed, 2018-05-16 at 11:44 +0100, Szabolcs Nagy wrote:
> On 16/05/18 00:07, Steve Ellcey wrote:
> > 
> > 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)

Szabolcs,

I am having trouble figuring out how to document these issues.  The
problem is that I don't understand the details of the
syscall/vsyscall/vdso interfaces well enough to add intelligent
comments to the code that would address your comments.

Any chance you could add the comments that you think are needed and
check the patch in?  It seems like you and Adhemerval are both happy
with the code, it is just the comments (and the test if you want to use
the nanosleep test instead of adding the new ones) that need fixing.

Steve Ellcey
sellcey@cavium.com