[ovs-dev] [PATCH 1/2] ofproto: Add struct ofproto_flow_mod.

Jarno Rajahalme jrajahalme at nicira.com
Wed Jul 1 19:45:04 UTC 2015


It is cleaner to not use ofp_bundle_entry for non-bundle flow mods.
To address this, the new struct ofproto_flow_mod combines an
ofputil_flow_mod and the necessary execution context for executing the
start, revert, and finish phases of the flow mod, which all were
previously members of struct ofp_bundle_entry.

This also simplifies many of the function prototypes introduced with
the OF 1.4 bundles code.  However, in case of learn action execution
this approach requires a new copy of the ofputil_flow_mod.  This could
be avoided by making struct ofproto_flow_mod more complex, but it
seems not worth the complication.

As part of carving out the execution context from ofp_bundle_entry to
ofproto_flow_mod, the 'version' member is now also in
ofproto_flow_mod, as it makes sense for flow mods, but not for port
mods.  Now that the functions operate on the version also get the full
execution context, they use 'version' instead of
'ofproto->tables_version'. This allows ofproto->tables_version to be
changed only when a new version is committed.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 ofproto/bundles.c          |    2 +-
 ofproto/bundles.h          |   14 +--
 ofproto/ofproto-dpif.c     |   79 +++++++------
 ofproto/ofproto-dpif.h     |    3 +-
 ofproto/ofproto-provider.h |   18 ++-
 ofproto/ofproto.c          |  278 ++++++++++++++++++++++----------------------
 6 files changed, 206 insertions(+), 188 deletions(-)

diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index 686d61f..003b20b 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -67,7 +67,7 @@ ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle,
     LIST_FOR_EACH_POP (msg, node, &bundle->msg_list) {
         if (success && msg->type == OFPTYPE_FLOW_MOD) {
             /* Tell connmgr about successful flow mods. */
-            ofconn_report_flow_mod(ofconn, msg->fm.command);
+            ofconn_report_flow_mod(ofconn, msg->ofm.fm.command);
         }
         ofp_bundle_entry_free(msg);
     }
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index 65717df..1b2e333 100644
--- a/ofproto/bundles.h
+++ b/ofproto/bundles.h
@@ -34,18 +34,11 @@ extern "C" {
 struct ofp_bundle_entry {
     struct ovs_list   node;
     enum ofptype      type;  /* OFPTYPE_FLOW_MOD or OFPTYPE_PORT_MOD. */
-    long long         version;          /* Version in which the changes take
-                                         * effect. */
     union {
-        struct ofputil_flow_mod fm;   /* 'fm.ofpacts' must be malloced. */
-        struct ofputil_port_mod pm;
+        struct ofproto_flow_mod ofm;   /* ofm.fm.ofpacts must be malloced. */
+        struct ofproto_port_mod opm;
     };
 
-    /* Used during commit. */
-    struct ofport *port;                /* Affected port. */
-    struct rule_collection old_rules;   /* Affected rules. */
-    struct rule_collection new_rules;   /* Replacement rules. */
-
     /* OpenFlow header and some of the message contents for error reporting. */
     struct ofp_header ofp_msg[DIV_ROUND_UP(64, sizeof(struct ofp_header))];
 };
@@ -83,7 +76,6 @@ ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh)
     struct ofp_bundle_entry *entry = xmalloc(sizeof *entry);
 
     entry->type = type;
-    entry->version = 0;
 
     /* Max 64 bytes for error reporting. */
     memcpy(entry->ofp_msg, oh, MIN(ntohs(oh->length), sizeof entry->ofp_msg));
