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 <b...@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 <b...@nicira.com>
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
