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

[Ben Pfaff]

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 

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

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;
+}

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 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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Daniele Di Proietto]

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 
---
 include/openvswitch/ofp-actions.h |  12 
 lib/learn.c   |  28 
 lib/ofp-actions.c | 135 ++
 tests/ofp-actions.at  |  14 
 utilities/ovs-ofctl.8.in  |  19 ++
 5 files changed, 197 insertions(+), 11 deletions(-)

diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index 88f573dcd..c1199a4ec 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -652,6 +652,10 @@ struct ofpact_resubmit {
  * If NX_LEARN_F_SEND_FLOW_REM is set, then the learned flows will have their
  * OFPFF_SEND_FLOW_REM flag set.
  *
+ * If NX_LEARN_F_WRITE_RESULT is set, then the actions will write whether the
+ * learn operation succeded on a bit.  If the learn is successful the bit will
+ * be set, otherwise (e.g. if the limit is hit) the bit will be unset.
+ *
  * If NX_LEARN_F_DELETE_LEARNED is set, then removing this action will delete
  * all the flows from the learn action's 'table_id' that have the learn
  * action's 'cookie'.  Important points:
@@ -677,6 +681,7 @@ struct ofpact_resubmit {
 enum nx_learn_flags {
 NX_LEARN_F_SEND_FLOW_REM = 1 << 0,
 NX_LEARN_F_DELETE_LEARNED = 1 << 1,
+NX_LEARN_F_WRITE_RESULT = 1 << 2,
 };
 
 #define NX_LEARN_N_BITS_MASK0x3ff
@@ -740,6 +745,13 @@ struct ofpact_learn {
 ovs_be64 cookie;   /* Cookie for new flow. */
 uint16_t fin_idle_timeout; /* Idle timeout after FIN, if nonzero. */
 uint16_t fin_hard_timeout; /* Hard timeout after FIN, if nonzero. */
+/* If the number of learned flows on 'table_id' with 'cookie' exceeds
+ * this, the learn action will not add a new flow. */
+uint32_t limit;
+/* Used only if 'flags' has NX_LEARN_F_WRITE_RESULT.  If the execution
+ * failed to install a new flow because 'limit' is exceeded,
+ * result_dst will be set to 0, otherwise to 1. */
+struct mf_subfield result_dst;
 );
 
 struct ofpact_learn_spec specs[];
diff --git a/lib/learn.c b/lib/learn.c
index ce52c35f2..b1b8bc52b 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -406,6 +406,26 @@ learn_parse__(char *orig, char *arg, struct ofpbuf 
*ofpacts)
 learn->flags |= NX_LEARN_F_SEND_FLOW_REM;
 } else if (!strcmp(name, "delete_learned")) {
 learn->flags |= NX_LEARN_F_DELETE_LEARNED;
+} else if (!strcmp(name, "limit")) {
+learn->limit = atoi(value);
+} else if (!strcmp(name, "result_dst")) {
+char *error;
+learn->flags |= NX_LEARN_F_WRITE_RESULT;
+error = mf_parse_subfield(>result_dst, value);
+if (error) {
+return error;
+}
+if (!mf_nxm_header(learn->result_dst.field->id)) {
+return xasprintf("experimenter OXM field '%s' not supported",
+ value);
+}
+if (!learn->result_dst.field->writable) {
+return xasprintf("%s is read-only", value);
+}
+if (learn->result_dst.n_bits != 1) {
+return xasprintf("result_dst in 'learn' action must be a "
+ "single bit");
+}
 } else {
 struct ofpact_learn_spec *spec;
 char *error;
@@ -487,6 +507,14 @@ learn_format(const struct ofpact_learn *learn, struct ds 
*s)
 ds_put_format(s, ",%scookie=%s%#"PRIx64,
   colors.param, colors.end, ntohll(learn->cookie));
 }
+if (learn->limit != 0) {
+ds_put_format(s, ",%slimit=%s%"PRIu32,
+  colors.param, colors.end, learn->limit);
+}
+if (learn->flags & NX_LEARN_F_WRITE_RESULT) {
+ds_put_format(s, ",%sresult_dst=%s", colors.param, colors.end);
+mf_format_subfield(>result_dst, s);
+}
 
 OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
 unsigned int n_bytes = DIV_ROUND_UP(spec->n_bits, 8);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 78f8c4366..1c77e7bbd 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -292,6 +292,8 @@ enum ofp_raw_action_type {
 
 /* NX1.0+(16): struct nx_action_learn, ... VLMFF */
 NXAST_RAW_LEARN,
+/* NX1.0+(44): struct nx_action_learn2, ... VLMFF */
+NXAST_RAW_LEARN2,
 
 /* NX1.0+(17): void. */
 NXAST_RAW_EXIT,
@@ -4028,7 +4030,7 @@ format_RESUBMIT(const struct ofpact_resubmit *a, struct 
ds *s)
 }
 }
 
-/* Action structure for