On 10 January 2017 at 03:45, Paul Blakey <pa...@mellanox.com> wrote:
>>> + struct netdev_list_element *element;
>>> + struct ovs_list port_list;
>>> + int ports = netdev_hmap_port_get_list(dpif_->dpif_class,
>>> &port_list);
>>> + int i = 0;
>>> +
>>> + dump->netdev_dumps =
>>> + ports ? xzalloc(sizeof(struct netdev_flow_dump *) * ports) : 0;
>>
>> Can this be sizeof(dump->netdev_dumps)?
>
> Do you mean sizeof(*dump-netdev_dumps), or sizeof(dump->netdev_dumps[0]), if
> so yes.
Yes that's what I meant.
<snip>
>>> @@ -1466,6 +1521,90 @@ dpif_netlink_flow_to_dpif_flow(struct dpif *dpif,
>>> struct dpif_flow *dpif_flow,
>>> dpif_netlink_flow_get_stats(datapath_flow, &dpif_flow->stats);
>>> }
>>>
>>> +static void
>>> +dpif_netlink_advance_netdev_dump(struct dpif_netlink_flow_dump_thread
>>> *thread)
>>> +{
>>> + struct dpif_netlink_flow_dump *dump = thread->dump;
>>> +
>>> + ovs_mutex_lock(&dump->netdev_lock);
>>> + /* if we haven't finished (dumped everything) */
>>> + if (dump->netdev_given < dump->netdev_num) {
>>> + /* if we are the first to find that given dump is finished
>>> + * (for race condition, e.g 3 finish dump 0 at the same time) */
>>
>> Why is there a race condition here if this is executed under netdev_lock?
>
> The design is such that all threads are working together on the first dump
> to the last, in order. (at first they all on dump 0),
> and when one thread finds that the given dump is finished, they all move to
> the next.
> As the comment tried to explain, if 3 (or 2+) threads are working on the
> first dump, dump 0,
> (thread->netdev_cur_dump == 0) and finish at the same time, they all call
> advance func.
> Now the first one to get the lock advances the shared given dump, which
> signify which highest dump we have given
> (and all lower dumps have finished). The rest now enter and we check if the
> dump they have found to be
> finished is higher then the new one that was given, if not they catch up, so
> now all of them will work on dump 1.
>
> The race is that if 2 or more threads worked on the same dump and finished
> at the same time,
> if we just increased netdev_given without checking (thread->cur == given)
> for both of them,
> we would have increased given twice and skip one dump.
Thanks for the explanation. I think that the code would benefit from
having this written somewhere - for instance, above the
dpif_netlink_advance_netdev_dump() function.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
