| Message ID | AS4PR08MB790126C706EFA1DCC038BE6183409@AS4PR08MB7901.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use C11 atomics instead of atomic_increment(_val) | expand |
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.
Hi Adhemerval, In general you only need acquire/release for locks that synchronize access to shared data which does not use atomic accesses. If there is no such shared data, there is no need for acquire/release. Similarly only using acquire and never release (as the old atomics did) makes no sense - you need a release atomic to synchronize with. > I am not sure if relaxed MO is correct here, shouldn't it synchronize with the > __nptl_setxid_sighandler ? The counter cntr in nptl/nptl_setxid.c is just a simple atomic counter to keep track of the number of outstanding signals that were sent to threads. There is no data it is trying to synchronize with, so release/acquire semantics do not make sense here. > 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. The same is true for pthread_total counter, it is used to call exit when the last thread stops. Note while the increment is done optimistically (thread creation can fail, and then it is decremented again), the decrement resulting in a call to exit can only happen once even if multiple threads are finishing concurrently (and that happen once is what is required). Cheers, Wilco > 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/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 ?
On 22/09/22 10:27, Wilco Dijkstra wrote: > Hi Adhemerval, > > In general you only need acquire/release for locks that synchronize access to > shared data which does not use atomic accesses. If there is no such shared > data, there is no need for acquire/release. Similarly only using acquire and never > release (as the old atomics did) makes no sense - you need a release atomic > to synchronize with. Alright, I agree that default to acquire MO as the most atomic does today does not make much sense. > >> I am not sure if relaxed MO is correct here, shouldn't it synchronize with the >> __nptl_setxid_sighandler ? > > The counter cntr in nptl/nptl_setxid.c is just a simple atomic counter to keep > track of the number of outstanding signals that were sent to threads. There is > no data it is trying to synchronize with, so release/acquire semantics do not > make sense here. Ack. > >> 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. > > The same is true for pthread_total counter, it is used to call exit when the last > thread stops. Note while the increment is done optimistically (thread creation > can fail, and then it is decremented again), the decrement resulting in a call to > exit can only happen once even if multiple threads are finishing concurrently > (and that happen once is what is required). Ack. > > Cheers, > Wilco > The patch looks ok then. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > >> 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/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/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 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 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 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 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; 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. */ 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); } } 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 {