[RFC,v2] pthread_setspecific: Provide signal-safety across keys

  The intent here is to provide signal-safety against callers to
pthread_{get/set}specific that work on different pthread keys,
without hurting performance of the normal use-cases that do not
care about signal-safety.

One thing to keep in mind is that callers of pthread_{get/set}specific
on a given key can disable signals if they require signal-safety
against operations on their key.

The real problem in my situation (lttng-ust tracer) is having
pthread_setspecific (invoked by the application) do memory allocation
and update pointers without disabling signals when it needs to allocate
"level2". This only happens the first time a key >=
PTHREAD_KEY_2NDLEVEL_SIZE is encountered by a thread. If lttng-ust
tracing inserted into a signal handler nests over this allocation, the
application pthread key specific may be lost, and memory leaked.

Also, use directly mmap rather than calloc to ensure signal-safety.

Make just the memory allocation part of pthread_setspecific signal-safe.
Given this is not required very often, it should not cause a significant
overhead.

Document those new non-POSIX async-signal-safety guarantees in
pthread_getspecific and pthread_setspecific user documentation.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Carlos O'Donell" <carlos@redhat.com>
CC: Florian Weimer <fw@deneb.enyo.de>
CC: "Ben Maurer" <bmaurer@fb.com>
CC: libc-alpha@sourceware.org
---
 manual/threads.texi        | 18 ++++++++++--------
 nptl/pthread_create.c      |  3 ++-
 nptl/pthread_setspecific.c | 24 +++++++++++++++++++++---
 3 files changed, 33 insertions(+), 12 deletions(-)
  

On 10/17/2017 04:24 PM, Mathieu Desnoyers wrote:
> The intent here is to provide signal-safety against callers to
> pthread_{get/set}specific that work on different pthread keys,
> without hurting performance of the normal use-cases that do not
> care about signal-safety.

...

I just wanted to note 2 things before we go any further:

(1) We should be able to accept your patch even though you don't
    have a copyright assignment (that I can see) with the FSF.
    The reason for this is that the patch is small and is a limited
    number of lines.

(2) After this patch, because submitted lines of changes are cumulative,
    we are going to need a copyright assignment to accept further changes.

You can view the project Contribution Checklist here:
https://sourceware.org/glibc/wiki/Contribution%20checklist

We'll help you work through the more esoteric aspects of the list, so
don't worry about those. However, I wanted to make the copyright aspect
as clear as possible.

It would be awesome to continue to receive contributions from someone as
experienced as yourself, particularly for parallelism and concurrency issues,
or issues that impact lttng. So if you need any help with assignments, please
feel free to email me and I can help answer any questions as a steward for
the project.
  

* Carlos O'Donell:

> On 10/17/2017 04:24 PM, Mathieu Desnoyers wrote:
>> The intent here is to provide signal-safety against callers to
>> pthread_{get/set}specific that work on different pthread keys,
>> without hurting performance of the normal use-cases that do not
>> care about signal-safety.
>
> ...
>
> I just wanted to note 2 things before we go any further:
>
> (1) We should be able to accept your patch even though you don't
>     have a copyright assignment (that I can see) with the FSF.
>     The reason for this is that the patch is small and is a limited
>     number of lines.

A complete patch will likely be over the threshold, though.  And the
assignment is by no means a guarantee we will accept the patch, of
course.
  

* Mathieu Desnoyers:

