diff mbox

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

Message ID 20170225025801.86791-3-diproiettod@vmware.com
State Changes Requested
Headers show

Commit Message

Daniele Di Proietto Feb. 25, 2017, 2:57 a.m. UTC 
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@vmware.com>
---
 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(-)

Comments

Ben Pfaff March 7, 2017, 6:13 p.m. UTC | #1 
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@vmware.com>

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.
Daniele Di Proietto March 8, 2017, 7:25 p.m. UTC | #2 
On 07/03/2017 10:13, "Ben Pfaff" <blp@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@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.
diff mbox

Patch

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_MASK    0x3ff
@@ -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(&learn->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(&learn->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 NXAST_LEARN.
+/* Action structure for NXAST_LEARN and NXAST_LEARN2.
  *
  * This action adds or modifies a flow in an OpenFlow table, similar to
  * OFPT_FLOW_MOD with OFPFC_MODIFY_STRICT as 'command'.  The new flow has the
@@ -4259,6 +4261,34 @@  struct nx_action_learn {
 };
 OFP_ASSERT(sizeof(struct nx_action_learn) == 32);
 
+struct nx_action_learn2 {
+    ovs_be16 type;              /* OFPAT_VENDOR. */
+    ovs_be16 len;               /* At least 24. */
+    ovs_be32 vendor;            /* NX_VENDOR_ID. */
+    ovs_be16 subtype;           /* NXAST_LEARN2. */
+    ovs_be16 idle_timeout;      /* Idle time before discarding (seconds). */
+    ovs_be16 hard_timeout;      /* Max time before discarding (seconds). */
+    ovs_be16 priority;          /* Priority level of flow entry. */
+    ovs_be64 cookie;            /* Cookie for new flow. */
+    ovs_be16 flags;             /* NX_LEARN_F_*. */
+    uint8_t table_id;           /* Table to insert flow entry. */
+    uint8_t pad;                /* Must be zero. */
+    ovs_be16 fin_idle_timeout;  /* Idle timeout after FIN, if nonzero. */
+    ovs_be16 fin_hard_timeout;  /* Hard timeout after FIN, if nonzero. */
+    ovs_be32 limit;             /* Maximum number of learned flows. */
+
+    /* Where to store the result. */
+    ovs_be16 result_dst_ofs;    /* Starting bit offset in destination. */
+
+    ovs_be16 pad2;              /* Must be zero. */
+    /* Followed by:
+     * - OXM/NXM header for destination field (4 or 8 bytes),
+     *   if NX_LEARN_F_WRITE_RESULT is set in 'flags'
+     * - a sequence of flow_mod_spec elements, as described above,
+     *   until the end of the action is reached. */
+};
+OFP_ASSERT(sizeof(struct nx_action_learn2) == 40);
+
 static ovs_be16
 get_be16(const void **pp)
 {
@@ -4431,6 +4461,65 @@  decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
                                vl_mff_map, ofpacts);
 }
 
