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
* 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.
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.
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.
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.
Thanks,
Florian