On May 28, 2014, at 4:14 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
> On May 23, 2014, at 10:23 AM, Ben Pfaff <b...@nicira.com> wrote:
>
>> On Thu, May 22, 2014 at 05:37:41PM -0700, Jarno Rajahalme wrote:
>>> Add cmap_replace() and cmap_first(), as well as CMAP_FOR_EACH_SAFE and
>>> CMAP_FOR_EACH_CONTINUE to make porting existing hmap using code a bit
>>> easier.
>>>
>>> CMAP_FOR_EACH_SAFE is useful in RCU postponed destructors, when it is
>>> known that additional postponing is not needed.
>>>
>>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>>
>> I am not sure that ovsrcu_init() may be used in cmap_replace__() here.
>> I believe that we need to make sure that the store to replacement->next
>> becomes visible before the store to b->nodes[slot]. I am not sure that
>> a single release fence is sufficient, because I believe that the write
>> to replacement->next could still get reordered after the write to
>> b->nodes[slot]. After all, both are on the same side of the release
>> fence:
>>
>> /* The pointer to 'node' is changed to point to 'replacement',
>> * which is the next node if no replacement node is given. */
>> if (!replacement) {
>> replacement = cmap_node_next_protected(node);
>> } else {
>> /* 'replacement' takes the position of 'node' in the list. */
>> ovsrcu_init(&replacement->next, cmap_node_next_protected(node));
>> }
>>
>> if (b->nodes[slot] == node) {
>> b->nodes[slot] = replacement;
>> atomic_thread_fence(memory_order_release);
>>
>
> It seems to me that moving the fence up one line would solve the problem, no?
I just re-realized that this will go way with the patch 5/5 anyway. Do you want
me to merge patches 4 and 5, instead?
Jarno_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
