ofp-flow: Reduce memory consumption for ofputil_flow_mod, using minimatch.

Until now, struct ofputil_flow_mod, which represents an OpenFlow flow table
modification request, has incorporated a struct match, which made the
overall ofputil_flow_mod about 2.5 kB.  This is OK for a small number of
flows, but absurdly inflates memory requirements when there are hundreds of
thousands of flows.  This commit fixes the problem by changing struct match
to struct minimatch inside ofputil_flow_mod, which reduces its size to
about 100 bytes plus the actual size of the flow match (usually a few dozen
bytes).

This affects memory usage of ovs-ofctl (when it adds a large number of
flows) more than ovs-vswitchd.

Reported-by: Michael Ben-Ami <mbenami@digitalocean.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Reviewed-by: Armando Migliaccio <armamig@gmail.com>
Tested-by: Armando Migliaccio <armamig@gmail.com>
Reviewed-by: Jan Scheurich <jan.scheurich@ericsson.com>
Tested-by: Jan Scheurich <jan.scheurich@ericsson.com>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>

5 files changed, 47 insertions, 29 deletions

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index bc6429c25..a00bed704 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5019,7 +5019,9 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn)
         if (OVS_UNLIKELY(ctx->xin->trace)) {
             struct ds s = DS_EMPTY_INITIALIZER;
             ds_put_format(&s, "table=%"PRIu8" ", fm.table_id);
-            match_format(&fm.match, NULL, &s, OFP_DEFAULT_PRIORITY);
+            minimatch_format(&fm.match,
+                             ofproto_get_tun_tab(&ctx->xin->ofproto->up),
+                             NULL, &s, OFP_DEFAULT_PRIORITY);
             ds_chomp(&s, ' ');
             ds_put_format(&s, " priority=%d", fm.priority);
             if (fm.new_cookie) {
@@ -5090,6 +5092,8 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn)
             xlate_report_error(ctx, "LEARN action execution failed (%s).",
                                ofperr_to_string(error));
         }
+
+        minimatch_destroy(&fm.match);
     } else {
         xlate_report(ctx, OFT_WARN,
                      "suppressing side effects, so learn action ignored");
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c92c5bea1..1ed82d036 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5582,9 +5582,11 @@ odp_port_to_ofp_port(const struct ofproto_dpif *ofproto, odp_port_t odp_port)
     }
 }
 
+/* 'match' is non-const to allow for temporary modifications.  Any changes are
+ * restored before returning. */
 int
 ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
-                               const struct match *match, int priority,
+                               struct match *match, int priority,
                                uint16_t idle_timeout,
                                const struct ofpbuf *ofpacts,
                                struct rule **rulep)
@@ -5595,7 +5597,6 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
 
     fm = (struct ofputil_flow_mod) {
         .buffer_id = UINT32_MAX,
-        .match = *match,
         .priority = priority,
         .table_id = TBL_INTERNAL,
         .command = OFPFC_ADD,
@@ -5604,8 +5605,10 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
         .ofpacts = ofpacts->data,
         .ofpacts_len = ofpacts->size,
     };
-
+    minimatch_init(&fm.match, match);
     error = ofproto_flow_mod(&ofproto->up, &fm);
