| Message ID | AS4PR08MB79017C4FC5AE484617FAC654837F9@AS4PR08MB7901.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use C11 atomics instead of atomic_and/or | expand |
On 05/09/22 13:48, Wilco Dijkstra wrote: > Remove the 4 uses of atomic_and and atomic_or with atomic_fetch_and_acquire and > atomic_fetch_or_acquire > > Passes regress on AArch64. LGTM, the current atomic_or and atomic_and have acquire semantic already. What I am not not sure if if we need to use release MO on start_thread, since the idea is to synchronize with the locking on pthread_mutex_lock. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > --- > > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index d206ed7bf4c2305c0d65bc2a47baefe02969e3d2..d802c67b059af390e122e82f09e886d0c8950fd7 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -539,7 +539,7 @@ start_thread (void *arg) > # endif > this->__list.__next = NULL; > > - atomic_or (&this->__lock, FUTEX_OWNER_DIED); > + atomic_fetch_or_acquire (&this->__lock, FUTEX_OWNER_DIED); > futex_wake ((unsigned int *) &this->__lock, 1, > /* XYZ */ FUTEX_SHARED); > } > diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c > index 6e767a87247063c0ac84242ef13e72af79021104..439b1e6391c50d5922dec6c48e7f2a2a632a89d9 100644 > --- a/nptl/pthread_mutex_lock.c > +++ b/nptl/pthread_mutex_lock.c > @@ -462,7 +462,7 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) > > if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) > { > - atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); > + atomic_fetch_and_acquire (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); > > /* We got the mutex. */ > mutex->__data.__count = 1; > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index 0fcaabfb482546fd6f1f9cc4b13edc82f6e6796c..af70a60528cb101c8e52d4165950ee0d11f6f895 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -392,7 +392,7 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > > if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) > { > - atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); > + atomic_fetch_and_acquire (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); > > /* We got the mutex. */ > mutex->__data.__count = 1; > diff --git a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c > index 8a7de8e598803f606899fe1c9b8775bc24dd14ec..50524942a76c753ce4add20c35dfe7f659a1908b 100644 > --- a/nptl/pthread_mutex_trylock.c > +++ b/nptl/pthread_mutex_trylock.c > @@ -308,7 +308,7 @@ ___pthread_mutex_trylock (pthread_mutex_t *mutex) > > if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) > { > - atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); > + atomic_fetch_and_acquire (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); > > /* We got the mutex. */ > mutex->__data.__count = 1; >
Hi Adhemerval, > LGTM, the current atomic_or and atomic_and have acquire semantic already. What > I am not not sure if if we need to use release MO on start_thread, since the > idea is to synchronize with the locking on pthread_mutex_lock. Indeed, it is not clear what is being synchronized with the FUTEX_OWNER_DIED flag. However it is being used in a lock, so I kept the implied acquire MO of the old atomics. If it is simply a flag that does not affect locking and the data protected by the lock, then relaxed MO would be correct. However that requires detective work to try to reverse engineer the intended implementation... Cheers, Wilco
On 22/09/22 11:06, Wilco Dijkstra wrote: > Hi Adhemerval, > >> LGTM, the current atomic_or and atomic_and have acquire semantic already. What >> I am not not sure if if we need to use release MO on start_thread, since the >> idea is to synchronize with the locking on pthread_mutex_lock. > > Indeed, it is not clear what is being synchronized with the FUTEX_OWNER_DIED > flag. However it is being used in a lock, so I kept the implied acquire MO of the > old atomics. If it is simply a flag that does not affect locking and the data > protected by the lock, then relaxed MO would be correct. However that requires > detective work to try to reverse engineer the intended implementation... I tried to check with kernel robust implementation, but this specific code is a fallback that should be handled by the kernel. It currently used on very specific cases (qemu and some old architectures that do not support set_robust_list) so I think we reevaluate it later.
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index d206ed7bf4c2305c0d65bc2a47baefe02969e3d2..d802c67b059af390e122e82f09e886d0c8950fd7 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -539,7 +539,7 @@ start_thread (void *arg) # endif this->__list.__next = NULL; - atomic_or (&this->__lock, FUTEX_OWNER_DIED); + atomic_fetch_or_acquire (&this->__lock, FUTEX_OWNER_DIED); futex_wake ((unsigned int *) &this->__lock, 1, /* XYZ */ FUTEX_SHARED); } diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c index 6e767a87247063c0ac84242ef13e72af79021104..439b1e6391c50d5922dec6c48e7f2a2a632a89d9 100644 --- a/nptl/pthread_mutex_lock.c +++ b/nptl/pthread_mutex_lock.c @@ -462,7 +462,7 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) { - atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); + atomic_fetch_and_acquire (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); /* We got the mutex. */ mutex->__data.__count = 1; diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c index 0fcaabfb482546fd6f1f9cc4b13edc82f6e6796c..af70a60528cb101c8e52d4165950ee0d11f6f895 100644 --- a/nptl/pthread_mutex_timedlock.c +++ b/nptl/pthread_mutex_timedlock.c @@ -392,7 +392,7 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) { - atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); + atomic_fetch_and_acquire (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); /* We got the mutex. */ mutex->__data.__count = 1; diff --git a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c index 8a7de8e598803f606899fe1c9b8775bc24dd14ec..50524942a76c753ce4add20c35dfe7f659a1908b 100644 --- a/nptl/pthread_mutex_trylock.c +++ b/nptl/pthread_mutex_trylock.c @@ -308,7 +308,7 @@ ___pthread_mutex_trylock (pthread_mutex_t *mutex) if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) { - atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); + atomic_fetch_and_acquire (&mutex->__data.__lock, ~FUTEX_OWNER_DIED); /* We got the mutex. */ mutex->__data.__count = 1;