This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] nptl: Use C11-style atomics for access to __nptl_nthreads
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Fri, 15 Feb 2019 17:18:31 -0200
- Subject: Re: [PATCH] nptl: Use C11-style atomics for access to __nptl_nthreads
- References: <87pns7x40z.fsf@oldenburg2.str.redhat.com>
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. */
>