@@ -96,7 +88,7 @@ ofp_bundle_entry_free(struct ofp_bundle_entry *entry)
 {
     if (entry) {
         if (entry->type == OFPTYPE_FLOW_MOD) {
-            free(entry->fm.ofpacts);
+            free(entry->ofm.fm.ofpacts);
         }
         free(entry);
     }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 13d2f21..0a84525 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -392,9 +392,18 @@ static struct shash init_ofp_ports = SHASH_INITIALIZER(&init_ofp_ports);
  * it. */
 void
 ofproto_dpif_flow_mod(struct ofproto_dpif *ofproto,
-                      struct ofputil_flow_mod *fm)
+                      const struct ofputil_flow_mod *fm)
 {
-    ofproto_flow_mod(&ofproto->up, fm);
+    struct ofproto_flow_mod ofm;
+
+    /* Multiple threads may do this for the same 'fm' at the same time.
+     * Allocate ofproto_flow_mod with execution context from stack.
+     *
+     * Note: This copy could be avoided by making ofproto_flow_mod more
+     * complex, but that may not be desireable, and a learn action is not that
+     * fast to begin with. */
+    ofm.fm = *fm;
+    ofproto_flow_mod(&ofproto->up, &ofm);
 }
 
 /* Appends 'pin' to the queue of "packet ins" to be sent to the controller.
@@ -5477,28 +5486,28 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
                                const struct ofpbuf *ofpacts,
                                struct rule **rulep)
 {
-    struct ofputil_flow_mod fm;
+    struct ofproto_flow_mod ofm;
     struct rule_dpif *rule;
     int error;
 
-    fm.match = *match;
-    fm.priority = priority;
-    fm.new_cookie = htonll(0);
-    fm.cookie = htonll(0);
-    fm.cookie_mask = htonll(0);
-    fm.modify_cookie = false;
-    fm.table_id = TBL_INTERNAL;
-    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;
-    fm.ofpacts = ofpacts->data;
-    fm.ofpacts_len = ofpacts->size;
-
-    error = ofproto_flow_mod(&ofproto->up, &fm);
+    ofm.fm.match = *match;
+    ofm.fm.priority = priority;
+    ofm.fm.new_cookie = htonll(0);
+    ofm.fm.cookie = htonll(0);
+    ofm.fm.cookie_mask = htonll(0);
+    ofm.fm.modify_cookie = false;
+    ofm.fm.table_id = TBL_INTERNAL;
+    ofm.fm.command = OFPFC_ADD;
+    ofm.fm.idle_timeout = idle_timeout;
+    ofm.fm.hard_timeout = 0;
+    ofm.fm.importance = 0;
+    ofm.fm.buffer_id = 0;
+    ofm.fm.out_port = 0;
+    ofm.fm.flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY;
+    ofm.fm.ofpacts = ofpacts->data;
+    ofm.fm.ofpacts_len = ofpacts->size;
+
+    error = ofproto_flow_mod(&ofproto->up, &ofm);
     if (error) {
         VLOG_ERR_RL(&rl, "failed to add internal flow (%s)",
                     ofperr_to_string(error));
@@ -5508,8 +5517,8 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
 
     rule = rule_dpif_lookup_in_table(ofproto,
                                      ofproto_dpif_get_tables_version(ofproto),
-                                     TBL_INTERNAL, &fm.match.flow,
-                                     &fm.match.wc, false);
+                                     TBL_INTERNAL, &ofm.fm.match.flow,
+                                     &ofm.fm.match.wc, false);
     if (rule) {
         *rulep = &rule->up;
     } else {
@@ -5522,20 +5531,20 @@ int
 ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto,
                                   struct match *match, int priority)
 {
-    struct ofputil_flow_mod fm;
+    struct ofproto_flow_mod ofm;
     int error;
 
-    fm.match = *match;
-    fm.priority = priority;
-    fm.new_cookie = htonll(0);
-    fm.cookie = htonll(0);
-    fm.cookie_mask = htonll(0);
-    fm.modify_cookie = false;
-    fm.table_id = TBL_INTERNAL;
-    fm.flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY;
-    fm.command = OFPFC_DELETE_STRICT;
-
-    error = ofproto_flow_mod(&ofproto->up, &fm);
+    ofm.fm.match = *match;
+    ofm.fm.priority = priority;
+    ofm.fm.new_cookie = htonll(0);
+    ofm.fm.cookie = htonll(0);
+    ofm.fm.cookie_mask = htonll(0);
+    ofm.fm.modify_cookie = false;
+    ofm.fm.table_id = TBL_INTERNAL;
+    ofm.fm.flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY;
+    ofm.fm.command = OFPFC_DELETE_STRICT;
+
+    error = ofproto_flow_mod(&ofproto->up, &ofm);
     if (error) {
         VLOG_ERR_RL(&rl, "failed to delete internal flow (%s)",
                     ofperr_to_string(error));
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index bb6df5e..4af21cc 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -168,7 +168,8 @@ void ofproto_dpif_send_packet_in(struct ofproto_dpif *,
                                  struct ofproto_packet_in *);
 bool ofproto_dpif_wants_packet_in_on_miss(struct ofproto_dpif *);
 int ofproto_dpif_send_packet(const struct ofport_dpif *, struct dp_packet *);
-void ofproto_dpif_flow_mod(struct ofproto_dpif *, struct ofputil_flow_mod *);
+void ofproto_dpif_flow_mod(struct ofproto_dpif *,
+                           const struct ofputil_flow_mod *);
 struct rule_dpif *ofproto_dpif_refresh_rule(struct rule_dpif *);
 
 struct ofport_dpif *odp_port_to_ofport(const struct dpif_backer *, odp_port_t);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index e5739b0..ac2f99f 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1755,7 +1755,23 @@ extern const struct ofproto_class ofproto_dpif_class;
 int ofproto_class_register(const struct ofproto_class *);
 int ofproto_class_unregister(const struct ofproto_class *);
 
-enum ofperr ofproto_flow_mod(struct ofproto *, struct ofputil_flow_mod *)
+/* flow_mod with execution context. */
+struct ofproto_flow_mod {
+    struct ofputil_flow_mod fm;
+
+    cls_version_t version;              /* Version in which changes take
+                                         * effect. */
+    struct rule_collection old_rules;   /* Affected rules. */
+    struct rule_collection new_rules;   /* Replacement rules. */
+};
+
+/* port_mod with execution context. */
+struct ofproto_port_mod {
+    struct ofputil_port_mod pm;
+    struct ofport *port;                /* Affected port. */
+};
+
+enum ofperr ofproto_flow_mod(struct ofproto *, struct ofproto_flow_mod *)
     OVS_EXCLUDED(ofproto_mutex);
 void ofproto_add_flow(struct ofproto *, const struct match *, int priority,
                       const struct ofpact *ofpacts, size_t ofpacts_len)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index c794f07..e40a80e 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -260,9 +260,8 @@ static enum ofperr replace_rule_create(struct ofproto *,
                                        struct rule **new_rule)
     OVS_REQUIRES(ofproto_mutex);
 
