[PATCH v5 2/3] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)

[Florian Weimer]

* 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.

> 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.

>> 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?

>>>  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.

>>> 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?

Thanks,
Florian