| Message ID | AS4PR08MB7901F73537A9576C8A6A1C9883409@AS4PR08MB7901.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use C11 atomics instead atomic_decrement_and_test | expand |
On 08/09/22 11:06, Wilco Dijkstra wrote: > Replace atomic_decrement_and_test with atomic_fetch_add_relaxed. > > Passes regress on AArch64. Some questions below. > > --- > > diff --git a/htl/pt-dealloc.c b/htl/pt-dealloc.c > index c776e3471dc376977ca7058f0d143f0a842d5799..86bbb3091fbd3758b06294ca705dc774cf8bf0eb 100644 > --- a/htl/pt-dealloc.c > +++ b/htl/pt-dealloc.c > @@ -33,7 +33,7 @@ extern pthread_mutex_t __pthread_free_threads_lock; > void > __pthread_dealloc (struct __pthread *pthread) > { > - if (!atomic_decrement_and_test (&pthread->nr_refs)) > + if (atomic_fetch_add_relaxed (&pthread->nr_refs, -1) != 1) > return; > > /* Withdraw this thread from the thread ID lookup table. */ Ok (and I am not sure why __pthread_create_internal usage does not use atomic at all). > diff --git a/htl/pt-exit.c b/htl/pt-exit.c > index f0759c8738e6da4c8e0ba47f6d15720203f6954e..3c0a8c52f34ed64b94ce71a0764512cb76351a3a 100644 > --- a/htl/pt-exit.c > +++ b/htl/pt-exit.c > @@ -50,7 +50,7 @@ __pthread_exit (void *status) > > /* Decrease the number of threads. We use an atomic operation to > make sure that only the last thread calls `exit'. */ > - if (atomic_decrement_and_test (&__pthread_total)) > + if (atomic_fetch_add_relaxed (&__pthread_total, -1) == 1) > /* We are the last thread. */ > exit (0); > > diff --git a/manual/llio.texi b/manual/llio.texi I am not sure if MO is suffice here, shouldn't it synchronize with the update from __pthread_create_internal? > index 4e6e3fb672bb8ecbb1bb52faf3d70d9b7b33973f..eba9a0e6593f17116b41ff302cfca05846727155 100644 > --- a/manual/llio.texi > +++ b/manual/llio.texi > @@ -2614,7 +2614,7 @@ aiocb64}, since the LFS transparently replaces the old interface. > @c free @ascuheap @acsmem > @c libc_thread_freeres > @c libc_thread_subfreeres ok > -@c atomic_decrement_and_test ok > +@c atomic_fetch_add_relaxed ok > @c td_eventword ok > @c td_eventmask ok > @c atomic_compare_exchange_bool_acq ok Ok. > diff --git a/nptl/cond-perf.c b/nptl/cond-perf.c > index 9c9488e2743f17eb4beeaadf45ef83a38a9f9bf9..baf69e6d34fdb3ffef97b32fb9db422077a0359b 100644 > --- a/nptl/cond-perf.c > +++ b/nptl/cond-perf.c > @@ -24,7 +24,7 @@ cons (void *arg) > > do > { > - if (atomic_decrement_and_test (&ntogo)) > + if (atomic_fetch_add_relaxed (&ntogo, -1) == 1) > { > pthread_mutex_lock (&mut2); > alldone = true; Ok, although this code is not used anywhere (neither for testing). Maybe it would be better to just remove it. > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 2602dba54e872f36bd54955af7587378e5dc3812..ada3fdd4d440c3fdcf4567734a452c8ba4d0fce1 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -489,7 +489,7 @@ start_thread (void *arg) > the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */ > atomic_fetch_or_relaxed (&pd->cancelhandling, EXITING_BITMASK); > > - if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads))) > + if (__glibc_unlikely (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) == 1)) > /* This was the last thread. */ > exit (0); > Same question from __pthread_total, is relaxed MO suffice here? > diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h > index a9e85f2b098dfc6790d719bba9662e428ab237e6..c10a16b2c4a61e74f6bf18396935b87208df4f7e 100644 > --- a/sysdeps/nptl/libc_start_call_main.h > +++ b/sysdeps/nptl/libc_start_call_main.h > @@ -65,7 +65,7 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL), > /* One less thread. Decrement the counter. If it is zero we > terminate the entire process. */ > result = 0; > - if (! atomic_decrement_and_test (&__nptl_nthreads)) > + if (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) != 1) > /* Not much left to do but to exit the thread, not the process. */ > while (1) > INTERNAL_SYSCALL_CALL (exit, 0); >
Hi Adhemerval, > + if (atomic_fetch_add_relaxed (&pthread->nr_refs, -1) != 1) > return; > > /* Withdraw this thread from the thread ID lookup table. */ > Ok (and I am not sure why __pthread_create_internal usage does not use atomic at all). Yes the non-atomic increment is a bug, an in/decrement could be lost if there is a data race. Is it still used or could we default to nptl and remove the htl stuff? > I am not sure if MO is suffice here, shouldn't it synchronize with the update > from __pthread_create_internal? As discussed in my previous mail, __pthread_total and __nptl_nthreads are simple counters that decide when to call exit for the last thread. > Ok, although this code is not used anywhere (neither for testing). Maybe it would be better > to just remove it. Yes, it looks like it was added 20 years ago as some kind of internal benchmark - I'll remove it in the commit. Cheers, Wilco
On 22/09/22 10:55, Wilco Dijkstra wrote: > Hi Adhemerval, > >> + if (atomic_fetch_add_relaxed (&pthread->nr_refs, -1) != 1) >> return; >> >> /* Withdraw this thread from the thread ID lookup table. */ > >> Ok (and I am not sure why __pthread_create_internal usage does not use atomic at all). > > Yes the non-atomic increment is a bug, an in/decrement could be lost if there is > a data race. Is it still used or could we default to nptl and remove the htl stuff? The htl is used the pthread implementation on Hurd. I am not sure if this support multiprocessor environments, so I will let Hurd developers figure out if they need to fix it. > >> I am not sure if MO is suffice here, shouldn't it synchronize with the update >> from __pthread_create_internal? > > As discussed in my previous mail, __pthread_total and __nptl_nthreads are simple > counters that decide when to call exit for the last thread. Ack. > >> Ok, although this code is not used anywhere (neither for testing). Maybe it would be better >> to just remove it. > > Yes, it looks like it was added 20 years ago as some kind of internal benchmark - I'll remove it > in the commit. Ok. > > Cheers, > Wilco Patch looks good me then. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
diff --git a/htl/pt-dealloc.c b/htl/pt-dealloc.c index c776e3471dc376977ca7058f0d143f0a842d5799..86bbb3091fbd3758b06294ca705dc774cf8bf0eb 100644 --- a/htl/pt-dealloc.c +++ b/htl/pt-dealloc.c @@ -33,7 +33,7 @@ extern pthread_mutex_t __pthread_free_threads_lock; void __pthread_dealloc (struct __pthread *pthread) { - if (!atomic_decrement_and_test (&pthread->nr_refs)) + if (atomic_fetch_add_relaxed (&pthread->nr_refs, -1) != 1) return; /* Withdraw this thread from the thread ID lookup table. */ diff --git a/htl/pt-exit.c b/htl/pt-exit.c index f0759c8738e6da4c8e0ba47f6d15720203f6954e..3c0a8c52f34ed64b94ce71a0764512cb76351a3a 100644 --- a/htl/pt-exit.c +++ b/htl/pt-exit.c @@ -50,7 +50,7 @@ __pthread_exit (void *status) /* Decrease the number of threads. We use an atomic operation to make sure that only the last thread calls `exit'. */ - if (atomic_decrement_and_test (&__pthread_total)) + if (atomic_fetch_add_relaxed (&__pthread_total, -1) == 1) /* We are the last thread. */ exit (0); diff --git a/manual/llio.texi b/manual/llio.texi index 4e6e3fb672bb8ecbb1bb52faf3d70d9b7b33973f..eba9a0e6593f17116b41ff302cfca05846727155 100644 --- a/manual/llio.texi +++ b/manual/llio.texi @@ -2614,7 +2614,7 @@ aiocb64}, since the LFS transparently replaces the old interface. @c free @ascuheap @acsmem @c libc_thread_freeres @c libc_thread_subfreeres ok -@c atomic_decrement_and_test ok +@c atomic_fetch_add_relaxed ok @c td_eventword ok @c td_eventmask ok @c atomic_compare_exchange_bool_acq ok diff --git a/nptl/cond-perf.c b/nptl/cond-perf.c index 9c9488e2743f17eb4beeaadf45ef83a38a9f9bf9..baf69e6d34fdb3ffef97b32fb9db422077a0359b 100644 --- a/nptl/cond-perf.c +++ b/nptl/cond-perf.c @@ -24,7 +24,7 @@ cons (void *arg) do { - if (atomic_decrement_and_test (&ntogo)) + if (atomic_fetch_add_relaxed (&ntogo, -1) == 1) { pthread_mutex_lock (&mut2); alldone = true; diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index 2602dba54e872f36bd54955af7587378e5dc3812..ada3fdd4d440c3fdcf4567734a452c8ba4d0fce1 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -489,7 +489,7 @@ start_thread (void *arg) the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */ atomic_fetch_or_relaxed (&pd->cancelhandling, EXITING_BITMASK); - if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads))) + if (__glibc_unlikely (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) == 1)) /* This was the last thread. */ exit (0); diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h index a9e85f2b098dfc6790d719bba9662e428ab237e6..c10a16b2c4a61e74f6bf18396935b87208df4f7e 100644 --- a/sysdeps/nptl/libc_start_call_main.h +++ b/sysdeps/nptl/libc_start_call_main.h @@ -65,7 +65,7 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL), /* One less thread. Decrement the counter. If it is zero we terminate the entire process. */ result = 0; - if (! atomic_decrement_and_test (&__nptl_nthreads)) + if (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) != 1) /* Not much left to do but to exit the thread, not the process. */ while (1) INTERNAL_SYSCALL_CALL (exit, 0);