This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [Patch] Use VDSO interface for gettimeofday on aarch64
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)
>>
>