On Tue, Aug 05, 2014 at 01:54:45PM -0700, Jarno Rajahalme wrote:
> 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.
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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
