On May 27, 2014, at 4:58 PM, Ben Pfaff <[email protected]> wrote:
> 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 <[email protected]>
>
> 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.
>
Oops, I wondered about the syntax, but did not check where it was defined
sorry. I defined it as a static function instead.
> 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 (;;) {
>
Done. Mixing declarations and code in action…
> I think this makes the code uglier but it also seems to put the memory
> ordering issues to rest.
>
> Acked-by: Ben Pfaff <[email protected]>
I’m almost ready to push the series, waiting for your final comment on 4/5.
Jarno
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
