[ovs-dev] [PATCH v3] ofproto: Reduce bundle memory use.

Jarno Rajahalme jarno at ovn.org
Sat Jul 30 01:37:57 UTC 2016


Instead of storing the (big) struct ofputil_flow_mod, create the new
rule and/or create the rule criteria for matching at bundle message
insert time.  These can be uninitialized right after the start phase
during the commit, which may also reduce the total memory needed
during the bundle commit.

Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
---
v3: Reduced code duplication + use the template rule for the first
    modified instead of creating another rule for it.

 ofproto/bundles.c          |   2 +-
 ofproto/bundles.h          |   4 +-
 ofproto/ofproto-provider.h |  43 +++-
 ofproto/ofproto.c          | 577 ++++++++++++++++++++++++++++-----------------
 tests/ofproto.at           |   4 -
 5 files changed, 400 insertions(+), 230 deletions(-)

diff --git a/ofproto/bundles.c b/ofproto/bundles.c
index aa8e58c..e8fb7ba 100644
--- a/ofproto/bundles.c
+++ b/ofproto/bundles.c
@@ -66,7 +66,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->ofm.fm.command);
+            ofconn_report_flow_mod(ofconn, msg->ofm.command);
         }
         ofp_bundle_entry_free(msg);
     }
diff --git a/ofproto/bundles.h b/ofproto/bundles.h
index 2054303..802de77 100644
--- a/ofproto/bundles.h
+++ b/ofproto/bundles.h
@@ -36,7 +36,7 @@ struct ofp_bundle_entry {
     enum ofptype      type;  /* OFPTYPE_FLOW_MOD, OFPTYPE_PORT_MOD, or
                               * OFPTYPE_GROUP_MOD. */
     union {
-        struct ofproto_flow_mod ofm;   /* ofm.fm.ofpacts must be malloced. */
+        struct ofproto_flow_mod ofm;
         struct ofproto_port_mod opm;
         struct ofproto_group_mod ogm;
     };
@@ -90,7 +90,7 @@ ofp_bundle_entry_free(struct ofp_bundle_entry *entry)
 {
     if (entry) {
         if (entry->type == OFPTYPE_FLOW_MOD) {
-            free(entry->ofm.fm.ofpacts);
+            ofproto_flow_mod_uninit(&entry->ofm);
         } else if (entry->type == OFPTYPE_GROUP_MOD) {
             ofputil_uninit_group_mod(&entry->ogm.gm);
         }
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 67a85e0..401d1b5 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1838,25 +1838,64 @@ extern const struct ofproto_class ofproto_dpif_class;
 int ofproto_class_register(const struct ofproto_class *);
 int ofproto_class_unregister(const struct ofproto_class *);
 
+/* Criteria that flow_mod and other operations use for selecting rules on
+ * which to operate. */
+struct rule_criteria {
+    /* An OpenFlow table or 255 for all tables. */
+    uint8_t table_id;
+
+    /* OpenFlow matching criteria.  Interpreted different in "loose" way by
+     * collect_rules_loose() and "strict" way by collect_rules_strict(), as
+     * defined in the OpenFlow spec. */
+    struct cls_rule cr;
+    ovs_version_t version;
+
+    /* Matching criteria for the OpenFlow cookie.  Consider a bit B in a rule's
+     * cookie and the corresponding bits C in 'cookie' and M in 'cookie_mask'.
+     * The rule will not be selected if M is 1 and B != C.  */
+    ovs_be64 cookie;
+    ovs_be64 cookie_mask;
+
+    /* Selection based on actions within a rule:
+     *
+     * If out_port != OFPP_ANY, selects only rules that output to out_port.
+     * If out_group != OFPG_ALL, select only rules that output to out_group. */
+    ofp_port_t out_port;
+    uint32_t out_group;
+
+    /* If true, collects only rules that are modifiable. */
+    bool include_hidden;
+    bool include_readonly;
+};
+
 /* flow_mod with execution context. */
 struct ofproto_flow_mod {
-    struct ofputil_flow_mod fm;
+    /* Allocated by 'init' phase, may be freed after 'start' phase, as these
+     * are not needed for 'revert' nor 'finish'. */
+    struct rule *temp_rule;
+    struct rule_criteria criteria;
+    struct cls_conjunction *conjs;
+    size_t n_conjs;
 
     /* Replicate needed fields from ofputil_flow_mod to not need it after the
      * flow has been created. */
     uint16_t command;
     uint32_t buffer_id;
     bool modify_cookie;
-
+    /* Fields derived from ofputil_flow_mod. */
     bool modify_may_add_flow;
     enum nx_flow_update_event event;
 
+    /* These are only used during commit execution.
+     * ofproto_flow_mod_uninit() does NOT clean these up. */
     ovs_version_t version;              /* Version in which changes take
                                          * effect. */
     struct rule_collection old_rules;   /* Affected rules. */
     struct rule_collection new_rules;   /* Replacement rules. */
 };
 
+void ofproto_flow_mod_uninit(struct ofproto_flow_mod *);
+
 /* port_mod with execution context. */
 struct ofproto_port_mod {
     struct ofputil_port_mod pm;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 8e59c69..0a5383b 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -129,36 +129,6 @@ static void eviction_group_add_rule(struct rule *)
 static void eviction_group_remove_rule(struct rule *)
     OVS_REQUIRES(ofproto_mutex);
 
-/* Criteria that flow_mod and other operations use for selecting rules on
- * which to operate. */
-struct rule_criteria {
-    /* An OpenFlow table or 255 for all tables. */
-    uint8_t table_id;
-
-    /* OpenFlow matching criteria.  Interpreted different in "loose" way by
-     * collect_rules_loose() and "strict" way by collect_rules_strict(), as
-     * defined in the OpenFlow spec. */
-    struct cls_rule cr;
-    ovs_version_t version;
-
-    /* Matching criteria for the OpenFlow cookie.  Consider a bit B in a rule's
-     * cookie and the corresponding bits C in 'cookie' and M in 'cookie_mask'.
-     * The rule will not be selected if M is 1 and B != C.  */
-    ovs_be64 cookie;
-    ovs_be64 cookie_mask;
-
-    /* Selection based on actions within a rule:
-     *
-     * If out_port != OFPP_ANY, selects only rules that output to out_port.
-     * If out_group != OFPG_ALL, select only rules that output to out_group. */
-    ofp_port_t out_port;
-    uint32_t out_group;
-
-    /* If true, collects only rules that are modifiable. */
-    bool include_hidden;
-    bool include_readonly;
-};
-
 static void rule_criteria_init(struct rule_criteria *, uint8_t table_id,
                                const struct match *match, int priority,
                                ovs_version_t version,
@@ -264,17 +234,19 @@ struct openflow_mod_requester {
 };
 
 /* OpenFlow. */
-static enum ofperr replace_rule_create(struct ofproto *,
-                                       struct ofproto_flow_mod *ofm,
-                                       const struct ofputil_flow_mod *,
-                                       struct cls_rule *cr, uint8_t table_id,
-                                       struct rule *old_rule,
+static enum ofperr ofproto_rule_create(struct ofproto *, struct cls_rule *,
+                                       uint8_t table_id, ovs_be64 new_cookie,
+                                       uint16_t idle_timeout,
+                                       uint16_t hard_timeout,
+                                       enum ofputil_flow_mod_flags flags,
+                                       uint16_t importance,
+                                       const struct ofpact *ofpacts,
+                                       size_t ofpacts_len,
                                        struct rule **new_rule)
-    OVS_REQUIRES(ofproto_mutex);
+    OVS_NO_THREAD_SAFETY_ANALYSIS;
 
-static void replace_rule_start(struct ofproto *, ovs_version_t version,
-                               struct rule *old_rule, struct rule *new_rule,
-                               struct cls_conjunction *, size_t n_conjs)
+static void replace_rule_start(struct ofproto *, struct ofproto_flow_mod *,
+                               struct rule *old_rule, struct rule *new_rule)
     OVS_REQUIRES(ofproto_mutex);
 
 static void replace_rule_revert(struct ofproto *, struct rule *old_rule,
@@ -297,6 +269,10 @@ static void send_buffered_packet(const struct openflow_mod_requester *,
 
 static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id);
 static void handle_openflow(struct ofconn *, const struct ofpbuf *);
+static enum ofperr ofproto_flow_mod_init(struct ofproto *,
+                                         struct ofproto_flow_mod *,
+                                         const struct ofputil_flow_mod *fm)
+    OVS_EXCLUDED(ofproto_mutex);
 static enum ofperr ofproto_flow_mod_start(struct ofproto *,
                                           struct ofproto_flow_mod *)
     OVS_REQUIRES(ofproto_mutex);
@@ -2196,8 +2172,7 @@ ofproto_flow_mod(struct ofproto *ofproto, const struct ofputil_flow_mod *fm)
                                       OVS_VERSION_MAX));
         if (rule) {
             /* Reading many of the rule fields and writing on 'modified'
-             * requires the rule->mutex.  Also, rule->actions may change
-             * if rule->mutex is not held. */
+             * requires the rule->mutex. */
             const struct rule_actions *actions;
 
             ovs_mutex_lock(&rule->mutex);
@@ -4073,6 +4048,7 @@ static void
 rule_criteria_destroy(struct rule_criteria *criteria)
 {
     cls_rule_destroy(&criteria->cr);
+    criteria->version = OVS_VERSION_NOT_REMOVED; /* Mark as destroyed. */
 }
 
 /* Schedules postponed removal of rules, destroys 'rules'. */
@@ -4627,7 +4603,6 @@ evict_rules_from_table(struct oftable *table)
 static void
 get_conjunctions(const struct ofputil_flow_mod *fm,
                  struct cls_conjunction **conjsp, size_t *n_conjsp)
-    OVS_REQUIRES(ofproto_mutex)
 {
     struct cls_conjunction *conjs = NULL;
     int n_conjs = 0;
@@ -4673,24 +4648,28 @@ get_conjunctions(const struct ofputil_flow_mod *fm,
  * be reverted.
  *
  * The caller retains ownership of 'fm->ofpacts'. */
+
+/* Creates a new flow according to 'fm' and stores it to 'ofm' for later
+ * reference.  If the flow replaces other flow, it will be updated to match
+ * modify semantics later.
+ *
+ * On successful return the caller must complete the operation by calling
+ * add_flow_start(), and if that succeeds, then either add_flow_finish(), or
+ * add_flow_revert() if the operation needs to be reverted due to a later
+ * failure.
+ */
 static enum ofperr
-add_flow_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
-    OVS_REQUIRES(ofproto_mutex)
+add_flow_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
+              const struct ofputil_flow_mod *fm)
+    OVS_EXCLUDED(ofproto_mutex)
 {
-    struct ofputil_flow_mod *fm = &ofm->fm;
-    struct rule **old_rule = rule_collection_stub(&ofm->old_rules);
-    struct rule **new_rule = rule_collection_stub(&ofm->new_rules);
     struct oftable *table;
     struct cls_rule cr;
-    struct rule *rule;
     uint8_t table_id;
-    struct cls_conjunction *conjs;
-    size_t n_conjs;
     enum ofperr error;
 
     if (!check_table_id(ofproto, fm->table_id)) {
-        error = OFPERR_OFPBRC_BAD_TABLE_ID;
-        return error;
+        return OFPERR_OFPBRC_BAD_TABLE_ID;
     }
 
     /* Pick table. */
@@ -4727,47 +4706,71 @@ add_flow_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
 
     cls_rule_init(&cr, &fm->match, fm->priority);
 
+    /* Allocate new rule.  Destroys 'cr'. */
+    error = ofproto_rule_create(ofproto, &cr, table - ofproto->tables,
+                                fm->new_cookie, fm->idle_timeout,
+                                fm->hard_timeout, fm->flags, fm->importance,
+                                fm->ofpacts, fm->ofpacts_len, &ofm->temp_rule);
+    if (error) {
+        return error;
+    }
+
+    get_conjunctions(fm, &ofm->conjs, &ofm->n_conjs);
+    return 0;
+}
+
+static enum ofperr
+add_flow_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    struct rule *old_rule = NULL;
+    struct rule *new_rule = ofm->temp_rule;
+    const struct rule_actions *actions = rule_get_actions(new_rule);
+    struct oftable *table = &ofproto->tables[new_rule->table_id];
+    enum ofperr error;
+
+    /* Must check actions while holding ofproto_mutex to avoid a race. */
+    error = ofproto_check_ofpacts(ofproto, actions->ofpacts,
+                                  actions->ofpacts_len);
+    if (error) {
+        return error;
+    }
+
     /* Check for the existence of an identical rule.
      * This will not return rules earlier marked for removal. */
-    rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls, &cr,
-                                                           ofm->version));
-    *old_rule = rule;
-    if (!rule) {
+    old_rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls,
+                                                               &new_rule->cr,
+                                                               ofm->version));
+    if (!old_rule) {
         /* Check for overlap, if requested. */
-        if (fm->flags & OFPUTIL_FF_CHECK_OVERLAP
-            && classifier_rule_overlaps(&table->cls, &cr, ofm->version)) {
-            cls_rule_destroy(&cr);
+        if (new_rule->flags & OFPUTIL_FF_CHECK_OVERLAP
+            && classifier_rule_overlaps(&table->cls, &new_rule->cr,
+                                        ofm->version)) {
             return OFPERR_OFPFMFC_OVERLAP;
         }
 
         /* If necessary, evict an existing rule to clear out space. */
         if (table->n_flows >= table->max_flows) {
-            if (!choose_rule_to_evict(table, &rule)) {
-                error = OFPERR_OFPFMFC_TABLE_FULL;
-                cls_rule_destroy(&cr);
-                return error;
+            if (!choose_rule_to_evict(table, &old_rule)) {
+                return OFPERR_OFPFMFC_TABLE_FULL;
             }
-            eviction_group_remove_rule(rule);
-            /* Marks '*old_rule' as an evicted rule rather than replaced rule.
+            eviction_group_remove_rule(old_rule);
+            /* Marks 'old_rule' as an evicted rule rather than replaced rule.
              */
-            rule->removed_reason = OFPRR_EVICTION;
-            *old_rule = rule;
+            old_rule->removed_reason = OFPRR_EVICTION;
         }
     } else {
         ofm->modify_cookie = true;
     }
 
-    /* Allocate new rule. */
-    error = replace_rule_create(ofproto, ofm, fm, &cr, table - ofproto->tables,
-                                rule, new_rule);
-    if (error) {
-        return error;
+    if (old_rule) {
+        rule_collection_add(&ofm->old_rules, old_rule);
     }
+    /* Take ownership of the temp_rule. */
+    rule_collection_stub(&ofm->new_rules)[0] = new_rule;
+    ofm->temp_rule = NULL;
 
-    get_conjunctions(fm, &conjs, &n_conjs);
-    replace_rule_start(ofproto, ofm->version, rule, *new_rule, conjs, n_conjs);
-    free(conjs);
-
+    replace_rule_start(ofproto, ofm, old_rule, new_rule);
     return 0;
 }
 
@@ -4776,14 +4779,10 @@ static void
 add_flow_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
-    struct rule *old_rule = rule_collection_stub(&ofm->old_rules)[0];
+    struct rule *old_rule = rule_collection_n(&ofm->old_rules)
+        ? rule_collection_stub(&ofm->old_rules)[0] : NULL;
     struct rule *new_rule = rule_collection_stub(&ofm->new_rules)[0];
 
-    if (old_rule && old_rule->removed_reason == OFPRR_EVICTION) {
-        /* Revert the eviction. */
-        eviction_group_add_rule(old_rule);
-    }
-
     replace_rule_revert(ofproto, old_rule, new_rule);
 }
 
@@ -4793,7 +4792,8 @@ add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                 const struct openflow_mod_requester *req)
     OVS_REQUIRES(ofproto_mutex)
 {
-    struct rule *old_rule = rule_collection_stub(&ofm->old_rules)[0];
+    struct rule *old_rule = rule_collection_n(&ofm->old_rules)
+        ? rule_collection_stub(&ofm->old_rules)[0] : NULL;
     struct rule *new_rule = rule_collection_stub(&ofm->new_rules)[0];
     struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
 
@@ -4816,14 +4816,16 @@ add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
 
 /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
 
-/* Create a new rule based on attributes in 'fm', match in 'cr', 'table_id',
- * and 'old_rule'.  Note that the rule is NOT inserted into a any data
+/* Create a new rule.  Note that the rule is NOT inserted into a any data
  * structures yet.  Takes ownership of 'cr'. */
 static enum ofperr
-replace_rule_create(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
-                    const struct ofputil_flow_mod *fm,
-                    struct cls_rule *cr, uint8_t table_id,
-                    struct rule *old_rule, struct rule **new_rule)
+ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,
+                    uint8_t table_id, ovs_be64 new_cookie,
+                    uint16_t idle_timeout, uint16_t hard_timeout,
+                    enum ofputil_flow_mod_flags flags, uint16_t importance,
+                    const struct ofpact *ofpacts, size_t ofpacts_len,
+                    struct rule **new_rule)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     struct rule *rule;
     enum ofperr error;
@@ -4840,46 +4842,28 @@ replace_rule_create(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
     *CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto;
     cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), cr);
     ovs_refcount_init(&rule->ref_count);
-    rule->flow_cookie = fm->new_cookie;
-    rule->created = rule->modified = time_msec();
 
     ovs_mutex_init(&rule->mutex);
     ovs_mutex_lock(&rule->mutex);
-    rule->idle_timeout = fm->idle_timeout;
-    rule->hard_timeout = fm->hard_timeout;
-    *CONST_CAST(uint16_t *, &rule->importance) = fm->importance;
+    rule->flow_cookie = new_cookie;
+    rule->created = rule->modified = time_msec();
+    rule->idle_timeout = idle_timeout;
+    rule->hard_timeout = hard_timeout;
+    *CONST_CAST(uint16_t *, &rule->importance) = importance;
     rule->removed_reason = OVS_OFPRR_NONE;
 
     *CONST_CAST(uint8_t *, &rule->table_id) = table_id;
-    rule->flags = fm->flags & OFPUTIL_FF_STATE;
+    rule->flags = flags & OFPUTIL_FF_STATE;
+
     *CONST_CAST(const struct rule_actions **, &rule->actions)
-        = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
+        = rule_actions_create(ofpacts, ofpacts_len);
+
     ovs_list_init(&rule->meter_list_node);
     rule->eviction_group = NULL;
-    ovs_list_init(&rule->expirable);
     rule->monitor_flags = 0;
     rule->add_seqno = 0;
     rule->modify_seqno = 0;
-
-    /* Copy values from old rule for modify semantics. */
-    if (old_rule && old_rule->removed_reason != OFPRR_EVICTION) {
-        bool change_cookie = (ofm->modify_cookie
-                              && fm->new_cookie != OVS_BE64_MAX
-                              && fm->new_cookie != old_rule->flow_cookie);
-
-        ovs_mutex_lock(&old_rule->mutex);
-        if (fm->command != OFPFC_ADD) {
-            rule->idle_timeout = old_rule->idle_timeout;
-            rule->hard_timeout = old_rule->hard_timeout;
-            *CONST_CAST(uint16_t *, &rule->importance) = old_rule->importance;
-            rule->flags = old_rule->flags;
-            rule->created = old_rule->created;
-        }
-        if (!change_cookie) {
-            rule->flow_cookie = old_rule->flow_cookie;
-        }
-        ovs_mutex_unlock(&old_rule->mutex);
-    }
+    ovs_list_init(&rule->expirable);
     ovs_mutex_unlock(&rule->mutex);
 
     /* Construct rule, initializing derived state. */
@@ -4896,16 +4880,37 @@ replace_rule_create(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
 }
 
 static void
-replace_rule_start(struct ofproto *ofproto, ovs_version_t version,
-                   struct rule *old_rule, struct rule *new_rule,
-                   struct cls_conjunction *conjs, size_t n_conjs)
+replace_rule_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
+                   struct rule *old_rule, struct rule *new_rule)
 {
     struct oftable *table = &ofproto->tables[new_rule->table_id];
 
     /* 'old_rule' may be either an evicted rule or replaced rule. */
     if (old_rule) {
+    /* Copy values from old rule for modify semantics. */
+        if (old_rule->removed_reason != OFPRR_EVICTION) {
+            bool change_cookie = (ofm->modify_cookie
+                                  && new_rule->flow_cookie != OVS_BE64_MAX
+                                  && new_rule->flow_cookie != old_rule->flow_cookie);
+
+            ovs_mutex_lock(&new_rule->mutex);
+            ovs_mutex_lock(&old_rule->mutex);
+            if (ofm->command != OFPFC_ADD) {
+                new_rule->idle_timeout = old_rule->idle_timeout;
+                new_rule->hard_timeout = old_rule->hard_timeout;
+                *CONST_CAST(uint16_t *, &new_rule->importance) = old_rule->importance;
+                new_rule->flags = old_rule->flags;
+                new_rule->created = old_rule->created;
+            }
+            if (!change_cookie) {
+                new_rule->flow_cookie = old_rule->flow_cookie;
+            }
+            ovs_mutex_unlock(&old_rule->mutex);
+            ovs_mutex_unlock(&new_rule->mutex);
+        }
+
         /* Mark the old rule for removal in the next version. */
-        cls_rule_make_invisible_in_version(&old_rule->cr, version);
+        cls_rule_make_invisible_in_version(&old_rule->cr, ofm->version);
 
         /* Remove the old rule from data structures. */
         ofproto_rule_remove__(ofproto, old_rule);
@@ -4918,7 +4923,8 @@ replace_rule_start(struct ofproto *ofproto, ovs_version_t version,
     ofproto_rule_insert__(ofproto, new_rule);
     /* Make the new rule visible for classifier lookups only from the next
      * version. */
-    classifier_insert(&table->cls, &new_rule->cr, version, conjs, n_conjs);
+    classifier_insert(&table->cls, &new_rule->cr, ofm->version, ofm->conjs,
+                      ofm->n_conjs);
 }
 
 static void
@@ -4928,6 +4934,11 @@ replace_rule_revert(struct ofproto *ofproto,
     struct oftable *table = &ofproto->tables[new_rule->table_id];
 
     if (old_rule) {
+        if (old_rule->removed_reason == OFPRR_EVICTION) {
+            /* Revert the eviction. */
+            eviction_group_add_rule(old_rule);
+        }
+
         /* Restore the old rule to data structures. */
         ofproto_rule_insert__(ofproto, old_rule);
 
@@ -4978,6 +4989,9 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
         learned_cookies_dec(ofproto, old_actions, dead_cookies);
 
         if (replaced_rule) {
+            enum nx_flow_update_event event = ofm->command == OFPFC_ADD
+                ? NXFME_ADDED : NXFME_MODIFIED;
+
             bool changed_cookie = (new_rule->flow_cookie
                                    != old_rule->flow_cookie);
 
@@ -4986,9 +5000,9 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                                                   old_actions->ofpacts,
                                                   old_actions->ofpacts_len);
 
-            if (ofm->event != NXFME_MODIFIED || changed_actions
+            if (event != NXFME_MODIFIED || changed_actions
                 || changed_cookie) {
-                ofmonitor_report(ofproto->connmgr, new_rule, ofm->event, 0,
+                ofmonitor_report(ofproto->connmgr, new_rule, event, 0,
                                  req ? req->ofconn : NULL,
                                  req ? req->request->xid : 0,
                                  changed_actions ? old_actions : NULL);
@@ -5003,51 +5017,72 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
     }
 }
 
+/* 'ofm->new_rules' has the template rule, which this function will consume. */
 static enum ofperr
 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);
-
     if (rule_collection_n(old_rules) > 0) {
-        struct cls_conjunction *conjs;
-        size_t n_conjs;
-
         /* Create a new 'modified' rule for each old rule. */
-        struct rule *old_rule;
-        RULE_COLLECTION_FOR_EACH (old_rule, old_rules) {
-            struct rule *new_rule;
-            struct cls_rule cr;
+        struct rule *old_rule, *new_rule;
+        const struct rule_actions *actions = rule_get_actions(ofm->temp_rule);
 
-            cls_rule_clone(&cr, &old_rule->cr);
-            error = replace_rule_create(ofproto, ofm, fm, &cr,
-                                        old_rule->table_id, old_rule,
-                                        &new_rule);
-            if (!error) {
-                rule_collection_add(new_rules, new_rule);
+        /* Must check actions while holding ofproto_mutex to avoid a race. */
+        error = ofproto_check_ofpacts(ofproto, actions->ofpacts,
+                                      actions->ofpacts_len);
+        if (error) {
+            return error;
+        }
+
+        /* Use the temp rule as the first new rule, and as the template for
+         * the rest. */
+        struct rule *temp = ofm->temp_rule;
+        ofm->temp_rule = NULL;   /* We consume the template. */
+
+        bool first = true;
+        RULE_COLLECTION_FOR_EACH (old_rule, old_rules) {
+            if (first) {
+                /* The template rule's match is possibly a loose one, so it
+                 * must be replaced with the old rule's match so that the new
+                 * rule actually replaces the old one. */
+                cls_rule_destroy(CONST_CAST(struct cls_rule *, &temp->cr));
+                cls_rule_clone(CONST_CAST(struct cls_rule *, &temp->cr),
+                               &old_rule->cr);
+                *CONST_CAST(uint8_t *, &temp->table_id) = old_rule->table_id;
+                rule_collection_add(new_rules, temp);
+                first = false;
             } else {
-                rule_collection_unref(new_rules);
-                rule_collection_destroy(new_rules);
-                return error;
+                struct cls_rule cr;
+                cls_rule_clone(&cr, &old_rule->cr);
+                error = ofproto_rule_create(ofproto, &cr, old_rule->table_id,
+                                            temp->flow_cookie,
+                                            temp->idle_timeout,
+                                            temp->hard_timeout, temp->flags,
+                                            temp->importance,
+                                            temp->actions->ofpacts,
+                                            temp->actions->ofpacts_len,
+                                            &new_rule);
+                if (!error) {
+                    rule_collection_add(new_rules, new_rule);
+                } else {
+                    rule_collection_unref(new_rules);
+                    rule_collection_destroy(new_rules);
+                    return error;
+                }
             }
         }
         ovs_assert(rule_collection_n(new_rules)
                    == rule_collection_n(old_rules));
 
-        get_conjunctions(fm, &conjs, &n_conjs);
-        struct rule *new_rule;
         RULE_COLLECTIONS_FOR_EACH (old_rule, new_rule, old_rules, new_rules) {
-            replace_rule_start(ofproto, ofm->version, old_rule, new_rule,
-                               conjs, n_conjs);
+            replace_rule_start(ofproto, ofm, old_rule, new_rule);
         }
-        free(conjs);
     } else if (ofm->modify_may_add_flow) {
-        /* No match, add a new flow. */
+        /* No match, add a new flow, consumes 'temp'. */
         error = add_flow_start(ofproto, ofm);
         new_rules->collection.n = 1;
     } else {
@@ -5057,6 +5092,24 @@ modify_flows_start__(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     return error;
 }
 
+static enum ofperr
+modify_flows_init_loose(struct ofproto *ofproto,
+                        struct ofproto_flow_mod *ofm,
+                        const struct ofputil_flow_mod *fm)
+    OVS_EXCLUDED(ofproto_mutex)
+{
+    rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, 0,
+                       OVS_VERSION_MAX, fm->cookie, fm->cookie_mask, OFPP_ANY,
+                       OFPG_ANY);
+    rule_criteria_require_rw(&ofm->criteria,
+                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
+    /* Must create a new flow in advance for the case that no matches are
+     * found.  Also used for template for multiple modified flows. */
+    add_flow_init(ofproto, ofm, fm);
+
+    return 0;
+}
+
 /* Implements OFPFC_MODIFY.  Returns 0 on success or an OpenFlow error code on
  * failure.
  *
@@ -5066,17 +5119,10 @@ static enum ofperr
 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;
 
-    rule_criteria_init(&criteria, fm->table_id, &fm->match, 0, OVS_VERSION_MAX,
-                       fm->cookie, fm->cookie_mask, OFPP_ANY, OFPG_ANY);
-    rule_criteria_require_rw(&criteria,
-                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
-    error = collect_rules_loose(ofproto, &criteria, old_rules);
-    rule_criteria_destroy(&criteria);
+    error = collect_rules_loose(ofproto, &ofm->criteria, old_rules);
 
     if (!error) {
         error = modify_flows_start__(ofproto, ofm);
@@ -5085,6 +5131,7 @@ modify_flows_start_loose(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     if (error) {
         rule_collection_destroy(old_rules);
     }
+
     return error;
 }
 
@@ -5096,10 +5143,7 @@ modify_flows_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     struct rule_collection *new_rules = &ofm->new_rules;
 
     /* Old rules were not changed yet, only need to revert new rules. */
-    if (rule_collection_n(old_rules) == 0
-        && rule_collection_n(new_rules) == 1) {
-        add_flow_revert(ofproto, ofm);
-    } else if (rule_collection_n(old_rules) > 0) {
+    if (rule_collection_n(old_rules) > 0) {
         struct rule *old_rule, *new_rule;
         RULE_COLLECTIONS_FOR_EACH (old_rule, new_rule, old_rules, new_rules) {
             replace_rule_revert(ofproto, old_rule, new_rule);
@@ -5140,33 +5184,40 @@ modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
     }
 }
 
+static enum ofperr
+modify_flow_init_strict(struct ofproto *ofproto OVS_UNUSED,
+                        struct ofproto_flow_mod *ofm,
+                        const struct ofputil_flow_mod *fm)
+    OVS_EXCLUDED(ofproto_mutex)
+{
+    rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, fm->priority,
+                       OVS_VERSION_MAX, fm->cookie, fm->cookie_mask, OFPP_ANY,
+                       OFPG_ANY);
+    rule_criteria_require_rw(&ofm->criteria,
+                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
+    /* Must create a new flow in advance for the case that no matches are
+     * found.  Also used for template for multiple modified flows. */
+    add_flow_init(ofproto, ofm, fm);
+
+    return 0;
+}
+
 /* 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 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;
 
-    rule_criteria_init(&criteria, fm->table_id, &fm->match, fm->priority,
-                       OVS_VERSION_MAX, fm->cookie, fm->cookie_mask, OFPP_ANY,
-                       OFPG_ANY);
-    rule_criteria_require_rw(&criteria,
-                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
-    error = collect_rules_strict(ofproto, &criteria, old_rules);
-    rule_criteria_destroy(&criteria);
+    error = collect_rules_strict(ofproto, &ofm->criteria, old_rules);
 
     if (!error) {
         /* collect_rules_strict() can return max 1 rule. */
         error = modify_flows_start__(ofproto, ofm);
     }
 
-    if (error) {
-        rule_collection_destroy(old_rules);
-    }
     return error;
 }
 
@@ -5262,23 +5313,29 @@ delete_flows__(struct rule_collection *rules,
     }
 }
 
+static enum ofperr
+delete_flows_init_loose(struct ofproto *ofproto OVS_UNUSED,
+                        struct ofproto_flow_mod *ofm,
+                        const struct ofputil_flow_mod *fm)
+    OVS_EXCLUDED(ofproto_mutex)
+{
+    rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, 0,
+                       OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
+                       fm->out_port, fm->out_group);
+    rule_criteria_require_rw(&ofm->criteria,
+                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
+    return 0;
+}
+
 /* Implements OFPFC_DELETE. */
 static enum ofperr
 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;
 
-    rule_criteria_init(&criteria, fm->table_id, &fm->match, 0, OVS_VERSION_MAX,
-                       fm->cookie, fm->cookie_mask, fm->out_port,
-                       fm->out_group);
-    rule_criteria_require_rw(&criteria,
-                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
-    error = collect_rules_loose(ofproto, &criteria, rules);
-    rule_criteria_destroy(&criteria);
+    error = collect_rules_loose(ofproto, &ofm->criteria, rules);
 
     if (!error) {
         delete_flows_start__(ofproto, ofm->version, rules);
@@ -5292,7 +5349,6 @@ delete_flows_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     OVS_REQUIRES(ofproto_mutex)
 {
     delete_flows_revert__(ofproto, &ofm->old_rules);
-    rule_collection_destroy(&ofm->old_rules);
 }
 
 static void
@@ -5304,24 +5360,30 @@ delete_flows_finish(struct ofproto *ofproto,
     delete_flows_finish__(ofproto, &ofm->old_rules, OFPRR_DELETE, req);
 }
 
+static enum ofperr
+delete_flows_init_strict(struct ofproto *ofproto OVS_UNUSED,
+                         struct ofproto_flow_mod *ofm,
+                         const struct ofputil_flow_mod *fm)
+    OVS_EXCLUDED(ofproto_mutex)
+{
+    rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, fm->priority,
+                       OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
+                       fm->out_port, fm->out_group);
+    rule_criteria_require_rw(&ofm->criteria,
+                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
+    return 0;
+}
+
 /* Implements OFPFC_DELETE_STRICT. */
 static enum ofperr
 delete_flow_start_strict(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;
 
-    rule_criteria_init(&criteria, fm->table_id, &fm->match, fm->priority,
-                       OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
-                       fm->out_port, fm->out_group);
-    rule_criteria_require_rw(&criteria,
-                             (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
-    error = collect_rules_strict(ofproto, &criteria, rules);
-    rule_criteria_destroy(&criteria);
+    error = collect_rules_strict(ofproto, &ofm->criteria, rules);
 
     if (!error) {
         delete_flows_start__(ofproto, ofm->version, rules);
@@ -5447,7 +5509,10 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
     struct ofproto_flow_mod ofm;
     enum ofperr error;
 
-    ofm.fm = *fm;
+    error = ofproto_flow_mod_init(ofproto, &ofm, fm);
+    if (error) {
+        return error;
+    }
 
     ovs_mutex_lock(&ofproto_mutex);
     ofm.version = ofproto->tables_version + 1;
@@ -7022,46 +7087,107 @@ handle_table_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     return table_mod(ofproto, &tm);
 }
 
+/* Free resources that may be allocated by ofproto_flow_mod_init(). */
+void
+ofproto_flow_mod_uninit(struct ofproto_flow_mod *ofm)
+{
+    if (ofm->temp_rule) {
+        ofproto_rule_unref(ofm->temp_rule);
+        ofm->temp_rule = NULL;
+    }
+    if (ofm->criteria.version != OVS_VERSION_NOT_REMOVED) {
+        rule_criteria_destroy(&ofm->criteria);
+    }
+    if (ofm->conjs) {
+        free(ofm->conjs);
+        ofm->conjs = NULL;
+        ofm->n_conjs = 0;
+    }
+}
+
 static enum ofperr
-ofproto_flow_mod_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
-    OVS_REQUIRES(ofproto_mutex)
+ofproto_flow_mod_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
+                      const struct ofputil_flow_mod *fm)
+    OVS_EXCLUDED(ofproto_mutex)
 {
-    /* Must check actions while holding ofproto_mutex to avoid a race. */
-    enum ofperr error = ofproto_check_ofpacts(ofproto, ofm->fm.ofpacts,
-                                              ofm->fm.ofpacts_len);
+    enum ofperr error;
+
+    /* Forward flow mod fields we need later. */
+    ofm->command = fm->command;
+    ofm->buffer_id = fm->buffer_id;
+    ofm->modify_cookie = fm->modify_cookie;
+
+    ofm->modify_may_add_flow = (fm->new_cookie != OVS_BE64_MAX
+                                && fm->cookie_mask == htonll(0));
+
+    /* Initialize state needed by ofproto_flow_mod_uninit(). */
+    ofm->temp_rule = NULL;
+    ofm->criteria.version = OVS_VERSION_NOT_REMOVED;
+    ofm->conjs = NULL;
+    ofm->n_conjs = 0;
+
+    switch (ofm->command) {
+    case OFPFC_ADD:
+        error = add_flow_init(ofproto, ofm, fm);
+        break;
+    case OFPFC_MODIFY:
+        error = modify_flows_init_loose(ofproto, ofm, fm);
+        break;
+    case OFPFC_MODIFY_STRICT:
+        error = modify_flow_init_strict(ofproto, ofm, fm);
+        break;
+    case OFPFC_DELETE:
+        error = delete_flows_init_loose(ofproto, ofm, fm);
+        break;
+    case OFPFC_DELETE_STRICT:
+        error = delete_flows_init_strict(ofproto, ofm, fm);
+        break;
+    default:
+        error = OFPERR_OFPFMFC_BAD_COMMAND;
+        break;
+    }
     if (error) {
-        return error;
+        ofproto_flow_mod_uninit(ofm);
     }
+    return error;
+}
 
-    /* Forward flow mod fields we need later. */
-    ofm->command = ofm->fm.command;
-    ofm->buffer_id = ofm->fm.buffer_id;
-    ofm->modify_cookie = ofm->fm.modify_cookie;
+static enum ofperr
+ofproto_flow_mod_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    enum ofperr error;
 
-    ofm->modify_may_add_flow = (ofm->fm.new_cookie != OVS_BE64_MAX
-                                && ofm->fm.cookie_mask == htonll(0));
+    rule_collection_init(&ofm->old_rules);
+    rule_collection_init(&ofm->new_rules);
 
     switch (ofm->command) {
     case OFPFC_ADD:
-        ofm->event = NXFME_ADDED;
-        return add_flow_start(ofproto, ofm);
-        /* , &be->old_rules.stub[0],
-           &be->new_rules.stub[0]); */
+        error = add_flow_start(ofproto, ofm);
+        break;
     case OFPFC_MODIFY:
-        ofm->event = NXFME_MODIFIED;
-        return modify_flows_start_loose(ofproto, ofm);
+        error = modify_flows_start_loose(ofproto, ofm);
+        break;
     case OFPFC_MODIFY_STRICT:
-        ofm->event = NXFME_MODIFIED;
-        return modify_flow_start_strict(ofproto, ofm);
+        error = modify_flow_start_strict(ofproto, ofm);
+        break;
     case OFPFC_DELETE:
-        ofm->event = NXFME_DELETED;
-        return delete_flows_start_loose(ofproto, ofm);
+        error = delete_flows_start_loose(ofproto, ofm);
+        break;
     case OFPFC_DELETE_STRICT:
-        ofm->event = NXFME_DELETED;
-        return delete_flow_start_strict(ofproto, ofm);
+        error = delete_flow_start_strict(ofproto, ofm);
+        break;
+    default:
+        OVS_NOT_REACHED();
     }
+    /* Release resources not needed after start. */
+    ofproto_flow_mod_uninit(ofm);
 
-    return OFPERR_OFPFMFC_BAD_COMMAND;
+    if (error) {
+        rule_collection_destroy(&ofm->old_rules);
+        rule_collection_destroy(&ofm->new_rules);
+    }
+    return error;
 }
 
 static void
@@ -7086,6 +7212,8 @@ ofproto_flow_mod_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
     default:
         break;
     }
+    rule_collection_destroy(&ofm->old_rules);
+    rule_collection_destroy(&ofm->new_rules);
 }
 
 static void
@@ -7113,6 +7241,9 @@ ofproto_flow_mod_finish(struct ofproto *ofproto,
         break;
     }
 
+    rule_collection_destroy(&ofm->old_rules);
+    rule_collection_destroy(&ofm->new_rules);
+
     if (req) {
         ofconn_report_flow_mod(req->ofconn, ofm->command);
     }
@@ -7318,6 +7449,7 @@ handle_bundle_control(struct ofconn *ofconn, const struct ofp_header *oh)
 
 static enum ofperr
 handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh)
+    OVS_EXCLUDED(ofproto_mutex)
 {
     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
     enum ofperr error;
@@ -7340,17 +7472,20 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh)
     if (type == OFPTYPE_PORT_MOD) {
         error = ofputil_decode_port_mod(badd.msg, &bmsg->opm.pm, false);
     } else if (type == OFPTYPE_FLOW_MOD) {
+        struct ofputil_flow_mod fm;
         struct ofpbuf ofpacts;
         uint64_t ofpacts_stub[1024 / 8];
 
         ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
-        error = ofputil_decode_flow_mod(&bmsg->ofm.fm, badd.msg,
+        error = ofputil_decode_flow_mod(&fm, badd.msg,
                                         ofconn_get_protocol(ofconn),
                                         &ofpacts,
                                         u16_to_ofp(ofproto->max_ports),
                                         ofproto->n_tables);
-        /* Move actions to heap. */
-        bmsg->ofm.fm.ofpacts = ofpbuf_steal_data(&ofpacts);
+        if (!error) {
+            error = ofproto_flow_mod_init(ofproto, &bmsg->ofm, &fm);
+            ofpbuf_uninit(&ofpacts);
+        }
     } else if (type == OFPTYPE_GROUP_MOD) {
         error = ofputil_decode_group_mod(badd.msg, &bmsg->ogm.gm);
     } else {
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 9a7ee89..147ac23 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -4921,8 +4921,6 @@ add table=254 actions=drop
 AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt 2>&1 | sed '/talking to/,$d'],
 [0], [dnl
 Error OFPBRC_EPERM for: OFPT_FLOW_MOD (OF1.4) (xid=0xb): ADD table:254 actions=drop
-Error OFPBFC_MSG_FAILED for: OFPT_BUNDLE_CONTROL (OF1.4) (xid=0xe):
- bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 ])
 
 AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
@@ -5408,8 +5406,6 @@ add table=254 actions=drop
 AT_CHECK([ovs-ofctl -O OpenFlow13 --bundle add-flows br0 flows.txt 2>&1 | sed '/talking to/,$d'],
 [0], [dnl
 Error OFPBRC_EPERM for: OFPT_FLOW_MOD (OF1.3) (xid=0xb): ADD table:254 actions=drop
-Error OFPBFC_MSG_FAILED for: ONFT_BUNDLE_CONTROL (OF1.3) (xid=0xe):
- bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
 ])
 
 AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
-- 
2.1.4





More information about the dev mailing list