[RFC,v4,02/24] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

  The nanosleep syscall is not supported on newer 32-bit platforms (such
as RV32). To fix this issue let's use clock_nanosleep_time64 if it is
avaliable.

Let's use CLOCK_REALTIME when calling clock_nanosleep_time64 as the
Linux specification says:
  "POSIX.1 specifies that nanosleep() should measure time against the
   CLOCK_REALTIME clock. However, Linux measures the time using the
   CLOCK_MONOTONIC clock. This probably does not matter, since the POSIX.1
   specification for clock_settime(2) says that discontinuous changes in
   CLOCK_REALTIME should not affect nanosleep()"

NOTE: We don't convert the struct timespec* remaining to a 64-bit value,
we need to kernel to provide __NR_clock_nanosleep_time64 and __TIMESIZE == 64
to get a 64bit timespec.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 ChangeLog                                    |  6 ++
 nptl/thrd_sleep.c                            | 60 +++++++++++++++-
 sysdeps/unix/sysv/linux/clock_nanosleep.c    | 61 +++++++++++++++-
 sysdeps/unix/sysv/linux/nanosleep.c          | 75 ++++++++++++++++++++
 sysdeps/unix/sysv/linux/nanosleep_nocancel.c | 47 ++++++++++++
 5 files changed, 243 insertions(+), 6 deletions(-)
  

On Fri, 9 Aug 2019, Alistair Francis wrote:

> diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> index 07a51808df2..fc495b56c67 100644
> --- a/nptl/thrd_sleep.c
> +++ b/nptl/thrd_sleep.c
> @@ -25,14 +25,68 @@ int
>  thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
>  {
>    INTERNAL_SYSCALL_DECL (err);
> -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
> +  int ret = -1;
> +
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> +                                 0, time_point, remaining);
> +#else

This still has the same problem as has been explained for previous 
versions of the patch series: if time_t is 32-bit but 
__ASSUME_TIME64_SYSCALLS is defined, it is not valid to call the 
clock_nanosleep_time64 with a 32-bit timespec; you have to call it with a 
64-bit timespec.  That is, if you call clock_nanosleep_time64 at all in 
the case where the original function was called with a 32-bit timespec 
(and I think you should do so) then you need to have conversions.

Please fix this *throughout* the patch series *before* posting the next 
version.  That is, for every patch in the series you need to consider what 
is appropriate for systems with 32-bit time - *not* just what is 
appropriate for RV32.

The pattern we have previously discussed for 64-bit time support is that 
the code should (a) define the function (__thrd_sleep_time64, say) for 
64-bit time (which then only needs conversions in the reverse direction - 
if the 64-bit syscall is not in fact available, but the 32-bit one is), 
(b) if __TIMESIZE != 64, defines the 32-bit function as a thin wrapper 
round that, (c) in the appropriate internal header, has a #define of the 
name such as __thrd_sleep_time64 to the name such as thrd_sleep, in the 
case where __TIMESIZE == 64 and thus no wrapper is needed.

> +# ifdef __NR_clock_nanosleep_time64
> +#  if __TIMESIZE == 64
> +  long int ret_64;
> +
> +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> +                                    0, time_point, remaining);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    ret = ret_64;
> +#  else
> +  timespec64 ts64;
> +
> +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> +                                 CLOCK_REALTIME, 0, time_point,
> +                                 ts64);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      remaining->tv_sec = ts64.tv_sec;
> +      remaining->tv_nsec = ts64.tv_nsec;
> +    }

thrd_sleep has *two* timespec arguments, one input and one output.  It's 
not sufficient to just convert the output, as here; if you're doing 
conversions, you have to convert *both*.

It is valid for the output argument to be NULL.  You need to avoid writing 
to *remaining in that case.

Please try to test such 64-bit time changes in configurations covering as 
many of the different cases in the code as possible, both at compile time 
and at run time, and include details in the patch submissions of what 
configurations you tested (architecture, kernel headers version, 
--enable-kernel version, kernel version used when running the testsuite).  
I'd hope such testing would have shown up the issue with the output 
argument being NULL, as well as the ABI breakage.

Relevant configurations should include at least (a) one that has always 
had 64-bit time, e.g. x86_64; (b) one with 32-bit time and old kernel 
headers (any kernel version at runtime); (c) one with 32-bit time and new 
kernel headers, old kernel at runtime; (d) one with 32-bit time and new 
kernel headers, new kernel at runtime but no --enable-kernel; (e) one with 
32-bit time and new kernel at runtime and new --enable-kernel.  (New = 5.1 
or later.)  I think that's a basic minimum for testing any patches related 
to 64-bit time.  (If Florian's changes to provide syscall tables within 
glibc go in, case (b) disappears.)

