[03/13] nptl: Remove clockwait_tid

  It can be replaced with a __futex_abstimed_wait_cancelable64 call,
with the advantage that there is no need to further clock adjustments
to create a absolute timeout.  It allows to remove the now ununsed
futex_timed_wait_cancel64 internal function.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 nptl/pthread_join_common.c    | 70 ++++++-----------------------------
 sysdeps/nptl/futex-internal.h | 49 ------------------------
 2 files changed, 12 insertions(+), 107 deletions(-)
  

On Mon, 23 Nov 2020 16:52:46 -0300
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote:

> It can be replaced with a __futex_abstimed_wait_cancelable64 call,
> with the advantage that there is no need to further clock adjustments
> to create a absolute timeout.  It allows to remove the now ununsed
> futex_timed_wait_cancel64 internal function.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  nptl/pthread_join_common.c    | 70
> ++++++----------------------------- sysdeps/nptl/futex-internal.h |
> 49 ------------------------ 2 files changed, 12 insertions(+), 107
> deletions(-)
> 
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index 67d8e2b780..8a95c58ff3 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -32,55 +32,6 @@ cleanup (void *arg)
>    atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
>  }
>  
> -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
> futex
> -   wake-up when the clone terminates.  The memory location contains
> the
> -   thread ID while the clone is running and is reset to zero by the
> kernel
> -   afterwards.  The kernel up to version 3.16.3 does not use the
> private futex
> -   operations for futex wake-up when the clone terminates.  */
> -static int
> -clockwait_tid (pid_t *tidp, clockid_t clockid,
> -               const struct __timespec64 *abstime)
> -{
> -  pid_t tid;
> -  int ret;
> -
> -  if (! valid_nanoseconds (abstime->tv_nsec))
> -    return EINVAL;
> -
> -  /* Repeat until thread terminated.  */
> -  while ((tid = *tidp) != 0)
> -    {
> -      struct __timespec64 rt;
> -
> -      /* Get the current time. This can only fail if clockid is
> -         invalid. */
> -      if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
> -        return EINVAL;
> -
> -      /* Compute relative timeout.  */
> -      rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> -      rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> -      if (rt.tv_nsec < 0)
> -        {
> -          rt.tv_nsec += 1000000000;
> -          --rt.tv_sec;
> -        }
> -
> -      /* Already timed out?  */
> -      if (rt.tv_sec < 0)
> -        return ETIMEDOUT;
> -
> -      /* If *tidp == tid, wait until thread terminates or the wait
> times out.
> -         The kernel up to version 3.16.3 does not use the private
> futex
> -         operations for futex wake-up when the clone terminates.  */
> -      ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
> -      if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
> -        return -ret;
> -    }
> -
> -  return 0;
> -}
> -
>  int
>  __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
>                          clockid_t clockid,
> @@ -137,15 +88,18 @@ __pthread_clockjoin_ex (pthread_t threadid, void
> **thread_return, un-wait-ed for again.  */
>        pthread_cleanup_push (cleanup, &pd->joinid);
>  
> -      if (abstime != NULL)
> -	result = clockwait_tid (&pd->tid, clockid, abstime);
> -      else
> -	{
> -	  pid_t tid;
> -	  /* We need acquire MO here so that we synchronize with the
> -	     kernel's store to 0 when the clone terminates. (see
> above)  */
> -	  while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> -	    lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED);
> +     /* We need acquire MO here so that we synchronize with the
> +        kernel's store to 0 when the clone terminates. (see above)
> */
> +      pid_t tid;
> +      while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> +        {
> +	  int ret = __futex_abstimed_wait_cancelable64 (
> +	    (unsigned int *) &pd->tid, tid, clockid, abstime,
> LLL_SHARED);
> +	  if (ret == ETIMEDOUT || ret == EOVERFLOW)
> +	    {
> +	      result = ret;
> +	      break;
> +	    }
>  	}
>  
>        pthread_cleanup_pop (0);
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index 96d1318091..d5f13d15fb 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -390,55 +390,6 @@ futex_unlock_pi (unsigned int *futex_word, int
> private) }
>  }
>  
> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
> -                           const struct __timespec64 *timeout, int
> private) -{
> -  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> -                                     __lll_private_flag (FUTEX_WAIT,
> private),
> -                                     tid, timeout);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> -  if (err == -ENOSYS)
> -    {
> -      if (in_time_t_range (timeout->tv_sec))
> -        {
> -          struct timespec ts32 = valid_timespec64_to_timespec
> (*timeout); -
> -          err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
> -                                         __lll_private_flag
> (FUTEX_WAIT,
> -
> private),
> -                                         tid, &ts32);
> -        }
> -      else
> -        err = -EOVERFLOW;
> -    }
> -#endif
> -  switch (err)
> -    {
> -    case 0:
> -    case -EAGAIN:
> -    case -EINTR:
> -    case -ETIMEDOUT:
> -    case -EDEADLK:
> -    case -ENOSYS:
> -    case -EOVERFLOW:  /* Passed absolute timeout uses 64 bit time_t
> type, but
> -                         underlying kernel does not support 64 bit
> time_t futex
> -                         syscalls.  */
> -    case -EPERM:  /*  The caller is not allowed to attach itself to
> the futex.
> -		      Used to check if PI futexes are supported by
> the
> -		      kernel.  */
> -      return -err;
> -
> -    case -EINVAL: /* Either due to wrong alignment or due to the
> timeout not
> -		     being normalized.  Must have been caused by a
> glibc or
> -		     application bug.  */
> -    case -EFAULT: /* Must have been caused by a glibc or application
> bug.  */
> -    /* No other errors are documented at this time.  */
> -    default:
> -      futex_fatal_error ();
> -    }
> -}
> -
>  /* The futex_abstimed_wait_cancelable64 has been moved to a separate
> file to avoid problems with exhausting available registers on some
> architectures
>     - e.g. on m68k architecture.  */

