Message ID | 87pns7x40z.fsf@oldenburg2.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | nptl: Use C11-style atomics for access to __nptl_nthreads | expand |
On 04/02/2019 08:05, Florian Weimer wrote: > 2019-02-03 Florian Weimer <fweimer@redhat.com> > > * nptl/allocatestack.c (__reclaim_stacks): Use C-11-style atomic > access for __nptl_nthreads. > * nptl/pthread_create.c (__nptl_deallocate_tsd): Likewise. > (START_THREAD_DEFN): Likewise. > (__pthread_create_2_1): Likewise. > > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index 670cb8ffe6..0dbb155f70 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -952,7 +952,7 @@ __reclaim_stacks (void) > list_add (&self->list, &stack_used); > > /* There is one thread running. */ > - __nptl_nthreads = 1; > + atomic_store_relaxed (&__nptl_nthreads, 1); > > in_flight_stack = 0; > Ok. > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 04d1c6453e..15fbeceac7 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -341,7 +341,7 @@ __nptl_main_thread_exited (void) > { > __nptl_deallocate_tsd (); > > - if (! atomic_decrement_and_test (&__nptl_nthreads)) > + if (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) > 1) > /* Exit the thread, but not the process. */ > __exit_thread (); > } It changes the semantic from acquire to relaxed. I think we should document why it is safe. > @@ -510,7 +510,7 @@ START_THREAD_DEFN > /* If this is the last thread we terminate the process now. We > do not notify the debugger, it might just irritate it if there > is no thread left. */ > - if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads))) > + if (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) == 1) > /* This was the last thread. */ > exit (0); I am not sure if is the correct semantic. Using a similar analogy, c++ share_ptr uses memory_order_acq_rel for the decrement. Are we sure relaxed semantic is correct here? > > @@ -769,7 +769,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, > collect_default_sched (pd); > } > > - if (__glibc_unlikely (__nptl_nthreads == 1)) > + if (__glibc_unlikely (atomic_load_relaxed (&__nptl_nthreads) == 1)) > _IO_enable_locks (); > > /* Pass the descriptor to the caller. */ > @@ -785,7 +785,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 > @@ -850,7 +850,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, > NOTES above). */ > > /* Oops, we lied for a second. */ > - atomic_decrement (&__nptl_nthreads); > + atomic_fetch_add_relaxed (&__nptl_nthreads, -1); > > /* Perhaps a thread wants to change the IDs and is waiting for this > stillborn thread. */ >
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index 670cb8ffe6..0dbb155f70 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -952,7 +952,7 @@ __reclaim_stacks (void) list_add (&self->list, &stack_used); /* There is one thread running. */ - __nptl_nthreads = 1; + atomic_store_relaxed (&__nptl_nthreads, 1); in_flight_stack = 0; diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index 04d1c6453e..15fbeceac7 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -341,7 +341,7 @@ __nptl_main_thread_exited (void) { __nptl_deallocate_tsd (); - if (! atomic_decrement_and_test (&__nptl_nthreads)) + if (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) > 1) /* Exit the thread, but not the process. */ __exit_thread (); } @@ -510,7 +510,7 @@ START_THREAD_DEFN /* If this is the last thread we terminate the process now. We do not notify the debugger, it might just irritate it if there is no thread left. */ - if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads))) + if (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) == 1) /* This was the last thread. */ exit (0); @@ -769,7 +769,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, collect_default_sched (pd); } - if (__glibc_unlikely (__nptl_nthreads == 1)) + if (__glibc_unlikely (atomic_load_relaxed (&__nptl_nthreads) == 1)) _IO_enable_locks (); /* Pass the descriptor to the caller. */ @@ -785,7 +785,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 @@ -850,7 +850,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, NOTES above). */ /* Oops, we lied for a second. */ - atomic_decrement (&__nptl_nthreads); + atomic_fetch_add_relaxed (&__nptl_nthreads, -1); /* Perhaps a thread wants to change the IDs and is waiting for this stillborn thread. */