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

[Florian Weimer <fweimer@redhat.com>]

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

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.

Thanks,
Florian