Add support for OpenFlow 1.4+ "importance" values.

This patch enables a user to set importance for a new rule via add-flow OF1.4+ in the OVS and display the same via dump-flows command OF1.4+. The changes are made in accordance with OpenFlow 1.4 specs to implement eviction on the basis of "importance". This patch also enhances the diff-flows & replace-flows CLI for addition of importance parameter in a rule. This doesn't actually implement eviction on the basis of importance, which will happen in a later patch. Signed-off-by: Rishi Bamba <[email protected]> Signed-off-by: Ben Pfaff <[email protected]>
dkarhi · Nov 10, 2014 · ca26eb4 · ca26eb4
1 parent 849d564
commit ca26eb4
Show file tree

Hide file tree

Showing 16 changed files with 129 additions and 6 deletions.
diff --git a/AUTHORS b/AUTHORS
@@ -122,6 +122,7 @@ Ravi Kerur              [email protected]
 Reid Price              [email protected]
 Remko Tronçon           [email protected]
 Rich Lane               [email protected]
+Rishi Bamba             [email protected]
 Rob Hoes                [email protected]
 Romain Lenglet          [email protected]
 Ryan Wilson             [email protected]

diff --git a/DESIGN.md b/DESIGN.md
@@ -277,8 +277,11 @@ The table for 1.3 is the same as the one shown above for 1.2.
 OpenFlow 1.4
 ------------
 
-OpenFlow 1.4 does not change flow_mod semantics.
-
+OpenFlow 1.4 adds the "importance" field to flow_mods, but it does not
+explicitly specify which kinds of flow_mods set the importance.For
+consistency, Open vSwitch uses the same rule for importance as for
+idle_timeout and hard_timeout, that is, only an "ADD" flow_mod sets
+the importance.  (This issue has been filed with the ONF as EXT-496.)
 
 OFPT_PACKET_IN
 ==============

diff --git a/NEWS b/NEWS
@@ -25,6 +25,7 @@ Post-v2.3.0
      * "resubmit" actions may now be included in action sets.  The resubmit
        is executed last, and only if the action set has no "output" or "group"
        action.
+     * OpenFlow 1.4+ flow "importance" is now maintained in the flow table.
    - ovs-pki: Changed message digest algorithm from MD5 to SHA-1 because
      MD5 is no longer secure and some operating systems have started to disable
      it in OpenSSL.

diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
@@ -340,7 +340,7 @@ struct ofp11_flow_mod {
                                     output group. A value of OFPG11_ANY
                                     indicates no restriction. */
     ovs_be16 flags;              /* One of OFPFF_*. */
-    uint8_t pad[2];
+    ovs_be16 importance;         /* Eviction precedence (OF1.4+). */
     /* Followed by an ofp11_match structure. */
     /* Followed by an instruction set. */
 };
