Re: [PATCH] Make pthread_getspecific async-signal-safe

[Andrew Hunter <ahh at google dot com>]

On Tue, Dec 16, 2014 at 1:05 PM, Torvald Riegel <triegel@redhat.com> wrote:
>
> I agree.
>
> Also, you are introducing concurrency, and we require new glibc code to
> be data-race-free as defined by C11:
> https://sourceware.org/glibc/wiki/Concurrency
>
> Therefore, please use atomic operations where necessary, and document
> how and why that works.
>
> I'm aware that this will be a little challenging given that we don't
> have existing support for sigatomic_t in glibc, and that the C and C++
> standards could specify what signal handlers are allowed to do more
> clearly.
>
> However, we do need to request the compiler to not reorder memory
> accesses

No, we don't.  No ordering constraints are required anywhere in this
patch [1] as I understand it.

> (ISTM you are relying on an ordering of accesses to the seq and
> data fields in this patch).

No; the current code at HEAD relies on this (at least if getspecific
is used from a signal handler); things work fine if we end up writing
seq then data, but writing data then seq leads to loss of values. My
patch eliminates that dependence.  (Racy get during a set might see
the old value or NULL, but that's going to be a possible outcome for
any racy get.)

We could instead use a patch that in fact enforces the correct order
in setspecific, and it wouldn't be particularly difficult once we
decide, essentially, how to write barriers.  But my patch should
obviate the need for any such decision (for now :)).

Strictly speaking as I understand the standard, any objects read need
to be of atomic types--though relaxed loads would be sufficient
throughout--but it's very unclear to me if that's worth doing here.
The only changes would be essentially erasing the types of the members
of pthread_key_data so we can "atomically" access them--do you really
see that as necessary here?  I'm aware of the new code/old code
distinctions in glibc, but consider for example sighandler_setxid - do
you want to rewrite struct pthread.pid to atomic so it can be read
there?  Let's not even talk about __pthread_unwind (called from the
cancellation signal...).

Given that there are no actual ordering constraints, do we still need
to respect the need for "atomics" here?

> The simple thing would be to use normal
> atomics with acquire/release memory order, but we don't want this
> overhead for TLS.  So we will need something else.  I'd suggest
> exploring two options: (1) relaxed memory order atomics and explicit
> compiler barriers or (2) atomic operations for sigatomic_t with
> something like acq/rel barriers.  The latter might be cleaner and more
> intuitive to use, but would need more invention.
>


I'll have to consult some more standardese experts, but I think the
correct way to do this would be an
atomic_signal_fence(release)(http://naipc.uchicago.edu/2014/ref/cppreference/en/c/atomic/atomic_signal_fence.html)
in setspecific between level2->seq =  seq and level2->data = value,
but the linked documentation says to prefer local variants to C11 APIs
and you don't seem to have one for this.

So I suggest we stick with the approach in this patch.

[1] Strictly speaking once we do any sort of non-atomic access to
THREAD_SELF in a signal handler it's entirely game over, but that's a
whole other issue.  Given that's a non-portable macro definition
calling into assembly, I think that's going to be fine so long as
arches define it properly (x86 does.)