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

[Florian Weimer <fweimer@redhat.com>]

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

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

Thanks,
Florian