+/* Converts 'nal' into a "struct ofpact_learn" and appends that struct to
+ * 'ofpacts'.  Returns 0 if successful, otherwise an OFPERR_*. */
+static enum ofperr
+decode_NXAST_RAW_LEARN2(const struct nx_action_learn2 *nal,
+                        enum ofp_version ofp_version OVS_UNUSED,
+                        const struct vl_mff_map *vl_mff_map,
+                        struct ofpbuf *ofpacts)
+{
+    struct ofpbuf b = ofpbuf_const_initializer(nal, ntohs(nal->len));
+    struct ofpact_learn *learn;
+
+    if (nal->pad || nal->pad2) {
+        return OFPERR_OFPBAC_BAD_ARGUMENT;
+    }
+
+    learn = ofpact_put_LEARN(ofpacts);
+
+    learn->idle_timeout = ntohs(nal->idle_timeout);
+    learn->hard_timeout = ntohs(nal->hard_timeout);
+    learn->priority = ntohs(nal->priority);
+    learn->cookie = nal->cookie;
+    learn->table_id = nal->table_id;
+    learn->fin_idle_timeout = ntohs(nal->fin_idle_timeout);
+    learn->fin_hard_timeout = ntohs(nal->fin_hard_timeout);
+    learn->limit = ntohl(nal->limit);
+
+    learn->flags = ntohs(nal->flags);
+    if (learn->flags & ~(NX_LEARN_F_SEND_FLOW_REM |
+                         NX_LEARN_F_DELETE_LEARNED |
+                         NX_LEARN_F_WRITE_RESULT)) {
+        return OFPERR_OFPBAC_BAD_ARGUMENT;
+    }
+
+    if (learn->table_id == 0xff) {
+        return OFPERR_OFPBAC_BAD_ARGUMENT;
+    }
+
+    ofpbuf_pull(&b, sizeof *nal);
+
+    if (learn->flags & NX_LEARN_F_WRITE_RESULT) {
+        enum ofperr error = nx_pull_header(&b, vl_mff_map,
+                                           &learn->result_dst.field,
+                                           NULL);
+        if (error) {
+            return error;
+        }
+        if (!learn->result_dst.field->writable) {
+            return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
+        }
+        learn->result_dst.ofs = ntohs(nal->result_dst_ofs);
+        learn->result_dst.n_bits = 1;
+    } else if (nal->result_dst_ofs) {
+        return OFPERR_OFPBAC_BAD_ARGUMENT;
+    }
+
+    return decode_LEARN_common(b.data, (char *) nal + ntohs(nal->len),
+                               vl_mff_map, ofpacts);
+}
+
 static void
 put_be16(struct ofpbuf *b, ovs_be16 x)
 {
@@ -4460,19 +4549,43 @@  encode_LEARN(const struct ofpact_learn *learn,
              enum ofp_version ofp_version OVS_UNUSED, struct ofpbuf *out)
 {
     const struct ofpact_learn_spec *spec;
-    struct nx_action_learn *nal;
     size_t start_ofs;
 
     start_ofs = out->size;
-    nal = put_NXAST_LEARN(out);
-    nal->idle_timeout = htons(learn->idle_timeout);
-    nal->hard_timeout = htons(learn->hard_timeout);
-    nal->fin_idle_timeout = htons(learn->fin_idle_timeout);
-    nal->fin_hard_timeout = htons(learn->fin_hard_timeout);
-    nal->priority = htons(learn->priority);
-    nal->cookie = learn->cookie;
-    nal->flags = htons(learn->flags);
-    nal->table_id = learn->table_id;
+
+    if (learn->ofpact.raw == NXAST_RAW_LEARN2
+        || learn->limit != 0
+        || learn->flags & NX_LEARN_F_WRITE_RESULT) {
+        struct nx_action_learn2 *nal;
+
+        nal = put_NXAST_LEARN2(out);
+        nal->idle_timeout = htons(learn->idle_timeout);
+        nal->hard_timeout = htons(learn->hard_timeout);
+        nal->fin_idle_timeout = htons(learn->fin_idle_timeout);
+        nal->fin_hard_timeout = htons(learn->fin_hard_timeout);
+        nal->priority = htons(learn->priority);
+        nal->cookie = learn->cookie;
+        nal->flags = htons(learn->flags);
+        nal->table_id = learn->table_id;
+        nal->limit = htonl(learn->limit);
+        nal->result_dst_ofs = htons(learn->result_dst.ofs);
+
+        if (learn->flags & NX_LEARN_F_WRITE_RESULT) {
+            nx_put_header(out, learn->result_dst.field->id, 0, false);
+        }
+    } else {
+        struct nx_action_learn *nal;
+
+        nal = put_NXAST_LEARN(out);
+        nal->idle_timeout = htons(learn->idle_timeout);
+        nal->hard_timeout = htons(learn->hard_timeout);
+        nal->fin_idle_timeout = htons(learn->fin_idle_timeout);
+        nal->fin_hard_timeout = htons(learn->fin_hard_timeout);
+        nal->priority = htons(learn->priority);
+        nal->cookie = learn->cookie;
+        nal->flags = htons(learn->flags);
+        nal->table_id = learn->table_id;
+    }
 
     OFPACT_LEARN_SPEC_FOR_EACH (spec, learn) {
         put_u16(out, spec->n_bits | spec->dst_type | spec->src_type);
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 6384c48d9..694afc9f3 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -258,6 +258,20 @@  ffff 0020 00002320 002a 000000000000 dnl
 0001 0008 0005 0000 dnl
 0000 0008 000a 0000
 
+# actions=learn(table=2,idle_timeout=10,hard_timeout=20,fin_idle_timeout=2,fin_hard_timeout=4,priority=80,cookie=0x123456789abcdef0,limit=1,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[])
+ffff 0050 00002320 002c 000a 0014 0050 123456789abcdef0 0000 02 00 0002 0004 00000001 0000 0000 dnl
+000c 00000802 0000 00000802 0000 dnl
+0030 00000406 0000 00000206 0000 dnl
+1010 00000002 0000 dnl
+00000000
+
+# actions=learn(table=2,idle_timeout=10,hard_timeout=20,fin_idle_timeout=2,fin_hard_timeout=4,priority=80,cookie=0x123456789abcdef0,limit=1,result_dst=NXM_NX_REG0[8],NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[])
+ffff 0050 00002320 002c 000a 0014 0050 123456789abcdef0 0004 02 00 0002 0004 00000001 0008 0000 dnl
+00010004 dnl
+000c 00000802 0000 00000802 0000 dnl
+0030 00000406 0000 00000206 0000 dnl
+1010 00000002 0000
+
 # actions=group:5
 ffff 0010 00002320 0028 0000 00000005
 
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 2ee3193d4..85a4f574f 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1432,6 +1432,25 @@  the specified table with the specified cookie.
 .IP
 This flag was added in Open vSwitch 2.4.
 .
+.IP \fBlimit=\fInumber\fR
+If the number of learned flows in table \fBtable\fR with cookie id
+\fBcookie\fR exceeds \fInumber\fR, a new flow will not be learned by this
+action.  Unlike the \fBdelete_learned\fR behavior, only flows inserted by
+a learn action with a limit are counted.  If a flow is installed by the
+controller or if it's learned by a learn action without any limit, it's not
+included when checking if the limit is exceeded.  By default there's no limit.
+.
+.IP
+This flag was added in Open vSwitch 2.8.
+.
+.IP \fBresult_dst=\fIfield\fB[\fIbit\fB]\fR
+If learning failed (because the number of learned flows exceeds \fBlimit\fR),
+the action sets \fIfield\fB[\fIbit\fB]\fR to 0, otherwise it will be set to 1.
+\fIfield\fB[\fIbit\fB]\fR must be a single bit.
+.
+.IP
+This flag was added in Open vSwitch 2.8.
+.
 .IP \fIfield\fB=\fIvalue\fR
 .IQ \fIfield\fB[\fIstart\fB..\fIend\fB]=\fIsrc\fB[\fIstart\fB..\fIend\fB]\fR
 .IQ \fIfield\fB[\fIstart\fB..\fIend\fB]\fR