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

Ben Pfaff blp at ovn.org
Tue Mar 7 18:13:45 UTC 2017

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>

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

reporting must-be-zero errors here:
+    if (nal->pad || nal->pad2) {
+    }

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

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'

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.

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