This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] [BZ 18034] [AArch64] Lazy TLSDESC relocation data race fix
- From: Szabolcs Nagy <szabolcs dot nagy at arm dot com>
- To: Torvald Riegel <triegel at redhat dot com>
- Cc: "libc-alpha at sourceware dot org" <libc-alpha at sourceware dot org>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>
- Date: Wed, 27 May 2015 12:27:38 +0100
- Subject: Re: [PATCH] [BZ 18034] [AArch64] Lazy TLSDESC relocation data race fix
- Authentication-results: sourceware.org; auth=none
- References: <553793A3 dot 7030206 at arm dot com> <1429718899 dot 6557 dot 17 dot camel at triegel dot csb> <553E5381 dot 504 at arm dot com> <1432672677 dot 26239 dot 41 dot camel at triegel dot csb>
On 26/05/15 21:37, Torvald Riegel wrote:
> On Mon, 2015-04-27 at 16:19 +0100, Szabolcs Nagy wrote:
>> On 22/04/15 17:08, Torvald Riegel wrote:
>>> On Wed, 2015-04-22 at 13:27 +0100, Szabolcs Nagy wrote:
>>>> Lazy TLSDESC initialization needs to be synchronized with
>> concurrent TLS
>>>> accesses.
>>>
>>> Please get familiar with
>> https://sourceware.org/glibc/wiki/Concurrency
>>> Fixes to existing code should use the C11 memory model for
>> concurrent
>>> code, especially if the fix is about a concurrency issue.
>>>
>>> I agree with Adhemerval that a release MO store seems to be
>> sufficient
>>> in this case.
>>
>> I used a fence instead of the suggested atomic store, because I
>> think this part of the concurrency wiki is problematic if glibc
>> ever tries to move to C11:
>>
>> "We (currently) do not use special types for the variables accessed
>> by atomic operations, but the underlying non-atomic types with
>> suitable alignment on each architecture."
>
> No, I don't see how it would be problematic. Why do you think this is
> the case?
ok, i was thinking about type system issues but that's
probably not relevant.
(_Atomic int* is incompatible with int* and changing
the type everywhere to _Atomic might not work if the
type is externally visible like pthread types).
> This should have relaxed atomic accesses for all things concurrently
> accessed. As a result, you can drop the volatile qualification I
> believe (I haven't checked, but it seems this was a pre-memory-model way
> to avoid compiler reordering -- it's not actually observable behavior
> that we'd need it for).
i guess volatile prevents reordering wrt other volatiles
(but not wrt other non-volatiles).
the other use of volatile is to prevent spurious loads, eg.
x = *p;
y = x;
z = x;
can be "optimized" into
y = *p;
z = *p;
or even
y = z = *p & *p;
if every access to *p is changed to use an atomic function
then dropping volatile is ok, but in this case td is passed
around and used in _dl_tlsdesc_resolve_early_return_p too,
so i'm not sure if it's ok to remove volatile yet.
is it ok if that's fixed as a separate commit?
> The release store would be clear, but you can use the release fence as
> well.
>
ok, i'll use release store then.
>> +_dl_tlsdesc_return_lazy:
>> + /* The ldar guarantees ordering between the load from [x0] at
>> the
>> + call site and the load from [x0,#8] here for lazy
>> relocation. */
>
> You should point out where the matching release MO operation is. For
> example, you could say somethign along the lines of:
>
> "The ldar ensures that we synchronize-with with the release MO store in
> _dl_tlsdesc_resolve_rela_fixup; as a result, the load from [x0,#8] will
> happen after the initialization of td->arg in
> _dl_tlsdesc_resolve_rela_fixup."
>
ok
>> + /* Store-store barrier so the two writes are not reordered. */
>
> Say that this release MO store is meant to synchronize with the acquire
> MO operation in _dl_tlsdesc_undefweak.
ok
> You should also document why the relaxed MO load in
> _dl_tlsdesc_resolve_early_return_p is sufficient (see the other part of
> the email thread -- I now see what you meant but this isn't obvious at
> all). The acquire MO operations that make it work are added by you in
> this patch.
ok,
but i think that should be fixed separately
together with other archs.