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

Jarno Rajahalme jrajahalme at nicira.com
Tue Aug 5 20:54:45 UTC 2014


Thanks for the reviews!

I have pushed the series to master, some answers/comments below,

 Jarno

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.

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.

I’ll push the patches without these changes, we can improve on this regard later.

> 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.

> 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.

> 
> 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.

> Acked-by: Ben Pfaff <blp at nicira.com>

  Jarno


More information about the dev mailing list