On Thu, May 22, 2014 at 05:37:42PM -0700, Jarno Rajahalme wrote:
> The documentation of the memory order argument of atomic operations
> states that memory loads after an atomic load with a
> memory_order_acquire cannot be moved before the atomic operation.
> This still leaves open the possibility that non-atomic loads before
> the atomic load could be moved after it, hence breaking down the
> synchronization used for cmap_find().
>
> This patch fixes this my using atomics (with appropriate memory_order)
> also for the struct cmap_node pointer and hash values.
>
> struct cmap_node itself is used for the pointer, since it already
> contains an RCU pointer (which is atomic) for a struct cmap_node.
> This also helps simplify some of the code previously dealing
> separately with the cmap_node pointer in the bucket v.s. a cmap_node.
>
> Hash values are also accessed using atomics. Otherwise it might be
> legal for a compiler to read the hash values once, and keep using the
> same values through all the retries.
>
> These should have no effect on performance.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
Why is cmap_get_hash__() a macro instead of an inline function? At
any rate, we can't use the ({ ... }) GCC extension in code compiled
everywhere.
I don't really like the look of this:
+ for (struct cmap_node *iter = &b->nodes[slot];;) {
Would you mind writing it as this?
+ struct cmap_node *iter = &b->nodes[slot];
+ for (;;) {
I think this makes the code uglier but it also seems to put the memory
ordering issues to rest.
Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
