[ovs-dev] [PATCH] cmap: Fix memory ordering for counter_changed().

Jarno Rajahalme jrajahalme at nicira.com
Wed May 21 19:39:11 UTC 2014


Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

I still worry about reordering of the non-atomic reads in between the read_counter() calls, as in principle the acquire barrier does not prevent loads before such a barrier to be moved after it. Thus, this could happen:

for (;;) {
    c = read_even_counter()
    check = read_counter()
    read hash value
    read camp_node pointer
} while (c != check)

While we need something like this:

for (;;) {
    c = read_even_counter()
    atomic_thread_fence(memory_order_acquire);
    read hash value
    read camp_node pointer
    atomic_thread_fence(memory_order_acquire);
    check = read_counter()
} while (c != check)

to guarantee that the hash and node values are read after the even counter and before the check.

I’ll post a later patch using atomic_thread_fence(memory_model_acquire) to address this.

  Jarno

On May 21, 2014, at 7:43 AM, Ben Pfaff <blp at nicira.com> wrote:

> Release memory ordering only affects visibility of stores, and is not
> allowed on a memory read.  Some compilers enforce this, making this code
> fail to compile.
> 
> Reported-by: Alex Wang <alexw at nicira.com>
> Reported-by: Kmindg G <kmindg at gmail.com>
> CC: Jarno Rajahalme <jrajahalme at nicira.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> lib/cmap.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/cmap.c b/lib/cmap.c
> index cfd66fb..1eb79b4 100644
> --- a/lib/cmap.c
> +++ b/lib/cmap.c
> @@ -245,11 +245,11 @@ cmap_is_empty(const struct cmap *cmap)
> }
> 
> static uint32_t
> -read_counter(struct cmap_bucket *bucket, memory_order order)
> +read_counter(struct cmap_bucket *bucket)
> {
>     uint32_t counter;
> 
> -    atomic_read_explicit(&bucket->counter, &counter, order);
> +    atomic_read_explicit(&bucket->counter, &counter, memory_order_acquire);
>     return counter;
> }
> 
> @@ -259,7 +259,7 @@ read_even_counter(struct cmap_bucket *bucket)
>     uint32_t counter;
> 
>     do {
> -        counter = read_counter(bucket, memory_order_acquire);
> +        counter = read_counter(bucket);
>     } while (OVS_UNLIKELY(counter & 1));
> 
>     return counter;
> @@ -268,7 +268,7 @@ read_even_counter(struct cmap_bucket *bucket)
> static bool
> counter_changed(struct cmap_bucket *b, uint32_t c)
> {
> -    return OVS_UNLIKELY(read_counter(b, memory_order_release) != c);
> +    return OVS_UNLIKELY(read_counter(b) != c);
> }
> 
> /* Searches 'cmap' for an element with the specified 'hash'.  If one or more is
> -- 
> 1.9.1
> 




More information about the dev mailing list