Re: [PATCH 2/5] nptl: Add rseq registration

[Florian Weimer <fweimer@redhat.com>]

* Mathieu Desnoyers:

> [ Adding other kernel rseq maintainers in CC. ]
>
> ----- On Dec 6, 2021, at 12:14 PM, Florian Weimer fweimer@redhat.com wrote:
>
>> * Mathieu Desnoyers:
>> 
>>> ----- On Dec 6, 2021, at 8:46 AM, Florian Weimer fweimer@redhat.com wrote:
>>> [...]
>>>> @@ -406,6 +407,9 @@ struct pthread
>>>>   /* Used on strsignal.  */
>>>>   struct tls_internal_t tls_state;
>>>> 
>>>> +  /* rseq area registered with the kernel.  */
>>>> +  struct rseq rseq_area;
>>>
>>> The rseq UAPI requires that the fields within the rseq_area
>>> are read-written with single-copy atomicity semantics.
>>>
>>> So either we define a "volatile struct rseq" here, or we'll need
>>> to wrap all accesses with the proper volatile casts, or use the
>>> relaxed_mo atomic accesses.
>> 
>> Under the C memory model, neither volatile nor relaxed MO result in
>> single-copy atomicity semantics.  So I'm not sure what to make of this.
>> Surely switching to inline assembly on all targets is over the top.
>> 
>> I think we can rely on a plain read doing the right thing for us.
>
> AFAIU, the plain read does not prevent the compiler from re-loading the
> value in case of high register pressure.
>
> Accesses to rseq fields such as cpu_id need to be done as if those were
> concurrently modified by a signal handler nesting on top of the user-space
> code, with the particular twist that blocking signals has no effect on
> concurrent updates.
>
> I do not think we need to do the load in assembly. I was under the impression
> that both volatile load and relaxed MO result in single-copy atomicity
> semantics for an aligned pointer. Perhaps Paul, Peter, Boqun have something
> to add here ?

The C memory model is broken and does not prevent out-of-thin-air
values.  As far as I know, this breaks single-copy atomicity.  In
practice, compilers will not exercise the latitude offered by the memory
model.  volatile does not ensure absence of data races.

Using atomics or volatile would require us to materialize the thread
pointer, given the current internal interfaces we have, and I don't want
to do this because this is supposed to be performance-critical code.
The compiler barrier inherent to the function call will have to be
enough.  I can add a comment to this effect:

  /* This load has single-copy atomicity semantics (as required for
     rseq) because the function call implies a compiler barrier.  */

Thanks,
Florian