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: Torvald Riegel <triegel at redhat dot com>
- To: Szabolcs Nagy <szabolcs dot nagy at arm 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 15:02:42 +0200
- 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> <5565AA2A dot 7010509 at arm dot com>
On Wed, 2015-05-27 at 12:27 +0100, Szabolcs Nagy wrote:
> On 26/05/15 21:37, Torvald Riegel wrote:
> > 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 atomic ops will take care of this (or, allow one to take care of
this).
> 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;
Yes, that's allowed for non-atomic non-volatile (either due to the
data-race-freedom requirement for non-atomic, or due to no volatile
constraints regarding equality to abstract machine). If the load from
*p were even a memory_order_relaxed atomic load, the compiler would not
be allowed to re-load.
> 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.
Yes, the clean thing to do would be to change it everywhere at once.
> is it ok if that's fixed as a separate commit?
Yes, if you promise to submit such a patch :)
Please still use mo_relaxed atomic accesses though, so that we can take
care of the DRF requirement for the code you changed.
> > 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.
>
If you want to merge that into, for example, using a common tlsdesc.c,
then that's fine with me. I just don't want this information to get
lost; I think it's non-obvious and thus should be documented.