This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 01/12] linux: Fix vDSO macros build with time64 interfaces
On 13/12/2019 11:03, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 13/12/2019 08:51, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> diff --git a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>>>> index 7e772e05ce..07d38466e2 100644
>>>> --- a/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>>>> +++ b/sysdeps/unix/sysv/linux/aarch64/gettimeofday.c
>>>> @@ -22,10 +22,6 @@
>>>>
>>>> #include <time.h>
>>>> #include <sysdep.h>
>>>> -
>>>> -#ifdef HAVE_GETTIMEOFDAY_VSYSCALL
>>>> -# define HAVE_VSYSCALL
>>>> -#endif
>>>> #include <sysdep-vdso.h>
>>>>
>>>> /* Used as a fallback in the ifunc resolver if VDSO is not available
>>>> @@ -36,7 +32,9 @@ __gettimeofday_vsyscall (struct timeval *restrict tv, void *restrict tz)
>>>> if (__glibc_unlikely (tz != 0))
>>>> memset (tz, 0, sizeof *tz);
>>>>
>>>> - return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);
>>>> + if (INLINE_VSYSCALL (gettimeofday, 2, tv, tz) == 0)
>>>> + return 0;
>>>> + return INLINE_SYSCALL_CALL (gettimeofday, tv, tz);
>>>> }
>>>
>>> Given that this is the fallback function why do we try INLINE_VSYSCALL
>>> first?
>>>
>>> (The static case would need adjusting, of course.)
>>
>> Because it will be used on static build and the fallback case will be
>> unlikely. But I can add static only case that uses vDSO plus syscall and
>> change the shared fallback case that just issues the syscall.
>
> I think that would make more sense, yes.
>
>>>> diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
>>>> index 875c4fe905..4ea56c9a4b 100644
>>>> --- a/sysdeps/unix/sysv/linux/clock_gettime.c
>>>> +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
>>>> @@ -21,10 +21,6 @@
>>>> #include <errno.h>
>>>> #include <time.h>
>>>> #include "kernel-posix-cpu-timers.h"
>>>> -
>>>> -#ifdef HAVE_CLOCK_GETTIME_VSYSCALL
>>>> -# define HAVE_VSYSCALL
>>>> -#endif
>>>> #include <sysdep-vdso.h>
>>>>
>>>> #include <shlib-compat.h>
>>>> @@ -33,24 +29,39 @@
>>>> int
>>>> __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
>>>> {
>>>> + int r = -1;
>>>> +
>>>> #ifdef __ASSUME_TIME64_SYSCALLS
>>>> + /* 64 bit ABIs or Newer 32-bit ABIs that only support 64-bit time_t. */
>>>> +# ifdef __NR_clock_gettime64
>>>> + r = INLINE_SYSCALL_CALL (clock_gettime64, clock_id, tp);
>>>> +# else
>>>> +# ifdef HAVE_CLOCK_GETTIME_VSYSCALL
>>>> + r = INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
>>>> +# endif
>>>> + if (r == -1)
>>>> + r = INLINE_SYSCALL_CALL (clock_gettime, clock_id, tp);
>>>
>>> Why do you check __NR_clock_gettime64 first? Won't this make the vDSO
>>> unused?
>>
>> The vDSO support for clock_gettime64 was added later in this set. I
>> explicit removed because even if an architecture sets
>> HAVE_CLOCK_GETTIME64_VSYSCALL, it won't build.
>
> Ah, I see it now:
>
> /* Get current value of CLOCK and store it in TP. */
> int
> __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
> {
> int r = -1;
>
> #ifdef __ASSUME_TIME64_SYSCALLS
> /* 64 bit ABIs or Newer 32-bit ABIs that only support 64-bit time_t. */
> # ifdef __NR_clock_gettime64
> # ifdef HAVE_CLOCK_GETTIME64_VSYSCALL
> r = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> # endif
> if (r == -1)
> r = INLINE_SYSCALL_CALL (clock_gettime64, clock_id, tp);
> # else
> # ifdef HAVE_CLOCK_GETTIME_VSYSCALL
> r = INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> # endif
> if (r == -1)
> r = INLINE_SYSCALL_CALL (clock_gettime, clock_id, tp);
> # endif
> #else
> /* Old 32-bit ABI with possible 64-bit time_t support. */
> # ifdef __NR_clock_gettime64
> # ifdef HAVE_CLOCK_GETTIME64_VSYSCALL
> r = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> # endif
> if (r == -1)
> r = INLINE_SYSCALL_CALL (clock_gettime64, clock_id, tp);
> # endif
> if (r == -1)
> {
> /* Fallback code that uses 32-bit support. */
> struct timespec tp32;
> # ifdef HAVE_CLOCK_GETTIME_VSYSCALL
> r = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
> # endif
> if (r == -1)
> r = INLINE_SYSCALL_CALL (clock_gettime, clock_id, &tp32);
> if (r == 0)
> *tp = valid_timespec_to_timespec64 (tp32);
> }
> #endif
>
> return r;
> }
>
> This looks quite ugly to me.
It is with current constraints. The idea is that eventually
!__ASSUME_TIME64_SYSCALLS path will be phase out.
>
> If I read it correctly, it still does not cover for 32-bit the case of
> an old kernel without clock_gettime64 support. Here, INLINE_VSYSCALL
> for clock_gettimeofday64 will fail (without a context switch), then
> INLINE_SYSCALL_CALL will fail, *with* a context switch, and only then,
> and only then the INLINE_VSYSCALL call for clock_gettimeofday will
> suceed.
It does cover, it is just not optimal for older kernels. This patch is
following the current semantic on the clock_gettime64. I see what you
are proposing as an additional optimization.
>
> Since this is used to implement clock_gettime:
>
> #if __TIMESIZE != 64
> int
> __clock_gettime (clockid_t clock_id, struct timespec *tp)
> {
> int ret;
> struct __timespec64 tp64;
>
> ret = __clock_gettime64 (clock_id, &tp64);
>
> if (ret == 0)
> {
> if (! in_time_t_range (tp64.tv_sec))
> {
> __set_errno (EOVERFLOW);
> return -1;
> }
>
> *tp = valid_timespec64_to_timespec (tp64);
> }
>
> return ret;
> }
> #endif
>
> it will impact quite a lot of installations. We know that
> clock_gettimeofday performance is critical to many users, eveon i386.
I agree and that's why I initially preferred to not implement the
old time32 on top of the time64 ones. But it was decided and this
is the most straightforward way to progressively test the new
interfaces without require doing a full switch to time64.
>
> The main question is whether it is worth supporting clock_gettime64
> without vDSO support. If it is not, at startup, the loader should
> select a function pointer for the clock_gettime64 implementation used by
> the clock_gettime64 wrapper:
>
> (a) kernel-provided clock_gettime64 from the vDSO
> (b) glibc clock_gettime64 implementation on top of clock_gettime vDSO
> (c) glibc clock_gettime64 implementation on top of clock_gettime syscall
>
> Checking the presence of vDSO symbols is reasonably efficient because
> it's just a hash table lookup (especially if _dl_lookup_direct is used).
> We would have two indirect calls for the legacy vDSO case, but getting
> rid of that would mean to use an IFUNC for clock_gettime and
> clock_gettime64, driving up complexity again.
>
> Unfortunately, writing (b) and (c) may not be easy on architectures
> where INTERNAL_VSYSCALL_CALL uses a non-standard calling convention with
> inline assembly, due to lack of proper GCC support for it.
>
> If we need to support systems without clock_gettime64 in the vDSO, we
> have a problem because that requires system call probing, which is
> probably not something that we want to do at startup.
Linux is moving to make the vDSO infrastructure more generic and easy
so the architectures will require less boilerplate to enable it. However
I do see that it might take some time to enable on all architectures and
there might be some kernel configuration that might explicit disable
clock_gettime64 vDSO.
But I think essentially what you are suggesting is an optimization to a
scenario that in practice should be unusual: a glibc build with a v5.1+
kernel headers, but deployed in a older kernel without time64 support.
This scenario could be a more common possibility to static build, so
an possible option is to use the a global flag atomically set in the
case of ENOSYS (as discussed in time64_t previously). We could set
it by syscall interface or globally to assume or not kernel support
time64_t. But again I think this should be handled as an optimization,
rather than a requisite/blocker.