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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
