[ovs-dev] [PATCH 5/5] lib/cmap: Use atomics for all bucket data.

[Ben Pfaff]

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 <jrajahalme at 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 <blp at nicira.com>