[PATCH v5 2/3] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Thu Dec 4 16:36:39 GMT 2025
On 03/12/25 15:10, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
>
>>>> + unsigned int prevstate;
>>>> + do
>>>> + prevstate = atomic_load_relaxed (&pd->joinstate);
>>>> + while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate,
>>>> + THREAD_STATE_EXITING));
>>>> +
>>>
>>> Why acquire MO? I think even if the kernel uses a release store, it's
>>> not exactly clear what we are synchronizing with.
>>>
>>> And this looks like an unconditional atomic_exchange.
>>
>> My understanding here in since this code compete with the
>> pthread_detach (issued by a different thread), the CAS is need to
>> determine which is one responsible to free the TCB: either the thread
>> itself (below with __nptl_free_tcb), or pthread_detach with
>> __pthread_join() call.
>>
>> I am not sure about using an unconditional atomic_exchange, it means
>> will need to change pthread_detach() to ignore THREAD_STATE_EXITING
>> state and call __nptl_free_tcb unconditionally. And ignoring
>> THREAD_STATE_EXITINGT makes the sysdeps/pthread/tst-detach1.c usage
>> tricky to handle.
>
> But isn't the loop above an unconditional exchange? I thought that
> atomic_compare_exchange_weak_acquire helpfully updates prevstate to the
> value after the failed compare. If you don't want to overwrite other
> states, you need to re-check prevstate before retrying the CAS.
It is an I agree that we can use an atomic exchange here. I got confused
because I haven't clicked that atomic_exchange returns the current value.
>
>> And I use the acquire because this is the current practice for concurrent
>> implementations, like __new_sem_wait_fast. But I am not really an concurrency
>> expert so I am not fully sure if this is the correct MO here.
>
> Yeah, I found this concurrency stuff confusing as well.
>
>>>> @@ -706,7 +725,9 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>>> /* Initialize the field for the ID of the thread which is waiting
>>>> for us. This is a self-reference in case the thread is created
>>>> detached. */
>>>> - pd->joinid = iattr->flags & ATTR_FLAG_DETACHSTATE ? pd : NULL;
>>>> + pd->joinstate = iattr->flags & ATTR_FLAG_DETACHSTATE
>>>> + ? THREAD_STATE_DETACHED
>>>> + : THREAD_STATE_JOINABLE;
>>>
>>> There's also an assignment in create_thread above.
>>
>> Do mean the '.child_tid = (uintptr_t) &pd->joinstate,' in clone setup?
>> If so, my undertanding is kernel will only set the value at thread
>> termination.
>
> I left that in my review by mistake, the referenced assignment was for
> the failed case only, so not actually redundant.
Ack.
>
>>> Can we put in an assert that checks for prevstate ==
>>> PTHREAD_STATE_EXITING || prevstate == PTHREAD_STATE_EXITED? Or would
>>> that reveal to many bugs in existing applications?
>>
>> I am not sure, I tend to avoid adding assert on pthread functions because we
>> do support some UB (as defined by POSIX) so we can't guarantee that programs
>> do not rely on such semantics.
>
> Okay.
>
>>>> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
>>>> index 9109b62276..f0ae5edbcb 100644
>>>> --- a/nptl/pthread_join_common.c
>>>> +++ b/nptl/pthread_join_common.c
>>>> @@ -22,116 +22,78 @@
>>>> #include <time.h>
>>>> #include <futex-internal.h>
>>>>
>>>> +/* Check for a possible deadlock situation where the threads are waiting for
>>>> + each other to finish. Note that this is a "may" error. To be 100% sure we
>>>> + catch this error we would have to lock the data structures but it is not
>>>> + necessary. In the unlikely case that two threads are really caught in this
>>>> + situation they will deadlock. It is the programmer's problem to figure
>>>> + this out. */
>>>> +static inline bool
>>>> +check_for_deadlock (struct pthread *pd)
>>>> {
>>>> + struct pthread *self = THREAD_SELF;
>>>> + return ((pd == self
>>>> + || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED
>>>> + && (pd->cancelhandling
>>>> + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
>>>> + | TERMINATED_BITMASK)) == 0))
>>>> + && !cancel_enabled_and_canceled (self->cancelhandling));
>>>> }
>>>
>>> Why acquire MO?
>>>
>>> Is the condition really correct (beyond pd ==self)? Why is
>>> self->joinstate == THREAD_STATE_DETACHED treated differently here?
>>>
>>> I think with pd->joinid gone, there really isn't much we can check here
>>> anymore. Maybe just stick to pd == self and do away with this separate
>>> function altogether?
>>
>> Ack, I am not really found of this 'may' deadlock check and unfortunately
>> now that is backed on glibc semantic I did not want to changed it too
>> much.
>
> Sorry, what do you mean?
I meant that I do not really like this really complex deadlock check
and your suggestion seems better.
>
>>>> int
>>>> __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
>>>> clockid_t clockid,
>>>> + const struct __timespec64 *abstime)
>>>
>>>> + int result = 0;
>>>> + unsigned int state;
>>>> + while ((state = atomic_load_acquire (&pd->joinstate))
>>>> + != THREAD_STATE_EXITED)
>>>> {
>>>> + if (check_for_deadlock (pd))
>>>> + return EDEADLK;
>>>> +
>>>> + /* POSIX states calling pthread_join on a non joinable thread is
>>>> + undefined. However, if PD is still in the cache we can warn
>>>> + the caller. */
>>>> + if (state == THREAD_STATE_DETACHED)
>>>> + return EINVAL;
>>>> +
>>>> + /* pthread_join is a cancellation entrypoint and we use the same
>>>> + rationale for pthread_timedjoin_np.
>>>> +
>>>> + The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
>>>> + a memory zeroing and futex wake-up when the process terminates.
>>>> + The futex operation is not private. */
>>>> + int ret = __futex_abstimed_wait_cancelable64 (&pd->joinstate, state,
>>>> + clockid, abstime,
>>>> + LLL_SHARED);
>>>> + if (ret == ETIMEDOUT || ret == EOVERFLOW)
>>>> + {
>>>> + result = ret;
>>>> + break;
>>>> }
>>>> }
>>>>
>>>> void *pd_result = pd->result;
>>>> if (__glibc_likely (result == 0))
>>>> {
>>>> if (thread_return != NULL)
>>>> *thread_return = pd_result;
>>>>
>>>> /* Free the TCB. */
>>>> __nptl_free_tcb (pd);
>>>> }
>>>
>>> I'm trying to understand if this leaks the TCB on cancellation. As far
>>> as I can tell, phtread_join needs to be called again if it gets
>>> canceled, so this should be okay.
>>>
>>> For synchronization purposes, rather than relying on acquire loads on
>>> joinstate (where the kernel may or may not perform a release store),
>>> maybe we should use an acquire load on pd->result (and a corresponding
>>> release store)?
>>
>> I am not sure, the 'joinstate' allows us to determine which is the thread
>> responsible to free the TCB state; using 'result' will require additional
>> synchronization and some extra care on its state to certify that we can
>> assume which thread can actually free the TCB.
>
> Sorry, I meant something else: We need to take steps so that the return
> from the thread start routine (or the pthread_exit call) synchronizes
> with the pthread_join call. Doing this via joinstate only works if the
> kernel performs a release store (which isn't specified in the clone(2)
> manual page). We could do a release store on pd->result ourselves and
> an acquire load from pd->result in the thread calling pthread_join.
>
> But this is a distraction because we've always relayed on this CLEARTID
> release semantics. The change here is just the name of the field.
So checking the kernel source code I think we can rely that it does
issue a release store:
kernel/fork.c
1422 /*
1423 * Signal userspace if we're not exiting with a core dump
1424 * because we want to leave the value intact for debugging
1425 * purposes.
1426 */
1427 if (tsk->clear_child_tid) {
1428 if (atomic_read(&mm->mm_users) > 1) {
1429 /*
1430 * We don't check the error code - if userspace has
1431 * not set up a proper pointer then tough luck.
1432 */
1433 put_user(0, tsk->clear_child_tid);
1434 do_futex(tsk->clear_child_tid, FUTEX_WAKE,
1435 1, NULL, NULL, 0, 0);
1436 }
1437 tsk->clear_child_tid = NULL;
1438 }
1439
Although I am not sure if this is properly documented.
>
>>>> diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
>>>> index 54b528fd19..e9a10aee53 100644
>>>> --- a/nptl/pthread_tryjoin.c
>>>> +++ b/nptl/pthread_tryjoin.c
>>>> @@ -21,15 +21,18 @@
>>>> int
>>>> __pthread_tryjoin_np (pthread_t threadid, void **thread_return)
>>>> {
>>>> + /* The joinable state (THREAD_STATE_JOINABLE) is straightforward: the thread
>>>> + hasn't finished yet, so trying to join might block.
>>>> +
>>>> + The exiting thread (THREAD_STATE_EXITING) also might result in a blocking
>>>> + call: a detached thread might change its state to exiting, and an exiting
>>>> + thread might take some time to exit (and thus let the kernel set the
>>>> + state to THREAD_STATE_EXITED). */
>>>>
>>>> + struct pthread *pd = (struct pthread *) threadid;
>>>> + return atomic_load_acquire (&pd->joinstate) != THREAD_STATE_EXITED
>>>> + ? EBUSY
>>>> + : __pthread_clockjoin_ex (threadid, thread_return, 0, NULL);
>>>> }
>>>> versioned_symbol (libc, __pthread_tryjoin_np, pthread_tryjoin_np, GLIBC_2_34);
>>>
>>> Pre-existing issue: pthread_tryjoin_np cannot be a cancellation point
>>> because it is declared _THROW.
>>
>> Before the patch we have the 'block' argument o specify whether to call
>> __futex_abstimed_wait_cancelable64, now if __pthread_clockjoin_ex is called
>> 'joinstate' is assumed to be THREAD_STATE_EXITED.
>>
>> The pthread_detach will only change the 'joinstate' if previous state iff
>> THREAD_STATE_JOINABLE, so even concurrent pthread_detach will not change this
>> invariant.
>
> Ahh, so you are saying we never call __futex_abstimed_wait_cancelable64?
Yes, I will add comment on pthread_tryjoin_np.
More information about the Libc-alpha
mailing list