(In this case, you're also passing the struct ts64 by value to the syscall 
rather than a pointer to it.  And as this is C, not C++, I'm not sure a 
declaration "timespec64 ts64;" without "struct" would have compiled.)

> diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c

> +#if __TIMESIZE == 32
> +struct timespec64
> +{
> +  long long int tv_sec;   /* Seconds.  */
> +  long int tv_nsec;  /* Nanoseconds.  */
> +};

If we need such a structure different from the "struct __timespec64" in 
Lukasz's patches, it surely does not belong in the .c file for one 
particular function without a very detailed comment explaining exactly why 
it's so specific to that function rather than in a more generic internal 
header.
  

On Mon, Aug 12, 2019 at 10:22 AM Joseph Myers
<joseph@codesourcery.com> wrote:
>
> On Fri, 9 Aug 2019, Alistair Francis wrote:
>
> > diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> > index 07a51808df2..fc495b56c67 100644
> > --- a/nptl/thrd_sleep.c
> > +++ b/nptl/thrd_sleep.c
> > @@ -25,14 +25,68 @@ int
> >  thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> >  {
> >    INTERNAL_SYSCALL_DECL (err);
> > -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
> > +  int ret = -1;
> > +
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> > +                                 0, time_point, remaining);
> > +#else
>
> This still has the same problem as has been explained for previous
> versions of the patch series: if time_t is 32-bit but
> __ASSUME_TIME64_SYSCALLS is defined, it is not valid to call the
> clock_nanosleep_time64 with a 32-bit timespec; you have to call it with a
> 64-bit timespec.  That is, if you call clock_nanosleep_time64 at all in
> the case where the original function was called with a 32-bit timespec
> (and I think you should do so) then you need to have conversions.

As discussed in a different thread, I will update the series to
support 32 and 64 bit time_t with __ASSUME_TIME64_SYSCALLS defined.

>
> Please fix this *throughout* the patch series *before* posting the next
> version.  That is, for every patch in the series you need to consider what
> is appropriate for systems with 32-bit time - *not* just what is
> appropriate for RV32.
>
> The pattern we have previously discussed for 64-bit time support is that
> the code should (a) define the function (__thrd_sleep_time64, say) for
> 64-bit time (which then only needs conversions in the reverse direction -
> if the 64-bit syscall is not in fact available, but the 32-bit one is),
> (b) if __TIMESIZE != 64, defines the 32-bit function as a thin wrapper
> round that, (c) in the appropriate internal header, has a #define of the

Doesn't having a thing wrapper around __thrd_sleep64() result in
unnecessary conversions? When __NR_clock_nanosleep_time64 is not
defined we will end up converting a 32-bit time_t to a 64-bit time_t
just to convert it back to a 32-bit time_t.

It seems simpler to me to just keep the structure here and fix the
32-bit time_t when __ASSUME_TIME64_SYSCALLS is defined. That will
probably result in a helper function for
defined(__ASSUME_TIME64_SYSCALLS) ||
__NR_clock_nanosleep_time64 as they are very similar.

> name such as __thrd_sleep_time64 to the name such as thrd_sleep, in the
> case where __TIMESIZE == 64 and thus no wrapper is needed.
>
> > +# ifdef __NR_clock_nanosleep_time64
> > +#  if __TIMESIZE == 64
> > +  long int ret_64;
> > +
> > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> > +                                    0, time_point, remaining);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    ret = ret_64;
> > +#  else
> > +  timespec64 ts64;
> > +
> > +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> > +                                 CLOCK_REALTIME, 0, time_point,
> > +                                 ts64);
> > +
> > +  if (ret == 0 || errno != ENOSYS)
> > +    {
> > +      remaining->tv_sec = ts64.tv_sec;
> > +      remaining->tv_nsec = ts64.tv_nsec;
> > +    }
>
> thrd_sleep has *two* timespec arguments, one input and one output.  It's
> not sufficient to just convert the output, as here; if you're doing
> conversions, you have to convert *both*.

Good point, I will fix this.

>
> It is valid for the output argument to be NULL.  You need to avoid writing
> to *remaining in that case.

I will fix this.

>
> Please try to test such 64-bit time changes in configurations covering as
> many of the different cases in the code as possible, both at compile time
> and at run time, and include details in the patch submissions of what
> configurations you tested (architecture, kernel headers version,
> --enable-kernel version, kernel version used when running the testsuite).
> I'd hope such testing would have shown up the issue with the output
> argument being NULL, as well as the ABI breakage.

Before I send a patch series I will run more tests.

