[ovs-dev] [PATCH] dpif-netdev: removed hmap flow_table

Daniele Di Proietto ddiproietto at vmware.com
Fri May 16 23:17:34 UTC 2014


On May 16, 2014, at 9:26 AM, Ben Pfaff <blp at nicira.com> wrote:

> This needs {}:
>> +    if (!netdev_flow)
>> +        return NULL;
> 

Sure, i’ll fix it

> VLOG_ERR is pretty strong.  ovs-appctl(8) says that it means that "A
> high-level operation or a subsystem failed.  Attention is warranted."
> That is, it generally indicates a bug or a failure that might require
> the admin to take a look.  Is this such a situation?  I can imagine
> races where this might happen, although I guess that they should be rare
> in practice.
>> +    if (flow_equal(&netdev_flow->flow, flow)) {
>> +        return netdev_flow;
>> +    } else {
>> +        VLOG_ERR("classifier_lookup returned different flow");
>>     }

No problem, I’ll change it to VLOG_WARN, that seems more appropriate.

> I don't think that it is safe to use a cls_cursor to iterate flows in
> the flow dump path.  The classifier isn't locked from one
> dpif_netdev_flow_dump_next() to the next, which means that any given
> flow might get deleted from the classifier from one to the next.  That
> includes both the most recently returned flow and the next one that will
> be returned.  Basically, we have to either somehow prevent at least one
> of those flows from being deleted, or use some other means for
> iteration.  That's why the hmap iteration used the indirect
> hmap_at_position() call for iteration: it's always safe, even if it's
> otherwise undesirable

I didn’t think of that. I assumed that a cls_cursor_next() had the same requirements as hmap_at_position().
I’ve implemented classifier_at_position() to address the issue. I’ll post it along with a new version of the patch.

Thanks for the review,

Daniele


More information about the dev mailing list