[ovs-dev] [PATCH 2/7] ofp-actions: Add limit to learn action.

Daniele Di Proietto diproiettod at vmware.com
Wed Mar 8 19:25:09 UTC 2017






On 07/03/2017 10:13, "Ben Pfaff" <blp at ovn.org> wrote:

>On Fri, Feb 24, 2017 at 06:57:56PM -0800, Daniele Di Proietto wrote:
>> This commit adds a new feature to the learn actions: the possibility to
>> limit the number of learned flows.
>> 
>> To be compatible with users of the old learn action, a new structure is
>> introduced as well as a new OpenFlow raw action number.
>> 
>> This commit only implements parsing of the action and documentation.
>> A later commit will implement the feature in ofproto-dpif.
>> 
>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>

Thanks a lot for taking a look at this

>
>I'm not sure why this is a separate commit.

I thought it was easier to review, but I decided to squash it with the ofproto
part for v2, since they touch different files.

>
>OFPERR_NXBAC_MUST_BE_ZERO is clearer than OFPERR_OFPBAC_BAD_ARGUMENT for
>reporting must-be-zero errors here:
>+    if (nal->pad || nal->pad2) {
>+        return OFPERR_OFPBAC_BAD_ARGUMENT;
>+    }

Changed thanks

>
>I think that struct nx_action_learn2 has struct nx_action_learn as a
>prefix.  Would it make sense to either make it nx_action_learn a member
>of nx_action_learn2 or to make struct nx_action_learn2 just the last 8
>bytes that are not in nx_action_learn?  Maybe it would factor out some
>code?

I wasn't sure about this.  I made nx_action_learn part of nx_action_learn2
in v2 and it looks better, thanks

>
>I am not sure why experimenter OXM fields are not supported.  It looks
>like the code actually supports them, other than the check that rejects
>them.  If they cannot be supported then the header is always exactly 4
>bytes so the comment in nx_action_learn2 should be updated:
>    /* Followed by:
>     * - OXM/NXM header for destination field (4 or 8 bytes),
>     *   if NX_LEARN_F_WRITE_RESULT is set in 'flags'

You're right, we can use experimenter OXM fields.  I removed the restriction
from the code

>The documented behavior is that flows installed other than by a "learn"
>action with limit= don't count.  Is there a way for a controller (or an
>administrator using ovs-ofctl) to discover which flows were added by
>such an action?  Otherwise it's going to be confusing.  Also that
>behavior is going to break if someone dumps the flow table, restarts
>OVS, and restores the flow table (which some OVS scripts do
>automatically on upgrade), since there's presumably no way to save and
>restore whether the flow was added by a learn action.  So at first, at
>least, this seems like an undesirable design.

That's a very good point, I haven't considered it at all.

Let me try to explain why I was doing this: there's a small window between
the OpenFlow flow removal and the datapath flow removal in which the packets
still follow the old flow table.  In this series I introduced a lot of
code to make sure that during this window we wouldn't allow learning new
flows if the limit was exceeded by holding counter references from the
OpenFlow flows and the ukeys.  This meant that I had to introduce a separate
mutex (in cookie counter) to synchronize handlers and revalidators.  I didn't
want to do that for every flow, only for learned flow, for performance reason.

Considering that:

* To support this we have to save state that cannot be preserved across restarts
* Even when the controller changes the flow table, there's still the same window
  between flow mods and datapath changes.
* It makes the code so much simpler

I've decided to change the design entirely and only enforce the limit on the
OpenFlow table, without waiting for revalidators to delete the datapath flows.

In v2 there's no distinction between learned flows and flows installed by a
controller.

Thanks,

Daniele

>
>I haven't read the remaining commits to add this feature so perhaps I'll
>have more comments.


More information about the dev mailing list