[ovs-dev] [PATCH v3 7/8] lib/ovs-atomic: Native support for x86_64 with GCC.

[Ben Pfaff]

On Tue, Aug 05, 2014 at 01:54:45PM -0700, Jarno Rajahalme wrote:
> On Aug 5, 2014, at 11:35 AM, Ben Pfaff <blp at nicira.com> wrote:
> > 
> > Your research is far stronger than mine on this.  I have only a few
> > comments.
> > 
> > This ignores the possibility of misaligned atomic variables
> > (especially in atomic_is_lock_free()).  All the other existing
> > implementations ignore that possibility, too, which might be entirely
> > fair, so I'm pointing it out only for completeness.
> > 
> 
> If there was a GCC builtin for testing alignment at the compile
> time, it would be great. Do you know if there is one? Maybe it would
> be prudent to add explicit alignment checks on atomic_is_lock_free()
> though.

I don't think there's a great solution, but I don't think it really
matters either.  Nothing in the whole tree uses any
atomic_is_lock_free() or any of the variants.  I only included them
originally for completeness, but I'm not sure what they're good for in
the end.

> Also, on x86, all locked instructions are atomic, regardless of
> alignment, so it would be possible to implement those. However, I?d
> rather flag unaligned atomic variables as an error.

Yeah, I don't think we should use them (or that we try).

> > The xchg instruction always implies a lock prefix, which makes one
> > wonder whether "xchg" is more expensive than unlocked cmpxchg.  Beats
> > me.
> 
> Yes it is. However, we only use ?xchg? or the ?lock? prefix when we
> actually need the locking to make the operation atomic.

OK.

> > I see a few uses of the ORDER argument that might better have
> > parentheses, e.g.:
> >        if (ORDER > memory_order_consume) {
> 
> I added the parenthesis to these, even though we have already
> required (even though we do not enforce it) that the memory order
> argument be compile-time constant so this should not matter that
> much.

Yes, I agree.

> > In the notes (thanks a lot for the notes, by the way), I see:
> > * - Stores are not reordered with other stores, except for special
> > *   instructions (CLFLUSH, streaming stores, string operations).  However,
> > *   these are not emitted by compilers.
> > which makes me worry slightly because compilers do sometimes use the
> > "stos" string instruction for initializing data structures.  Do you
> > know what the deal is with the string instructions?
> 
> After reading some more, I see that the writes performed by a single
> fast string operation (e.g., ?stos?) are allowed to happen
> out-of-order. This could be a problem if the compiler combined an
> atomic release operation with a preceding memset. For example:
> 
>   memset(&foo, 0, 4);
>   atomic_store(&pub, NULL, memory_order_release);
> 
> Here writing a NULL pointer is supposed to act as a release
> operation. If the compiler was smart and combined this all to a
> single ?stos?, the release semantics could be violated (store on
> ?pub? could appear in memory before stores on ?foo?).
> 
> To prevent such reordering in the more normal cases, we issue a
> compiler memory barrier as part of the atomic store-release
> operation. This same memory barrier should tell the compiler that
> the two operations can not be combined to a single ?stos?
> instruction. Also, in practice the pattern where the store-release
> writes the same contents as the preceding data initialization should
> be very rare. So, even if there were compiler bugs w.r.t. not
> considering that the stores of a single fast string operation may
> take place out-of-order, there should not be any concern in
> practice.
> 
> With the memory_order_relaxed we do not issue the barrier, but then
> the order does not matter (I think the store would still be atomic,
> as long as the atomic variable is properly aligned).
> 
> I added a note on this to the comment.

Thanks for the analysis.  That is reassuring.