This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/4] Add atomic operations similar to those provided by C11.
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Torvald Riegel <triegel at redhat dot com>
- Cc: GLIBC Devel <libc-alpha at sourceware dot org>
- Date: Thu, 30 Oct 2014 20:12:48 +0000
- Subject: Re: [PATCH 2/4] Add atomic operations similar to those provided by C11.
- Authentication-results: sourceware.org; auth=none
- References: <1414617613 dot 10085 dot 23 dot camel at triegel dot csb> <1414619416 dot 10085 dot 46 dot camel at triegel dot csb> <Pine dot LNX dot 4 dot 64 dot 1410292156440 dot 15119 at digraph dot polyomino dot org dot uk> <1414622734 dot 10085 dot 76 dot camel at triegel dot csb> <Pine dot LNX dot 4 dot 64 dot 1410292257040 dot 15119 at digraph dot polyomino dot org dot uk> <1414661232 dot 10085 dot 89 dot camel at triegel dot csb> <Pine dot LNX dot 4 dot 64 dot 1410301711530 dot 2316 at digraph dot polyomino dot org dot uk> <1414693238 dot 10085 dot 150 dot camel at triegel dot csb>
On Thu, 30 Oct 2014, Torvald Riegel wrote:
> > See for example elf/dl-lookup.c:do_lookup_x, using an asm to ensure reads
> > happen in a particular order:
> >
> > size_t n = scope->r_nlist;
> > /* Make sure we read the value before proceeding. Otherwise we
> > might use r_list pointing to the initial scope and r_nlist being
> > the value after a resize. That is the only path in dl-open.c not
> > protected by GSCOPE. A read barrier here might be to expensive. */
> > __asm volatile ("" : "+r" (n), "+m" (scope->r_list));
> > struct link_map **list = scope->r_list;
>
> That ensures that the compiler won't reorder the reads -- but it does
> *not* ensure that reads happen in a particular order when executed on an
> arch with a weak memory model. If we don't care about HW reordering, we
> won't need the asm (unless perhaps we communicate with external stuff
> through volatile, or do something behind the compiler's back), because
> the language semantics for sequential code are taking care of that.
>
> Thus, my guess is that this is a bug.
My guess is that on the system where a problem reordering was observed, it
was sufficient to stop the compiler from reordering the loads (but there
might be issues for some other system). And this sort of code, where
there aren't other atomic operations nearby, is a case where we'd need to
be particularly careful about checking the performance impact of inserting
barriers.
> > > Can we at least agree on having all glibc code use our own
> > > atomic_store_relaxed / atomic_load_relaxed (see the patch)? Then we can
> > > still argue whether to use __atomic* to implement these. But at least
> > > we can easily switch to __atomic in the future. Any objections?
> >
> > I think explicit atomic loads / stores are fine, subject to checking code
> > generation before making them use __atomic_*.
>
> How do you envision to do this?
>
> Compare generated code on all archs before and after a change, for each
> algorithm on its own?
Comparing code for one architecture, at the time of each conversion, seems
appropriate (with additional benchmarking for cases that don't otherwise
involve atomics).
--
Joseph S. Myers
joseph@codesourcery.com