Re: [PATCH] y2038: nptl: Provide __futex_clock_wait_bitset64 to support 64 bit bitset

[Lukasz Majewski <lukma@denx.de>]

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

Hi Stefan,

Thank you for testing.

> On 10/19/20 4:57 PM, Lukasz Majewski wrote:
> > The commit:
> > "y2038: nptl: Convert pthread_mutex_{clock|timed}lock to support 64
> > bit" SHA1: 29e9874a048f47e2d46c40253036c8d2de921548
> > 
> > introduced support for 64 bit timeouts. Unfortunately, it was
> > missing the code for bitset - i.e. lll_futex_clock_wait_bitset C
> > preprocessor macro was used. As a result the 64 bit struct
> > __timespec64 was coerced to 32 bit struct timespec and regression
> > visible as timeout was observed (nptl/tst-robust10 on s390).
> > 
> > Reported-by: Stefan Liebler <stli@linux.ibm.com>  
> 
> I've successfully run some tests on s390 (31bit) / x86 (32bit) with
> this patch.
> - strace output of nptl/tst-robust10 on s390 (31bit):
> 10:41:37.553933 futex_time64(0x406144,
> FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 2150823950,
> {tv_sec=1603183298, tv_nsec=553918404}, FUTEX_BITSET_MATCH_ANY) = -1
> ETIMEDOUT (Connection timed out) <1.000090>
> => Now there is only this single FUTEX_WAIT_BITSET call which times
> out after a second.

The tv_sec=1603183298 now seems to be correct.

> 
> - strace output of nptl/tst-robust10 on x86 (32bit):
> 10:45:50.985125 futex_time64(0x804f0f4,
> FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, 2147912575,
> {tv_sec=1603183551, tv_nsec=985109825}, FUTEX_BITSET_MATCH_ANY) = -1
> ETIMEDOUT (Connection timed out) <1.000439>
> => Now tv_nsec is not zero anymore and it times out after a second.  
> 
> I have one question - see below.
> 
> Thanks,
> Stefan
> 
> > ---
> >  nptl/pthread_mutex_timedlock.c |  2 +-
> >  sysdeps/nptl/futex-internal.c  | 48
> > +++++++++++++++++++++++++++++++++- sysdeps/nptl/futex-internal.h  |
> >  5 ++++ 3 files changed, 53 insertions(+), 2 deletions(-)
> > 
> > diff --git a/nptl/pthread_mutex_timedlock.c
> > b/nptl/pthread_mutex_timedlock.c index dc40399f02..fe9e651f6c 100644
> > --- a/nptl/pthread_mutex_timedlock.c
> > +++ b/nptl/pthread_mutex_timedlock.c
> > @@ -268,7 +268,7 @@ __pthread_mutex_clocklock_common
> > (pthread_mutex_t *mutex, assume_other_futex_waiters |=
> > FUTEX_WAITERS; 
> >  	  /* Block using the futex.  */
> > -	  int err = lll_futex_clock_wait_bitset
> > (&mutex->__data.__lock,
> > +	  int err = __futex_clock_wait_bitset64
> > (&mutex->__data.__lock, oldval, clockid, abstime,
> >  	      PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
> >  	  /* The futex call timed out.  */
> > diff --git a/sysdeps/nptl/futex-internal.c
> > b/sysdeps/nptl/futex-internal.c index 2e1919f056..a978ad0ad2 100644
> > --- a/sysdeps/nptl/futex-internal.c
> > +++ b/sysdeps/nptl/futex-internal.c
> > @@ -70,7 +70,28 @@ __futex_abstimed_wait32 (unsigned int*
> > futex_word, abstime != NULL ? &ts32 : NULL,
> >                                  NULL /* Unused.  */,
> > FUTEX_BITSET_MATCH_ANY); }
> > -#endif
> > +
> > +static int
> > +__futex_clock_wait_bitset32 (int *futexp, int val, clockid_t
> > clockid,
> > +                             const struct __timespec64 *abstime,
> > int private) +{
> > +  struct timespec ts32;
> > +
> > +  if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
> > +    return -EOVERFLOW;
> > +
> > +  const unsigned int clockbit =
> > +    (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> > +  const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> > private); +
> > +  if (abstime != NULL)
> > +    ts32 = valid_timespec64_to_timespec (*abstime);
> > +
> > +  return INTERNAL_SYSCALL_CALL (futex, futexp, op, val,
> > +                                abstime != NULL ? &ts32 : NULL,
> > +                                NULL /* Unused.  */,
> > FUTEX_BITSET_MATCH_ANY); +}
> > +#endif /* ! __ASSUME_TIME64_SYSCALLS */
> >  
> >  int
> >  __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> > @@ -222,3 +243,28 @@ __futex_clocklock_wait64 (int *futex, int val,
> > clockid_t clockid, 
> >    return -err;
> >  }
> > +
> > +int
> > +__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t
> > clockid,
> > +                             const struct __timespec64 *abstime,
> > +                             int private)
> > +{
> > +  long int relowlevellock-futex.ht;  
> Is there a reason why long int is used instead of int (which is also
> returned by this function)?

The "long int" type for ret was in the original
lll_futex_clock_wait_bitset() macro in:
./sysdeps/nptl/lowlevellock-futex.h (line 102).

It is also returned when this macro is pasted.

The reason may be that lll_syscall_futex() macro also uses long int to
get return value from INTERNAL_SYSCALL() (Line 70 in the above file).

Then depending on architecture either long or int are used to get the
status of the syscall.


However, in the end the value returned by lll_futex_clock_wait_bitset()
is casted to int anyway:

git grep -n "= lll_futex_clock_wait_bitset"
nptl/pthread_mutex_timedlock.c:271:
int err = lll_futex_clock_wait_bitset (&mutex->__data.__lock,

sysdeps/nptl/futex-internal.h:288:
int err = lll_futex_clock_wait_bitset (futex_word, expected,

sysdeps/nptl/futex-internal.h:324:
int err = lll_futex_clock_wait_bitset (futex_word, expected,

Hence, IMHO it would be correct to change 'long int' to 'int'. Or am I
wrong? 

> > +  if (! lll_futex_supported_clockid (clockid))
> > +    {
> > +      return -EINVAL;
> > +    }
> > +
> > +  const unsigned int clockbit =
> > +    (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> > +  const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> > private); +
> > +  ret = INTERNAL_SYSCALL_CALL (futex_time64, futexp, op, val,
> > +                               abstime, NULL /* Unused.  */,
> > +                               FUTEX_BITSET_MATCH_ANY);
> > +#ifndef __ASSUME_TIME64_SYSCALLS
> > +  if (ret == -ENOSYS)
> > +    ret = __futex_clock_wait_bitset32 (futexp, val, clockid,
> > abstime, private); +#endif
> > +  return ret;
> > +}
> > diff --git a/sysdeps/nptl/futex-internal.h
> > b/sysdeps/nptl/futex-internal.h index 8a5f62768f..cd356e4fa8 100644
> > --- a/sysdeps/nptl/futex-internal.h
> > +++ b/sysdeps/nptl/futex-internal.h
> > @@ -603,4 +603,9 @@ __futex_clocklock64 (int *futex, clockid_t
> > clockid, return err;
> >  }
> >  
> > +int
> > +__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t
> > clockid,
> > +                             const struct __timespec64 *abstime,
> > +                             int private) attribute_hidden;
> > +
> >  #endif  /* futex-internal.h */
> >   
> 


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 --]