Re: [PATCH] Use C11 atomics instead of atomic_increment(_val)

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 08/09/22 11:01, Wilco Dijkstra wrote:
> Replace atomic_increment and atomic_increment_val with atomic_fetch_add_relaxed.
> One case in sem_post.c uses release semantics (see comment above it).
> 
> Passes regress on AArch64.

Looks good, I am just not sure about two relaxed MO below.

> 
> ---
> 
> diff --git a/htl/pt-create.c b/htl/pt-create.c
> index ce52ed9f52210a4e4c7a049ebee817ec9ccfeeb1..14f02cd2b8a19e8581a170dfba2b948ef8304203 100644
> --- a/htl/pt-create.c
> +++ b/htl/pt-create.c
> @@ -228,7 +228,7 @@ __pthread_create_internal (struct __pthread **thread,
>       the number of threads from within the new thread isn't an option
>       since this thread might return and call `pthread_exit' before the
>       new thread runs.  */
> -  atomic_increment (&__pthread_total);
> +  atomic_fetch_add_relaxed (&__pthread_total, 1);
>  
>    /* Store a pointer to this thread in the thread ID lookup table.  We
>       could use __thread_setid, however, we only lock for reading as no

Not sure if this is correct for hurd, __pthread_total is used on __pthread_exit
to call exit although for i686 atomic_decrement_and_test will be used and it
has strong MO.

> diff --git a/manual/ipc.texi b/manual/ipc.texi
> index 081b98fe29e0b3b5b7f4f916ad2085d170bd3825..f7cbdc3e09b0b4aea9a96ddcaf571c474024cc32 100644
> --- a/manual/ipc.texi
> +++ b/manual/ipc.texi
> @@ -85,7 +85,7 @@ by @theglibc{}.
>  
>  @deftypefun int sem_wait (sem_t *@var{sem});
>  @safety{@prelim{}@mtsafe{}@assafe{}@acunsafe{@acucorrupt{}}}
> -@c atomic_increment (nwaiters) acucorrupt
> +@c atomic_fetch_add_relaxed (nwaiters) acucorrupt
>  @c
>  @c Given the use atomic operations this function seems
>  @c to be AS-safe.  It is AC-unsafe because there is still
> diff --git a/manual/llio.texi b/manual/llio.texi
> index 92bfd93e067ce2a782369084ed7dae099300f418..4e6e3fb672bb8ecbb1bb52faf3d70d9b7b33973f 100644
> --- a/manual/llio.texi
> +++ b/manual/llio.texi
> @@ -2528,7 +2528,7 @@ aiocb64}, since the LFS transparently replaces the old interface.
>  @c      _dl_allocate_tls_init ok
>  @c       GET_DTV ok
>  @c     mmap ok
> -@c     atomic_increment_val ok
> +@c     atomic_fetch_add_relaxed ok
>  @c     munmap ok
>  @c     change_stack_perm ok
>  @c      mprotect ok
> @@ -2567,7 +2567,7 @@ aiocb64}, since the LFS transparently replaces the old interface.
>  @c     do_clone @asulock @ascuheap @aculock @acsmem
>  @c      PREPARE_CREATE ok
>  @c      lll_lock (pd->lock) @asulock @aculock
> -@c      atomic_increment ok
> +@c      atomic_fetch_add_relaxed ok
>  @c      clone ok
>  @c      atomic_decrement ok
>  @c      atomic_exchange_acquire ok

Ok.

> diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c
> index aa863c7ea8122ea01d1aa4cffe101bbb7c11270c..3b7e2d434abe8a15145349d1a08a4e706061c74d 100644
> --- a/nptl/nptl_setxid.c
> +++ b/nptl/nptl_setxid.c
> @@ -163,7 +163,7 @@ setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
>    /* If this failed, it must have had not started yet or else exited.  */
>    if (!INTERNAL_SYSCALL_ERROR_P (val))
>      {
> -      atomic_increment (&cmdp->cntr);
> +      atomic_fetch_add_relaxed (&cmdp->cntr, 1);
>        return 1;
>      }
>    else

I am not sure if relaxed MO is correct here, shouldn't it synchronize with the
__nptl_setxid_sighandler ?

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 8533d609684bc6681fbdccd9f2635d03ef3aa937..2602dba54e872f36bd54955af7587378e5dc3812 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -759,7 +759,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>       we momentarily store a false value; this doesn't matter because there
>       is no kosher thing a signal handler interrupting us right here can do
>       that cares whether the thread count is correct.  */
> -  atomic_increment (&__nptl_nthreads);
> +  atomic_fetch_add_relaxed (&__nptl_nthreads, 1);
>  
>    /* Our local value of stopped_start and thread_ran can be accessed at
>       any time. The PD->stopped_start may only be accessed if we have

Ok.

> diff --git a/nptl/sem_post.c b/nptl/sem_post.c
> index 9e5741753a741034db96cfff8b8978908fafc1f6..7ec21e92eb4c71d7f17764e96bc7603837f7522d 100644
> --- a/nptl/sem_post.c
> +++ b/nptl/sem_post.c
> @@ -91,7 +91,7 @@ __old_sem_post (sem_t *sem)
>    /* We must need to synchronize with consumers of this token, so the atomic
>       increment must have release MO semantics.  */
>    atomic_write_barrier ();
> -  (void) atomic_increment_val (futex);
> +  atomic_fetch_add_release (futex, 1);
>    /* We always have to assume it is a shared semaphore.  */
>    futex_wake (futex, 1, LLL_SHARED);
>    return 0;

Ok.

> diff --git a/nscd/cache.c b/nscd/cache.c
> index b66c35334a79a315ac9fd533df09ec097221d8e0..21af9a0f9550dfc7c1121fb5014cd80acdfcf696 100644
> --- a/nscd/cache.c
> +++ b/nscd/cache.c
> @@ -192,7 +192,7 @@ cache_add (int type, const void *key, size_t len, struct datahead *packet,
>  
>    /* We depend on this value being correct and at least as high as the
>       real number of entries.  */
> -  atomic_increment (&table->head->nentries);
> +  atomic_fetch_add_relaxed (&table->head->nentries, 1);
>  
>    /* It does not matter that we are not loading the just increment
>       value, this is just for statistics.  */

Ok.

> diff --git a/nscd/nscd_helper.c b/nscd/nscd_helper.c
> index 8e66fa2548e87c9b24cade41339c83f4cef7f2cf..d3e05e272a854e13c0e4d18594ea57336b8db2bf 100644
> --- a/nscd/nscd_helper.c
> +++ b/nscd/nscd_helper.c
> @@ -425,7 +425,7 @@ __nscd_get_map_ref (request_type type, const char *name,
>  				0))
>  	    cur = NO_MAPPING;
>  	  else
> -	    atomic_increment (&cur->counter);
> +	    atomic_fetch_add_relaxed (&cur->counter, 1);
>  	}
>      }
>  

Ok.

> diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
> index 4d486ca9b5026c7a7950bd7c7155212966df3c44..0b77a2d897027edc36e956301f018c2ab0121444 100644
> --- a/sysdeps/unix/sysv/linux/check_pf.c
> +++ b/sysdeps/unix/sysv/linux/check_pf.c
> @@ -72,8 +72,8 @@ static uint32_t nl_timestamp;
>  uint32_t
>  __bump_nl_timestamp (void)
>  {
> -  if (atomic_increment_val (&nl_timestamp) == 0)
> -    atomic_increment (&nl_timestamp);
> +  if (atomic_fetch_add_relaxed (&nl_timestamp, 1) + 1 == 0)
> +    atomic_fetch_add_relaxed (&nl_timestamp, 1);
>  
>    return nl_timestamp;
>  }
> @@ -309,7 +309,7 @@ __check_pf (bool *seen_ipv4, bool *seen_ipv6,
>    if (cache_valid_p ())
>      {
>        data = cache;
> -      atomic_increment (&cache->usecnt);
> +      atomic_fetch_add_relaxed (&cache->usecnt, 1);
>      }
>    else
>      {
> 

Ok.