Message ID | d74ed5db-e4dd-5720-a301-c7133e4edff4@linux.vnet.ibm.com |
---|---|
State | Committed |
Headers | show |
On 04/30/2018 11:19 AM, Stefan Liebler wrote: > diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h > index 8326e2805c..bfbda99940 100644 > --- a/sysdeps/nptl/lowlevellock.h > +++ b/sysdeps/nptl/lowlevellock.h > @@ -181,11 +181,14 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *, > 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. */ > -#define lll_wait_tid(tid) \ > - do { \ > - __typeof (tid) __tid; \ > - while ((__tid = (tid)) != 0) \ > - lll_futex_wait (&(tid), __tid, LLL_SHARED);\ > +#define lll_wait_tid(tid) \ > + do { \ > + __typeof (tid) __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 (&(tid))) != 0) \ > + lll_futex_wait (&(tid), __tid, LLL_SHARED); \ > } while (0) > > extern int __lll_timedwait_tid (int *, const struct timespec *) This looks good to me, and improves the situation. Reviewed-by: Carlos O'Donell <carlos@redhat.com> I haven't had a chance to review the other P&C issues discussed by Torvald, but we should probably raise them in a new thread related to tid reloading and the consequences.
On 05/02/2018 06:27 AM, Carlos O'Donell wrote: > On 04/30/2018 11:19 AM, Stefan Liebler wrote: >> diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h >> index 8326e2805c..bfbda99940 100644 >> --- a/sysdeps/nptl/lowlevellock.h >> +++ b/sysdeps/nptl/lowlevellock.h >> @@ -181,11 +181,14 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *, >> 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. */ >> -#define lll_wait_tid(tid) \ >> - do { \ >> - __typeof (tid) __tid; \ >> - while ((__tid = (tid)) != 0) \ >> - lll_futex_wait (&(tid), __tid, LLL_SHARED);\ >> +#define lll_wait_tid(tid) \ >> + do { \ >> + __typeof (tid) __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 (&(tid))) != 0) \ >> + lll_futex_wait (&(tid), __tid, LLL_SHARED); \ >> } while (0) >> >> extern int __lll_timedwait_tid (int *, const struct timespec *) > > This looks good to me, and improves the situation. > > Reviewed-by: Carlos O'Donell <carlos@redhat.com> > I've just committed this part and opened the "Bug 23137 - s390: pthread_join sometimes block indefinitely (on 31bit and libc build with -Os)" (https://sourceware.org/bugzilla/show_bug.cgi?id=23137) > I haven't had a chance to review the other P&C issues discussed by Torvald, > but we should probably raise them in a new thread related to tid reloading > and the consequences. >
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h index 8326e2805c..bfbda99940 100644 --- a/sysdeps/nptl/lowlevellock.h +++ b/sysdeps/nptl/lowlevellock.h @@ -181,11 +181,14 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *, 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. */ -#define lll_wait_tid(tid) \ - do { \ - __typeof (tid) __tid; \ - while ((__tid = (tid)) != 0) \ - lll_futex_wait (&(tid), __tid, LLL_SHARED);\ +#define lll_wait_tid(tid) \ + do { \ + __typeof (tid) __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 (&(tid))) != 0) \ + lll_futex_wait (&(tid), __tid, LLL_SHARED); \ } while (0) extern int __lll_timedwait_tid (int *, const struct timespec *)