Re: [PATCH 05/11] nptl: Deallocate the thread stack on setup failure (BZ #19511)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 26/05/2021 14:58, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 26/05/2021 14:33, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> @@ -819,9 +809,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>>>>  	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
>>>>  	   must have been true because thread creation didn't fail, but
>>>>  	   thread attribute setting did.  */
>>>> -	/* See bug 19511 which explains why doing nothing here is a
>>>> -	   resource leak for a joinable thread.  */
>>>> -	assert (stopped_start);
>>>> +        {
>>>> +	  assert (stopped_start);
>>>> +          if (stopped_start)
>>>> +  	    /* State (a), we own PD. The thread blocked on this lock will
>>>> +               finish due setup_failed being set.  */
>>>> +	    lll_unlock (pd->lock, LLL_PRIVATE);
>>>> +        }
>>>>        else
>>>>  	{
>>>>  	  /* State (e) and we have ownership of PD (see CONCURRENCY
>>>
>>> The assert followed by the if doesn't look right.  One should check
>>> setup_failed, I assume.  Is there any way to trigger the broken assert
>>> or the hang from the missing wakeup?
>>
>> This just check what is described in the CONCURRENCY NOTES:
>>
>> 204    Axioms:                                                                                          
>> 205 
>> 206    * The create_thread function can never set stopped_start to false.                               
>> 207    * The created thread can read stopped_start but *never write to it*.   
> 
> I meant this:
> 
> +	  assert (stopped_start);
> +         if (stopped_start)
> 
> It doesn't look right to me.

Yeah, the 'if' check is not really required.  It will hit this code path
only if clone has succeeded ('thread_ran' is true) but either 
sched_setaffinity or sched_setscheduler has failed (in this case
'stopped_start' will be always true).

I changed to:

  if (thread_ran)
    {
      /* State (c) or (d) and we may not have PD ownership (see
         CONCURRENCY NOTES above).  We can assert that STOPPED_START
         must have been true because thread creation didn't fail, but
         thread attribute setting did.  */
      assert (stopped_start);
      /* State (a), we own PD. The thread blocked on this lock will
         finish due setup_failed being set.
         The thread will be responsible to cleanup any allocated
         resource.  */
      lll_unlock (pd->lock, LLL_PRIVATE).
    }

> 
>>> I happend to have looked at this today for the sigaltstack stuff.
>>>
>>> I felt that it would be simpler to perform the kernel-side setup
>>> (affinity and scheduling policy) on the new thread.  Essentially use
>>> stopped_start mode unconditionally.  And use a separate lock word for
>>> the barrier/latch, instead of reusing the pd->lock.
>>>
>>> The advantage is that we can pass an arbitrary temporary object into the
>>> internal start function, such as the signal mask.  The creating thread
>>> blocks until the initialization has happened and the immediate fate of
>>> the thread has been decided.  After that point the creating thread
>>> deallocates the temporary object, and if the thread couldn't start, any
>>> thread-related data.  Otherwise the new thread takes ownership of that
>>> data.
>>>
>>> I think it will be much easier to explain what is going on in the
>>> concurrency notes.  We have an additional synchronization step on all
>>> created threads, but I doubt that this matters in practice.
>>>
>>> What do you think?  I'm not sure if the change should happen as part of
>>> this series.
>>
>> I see two potential issues on this approach:
>>
>>   1. The cpuset information is tied to the pthread_attr_t and can be
>>      potentially large, it means that if we want to make the created
>>      thread to consume we will need to either duplicate, add something
>>      like a reference count, or add more synchronization for the 
>>      consumer.  None is really simpler to implement and I don't think 
>>      it is really an improvement.
> 
> No, I think that's covered by the temporary object thing.  The creating
> thread will block until it's guaranteed that the new thread won't need
> this data anymore.
> 
>>   2. For both schedule parameters and affinity ideally we want to inform
>>      right away to user if the operation has failed.  If we move it to
>>      thread itself, it still would require the synchronization to inform
>>      pthread_create the operation has failed.  I don't see much improvement,
>>      the pthread_create latency will be essentially the same.
> 
> Correct.  But it will be much simpler because we can treat all
> kernel-side initialization the same.  For example, sigaltstack might
> fail with invalid flags, too.  And we'd definitely have to run that on
> the new thread.

The small downside is we will need to consume some of the thread own stack
to pass such information, but it can be only a pointer in any case.

But I still see setting the scheduling/affinity synchronous to have some
advantage: we can fast exit in the case of failure and join the thread
on pthread_create (instead of making it act a detached one).  This make
the failure case not 'leak' the thread context and resource out of
pthread_create.

It might be somewhat complex to code it right now, since we need to take
care of destructor for thread_local variables and thread-local data,
__libc_thread_freeres, etc.

That is also why I used the current unwind mechanism to exit in case of
setup failure (maybe instead of adding the setup_failed on the IS_DETACHED
check the patch could just setup the thread as detached, the pd->joinid
could be set non-atomically since it owns the pd).

In any case, since we do not implement this I don't have a preference.
Your arbitrary temporary object seems ok and should work on the
scheduler/affinity setup.