> @@ -327,7 +328,7 @@ __nptl_deallocate_tsd (void)
>  	    {
>  	      /* The first block is allocated as part of the thread
>  		 descriptor.  */
> -	      free (level2);
> +	      (void) munmap (level2, PTHREAD_KEY_2NDLEVEL_SIZE * sizeof (*level2));
>  	      THREAD_SETMEM_NC (self, specific, cnt, NULL);

I think __nptl_deallocate_tsd needs to disable signals as well, even
when the level2 pointer is NULL, and keep them disabled until the
thread exits.  This is not a localized change; I don't know if it is
safe.
  

On 18/10/17 06:33, Florian Weimer wrote:
> * Mathieu Desnoyers:
> 
>> @@ -327,7 +328,7 @@ __nptl_deallocate_tsd (void)
>>  	    {
>>  	      /* The first block is allocated as part of the thread
>>  		 descriptor.  */
>> -	      free (level2);
>> +	      (void) munmap (level2, PTHREAD_KEY_2NDLEVEL_SIZE * sizeof (*level2));
>>  	      THREAD_SETMEM_NC (self, specific, cnt, NULL);
> 
> I think __nptl_deallocate_tsd needs to disable signals as well, even
> when the level2 pointer is NULL, and keep them disabled until the
> thread exits.  This is not a localized change; I don't know if it is
> safe.
> 

dtors must not run with signals blocked (posix does not
say that dtors may see changed signal mask)

i think it's the user's fault if there is async
pthread_getspecific call while a thread is exiting.
the user can block the signals before thread exit
if that's the intended behaviour.
  

----- On Oct 18, 2017, at 7:37 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:

> On 18/10/17 06:33, Florian Weimer wrote:
>> * Mathieu Desnoyers:
>> 
>>> @@ -327,7 +328,7 @@ __nptl_deallocate_tsd (void)
>>>  	    {
>>>  	      /* The first block is allocated as part of the thread
>>>  		 descriptor.  */
>>> -	      free (level2);
>>> +	      (void) munmap (level2, PTHREAD_KEY_2NDLEVEL_SIZE * sizeof (*level2));
>>>  	      THREAD_SETMEM_NC (self, specific, cnt, NULL);
>> 
>> I think __nptl_deallocate_tsd needs to disable signals as well, even
>> when the level2 pointer is NULL, and keep them disabled until the
>> thread exits.  This is not a localized change; I don't know if it is
>> safe.
>> 
> 
> dtors must not run with signals blocked (posix does not
> say that dtors may see changed signal mask)

Would it be more acceptable to have a "thread_exiting" flag,
and have pthread_{get,set}specific return NULL and EPERM
respectively when invoked from dtors and from signal handlers
nested over thread teardown ?

> 
> i think it's the user's fault if there is async
> pthread_getspecific call while a thread is exiting.
> the user can block the signals before thread exit
> if that's the intended behaviour.

In the lttng-ust tracer use-case, I cannot change the application.
A simple library could be instrumented, and use lttng-ust for
tracing, without any change to the application. So unfortunately
the solution you are proposing does not work in my case.

Thanks,

Mathieu
  

On 18/10/17 17:31, Mathieu Desnoyers wrote:
> ----- On Oct 18, 2017, at 7:37 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:
>> dtors must not run with signals blocked (posix does not
>> say that dtors may see changed signal mask)
> 
> Would it be more acceptable to have a "thread_exiting" flag,
> and have pthread_{get,set}specific return NULL and EPERM
> respectively when invoked from dtors and from signal handlers
> nested over thread teardown ?
> 
>>
>> i think it's the user's fault if there is async
>> pthread_getspecific call while a thread is exiting.
>> the user can block the signals before thread exit
>> if that's the intended behaviour.
> 
> In the lttng-ust tracer use-case, I cannot change the application.
> A simple library could be instrumented, and use lttng-ust for
> tracing, without any change to the application. So unfortunately
> the solution you are proposing does not work in my case.
> 

instead of adding hacks to the libc and abusing the tsd apis,
why not use tls? in c11 that is as-safe (unfortunately glibc
does not implement this correctly, but e.g. in musl libc all
tls accesses are as-safe so it can be done, with glibc you
would have to use initial-exec tls, so it won't work with
dlopen, but that's a glibc bug)

if you need dtors then the tsd apis can be used for that and
you can block signals in your dtors before cleaning tls state.

(this would be portable to any conforming libc implementation)
  

----- On Oct 19, 2017, at 4:55 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:

> On 18/10/17 17:31, Mathieu Desnoyers wrote:
>> ----- On Oct 18, 2017, at 7:37 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:
>>> dtors must not run with signals blocked (posix does not
>>> say that dtors may see changed signal mask)
>> 
>> Would it be more acceptable to have a "thread_exiting" flag,
>> and have pthread_{get,set}specific return NULL and EPERM
>> respectively when invoked from dtors and from signal handlers
>> nested over thread teardown ?
>> 
>>>
>>> i think it's the user's fault if there is async
>>> pthread_getspecific call while a thread is exiting.
>>> the user can block the signals before thread exit
>>> if that's the intended behaviour.
>> 
>> In the lttng-ust tracer use-case, I cannot change the application.
>> A simple library could be instrumented, and use lttng-ust for
>> tracing, without any change to the application. So unfortunately
>> the solution you are proposing does not work in my case.
>> 
> 
> instead of adding hacks to the libc and abusing the tsd apis,
> why not use tls? in c11 that is as-safe (unfortunately glibc
> does not implement this correctly, but e.g. in musl libc all
> tls accesses are as-safe so it can be done, with glibc you
> would have to use initial-exec tls, so it won't work with
> dlopen, but that's a glibc bug)

I indeed try to use TLS whenever possible. Unfortunately,
I also need to perform lazy thread registration (e.g. add
thread to a linked list for urcu-bp, or register to the kernel
new rseq system call), which needs to be paired with an
unregistration before the thread exits. I found that
pthread_key is a good match for that use-case. The main issue
is its non-signal-safety.

> 
> if you need dtors then the tsd apis can be used for that and
> you can block signals in your dtors before cleaning tls state.
> (this would be portable to any conforming libc implementation)

I'm not sure I understand your statement here. I need to set a
dtor from a signal handler, but AFAIU in the previous statement
you said tsd apis were not async signal safe. Perhaps I
mis-associate "tsd apis" to pthread_{get,set}specific and you
have something else in mind ?

By the way, rather than adding a new "EPERM" return code to
pthread_setspecific to handle the case where it is invoked
from a thread dtors (which would be non-posix and unexpected),
perhaps we should introduce a pthread_setspecific_np() ?

Thanks,

Mathieu
  

On 19/10/17 11:37, Mathieu Desnoyers wrote:
> ----- On Oct 19, 2017, at 4:55 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:
>> if you need dtors then the tsd apis can be used for that and
>> you can block signals in your dtors before cleaning tls state.
>> (this would be portable to any conforming libc implementation)
> 
> I'm not sure I understand your statement here. I need to set a
> dtor from a signal handler, but AFAIU in the previous statement
> you said tsd apis were not async signal safe. Perhaps I
> mis-associate "tsd apis" to pthread_{get,set}specific and you
> have something else in mind ?
> 

well the idea was that you use tls access in signal handler
and setup a tsd dtor in non-signal handler.
(at tls access you can check some flag to see if the tsd
for the thread is initialized or destructed)
but this assumes you can make calls in the thread outside
of a signal handler.

> By the way, rather than adding a new "EPERM" return code to
> pthread_setspecific to handle the case where it is invoked
> from a thread dtors (which would be non-posix and unexpected),
> perhaps we should introduce a pthread_setspecific_np() ?

pthread_setspecific has well defined behaviour in dtors
so EPERM is not ok.

but i dont think _np api for this is appropriate either.
  

----- On Oct 19, 2017, at 8:03 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:

> On 19/10/17 11:37, Mathieu Desnoyers wrote:
>> ----- On Oct 19, 2017, at 4:55 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:
>>> if you need dtors then the tsd apis can be used for that and
>>> you can block signals in your dtors before cleaning tls state.
>>> (this would be portable to any conforming libc implementation)
>> 
>> I'm not sure I understand your statement here. I need to set a
>> dtor from a signal handler, but AFAIU in the previous statement
>> you said tsd apis were not async signal safe. Perhaps I
>> mis-associate "tsd apis" to pthread_{get,set}specific and you
>> have something else in mind ?
>> 
> 
> well the idea was that you use tls access in signal handler
> and setup a tsd dtor in non-signal handler.
> (at tls access you can check some flag to see if the tsd
> for the thread is initialized or destructed)
> but this assumes you can make calls in the thread outside
> of a signal handler.

Unfortuntately, I need to setup the tsd dtor from the
signal handler.

> 
>> By the way, rather than adding a new "EPERM" return code to
>> pthread_setspecific to handle the case where it is invoked
>> from a thread dtors (which would be non-posix and unexpected),
>> perhaps we should introduce a pthread_setspecific_np() ?
> 
> pthread_setspecific has well defined behaviour in dtors
> so EPERM is not ok.
> 
> but i dont think _np api for this is appropriate either.

So far the _np API is the less unappealing solution I have
found to solve the problem at hand. Do you have alternative
solutions to propose ?

Thanks,

Mathieu
  

On 19/10/17 13:59, Mathieu Desnoyers wrote:
> ----- On Oct 19, 2017, at 8:03 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:
>>> By the way, rather than adding a new "EPERM" return code to
>>> pthread_setspecific to handle the case where it is invoked
>>> from a thread dtors (which would be non-posix and unexpected),
>>> perhaps we should introduce a pthread_setspecific_np() ?
>>
>> pthread_setspecific has well defined behaviour in dtors
>> so EPERM is not ok.
>>
>> but i dont think _np api for this is appropriate either.
> 
> So far the _np API is the less unappealing solution I have
> found to solve the problem at hand. Do you have alternative
> solutions to propose ?
> 

the original approach tried to make pthread_setspecific
as-safe as an extension which has non-trivial requirement
on thread exit as discussed:

thread exit retries the tsd dtors until all tsd values
are NULL, if setspecific may run in a signal handler
that means this loop should only stop if all tsd values
are still NULL after signals are masked (and signals
remain masked until the thread exits).

this way when your library is called you can always
do the 'register thread' operation and then the dtor
with 'unregister thread' operation will be run even
if the call happened in a signal interrupting tsd dtors.
(you may need to mask signals in your own dtor though)

currently the dtor retry loop only runs a fixed number
of iterations (dtors of non-NULL tsd values after the
last iteration are not run), but posix allows an infinite
loop there, which would be appropriate for as-safe
setspecific. (i don't know if that breaks anything)

your second approach exposes whether tsd dtors have
started running and then you skip 'register thread'
if it seems the thread is exiting. there are various
ways to expose this information (return value of
pthread_setspecific_np may not be the best one) and
it's not clear if this is useful for others as well
or just for this specific application (which does
not seem to care about library calls that happen
after the thread is exiting)
  

----- On Oct 19, 2017, at 12:55 PM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:

> On 19/10/17 13:59, Mathieu Desnoyers wrote:
>> ----- On Oct 19, 2017, at 8:03 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:
>>>> By the way, rather than adding a new "EPERM" return code to
>>>> pthread_setspecific to handle the case where it is invoked
>>>> from a thread dtors (which would be non-posix and unexpected),
>>>> perhaps we should introduce a pthread_setspecific_np() ?
>>>
>>> pthread_setspecific has well defined behaviour in dtors
>>> so EPERM is not ok.
>>>
>>> but i dont think _np api for this is appropriate either.
>> 
>> So far the _np API is the less unappealing solution I have
>> found to solve the problem at hand. Do you have alternative
>> solutions to propose ?
>> 
> 
> the original approach tried to make pthread_setspecific
> as-safe as an extension which has non-trivial requirement
> on thread exit as discussed:
> 
> thread exit retries the tsd dtors until all tsd values
> are NULL, if setspecific may run in a signal handler
> that means this loop should only stop if all tsd values
> are still NULL after signals are masked (and signals
> remain masked until the thread exits).
> 
> this way when your library is called you can always
> do the 'register thread' operation and then the dtor
> with 'unregister thread' operation will be run even
> if the call happened in a signal interrupting tsd dtors.
> (you may need to mask signals in your own dtor though)
> 
> currently the dtor retry loop only runs a fixed number
> of iterations (dtors of non-NULL tsd values after the
> last iteration are not run), but posix allows an infinite
> loop there, which would be appropriate for as-safe
> setspecific. (i don't know if that breaks anything)
> 
> your second approach exposes whether tsd dtors have
> started running and then you skip 'register thread'
> if it seems the thread is exiting. there are various
> ways to expose this information (return value of
> pthread_setspecific_np may not be the best one) and
> it's not clear if this is useful for others as well
> or just for this specific application (which does
> not seem to care about library calls that happen
> after the thread is exiting)

Good points! This brings me to a third approach that would
fit my use-case, and perhaps be more generic than the
pthread_setspecific_np approach. We could introduce a
new as-safe "query" function, e.g. pthread_exiting(),
which could be called by the application to conditionally
decide not to invoke pthread_setspecific() from dtors and
signal handlers nested over dtors. In applications like a
tracer, or liburcu-bp trying to register a thread, doing
that registration while in the thread dtors is pretty much
useless, so it could be simply skipped.

Thoughts ?

Thanks,

Mathieu
  

On 10/19/2017 12:42 PM, Mathieu Desnoyers wrote:
> Good points! This brings me to a third approach that would
> fit my use-case, and perhaps be more generic than the
> pthread_setspecific_np approach. We could introduce a
> new as-safe "query" function, e.g. pthread_exiting(),
> which could be called by the application to conditionally
> decide not to invoke pthread_setspecific() from dtors and
> signal handlers nested over dtors. In applications like a
> tracer, or liburcu-bp trying to register a thread, doing
> that registration while in the thread dtors is pretty much
> useless, so it could be simply skipped.

APIs that check status usually have lifetime decidability issues
with the objects they check. This is a deep design topic. I'm
not going to delve too deep here.

To be honest what I'd like to see from you before we go down the
deep topic of API design is this:

A detailed writeup of your use case and the behaviour you're trying
to support.

I still don't have a clear grasp on what appears to be two rather
specific requirements for the library and threads it creates.