Reviewed-by: Lukasz Majewski <lukma@denx.de>


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
  

* Adhemerval Zanella via Libc-alpha:

> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
> -                           const struct __timespec64 *timeout, int private)
> -{
> -  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> -                                     __lll_private_flag (FUTEX_WAIT, private),
> -                                     tid, timeout);

This uses FUTEX_WAIT.  But the replacement,
__futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.  I do not think
this is correct because the kernel will use FUTEX_WAKE internally for
the pd->tid wakeup relied upon by pthread_join.

This seems to cause pthread_join regressions on some kernel versions.

We need to audit all callers of __futex_abstimed_wait64 if they are
actually compatible with FUTEX_WAIT_BITSET.

Thanks,
Florian
  

On Dez 14 2020, Florian Weimer via Libc-alpha wrote:

> This uses FUTEX_WAIT.  But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.

FUTEX_WAIT is exactly equivalent to
FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.

Andreas.
  

On 14/12/2020 09:16, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> -static __always_inline int
>> -futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
>> -                           const struct __timespec64 *timeout, int private)
>> -{
>> -  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
>> -                                     __lll_private_flag (FUTEX_WAIT, private),
>> -                                     tid, timeout);
> 
> This uses FUTEX_WAIT.  But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.  I do not think
> this is correct because the kernel will use FUTEX_WAKE internally for
> the pd->tid wakeup relied upon by pthread_join.
> 
> This seems to cause pthread_join regressions on some kernel versions.
> 
> We need to audit all callers of __futex_abstimed_wait64 if they are
> actually compatible with FUTEX_WAIT_BITSET.

I don't think it should matter, as Andreas has put FUTEX_WAIT is exactly
as FUTEX_WAIT_BITSET plus FUTEX_BITSET_MATCH_ANY.  On a recent kernel:


  SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
                  struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
                  u32, val3)
  {
  [...]
    return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
  }

  long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
                u32 __user *uaddr2, u32 val2, u32 val3)
  {
  [...]
          case FUTEX_WAIT:
                  val3 = FUTEX_BITSET_MATCH_ANY;
                  fallthrough;
          case FUTEX_WAIT_BITSET:
                  return futex_wait(uaddr, flags, val, timeout, val3);
  [...]
  

* Andreas Schwab:

> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>
>> This uses FUTEX_WAIT.  But the replacement,
>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>
> FUTEX_WAIT is exactly equivalent to
> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.

Hmm.  I will continue debugging.  I encounter a missed wakeup in
pthread_join after making some changes that only should affect timing
(intl/tst-gettext6 is among the failures).

Reverting this patch here seems to help.

Thanks,
Florian
  

* Florian Weimer:

> * Andreas Schwab:
>
>> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>>
>>> This uses FUTEX_WAIT.  But the replacement,
>>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>>
>> FUTEX_WAIT is exactly equivalent to
>> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
>
> Hmm.  I will continue debugging.  I encounter a missed wakeup in
> pthread_join after making some changes that only should affect timing
> (intl/tst-gettext6 is among the failures).
>
> Reverting this patch here seems to help.

It's an accident, due to my changes and:

  /* In libc.so or ld.so all futexes are private.  */

in lowlevellock-futex.h.  So the patch posted here should be okay.

Thanks,
Florian