-static void replace_rule_start(struct ofproto *,
-                               struct rule *old_rule,
-                               struct rule *new_rule,
+static void replace_rule_start(struct ofproto *, cls_version_t version,
+                               struct rule *old_rule, struct rule *new_rule,
                                struct cls_conjunction *, size_t n_conjs)
     OVS_REQUIRES(ofproto_mutex);
 
@@ -292,17 +291,15 @@ static bool ofproto_group_exists(const struct ofproto *ofproto,
     OVS_EXCLUDED(ofproto->groups_rwlock);
 static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *);
 static void handle_openflow(struct ofconn *, const struct ofpbuf *);
-static enum ofperr do_bundle_flow_mod_start(struct ofproto *,
-                                            struct ofputil_flow_mod *,
-                                            struct ofp_bundle_entry *)
+static enum ofperr ofproto_flow_mod_start(struct ofproto *,
+                                          struct ofproto_flow_mod *)
     OVS_REQUIRES(ofproto_mutex);
-static void do_bundle_flow_mod_finish(struct ofproto *,
-                                      struct ofputil_flow_mod *,
-                                      const struct flow_mod_requester *,
-                                      struct ofp_bundle_entry *)
+static void ofproto_flow_mod_finish(struct ofproto *,
+                                    struct ofproto_flow_mod *,
+                                    const struct flow_mod_requester *)
     OVS_REQUIRES(ofproto_mutex);
 static enum ofperr handle_flow_mod__(struct ofproto *,
-                                     struct ofputil_flow_mod *,
+                                     struct ofproto_flow_mod *,
                                      const struct flow_mod_requester *)
     OVS_EXCLUDED(ofproto_mutex);
 static void calc_duration(long long int start, long long int now,
@@ -2057,11 +2054,11 @@ simple_flow_mod(struct ofproto *ofproto,
                 const struct ofpact *ofpacts, size_t ofpacts_len,
                 enum ofp_flow_mod_command command)
 {
-    struct ofputil_flow_mod fm;
+    struct ofproto_flow_mod ofm;
 
-    flow_mod_init(&fm, match, priority, ofpacts, ofpacts_len, command);
+    flow_mod_init(&ofm.fm, match, priority, ofpacts, ofpacts_len, command);
 
-    return handle_flow_mod__(ofproto, &fm, NULL);
+    return handle_flow_mod__(ofproto, &ofm, NULL);
 }
 
 /* Adds a flow to OpenFlow flow table 0 in 'p' that matches 'cls_rule' and
@@ -2113,9 +2110,11 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
  * This is a helper function for in-band control and fail-open and the "learn"
  * action. */
 enum ofperr
-ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
+ofproto_flow_mod(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_EXCLUDED(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+
     /* Optimize for the most common case of a repeated learn action.
      * If an identical flow already exists we only need to update its
      * 'modified' time. */
@@ -2156,7 +2155,7 @@ ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
         }
     }
 
-    return handle_flow_mod__(ofproto, fm, NULL);
+    return handle_flow_mod__(ofproto, ofm, NULL);
 }
 
 /* Searches for a rule with matching criteria exactly equal to 'target' in
@@ -4457,10 +4456,12 @@ get_conjunctions(const struct ofputil_flow_mod *fm,
  *
  * The caller retains ownership of 'fm->ofpacts'. */
 static enum ofperr
-add_flow_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-               struct rule **old_rule, struct rule **new_rule)
+add_flow_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule **old_rule = &ofm->old_rules.stub[0];
+    struct rule **new_rule = &ofm->new_rules.stub[0];
     struct oftable *table;
     struct cls_rule cr;
     struct rule *rule;
