[PATCH] Configure: Prefer gcc __atomic builtins instead of older __sync builtins

[Maxim Dounin]

Hello!

On Sun, Dec 10, 2017 at 02:58:17PM +0000, debayang.qdt wrote:

> > On Mon, Dec 04, 2017 at 04:37:50PM +0000, debayang.qdt wrote:
> > 
> > > For some architectures like armv8a  - newer GCC generates a full 
> > > barrier for the __sync operations compared to the __atomics .
> > > 
> > > This is seen to give some performance lag on these architectures when 
> > > using __sync compared to the atomics apis under high contention.
> > > 
> > > The C++ atomic ops looks good as well 
> > > (http://mailman.nginx.org/pipermail/nginx-devel/2016-September/008805.html), 
> > > However I would like to test it out and confirm.
> > > 
> > > e.g   sync_fetch_add with newer GCC:
> > > 
> > >   58:   f94007e0        ldr     x0, [sp,#8]
> > >   5c:   c85f7c01         ldxr    x1, [x0]
> > >   60:   91000821        add     x1, x1, #0x2
> > >   64:   c802fc01         stlxr   w2, x1, [x0]
> > >   68:   35ffffa2           cbnz    w2, 5c <testing+0xc>
> > >   6c:   d5033bbf        dmb     ish   
> > > 
> > > With atomics_fetch_add  with SEQ_CST:
> > > 
> > >   58:   f94007e0        ldr     x0, [sp,#8]
> > >   5c:   c85ffc01          ldaxr   x1, [x0]
> > >   60:   91000821       add     x1, x1, #0x2
> > >   64:   c802fc01        stlxr   w2, x1, [x0]
> > >   68:   35ffffa2          cbnz    w2, 5c <testing+0xc>
> >  
> > Well, this may actualy mean that the __atomic and stdatomic 
> > variants won't work for us, as it does not seem to imply a 
> > barrier protecting other variables.  While it may not be 
> > important for many uses of ngx_atomic_fetch_add(), it is 
> > certainly important for ngx_atomic_cmp_set() we use for shared 
> > memory mutexes, where it is assumed to be a full barrier at 
> > least for the memory area the mutex protects.
> >  
> > (Just for the record, the GCC change in question seems to be 
> > documented at 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65697.)
>  
> Thanks for the link.  As per the above discussion the legacy 
> sync calls were fixed to generate a barrier in gcc 5+ for 
> aarch64 platform - to conform with the __sync full barrier 
> specs.
>  
> IMO if the weak variants or _atomic helps multiple cases as you 
> mentioned - it still may make sense,
> rather than using sync calls always - as a catch all 
> synchronization mechanism, because it is costly on some 
> architectures.

I probably wasn't clear enough.  I mean to say that barrier 
semantics is not important for ngx_atomic_fetch_add(), at least 
for most of the uses.  Whether it helps or not is completely 
different question.  In practice we haven't seen any performance 
issues due to ngx_atomic_fetch_add().  We do them relatively 
rarely (at most several operations per request), and full barrier 
shouldn't be a problem.

> For the specific scenarios where we need strong memory order 
> requirements we can still use the atomics with explicit seq/cst 
> memory model and standalone fence along with it depending on 
> context.

Using separate barriers might result in worse performance, 
especially on i386 / amd64, where a barrier is generally implied 
by the operation itself.  And this will require looking through 
all the code to understand whether or not barriers are actually 
required in a particular context.

Actually, initially I was under the impression that C11 atomics 
will work for us out of the box.  But the GCC bug in question 
makes me think that it might not be the case.  This needs 
additional investigation.

Also, the patch for C11 atomics I've referenced earlier doesn't 
try to introduce load/store macros.  This seems to be required for 
at least some implementations though - for example, stdatomic.h on 
FreeBSD when used with gcc48.  So the patch needs more work.

Given all of the above, it is not clear if it at all worth the 
effort - as __sync atomics work fine for us with no known 
problems.

-- 
Maxim Dounin
http://mdounin.ru/