Re: [PATCH v2 2/3] Y2038: nptl: Provide futex_abstimed_wait64 supporting 64 bit time

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 6891 bytes --]

Hi Adhemerval,

> On 19/09/2020 10:07, Lukasz Majewski wrote:
> > This is the helper function, which uses struct __timespec64
> > to provide 64 bit absolute time to futex syscalls.
> > 
> > The aim of this function is to move convoluted pre-processor
> > macro code from sysdeps/nptl/lowlevellock-futex.h to C
> > function in futex-internal.c
> > 
> > The futex_abstimed_wait64 function has been put into a separate
> > file on the purpose - to avoid issues apparent on the m68k
> > architecture related to small number of available registers (there
> > is not enough registers to put all necessary arguments in them if
> > the above function would be added to futex-internal.h with
> > __always_inline attribute).
> > 
> > Additional precautions for m68k port have been taken - the
> > futex-internal.c file will be compiled with -fno-inline flag.  
> 
> LGTM with a nit regarding style and a clarification about 
> '-fno-inline'.

Please find the explanation below.

> 
> > 
> > ---
> > Changes for v2:
> > - Handle the case when *abstime pointer is NULL
> > ---
> >  sysdeps/nptl/futex-internal.c         | 70
> > +++++++++++++++++++++++++++ sysdeps/nptl/futex-internal.h         |
> >  6 +++ sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
> >  3 files changed, 78 insertions(+)
> > 
> > diff --git a/sysdeps/nptl/futex-internal.c
> > b/sysdeps/nptl/futex-internal.c index 3366aac162..3211b4c94f 100644
> > --- a/sysdeps/nptl/futex-internal.c
> > +++ b/sysdeps/nptl/futex-internal.c
> > @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned
> > int* futex_word, abstime != NULL ? &ts32 : NULL,
> >                                    NULL /* Unused.  */,
> > FUTEX_BITSET_MATCH_ANY); }
> > +
> > +static int
> > +__futex_abstimed_wait32 (unsigned int* futex_word,
> > +                         unsigned int expected, clockid_t clockid,
> > +                         const struct __timespec64* abstime,
> > +                         int private)
> > +{
> > +  struct timespec ts32;
> > +
> > +  if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
> > +    return -EOVERFLOW;
> > +
> > +  unsigned int clockbit = (clockid == CLOCK_REALTIME) ?
> > +    FUTEX_CLOCK_REALTIME : 0;
> > +  int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> > private); +
> > +  if (abstime != NULL)
> > +    ts32 = valid_timespec64_to_timespec (*abstime);
> > +
> > +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> > +                                abstime != NULL ? &ts32 : NULL,
> > +                                NULL /* Unused.  */,
> > FUTEX_BITSET_MATCH_ANY); +}
> >  #endif
> >  
> >  int  
> 
> Ok.
> 
> > @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned
> > int* futex_word, futex_fatal_error ();
> >      }
> >  }
> > +
> > +int
> > +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> > expected,
> > +                         clockid_t clockid,
> > +                         const struct __timespec64* abstime, int
> > private) +{
> > +  unsigned int clockbit;
> > +  int err;
> > +
> > +  /* Work around the fact that the kernel rejects negative timeout
> > values
> > +     despite them being valid.  */
> > +  if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec <
> > 0)))
> > +    return ETIMEDOUT;
> > +
> > +  if (! lll_futex_supported_clockid(clockid))
> > +    return EINVAL;  
> 
> Space before parentheses. 

Ok. Fixed.

> 
> > +
> > +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME :
> > 0;
> > +  int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> > private); +
> > +  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op,
> > expected,
> > +                               abstime, NULL /* Unused.  */,
> > +                               FUTEX_BITSET_MATCH_ANY);
> > +#ifndef __ASSUME_TIME64_SYSCALLS
> > +  if (err == -ENOSYS)
> > +    err = __futex_abstimed_wait32 (futex_word, expected,
> > +                                   clockid, abstime, private);
> > +#endif
> > +  switch (err)
> > +    {
> > +    case 0:
> > +    case -EAGAIN:
> > +    case -EINTR:
> > +    case -ETIMEDOUT:
> > +      return -err;
> > +
> > +    case -EFAULT: /* Must have been caused by a glibc or
> > application bug.  */
> > +    case -EINVAL: /* Either due to wrong alignment, unsupported
> > +		     clockid or due to the timeout not being
> > +		     normalized. Must have been caused by a glibc
> > or
> > +		     application bug.  */
> > +    case -ENOSYS: /* Must have been caused by a glibc bug.  */
> > +    /* No other errors are documented at this time.  */
> > +    default:
> > +      futex_fatal_error ();
> > +    }
> > +}  
> 
> Ok.
> 
> > diff --git a/sysdeps/nptl/futex-internal.h
> > b/sysdeps/nptl/futex-internal.h index 7f3910ad98..1ba0d61938 100644
> > --- a/sysdeps/nptl/futex-internal.h
> > +++ b/sysdeps/nptl/futex-internal.h
> > @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned
> > int* futex_word, const struct __timespec64* abstime,
> >                                      int private) attribute_hidden;
> >  
> > +int
> > +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int
> > expected,
> > +                         clockid_t clockid,
> > +                         const struct __timespec64* abstime,
> > +                         int private) attribute_hidden;
> > +
> >  #endif  /* futex-internal.h */  
> 
> Ok.
> 
> > diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile
> > b/sysdeps/unix/sysv/linux/m68k/Makefile index
> > be40fae68a..65164c5752 100644 ---
> > a/sysdeps/unix/sysv/linux/m68k/Makefile +++
> > b/sysdeps/unix/sysv/linux/m68k/Makefile @@ -21,3 +21,5 @@
> > sysdep-dl-routines += dl-static sysdep-others += lddlibc4
> >  install-bin += lddlibc4
> >  endif
> > +
> > +CFLAGS-futex-internal.c += -fno-inline
> >   
> 
> Do we still need it?

Unfortunately, yes. The m68k architecture has the issue with properly
compiling this code (in futex-internal.c) as it has very limited number
of registers (8 x 32 bit IIRC).

I did some investigation and it looks like static inline
in_time_t_range() function affects the compiler capabilities to
generate correct code.

It can be easily inlined on any architecture but m68k.

As m68k has issues with this code compilation - it is IMHO better to
disable inlining (-fno-inline) on this particular port.

As a result we would have slower execution only on relevant functions,
but 64 bit time support would be provided. 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]