@@ -4506,7 +4507,7 @@ add_flow_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
         return OFPERR_OFPBRC_EPERM;
     }
 
-    cls_rule_init(&cr, &fm->match, fm->priority, ofproto->tables_version + 1);
+    cls_rule_init(&cr, &fm->match, fm->priority, ofm->version);
 
     /* Check for the existence of an identical rule.
      * This will not return rules earlier marked for removal. */
@@ -4545,7 +4546,7 @@ add_flow_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     }
 
     get_conjunctions(fm, &conjs, &n_conjs);
-    replace_rule_start(ofproto, rule, *new_rule, conjs, n_conjs);
+    replace_rule_start(ofproto, ofm->version, rule, *new_rule, conjs, n_conjs);
     free(conjs);
 
     return 0;
@@ -4553,10 +4554,13 @@ add_flow_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 
 /* Revert the effects of add_flow_start(). */
 static void
-add_flow_revert(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                struct rule *old_rule, struct rule *new_rule)
+add_flow_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule *old_rule = ofm->old_rules.stub[0];
+    struct rule *new_rule = ofm->new_rules.stub[0];
+
     if (old_rule && fm->delete_reason == OFPRR_EVICTION) {
         /* Revert the eviction. */
         eviction_group_add_rule(old_rule);
@@ -4567,11 +4571,13 @@ add_flow_revert(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 
 /* To be called after version bump. */
 static void
-add_flow_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                const struct flow_mod_requester *req,
-                struct rule *old_rule, struct rule *new_rule)
+add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
+                const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule *old_rule = ofm->old_rules.stub[0];
+    struct rule *new_rule = ofm->new_rules.stub[0];
     struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
 
     replace_rule_finish(ofproto, fm, req, old_rule, new_rule, &dead_cookies);
@@ -4683,7 +4689,7 @@ replace_rule_create(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 }
 
 static void
-replace_rule_start(struct ofproto *ofproto,
+replace_rule_start(struct ofproto *ofproto, cls_version_t version,
                    struct rule *old_rule, struct rule *new_rule,
                    struct cls_conjunction *conjs, size_t n_conjs)
 {
@@ -4692,8 +4698,7 @@ replace_rule_start(struct ofproto *ofproto,
     /* 'old_rule' may be either an evicted rule or replaced rule. */
     if (old_rule) {
         /* Mark the old rule for removal in the next version. */
-        cls_rule_make_invisible_in_version(&old_rule->cr,
-                                           ofproto->tables_version + 1);
+        cls_rule_make_invisible_in_version(&old_rule->cr, version);
     } else {
         table->n_flows++;
     }
@@ -4792,11 +4797,12 @@ replace_rule_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 }
 
 static enum ofperr
-modify_flows_start__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                     struct rule_collection *old_rules,
-                     struct rule_collection *new_rules)
+modify_flows_start__(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule_collection *old_rules = &ofm->old_rules;
+    struct rule_collection *new_rules = &ofm->new_rules;
     enum ofperr error;
 
     rule_collection_init(new_rules);
@@ -4812,8 +4818,7 @@ modify_flows_start__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
             struct rule *new_rule;
             struct cls_rule cr;
 
-            cls_rule_clone_in_version(&cr, &old_rule->cr,
-                                      ofproto->tables_version + 1);
+            cls_rule_clone_in_version(&cr, &old_rule->cr, ofm->version);
             error = replace_rule_create(ofproto, fm, &cr, old_rule->table_id,
                                         old_rule, &new_rule);
             if (!error) {
@@ -4828,15 +4833,14 @@ modify_flows_start__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 
         get_conjunctions(fm, &conjs, &n_conjs);
         for (i = 0; i < old_rules->n; i++) {
-            replace_rule_start(ofproto, old_rules->rules[i],
+            replace_rule_start(ofproto, ofm->version, old_rules->rules[i],
                                new_rules->rules[i], conjs, n_conjs);
         }
         free(conjs);
     } else if (!(fm->cookie_mask != htonll(0)
                  || fm->new_cookie == OVS_BE64_MAX)) {
         /* No match, add a new flow. */
-        error = add_flow_start(ofproto, fm, &old_rules->rules[0],
-                               &new_rules->rules[0]);
+        error = add_flow_start(ofproto, ofm);
         if (!error) {
             ovs_assert(fm->delete_reason == OFPRR_EVICTION
                        || !old_rules->rules[0]);
@@ -4855,11 +4859,11 @@ modify_flows_start__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
  * 'ofconn' is used to retrieve the packet buffer specified in fm->buffer_id,
  * if any. */
 static enum ofperr
-modify_flows_start_loose(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                         struct rule_collection *old_rules,
-                         struct rule_collection *new_rules)
+modify_flows_start_loose(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule_collection *old_rules = &ofm->old_rules;
     struct rule_criteria criteria;
     enum ofperr error;
 
@@ -4871,7 +4875,7 @@ modify_flows_start_loose(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
     rule_criteria_destroy(&criteria);
 
     if (!error) {
-        error = modify_flows_start__(ofproto, fm, old_rules, new_rules);
+        error = modify_flows_start__(ofproto, ofm);
     }
 
     if (error) {
@@ -4881,14 +4885,15 @@ modify_flows_start_loose(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 }
 
 static void
-modify_flows_revert(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                    struct rule_collection *old_rules,
-                    struct rule_collection *new_rules)
+modify_flows_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct rule_collection *old_rules = &ofm->old_rules;
+    struct rule_collection *new_rules = &ofm->new_rules;
+
     /* Old rules were not changed yet, only need to revert new rules. */
     if (old_rules->n == 0 && new_rules->n == 1) {
-        add_flow_revert(ofproto, fm, old_rules->rules[0], new_rules->rules[0]);
+        add_flow_revert(ofproto, ofm);
     } else if (old_rules->n > 0) {
         for (size_t i = 0; i < old_rules->n; i++) {
             replace_rule_revert(ofproto, old_rules->rules[i],
@@ -4900,15 +4905,16 @@ modify_flows_revert(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 }
 
 static void
-modify_flows_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                    const struct flow_mod_requester *req,
-                    struct rule_collection *old_rules,
-                    struct rule_collection *new_rules)
+modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
+                    const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule_collection *old_rules = &ofm->old_rules;
+    struct rule_collection *new_rules = &ofm->new_rules;
+
     if (old_rules->n == 0 && new_rules->n == 1) {
-        add_flow_finish(ofproto, fm, req, old_rules->rules[0],
-                        new_rules->rules[0]);
+        add_flow_finish(ofproto, ofm, req);
     } else if (old_rules->n > 0) {
         struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
 
@@ -4929,11 +4935,11 @@ modify_flows_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 /* Implements OFPFC_MODIFY_STRICT.  Returns 0 on success or an OpenFlow error
  * code on failure. */
 static enum ofperr
-modify_flow_start_strict(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                         struct rule_collection *old_rules,
-                         struct rule_collection *new_rules)
+modify_flow_start_strict(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule_collection *old_rules = &ofm->old_rules;
     struct rule_criteria criteria;
     enum ofperr error;
 
@@ -4947,7 +4953,7 @@ modify_flow_start_strict(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 
     if (!error) {
         /* collect_rules_strict() can return max 1 rule. */
-        error = modify_flows_start__(ofproto, fm, old_rules, new_rules);
+        error = modify_flows_start__(ofproto, ofm);
     }
 
     if (error) {
@@ -4959,7 +4965,7 @@ modify_flow_start_strict(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 /* OFPFC_DELETE implementation. */
 
 static void
-delete_flows_start__(struct ofproto *ofproto,
+delete_flows_start__(struct ofproto *ofproto, cls_version_t version,
                      const struct rule_collection *rules)
     OVS_REQUIRES(ofproto_mutex)
 {
@@ -4968,8 +4974,7 @@ delete_flows_start__(struct ofproto *ofproto,
         struct oftable *table = &ofproto->tables[rule->table_id];
 
         table->n_flows--;
-        cls_rule_make_invisible_in_version(&rule->cr,
-                                           ofproto->tables_version + 1);
+        cls_rule_make_invisible_in_version(&rule->cr, version);
     }
 }
 
@@ -5015,7 +5020,7 @@ delete_flows__(struct rule_collection *rules,
     if (rules->n) {
         struct ofproto *ofproto = rules->rules[0]->ofproto;
 
-        delete_flows_start__(ofproto, rules);
+        delete_flows_start__(ofproto, ofproto->tables_version + 1, rules);
         ofproto_bump_tables_version(ofproto);
         delete_flows_finish__(ofproto, rules, reason, req);
         ofmonitor_flush(ofproto->connmgr);
@@ -5024,11 +5029,11 @@ delete_flows__(struct rule_collection *rules,
 
 /* Implements OFPFC_DELETE. */
 static enum ofperr
-delete_flows_start_loose(struct ofproto *ofproto,
-                         const struct ofputil_flow_mod *fm,
-                         struct rule_collection *rules)
+delete_flows_start_loose(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    const struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule_collection *rules = &ofm->old_rules;
     struct rule_criteria criteria;
     enum ofperr error;
 
@@ -5041,17 +5046,18 @@ delete_flows_start_loose(struct ofproto *ofproto,
     rule_criteria_destroy(&criteria);
 
     if (!error) {
-        delete_flows_start__(ofproto, rules);
+        delete_flows_start__(ofproto, ofm->version, rules);
     }
 
     return error;
 }
 
 static void
-delete_flows_revert(struct ofproto *ofproto,
-                    struct rule_collection *rules)
+delete_flows_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    struct rule_collection *rules = &ofm->old_rules;
+
     for (size_t i = 0; i < rules->n; i++) {
         struct rule *rule = rules->rules[i];
         struct oftable *table = &ofproto->tables[rule->table_id];
@@ -5067,21 +5073,22 @@ delete_flows_revert(struct ofproto *ofproto,
 
 static void
 delete_flows_finish(struct ofproto *ofproto,
-                    const struct ofputil_flow_mod *fm,
-                    const struct flow_mod_requester *req,
-                    struct rule_collection *rules)
+                    struct ofproto_flow_mod *ofm,
+                    const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
-    delete_flows_finish__(ofproto, rules, fm->delete_reason, req);
+    delete_flows_finish__(ofproto, &ofm->old_rules, ofm->fm.delete_reason,
+                          req);
 }
 
 /* Implements OFPFC_DELETE_STRICT. */
 static enum ofperr
 delete_flow_start_strict(struct ofproto *ofproto,
-                         const struct ofputil_flow_mod *fm,
-                         struct rule_collection *rules)
+                         struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
+    const struct ofputil_flow_mod *fm = &ofm->fm;
+    struct rule_collection *rules = &ofm->old_rules;
     struct rule_criteria criteria;
     enum ofperr error;
 
@@ -5094,7 +5101,7 @@ delete_flow_start_strict(struct ofproto *ofproto,
     rule_criteria_destroy(&criteria);
 
     if (!error) {
-        delete_flows_start__(ofproto, rules);
+        delete_flows_start__(ofproto, ofm->version, rules);
     }
 
     return error;
@@ -5186,7 +5193,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     OVS_EXCLUDED(ofproto_mutex)
 {
     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
-    struct ofputil_flow_mod fm;
+    struct ofproto_flow_mod ofm;
     uint64_t ofpacts_stub[1024 / 8];
     struct ofpbuf ofpacts;
     enum ofperr error;
@@ -5197,25 +5204,26 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     }
 
     ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
-    error = ofputil_decode_flow_mod(&fm, oh, ofconn_get_protocol(ofconn),
+    error = ofputil_decode_flow_mod(&ofm.fm, oh, ofconn_get_protocol(ofconn),
                                     &ofpacts,
                                     u16_to_ofp(ofproto->max_ports),
                                     ofproto->n_tables);
     if (!error) {
-        error = ofproto_check_ofpacts(ofproto, fm.ofpacts, fm.ofpacts_len);
+        error = ofproto_check_ofpacts(ofproto, ofm.fm.ofpacts,
+                                      ofm.fm.ofpacts_len);
     }
     if (!error) {
         struct flow_mod_requester req;
 
         req.ofconn = ofconn;
         req.request = oh;
-        error = handle_flow_mod__(ofproto, &fm, &req);
+        error = handle_flow_mod__(ofproto, &ofm, &req);
     }
     if (error) {
         goto exit_free_ofpacts;
     }
 
-    ofconn_report_flow_mod(ofconn, fm.command);
+    ofconn_report_flow_mod(ofconn, ofm.fm.command);
 
 exit_free_ofpacts:
     ofpbuf_uninit(&ofpacts);
@@ -5224,18 +5232,18 @@ exit:
 }
 
 static enum ofperr
-handle_flow_mod__(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
+handle_flow_mod__(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                   const struct flow_mod_requester *req)
     OVS_EXCLUDED(ofproto_mutex)
 {
-    struct ofp_bundle_entry be;
     enum ofperr error;
 
     ovs_mutex_lock(&ofproto_mutex);
-    error = do_bundle_flow_mod_start(ofproto, fm, &be);
+    ofm->version = ofproto->tables_version + 1;
+    error = ofproto_flow_mod_start(ofproto, ofm);
     if (!error) {
         ofproto_bump_tables_version(ofproto);
-        do_bundle_flow_mod_finish(ofproto, fm, req, &be);
+        ofproto_flow_mod_finish(ofproto, ofm, req);
     }
     ofmonitor_flush(ofproto->connmgr);
     ovs_mutex_unlock(&ofproto_mutex);
@@ -6458,14 +6466,14 @@ delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup)
     OVS_RELEASES(ofproto->groups_rwlock)
 {
     struct match match;
-    struct ofputil_flow_mod fm;
+    struct ofproto_flow_mod ofm;
 
     /* Delete all flow entries containing this group in a group action */
     match_init_catchall(&match);
-    flow_mod_init(&fm, &match, 0, NULL, 0, OFPFC_DELETE);
-    fm.delete_reason = OFPRR_GROUP_DELETE;
-    fm.out_group = ofgroup->group_id;
-    handle_flow_mod__(ofproto, &fm, NULL);
+    flow_mod_init(&ofm.fm, &match, 0, NULL, 0, OFPFC_DELETE);
+    ofm.fm.delete_reason = OFPRR_GROUP_DELETE;
+    ofm.fm.out_group = ofgroup->group_id;
+    handle_flow_mod__(ofproto, &ofm, NULL);
 
     hmap_remove(&ofproto->groups, &ofgroup->hmap_node);
     /* No-one can find this group any more. */
@@ -6608,49 +6616,45 @@ handle_table_mod(struct ofconn *ofconn, const struct ofp_header *oh)
 }
 
 static enum ofperr
-do_bundle_flow_mod_start(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                         struct ofp_bundle_entry *be)
+ofproto_flow_mod_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
-    switch (fm->command) {
+    switch (ofm->fm.command) {
     case OFPFC_ADD:
-        return add_flow_start(ofproto, fm, &be->old_rules.stub[0],
-                              &be->new_rules.stub[0]);
+        return add_flow_start(ofproto, ofm);
+        /* , &be->old_rules.stub[0],
+           &be->new_rules.stub[0]); */
     case OFPFC_MODIFY:
-        return modify_flows_start_loose(ofproto, fm, &be->old_rules,
-                                        &be->new_rules);
+        return modify_flows_start_loose(ofproto, ofm);
     case OFPFC_MODIFY_STRICT:
-        return modify_flow_start_strict(ofproto, fm, &be->old_rules,
-                                        &be->new_rules);
+        return modify_flow_start_strict(ofproto, ofm);
     case OFPFC_DELETE:
-        return delete_flows_start_loose(ofproto, fm, &be->old_rules);
+        return delete_flows_start_loose(ofproto, ofm);
 
     case OFPFC_DELETE_STRICT:
-        return delete_flow_start_strict(ofproto, fm, &be->old_rules);
+        return delete_flow_start_strict(ofproto, ofm);
     }
 
     return OFPERR_OFPFMFC_BAD_COMMAND;
 }
 
 static void
-do_bundle_flow_mod_revert(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                          struct ofp_bundle_entry *be)
+ofproto_flow_mod_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
-    switch (fm->command) {
+    switch (ofm->fm.command) {
     case OFPFC_ADD:
-        add_flow_revert(ofproto, fm, be->old_rules.stub[0],
-                        be->new_rules.stub[0]);
+        add_flow_revert(ofproto, ofm);
         break;
 
     case OFPFC_MODIFY:
     case OFPFC_MODIFY_STRICT:
-        modify_flows_revert(ofproto, fm, &be->old_rules, &be->new_rules);
+        modify_flows_revert(ofproto, ofm);
         break;
 
     case OFPFC_DELETE:
     case OFPFC_DELETE_STRICT:
-        delete_flows_revert(ofproto, &be->old_rules);
+        delete_flows_revert(ofproto, ofm);
         break;
 
     default:
@@ -6659,25 +6663,24 @@ do_bundle_flow_mod_revert(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
 }
 
 static void
-do_bundle_flow_mod_finish(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
-                          const struct flow_mod_requester *req,
-                          struct ofp_bundle_entry *be)
+ofproto_flow_mod_finish(struct ofproto *ofproto,
+                        struct ofproto_flow_mod *ofm,
+                        const struct flow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
-    switch (fm->command) {
+    switch (ofm->fm.command) {
     case OFPFC_ADD:
-        add_flow_finish(ofproto, fm, req, be->old_rules.stub[0],
-                        be->new_rules.stub[0]);
+        add_flow_finish(ofproto, ofm, req);
         break;
 
     case OFPFC_MODIFY:
     case OFPFC_MODIFY_STRICT:
-        modify_flows_finish(ofproto, fm, req, &be->old_rules, &be->new_rules);
+        modify_flows_finish(ofproto, ofm, req);
         break;
 
     case OFPFC_DELETE:
     case OFPFC_DELETE_STRICT:
-        delete_flows_finish(ofproto, fm, req, &be->old_rules);
+        delete_flows_finish(ofproto, ofm, req);
         break;
 
     default:
@@ -6706,7 +6709,7 @@ static enum ofperr
 do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
 {
     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
-    cls_version_t visible_version = ofproto->tables_version;
+    cls_version_t version = ofproto->tables_version + 1;
     struct ofp_bundle *bundle;
     struct ofp_bundle_entry *be;
     enum ofperr error;
@@ -6732,26 +6735,25 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                     error = OFPERR_OFPBFC_MSG_FAILED;
                 } else {
                     prev_is_port_mod = true;
-                    error = port_mod_start(ofconn, &be->pm, &be->port);
+                    error = port_mod_start(ofconn, &be->opm.pm, &be->opm.port);
                 }
             } else if (be->type == OFPTYPE_FLOW_MOD) {
                 /* Flow mods between port mods are applied as a single
                  * version, but the versions are published only after
                  * we know the commit is successful. */
                 if (prev_is_port_mod) {
-                    ++ofproto->tables_version;
+                    ++version;
                 }
                 prev_is_port_mod = false;
-                error = do_bundle_flow_mod_start(ofproto, &be->fm, be);
+                /* Store the version in which the changes should take
+                 * effect. */
+                be->ofm.version = version;
+                error = ofproto_flow_mod_start(ofproto, &be->ofm);
             } else {
                 OVS_NOT_REACHED();
             }
             if (error) {
                 break;
-            } else {
-                /* Store the version in which the changes should take
-                 * effect. */
-                be->version = ofproto->tables_version + 1;
             }
         }
 
@@ -6765,25 +6767,26 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
             /* 2. Revert.  Undo all the changes made above. */
             LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
                 if (be->type == OFPTYPE_FLOW_MOD) {
-                    do_bundle_flow_mod_revert(ofproto, &be->fm, be);
+                    ofproto_flow_mod_revert(ofproto, &be->ofm);
                 }
                 /* Nothing needs to be reverted for a port mod. */
             }
         } else {
             /* 4. Finish. */
             LIST_FOR_EACH (be, node, &bundle->msg_list) {
-                /* Bump the lookup version to the one of the current message.
-                 * This makes all the changes in the bundle at this version
-                 * visible to lookups at once. */
-                if (visible_version < be->version) {
-                    visible_version = be->version;
-                    ofproto->ofproto_class->set_tables_version(
-                        ofproto, visible_version);
-                }
                 if (be->type == OFPTYPE_FLOW_MOD) {
                     struct flow_mod_requester req = { ofconn, be->ofp_msg };
 
-                    do_bundle_flow_mod_finish(ofproto, &be->fm, &req, be);
+                    /* Bump the lookup version to the one of the current
+                     * message.  This makes all the changes in the bundle at
+                     * this version visible to lookups at once. */
+                    if (ofproto->tables_version < be->ofm.version) {
+                        ofproto->tables_version = be->ofm.version;
+                        ofproto->ofproto_class->set_tables_version(
+                            ofproto, ofproto->tables_version);
+                    }
+
+                    ofproto_flow_mod_finish(ofproto, &be->ofm, &req);
                 } else if (be->type == OFPTYPE_PORT_MOD) {
                     /* Perform the actual port mod. This is not atomic, i.e.,
                      * the effects will be immediately seen by upcall
@@ -6791,14 +6794,11 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                      * be noted that port configuration changes can originate
                      * also from OVSDB changes asynchronously to all upcall
                      * processing. */
-                    port_mod_finish(ofconn, &be->pm, be->port);
+                    port_mod_finish(ofconn, &be->opm.pm, be->opm.port);
                 }
             }
         }
 
-        /* Reset the tables_version. */
-        ofproto->tables_version = visible_version;
-
         ofmonitor_flush(ofproto->connmgr);
         ovs_mutex_unlock(&ofproto_mutex);
 
@@ -6885,23 +6885,23 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh)
     bmsg = ofp_bundle_entry_alloc(type, badd.msg);
 
     if (type == OFPTYPE_PORT_MOD) {
-        error = ofputil_decode_port_mod(badd.msg, &bmsg->pm, false);
+        error = ofputil_decode_port_mod(badd.msg, &bmsg->opm.pm, false);
     } else if (type == OFPTYPE_FLOW_MOD) {
         struct ofpbuf ofpacts;
         uint64_t ofpacts_stub[1024 / 8];
 
         ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
-        error = ofputil_decode_flow_mod(&bmsg->fm, badd.msg,
+        error = ofputil_decode_flow_mod(&bmsg->ofm.fm, badd.msg,
                                         ofconn_get_protocol(ofconn),
                                         &ofpacts,
                                         u16_to_ofp(ofproto->max_ports),
                                         ofproto->n_tables);
         /* Move actions to heap. */
-        bmsg->fm.ofpacts = ofpbuf_steal_data(&ofpacts);
+        bmsg->ofm.fm.ofpacts = ofpbuf_steal_data(&ofpacts);
 
-        if (!error && bmsg->fm.ofpacts_len) {
-            error = ofproto_check_ofpacts(ofproto, bmsg->fm.ofpacts,
-                                          bmsg->fm.ofpacts_len);
+        if (!error && bmsg->ofm.fm.ofpacts_len) {
+            error = ofproto_check_ofpacts(ofproto, bmsg->ofm.fm.ofpacts,
+                                          bmsg->ofm.fm.ofpacts_len);
         }
     } else {
         OVS_NOT_REACHED();
-- 
1.7.10.4




More information about the dev mailing list