+    minimatch_destroy(&fm.match);
+
     if (error) {
         VLOG_ERR_RL(&rl, "failed to add internal flow (%s)",
                     ofperr_to_string(error));
@@ -5615,8 +5618,7 @@ 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);
+                                     TBL_INTERNAL, &match->flow, &match->wc);
     if (rule) {
         *rulep = &rule->up;
     } else {
@@ -5634,7 +5636,6 @@ ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto,
 
     fm = (struct ofputil_flow_mod) {
         .buffer_id = UINT32_MAX,
-        .match = *match,
         .priority = priority,
         .table_id = TBL_INTERNAL,
         .out_port = OFPP_ANY,
@@ -5642,8 +5643,10 @@ ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto,
         .flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY,
         .command = OFPFC_DELETE_STRICT,
     };
-
+    minimatch_init(&fm.match, match);
     error = ofproto_flow_mod(&ofproto->up, &fm);
+    minimatch_destroy(&fm.match);
+
     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 90432fa26..47bf7f9f0 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -335,7 +335,7 @@ struct ofport_dpif *ofp_port_to_ofport(const struct ofproto_dpif *,
                                        ofp_port_t);
 
 int ofproto_dpif_add_internal_flow(struct ofproto_dpif *,
-                                   const struct match *, int priority,
+                                   struct match *, int priority,
                                    uint16_t idle_timeout,
                                    const struct ofpbuf *ofpacts,
                                    struct rule **rulep);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 92d4622b3..d636fb35c 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1199,7 +1199,7 @@ struct ofproto_class {
      *
      * If this function is NULL then table 0 is always chosen. */
     enum ofperr (*rule_choose_table)(const struct ofproto *ofproto,
-                                     const struct match *match,
+                                     const struct minimatch *match,
                                      uint8_t *table_idp);
 
     /* Life-cycle functions for a "struct rule".
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 57ce3c81f..36f4c0b16 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -133,7 +133,7 @@ static void eviction_group_remove_rule(struct rule *)
     OVS_REQUIRES(ofproto_mutex);
 
 static void rule_criteria_init(struct rule_criteria *, uint8_t table_id,
-                               const struct match *match, int priority,
+                               const struct minimatch *match, int priority,
                                ovs_version_t version,
                                ovs_be64 cookie, ovs_be64 cookie_mask,
                                ofp_port_t out_port, uint32_t out_group);
@@ -2104,7 +2104,6 @@ flow_mod_init(struct ofputil_flow_mod *fm,
               enum ofp_flow_mod_command command)
 {
     *fm = (struct ofputil_flow_mod) {
-        .match = *match,
         .priority = priority,
         .table_id = 0,
         .command = command,
@@ -2114,6 +2113,7 @@ flow_mod_init(struct ofputil_flow_mod *fm,
         .ofpacts = CONST_CAST(struct ofpact *, ofpacts),
         .ofpacts_len = ofpacts_len,
     };
+    minimatch_init(&fm->match, match);
 }
 
 static int
@@ -2123,10 +2123,10 @@ simple_flow_mod(struct ofproto *ofproto,
                 enum ofp_flow_mod_command command)
 {
     struct ofputil_flow_mod fm;
-
     flow_mod_init(&fm, match, priority, ofpacts, ofpacts_len, command);
-
-    return handle_flow_mod__(ofproto, &fm, NULL);
+    enum ofperr error = handle_flow_mod__(ofproto, &fm, NULL);
+    minimatch_destroy(&fm.match);
+    return error;
 }
 
 /* Adds a flow to OpenFlow flow table 0 in 'p' that matches 'cls_rule' and
@@ -3173,12 +3173,12 @@ learned_cookies_flush(struct ofproto *ofproto, struct ovs_list *dead_cookies)
 {
     struct learned_cookie *c;
 
+    struct minimatch match;
+
+    minimatch_init_catchall(&match);
     LIST_FOR_EACH_POP (c, u.list_node, dead_cookies) {
         struct rule_criteria criteria;
         struct rule_collection rules;
-        struct match match;
-
-        match_init_catchall(&match);
         rule_criteria_init(&criteria, c->table_id, &match, 0, OVS_VERSION_MAX,
                            c->cookie, OVS_BE64_MAX, OFPP_ANY, OFPG_ANY);
         rule_criteria_require_rw(&criteria, false);
@@ -3188,6 +3188,7 @@ learned_cookies_flush(struct ofproto *ofproto, struct ovs_list *dead_cookies)
 
         free(c);
     }
+    minimatch_destroy(&match);
 }
 
 static enum ofperr
@@ -4051,13 +4052,13 @@ next_matching_table(const struct ofproto *ofproto,
  * supplied as 0. */
 static void
 rule_criteria_init(struct rule_criteria *criteria, uint8_t table_id,
-                   const struct match *match, int priority,
+                   const struct minimatch *match, int priority,
                    ovs_version_t version, ovs_be64 cookie,
                    ovs_be64 cookie_mask, ofp_port_t out_port,
                    uint32_t out_group)
 {
     criteria->table_id = table_id;
-    cls_rule_init(&criteria->cr, match, priority);
+    cls_rule_init_from_minimatch(&criteria->cr, match, priority);
     criteria->version = version;
     criteria->cookie = cookie;
     criteria->cookie_mask = cookie_mask;
@@ -4303,9 +4304,12 @@ handle_flow_stats_request(struct ofconn *ofconn,
         return error;
     }
 
-    rule_criteria_init(&criteria, fsr.table_id, &fsr.match, 0, OVS_VERSION_MAX,
+    struct minimatch match;
+    minimatch_init(&match, &fsr.match);
+    rule_criteria_init(&criteria, fsr.table_id, &match, 0, OVS_VERSION_MAX,
                        fsr.cookie, fsr.cookie_mask, fsr.out_port,
                        fsr.out_group);
+    minimatch_destroy(&match);
 
     ovs_mutex_lock(&ofproto_mutex);
     error = collect_rules_loose(ofproto, &criteria, &rules);
@@ -4470,9 +4474,12 @@ handle_aggregate_stats_request(struct ofconn *ofconn,
         return error;
     }
 
-    rule_criteria_init(&criteria, request.table_id, &request.match, 0,
+    struct minimatch match;
+    minimatch_init(&match, &request.match);
+    rule_criteria_init(&criteria, request.table_id, &match, 0,
                        OVS_VERSION_MAX, request.cookie, request.cookie_mask,
                        request.out_port, request.out_group);
+    minimatch_destroy(&match);
 
     ovs_mutex_lock(&ofproto_mutex);
     error = collect_rules_loose(ofproto, &criteria, &rules);
@@ -4746,22 +4753,22 @@ add_flow_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
     }
 
     if (!(fm->flags & OFPUTIL_FF_HIDDEN_FIELDS)
-        && !match_has_default_hidden_fields(&fm->match)) {
+        && !minimatch_has_default_hidden_fields(&fm->match)) {
         VLOG_WARN_RL(&rl, "%s: (add_flow) only internal flows can set "
                      "non-default values to hidden fields", ofproto->name);
         return OFPERR_OFPBRC_EPERM;
     }
 
     if (!ofm->temp_rule) {
-        cls_rule_init(&cr, &fm->match, fm->priority);
+        cls_rule_init_from_minimatch(&cr, &fm->match, fm->priority);
 
         /* Allocate new rule.  Destroys 'cr'. */
+        uint64_t map = miniflow_get_tun_metadata_present_map(fm->match.flow);
         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,
-                                    fm->match.flow.tunnel.metadata.present.map,
+                                    fm->ofpacts_len, map,
                                     fm->ofpacts_tlv_bitmap, &ofm->temp_rule);
         if (error) {
             return error;
@@ -4958,7 +4965,7 @@ ofproto_flow_mod_init_for_learn(struct ofproto *ofproto,
     struct oftable *table = &ofproto->tables[fm->table_id];
     struct rule *rule;
 
-    rule = rule_from_cls_rule(classifier_find_match_exactly(
+    rule = rule_from_cls_rule(classifier_find_minimatch_exactly(
                                   &table->cls, &fm->match, fm->priority,
                                   OVS_VERSION_MAX));
     if (rule) {
@@ -5108,12 +5115,14 @@ ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref,
         if (limit) {
             struct rule_criteria criteria;
             struct rule_collection rules;
-            struct match match;
+            struct minimatch match;
 
-            match_init_catchall(&match);
+            minimatch_init_catchall(&match);
             rule_criteria_init(&criteria, rule->table_id, &match, 0,
                                OVS_VERSION_MAX, rule->flow_cookie,
                                OVS_BE64_MAX, OFPP_ANY, OFPG_ANY);
+            minimatch_destroy(&match);
+
             rule_criteria_require_rw(&criteria, false);
             collect_rules_loose(rule->ofproto, &criteria, &rules);
             if (rule_collection_n(&rules) >= limit) {
@@ -5797,6 +5806,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     if (!error) {
         struct openflow_mod_requester req = { ofconn, oh };
         error = handle_flow_mod__(ofproto, &fm, &req);
+        minimatch_destroy(&fm.match);
     }
 
     ofpbuf_uninit(&ofpacts);
@@ -7921,6 +7931,7 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh)
                                         ofproto->n_tables);
         if (!error) {
             error = ofproto_flow_mod_init(ofproto, &bmsg->ofm, &fm, NULL);
+            minimatch_destroy(&fm.match);
         }
     } else if (type == OFPTYPE_GROUP_MOD) {
         error = ofputil_decode_group_mod(badd.msg, &bmsg->ogm.gm);