>
> Relevant configurations should include at least (a) one that has always
> had 64-bit time, e.g. x86_64; (b) one with 32-bit time and old kernel
> headers (any kernel version at runtime); (c) one with 32-bit time and new
> kernel headers, old kernel at runtime; (d) one with 32-bit time and new
> kernel headers, new kernel at runtime but no --enable-kernel; (e) one with
> 32-bit time and new kernel at runtime and new --enable-kernel.  (New = 5.1
> or later.)  I think that's a basic minimum for testing any patches related
> to 64-bit time.  (If Florian's changes to provide syscall tables within
> glibc go in, case (b) disappears.)

Setting up all these cases will take a long time, which is why I
haven't done it yet for a RFC series.

>
> (In this case, you're also passing the struct ts64 by value to the syscall
> rather than a pointer to it.  And as this is C, not C++, I'm not sure a
> declaration "timespec64 ts64;" without "struct" would have compiled.)

Will fix.

>
> > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c
>
> > +#if __TIMESIZE == 32
> > +struct timespec64
> > +{
> > +  long long int tv_sec;   /* Seconds.  */
> > +  long int tv_nsec;  /* Nanoseconds.  */
> > +};
>
> If we need such a structure different from the "struct __timespec64" in
> Lukasz's patches, it surely does not belong in the .c file for one
> particular function without a very detailed comment explaining exactly why
> it's so specific to that function rather than in a more generic internal
> header.

This can probably be removed then.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
  

On Wed, 14 Aug 2019, Alistair Francis wrote:

> > The pattern we have previously discussed for 64-bit time support is that
> > the code should (a) define the function (__thrd_sleep_time64, say) for
> > 64-bit time (which then only needs conversions in the reverse direction -
> > if the 64-bit syscall is not in fact available, but the 32-bit one is),
> > (b) if __TIMESIZE != 64, defines the 32-bit function as a thin wrapper
> > round that, (c) in the appropriate internal header, has a #define of the
> 
> Doesn't having a thing wrapper around __thrd_sleep64() result in
> unnecessary conversions? When __NR_clock_nanosleep_time64 is not

The conversions are in userspace, i.e. a few instructions.

We need a clearly defined consistent convention for the pattern used for 
function implementations for 32-bit and 64-bit time, not every function 
doing things in its own way.  For functions such a 
pthread_mutex_timedlock, with hundreds of lines of code, thin wrappers are 
clearly the only reasonably approach to avoid large amounts of code 
duplication.  That in turn leads to using thin wrappers consistently 
everywhere the functions are defined in C, unless there is a clear reason 
for a particular function to be different.
  

14.08.2019 в 11:20:54 -0700 Alistair Francis написал:
>    On Mon, Aug 12, 2019 at 10:22 AM Joseph Myers
> <joseph@codesourcery.com> wrote:
> > The pattern we have previously discussed for 64-bit time support is that
> > the code should (a) define the function (__thrd_sleep_time64, say) for
> > 64-bit time (which then only needs conversions in the reverse direction -
> > if the 64-bit syscall is not in fact available, but the 32-bit one is),
> > (b) if __TIMESIZE != 64, defines the 32-bit function as a thin wrapper
> > round that, (c) in the appropriate internal header, has a #define of the
> 
> Doesn't having a thing wrapper around __thrd_sleep64() result in
> unnecessary conversions? When __NR_clock_nanosleep_time64 is not
> defined we will end up converting a 32-bit time_t to a 64-bit time_t
> just to convert it back to a 32-bit time_t.

The only purpose of handling __NR_clock_nanosleep_time64 being not
defined (when __ASSUME_TIME64_SYSCALLS is not defined too) is just to
avoid compilation failure with old kernel headers.  There is no point
to optimize this case.  And Florian proposed patches that remove it
altogether.

> It seems simpler to me to just keep the structure here and fix the
> 32-bit time_t when __ASSUME_TIME64_SYSCALLS is defined. That will
> probably result in a helper function for
> defined(__ASSUME_TIME64_SYSCALLS) ||
> __NR_clock_nanosleep_time64 as they are very similar.

If you add support for a 64-bit time version of a function on
__TIMESIZE==32 architectures, you'll end up with 2 functions.  And it
is much simpler to have 32-bit-time one as a thin wrapper around
64-bit-time one.  Code will be more tested this way.

And if you do not add such support it should be sufficient to do just

#ifdef __NR_nanosleep
  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
#else
  int ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, CLOCK_MONOTONIC,
                                     0, time_point, remaining);
#endif

plus
#define __NR_clock_nanosleep __NR_clock_nanosleep64
in sysdep.h for rv32.

(I am not sure whether the #ifdef __NR_nanosleep part is needed.)