This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]