[PATCH] Use C11 atomics instead of atomic_and/or

[PATCH] Use C11 atomics instead of atomic_and/or

[Wilco Dijkstra]

Remove the 4 uses of atomic_and and atomic_or with atomic_fetch_and_acquire and
atomic_fetch_or_acquire

Passes regress on AArch64.

---

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;

Re: [PATCH] Use C11 atomics instead of atomic_and/or

[Adhemerval Zanella Netto]

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

Re: [PATCH] Use C11 atomics instead of atomic_and/or

[Wilco Dijkstra]

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

Re: [PATCH] Use C11 atomics instead of atomic_and/or

[Adhemerval Zanella Netto]

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.