@@ -451,7 +451,8 @@ struct ofp11_flow_stats {
     ovs_be16 idle_timeout;     /* Number of seconds idle before expiration. */
     ovs_be16 hard_timeout;     /* Number of seconds before expiration. */
     ovs_be16 flags;            /* OF 1.3: Set of OFPFF*. */
-    uint8_t  pad2[4];          /* Align to 64-bits. */
+    ovs_be16 importance;       /* Eviction precedence (OF1.4+). */
+    uint8_t  pad2[2];          /* Align to 64-bits. */
     ovs_be64 cookie;           /* Opaque controller-issued identifier. */
     ovs_be64 packet_count;     /* Number of packets in flow. */
     ovs_be64 byte_count;       /* Number of bytes in flow. */

diff --git a/lib/learn.c b/lib/learn.c
@@ -101,6 +101,7 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow,
     fm->command = OFPFC_MODIFY_STRICT;
     fm->idle_timeout = learn->idle_timeout;
     fm->hard_timeout = learn->hard_timeout;
+    fm->importance = 0;
     fm->buffer_id = UINT32_MAX;
     fm->out_port = OFPP_NONE;
     fm->flags = 0;

diff --git a/lib/learning-switch.c b/lib/learning-switch.c
@@ -202,6 +202,7 @@ lswitch_handshake(struct lswitch *sw)
         fm.command = OFPFC_ADD;
         fm.idle_timeout = 0;
         fm.hard_timeout = 0;
+        fm.importance = 0;
         fm.buffer_id = UINT32_MAX;
         fm.out_port = OFPP_NONE;
         fm.out_group = OFPG_ANY;

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
@@ -248,6 +248,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
     enum {
         F_OUT_PORT = 1 << 0,
         F_ACTIONS = 1 << 1,
+        F_IMPORTANCE = 1 << 2,
         F_TIMEOUT = 1 << 3,
         F_PRIORITY = 1 << 4,
         F_FLAGS = 1 << 5,
@@ -264,7 +265,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
         break;
 
     case OFPFC_ADD:
-        fields = F_ACTIONS | F_TIMEOUT | F_PRIORITY | F_FLAGS;
+        fields = F_ACTIONS | F_TIMEOUT | F_PRIORITY | F_FLAGS | F_IMPORTANCE;
         break;
 
     case OFPFC_DELETE:
@@ -305,6 +306,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
     fm->buffer_id = UINT32_MAX;
     fm->out_port = OFPP_ANY;
     fm->flags = 0;
+    fm->importance = 0;
     fm->out_group = OFPG11_ANY;
     fm->delete_reason = OFPRR_DELETE;
     if (fields & F_ACTIONS) {
@@ -366,6 +368,8 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
                 error = str_to_u16(value, name, &fm->idle_timeout);
             } else if (fields & F_TIMEOUT && !strcmp(name, "hard_timeout")) {
                 error = str_to_u16(value, name, &fm->hard_timeout);
+            } else if (fields & F_IMPORTANCE && !strcmp(name, "importance")) {
+                error = str_to_u16(value, name, &fm->importance);
             } else if (!strcmp(name, "cookie")) {
                 char *mask = strchr(value, '/');
 

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
@@ -805,6 +805,9 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh, int verbosity)
     if (fm.hard_timeout != OFP_FLOW_PERMANENT) {
         ds_put_format(s, "hard:%"PRIu16" ", fm.hard_timeout);
     }
+    if (fm.importance != 0) {
+        ds_put_format(s, "importance:%"PRIu16" ", fm.importance);
+    }
     if (fm.priority != OFP_DEFAULT_PRIORITY && need_priority) {
         ds_put_format(s, "pri:%"PRIu16" ", fm.priority);
     }
@@ -1429,6 +1432,9 @@ ofp_print_flow_stats(struct ds *string, struct ofputil_flow_stats *fs)
     if (fs->flags) {
         ofp_print_flow_flags(string, fs->flags);
     }
+    if (fs->importance != 0) {
+        ds_put_format(string, "importance=%"PRIu16", ", fs->importance);
+    }
     if (fs->idle_age >= 0) {
         ds_put_format(string, "idle_age=%d, ", fs->idle_age);
     }

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
@@ -1700,6 +1700,11 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
 
         fm->idle_timeout = ntohs(ofm->idle_timeout);
         fm->hard_timeout = ntohs(ofm->hard_timeout);
+        if (oh->version >= OFP14_VERSION && ofm->command == OFPFC_ADD) {
+            fm->importance = ntohs(ofm->importance);
+        } else {
+            fm->importance = 0;
+        }
         fm->buffer_id = ntohl(ofm->buffer_id);
         error = ofputil_port_from_ofp11(ofm->out_port, &fm->out_port);
         if (error) {
@@ -1738,6 +1743,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
             fm->new_cookie = ofm->cookie;
             fm->idle_timeout = ntohs(ofm->idle_timeout);
             fm->hard_timeout = ntohs(ofm->hard_timeout);
+            fm->importance = 0;
             fm->buffer_id = ntohl(ofm->buffer_id);
             fm->out_port = u16_to_ofp(ntohs(ofm->out_port));
             fm->out_group = OFPG11_ANY;
@@ -1765,6 +1771,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
             fm->new_cookie = nfm->cookie;
             fm->idle_timeout = ntohs(nfm->idle_timeout);
             fm->hard_timeout = ntohs(nfm->hard_timeout);
+            fm->importance = 0;
             fm->buffer_id = ntohl(nfm->buffer_id);
             fm->out_port = u16_to_ofp(ntohs(nfm->out_port));
             fm->out_group = OFPG11_ANY;
@@ -2248,6 +2255,11 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         ofm->out_port = ofputil_port_to_ofp11(fm->out_port);
         ofm->out_group = htonl(fm->out_group);
         ofm->flags = raw_flags;
+        if (version >= OFP14_VERSION && fm->command == OFPFC_ADD) {
+            ofm->importance = htons(fm->importance);
+        } else {
+            ofm->importance = 0;
+        }
         ofputil_put_ofp11_match(msg, &fm->match, protocol);
         ofpacts_put_openflow_instructions(fm->ofpacts, fm->ofpacts_len, msg,
                                           version);
@@ -2834,6 +2846,11 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         fs->duration_nsec = ntohl(ofs->duration_nsec);
         fs->idle_timeout = ntohs(ofs->idle_timeout);
         fs->hard_timeout = ntohs(ofs->hard_timeout);
+        if (oh->version >= OFP14_VERSION) {
+            fs->importance = ntohs(ofs->importance);
+        } else {
+            fs->importance = 0;
+        }
         if (raw == OFPRAW_OFPST13_FLOW_REPLY) {
             error = ofputil_decode_flow_mod_flags(ofs->flags, -1, oh->version,
                                                   &fs->flags);
@@ -2875,6 +2892,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         fs->duration_nsec = ntohl(ofs->duration_nsec);
         fs->idle_timeout = ntohs(ofs->idle_timeout);
         fs->hard_timeout = ntohs(ofs->hard_timeout);
+        fs->importance = 0;
         fs->idle_age = -1;
         fs->hard_age = -1;
         fs->packet_count = ntohll(get_32aligned_be64(&ofs->packet_count));
@@ -2910,6 +2928,7 @@ ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         fs->priority = ntohs(nfs->priority);
         fs->idle_timeout = ntohs(nfs->idle_timeout);
         fs->hard_timeout = ntohs(nfs->hard_timeout);
+        fs->importance = 0;
         fs->idle_age = -1;
         fs->hard_age = -1;
         if (flow_age_extension) {
@@ -2977,6 +2996,11 @@ ofputil_append_flow_stats_reply(const struct ofputil_flow_stats *fs,
         ofs->priority = htons(fs->priority);
         ofs->idle_timeout = htons(fs->idle_timeout);
         ofs->hard_timeout = htons(fs->hard_timeout);
+        if (version >= OFP14_VERSION) {
+            ofs->importance = htons(fs->importance);
+        } else {
+            ofs->importance = 0;
+        }
         if (raw == OFPRAW_OFPST13_FLOW_REPLY) {
             ofs->flags = ofputil_encode_flow_mod_flags(fs->flags, version);
         } else {

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
@@ -306,6 +306,7 @@ struct ofputil_flow_mod {
     ofp_port_t out_port;
     uint32_t out_group;
     enum ofputil_flow_mod_flags flags;
+    uint16_t importance;     /* Eviction precedence. */
     struct ofpact *ofpacts;  /* Series of "struct ofpact"s. */
     size_t ofpacts_len;      /* Length of ofpacts, in bytes. */
 
@@ -355,6 +356,7 @@ struct ofputil_flow_stats {
     const struct ofpact *ofpacts;
     size_t ofpacts_len;
     enum ofputil_flow_mod_flags flags;
+    uint16_t importance;        /* Eviction precedence. */
 };
 
 int ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *,

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
@@ -5313,6 +5313,7 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
     fm.command = OFPFC_ADD;
     fm.idle_timeout = idle_timeout;
     fm.hard_timeout = 0;
+    fm.importance = 0;
     fm.buffer_id = 0;
     fm.out_port = 0;
     fm.flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY;

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
@@ -346,6 +346,9 @@ struct rule {
     uint16_t hard_timeout OVS_GUARDED; /* In seconds from ->modified. */
     uint16_t idle_timeout OVS_GUARDED; /* In seconds from ->used. */
 
+    /* Eviction precedence. */
+    uint16_t importance OVS_GUARDED;
+
     /* Eviction groups (see comment on struct eviction_group for explanation) .
      *
      * 'eviction_group' is this rule's eviction group, or NULL if it is not in

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
@@ -1895,6 +1895,7 @@ flow_mod_init(struct ofputil_flow_mod *fm,
     fm->command = command;
     fm->idle_timeout = 0;
     fm->hard_timeout = 0;
+    fm->importance = 0;
     fm->buffer_id = UINT32_MAX;
     fm->out_port = OFPP_ANY;
     fm->out_group = OFPG_ANY;
@@ -1991,6 +1992,7 @@ ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
             actions = rule_get_actions(rule);
             if (rule->idle_timeout == fm->idle_timeout
                 && rule->hard_timeout == fm->hard_timeout
+                && rule->importance == fm->importance
                 && rule->flags == (fm->flags & OFPUTIL_FF_STATE)
                 && (!fm->modify_cookie || (fm->new_cookie == rule->flow_cookie))
                 && ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
@@ -3809,6 +3811,7 @@ handle_flow_stats_request(struct ofconn *ofconn,
         fs.cookie = rule->flow_cookie;
         fs.idle_timeout = rule->idle_timeout;
         fs.hard_timeout = rule->hard_timeout;
+        fs.importance = rule->importance;
         created = rule->created;
         modified = rule->modified;
         actions = rule_get_actions(rule);
@@ -4238,6 +4241,7 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     ovs_mutex_lock(&rule->mutex);
     rule->idle_timeout = fm->idle_timeout;
     rule->hard_timeout = fm->hard_timeout;
+    rule->importance = fm->importance;
     ovs_mutex_unlock(&rule->mutex);
 
     *CONST_CAST(uint8_t *, &rule->table_id) = table - ofproto->tables;
@@ -4353,6 +4357,7 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
         if (fm->command == OFPFC_ADD) {
             rule->idle_timeout = fm->idle_timeout;
             rule->hard_timeout = fm->hard_timeout;
+            rule->importance = fm->importance;
             rule->flags = fm->flags & OFPUTIL_FF_STATE;
             rule->created = now;
         }
@@ -4366,7 +4371,7 @@ modify_flows__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
             cookies_insert(ofproto, rule);
         }
         if (fm->command == OFPFC_ADD) {
-            if (fm->idle_timeout || fm->hard_timeout) {
+            if (fm->idle_timeout || fm->hard_timeout || fm->importance) {
                 if (!rule->eviction_group) {
                     eviction_group_add_rule(rule);
                 }

diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
@@ -2751,3 +2751,57 @@ AT_CHECK([tail -1 stdout], [0],
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+dnl Check importance parameter added in OF1.4.
+dnl It validates whether importance set via add-flow via OpenFlow1.4+ gets
+dnl set or not by validating it againt the dump-flows output via OpenFlow1.4+
+dnl If add-flow or dump-flows is used with later version of OpenFlow prior to 1.4+
+dnl then the importance will be considered zero whether provided as an argument.
+
+AT_SETUP([ovs-ofctl rule with importance])
+OVS_VSWITCHD_START
+dnl Flow with importance parameter added via OF1.4+ and later version
+AT_CHECK([ovs-ofctl -O OpenFlow14 add-flow br0 priority=21,importance=21,actions=normal])
+AT_CHECK([ovs-ofctl add-flow br0 priority=22,importance=22,actions=normal])
+
+dnl Importance parameter will only be visible of flows that are added via OF1.4+ if dumped via OF1.4+
+AT_CHECK([ovs-ofctl -O OpenFlow14 dump-flows br0 | ofctl_strip], [0], [dnl
+OFPST_FLOW reply (OF1.4):
+ priority=22 actions=NORMAL
+ importance=21, priority=21 actions=NORMAL
+])
+
+dnl Importance parameter will not be visible if flow is dumped with previous version prior to OF1.4+ whether added via OF1.4+
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip], [0], [dnl
+NXST_FLOW reply:
+ priority=22 actions=NORMAL
+ priority=21 actions=NORMAL
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+
+dnl Importance parameter added in OF1.4.
+dnl This validates whether flows with importance
+dnl parameter are getting replaced with "replace-flows" or
+dnl not by validating dump-flows output after replace with the expected output.
+
+AT_SETUP([ovs-ofctl replace-flows with importance])
+OVS_VSWITCHD_START
+
+dnl Add flows to br0 with importance via OF1.4+. For more details refer "ovs-ofctl rule with importance" test case.
+for i in {1..8}; do echo "dl_vlan=$i,importance=$i,actions=drop"; done > add-flows.txt
+AT_CHECK([ovs-ofctl -O OpenFlow14 add-flows br0 add-flows.txt])
+
+dnl Replace some flows in the bridge.
+for i in {1..8..2}; do echo "dl_vlan=$i,importance=`expr $i + 10`,actions=drop"; done > replace-flows.txt
+AT_CHECK([ovs-ofctl -O OpenFlow14 replace-flows br0 replace-flows.txt])
+
+dnl Dump them and compare the dump flows output against the expected output.
+for i in `seq 1 8`; do if [[ `expr $i % 2` -eq 1 ]]; then importance=`expr $i + 10`; else importance=$i; fi; echo " importance=$importance, dl_vlan=$i actions=drop"; done | sort > expout
+AT_CHECK([ovs-ofctl -O OpenFlow14 dump-flows br0 | ofctl_strip | sed '/OFPST_FLOW/d' | sort],
+  [0], [expout])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
@@ -1847,6 +1847,14 @@ Causes the flow to expire after the given number of seconds,
 regardless of activity.  A value of 0 (the default) gives the flow no
 hard expiration deadline.
 .
+.IP "\fBimportance=\fIvalue\fR"
+Sets the importance of a flow.  The flow entry eviction mechanism can
+use importance as a factor in deciding which flow to evict.  A value
+of 0 (the default) makes the flow non-evictable on the basis of
+importance.  Specify a value between 0 and 65535.
+.IP
+Only OpenFlow 1.4 and later support \fBimportance\fR.
+.
 .IP "\fBsend_flow_rem\fR"
 Marks the flow with a flag that causes the switch to generate a ``flow
 removed'' message and send it to interested controllers when the flow