[zeromq-dev] atomic ops

[Martin Lucina]

John,

skaller at users.sourceforge.net said:
> I am looking at the atomic ops stuff, and I find this a bit suspicious,
> and also suboptimal:
> 
> #elif (defined __i386__ || defined __x86_64__) && defined __GNUC__
> #define ZMQ_ATOMIC_COUNTER_X86

Sure, it's suboptimal for a build on non-x86 (x86_64) and not using GCC (or
compilers pretending to be GCC, such as at least Intel icc).

> #elif defined ZMQ_ATOMIC_COUNTER_X86
>             integer_t oldval = -decrement;
>             volatile integer_t *val = &value;
>             __asm__ volatile ("lock; xaddl %0,%1"
>                 : "=r" (oldval), "=m" (*val)
>                 : "0" (oldval), "m" (*val)
>                 : "cc", "memory");
>             return oldval != decrement;
> 
> 
> First, I surely wouldn't trust inline assembler, and I'm suspicious of
> "lock; xadd". This code also won't be selected except on x86 or x86_64.

What exactly do you find suspicious about "lock; xadd"? Standard way to
implement atomics on x86.

> Wouldn't it be better to use this function:
> 
> type __sync_fetch_and_add (type *ptr, type value, ...)

Yes.

> which is guaranteed to work on all processors?
> 
> I'd trust gcc to do the right thing a lot more than inline assembler.
> That function is always available if __GNUC__ is set, on recent
> enough implementations of gcc. The spec comes from gcc 4.1.2.
> [Presumably config can double check..]

When I last reviewed that code I considered using the gcc intrinsics, but
the are only available since GCC 4.<something> and there are still many
people using older compilers. It takes several *years* for a new compiler
feature to trickle down to users.

So it'd have to be optional, feel free to implement it but you'll need to
get the autoconf-fu working to not break performance for users with older
gcc.

It's a shame unixy OSes aren't doing the right thing and collaborating to
provide a common <atomic.h> interface to userspace like NetBSD and Solaris
do (both of which the zmq atomics code supports).

-mato