ofproto-dpif-trace: Improve conjunctive match tracing.

A conjunctive flow consists of two or more multiple flows with conjunction actions. When input to the ofproto/trace command matches a conjunctive flow, it outputs flows of all dimensions. Acked-by: Simon Horman <horms@ovn.org> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
openvswitch · Nov 16, 2023 · 7b514ab · 7b514ab
1 parent c62b4ac
commit 7b514ab
Show file tree

Hide file tree

Showing 10 changed files with 240 additions and 30 deletions.
diff --git a/NEWS b/NEWS
@@ -7,6 +7,8 @@ Post-v3.2.0
        during the process, and error logs complaining unrecognized fields may
        be observed on old nodes.
    - ovs-appctl:
+     * 'ofproto/trace' now reports OpenFlow rules that make up a conjunctive
+       flow match.
      * Output of 'dpctl/show' command no longer shows interface configuration
        status, only values of the actual configuration options, a.k.a.
        'requested' configuration.  The interface configuration status,

diff --git a/lib/classifier.c b/lib/classifier.c
@@ -853,6 +853,32 @@ trie_ctx_init(struct trie_ctx *ctx, const struct cls_trie *trie)
     ctx->lookup_done = false;
 }
 
+static void
+insert_conj_flows(struct hmapx *conj_flows, uint32_t id, int priority,
+                  struct cls_conjunction_set **soft, size_t n_soft)
+{
+    struct cls_conjunction_set *conj_set;
+
+    if (!conj_flows) {
+        return;
+    }
+
+    for (size_t i = 0; i < n_soft; i++) {
+        conj_set = soft[i];
+
+        if (conj_set->priority != priority) {
+            continue;
+        }
+
+        for (size_t j = 0; j < conj_set->n; j++) {
+            if (conj_set->conj[j].id == id) {
+                hmapx_add(conj_flows, (void *) (conj_set->match->cls_rule));
+                break;
+            }
+        }
+    }
+}
+
 struct conjunctive_match {
     struct hmap_node hmap_node;
     uint32_t id;
@@ -933,11 +959,15 @@ free_conjunctive_matches(struct hmap *matches,
  * recursion within this function itself.
  *
  * 'flow' is non-const to allow for temporary modifications during the lookup.
- * Any changes are restored before returning. */
+ * Any changes are restored before returning.
+ *
+ * 'conj_flows' is an optional parameter.  If it is non-null, the matching
+ * conjunctive flows are inserted. */
 static const struct cls_rule *
 classifier_lookup__(const struct classifier *cls, ovs_version_t version,
                     struct flow *flow, struct flow_wildcards *wc,
-                    bool allow_conjunctive_matches)
+                    bool allow_conjunctive_matches,
+                    struct hmapx *conj_flows)
 {
     struct trie_ctx trie_ctx[CLS_MAX_TRIES];
     const struct cls_match *match;
@@ -1097,10 +1127,15 @@ classifier_lookup__(const struct classifier *cls, ovs_version_t version,
                 const struct cls_rule *rule;
 
                 flow->conj_id = id;
-                rule = classifier_lookup__(cls, version, flow, wc, false);
+                rule = classifier_lookup__(cls, version, flow, wc, false,
+                                           NULL);
                 flow->conj_id = saved_conj_id;
 
                 if (rule) {
+                    if (allow_conjunctive_matches) {
+                        insert_conj_flows(conj_flows, id, soft_pri, soft,
+                                          n_soft);
+                    }
                     free_conjunctive_matches(&matches,
                                              cm_stubs, ARRAY_SIZE(cm_stubs));
                     if (soft != soft_stub) {
@@ -1161,12 +1196,16 @@ classifier_lookup__(const struct classifier *cls, ovs_version_t version,
  * flow_wildcards_init_catchall()).
  *
  * 'flow' is non-const to allow for temporary modifications during the lookup.
- * Any changes are restored before returning. */
+ * Any changes are restored before returning.
+ *
+ * 'conj_flows' is an optional parameter.  If it is non-null, the matching
+ * conjunctive flows are inserted. */
 const struct cls_rule *
 classifier_lookup(const struct classifier *cls, ovs_version_t version,
-                  struct flow *flow, struct flow_wildcards *wc)
+                  struct flow *flow, struct flow_wildcards *wc,
+                  struct hmapx *conj_flows)
 {
-    return classifier_lookup__(cls, version, flow, wc, true);
+    return classifier_lookup__(cls, version, flow, wc, true, conj_flows);
 }
 
 /* Finds and returns a rule in 'cls' with exactly the same priority and

diff --git a/lib/classifier.h b/lib/classifier.h
@@ -299,6 +299,7 @@
  * parallel to the rule's removal. */
 
 #include "cmap.h"
+#include "hmapx.h"
 #include "openvswitch/match.h"
 #include "openvswitch/meta-flow.h"
 #include "pvector.h"
@@ -398,7 +399,8 @@ static inline void classifier_publish(struct classifier *);
  * and each other. */
 const struct cls_rule *classifier_lookup(const struct classifier *,
                                          ovs_version_t, struct flow *,
-                                         struct flow_wildcards *);
+                                         struct flow_wildcards *,
+                                         struct hmapx *conj_flows);
 bool classifier_rule_overlaps(const struct classifier *,
                               const struct cls_rule *, ovs_version_t);
 const struct cls_rule *classifier_find_rule_exactly(const struct classifier *,

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
@@ -115,7 +115,8 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr *ip6_dst,
         const struct cls_rule *cr_src;
         struct flow flow_src = {.ipv6_dst = *src, .pkt_mark = mark};
 
-        cr_src = classifier_lookup(&cls, OVS_VERSION_MAX, &flow_src, NULL);
+        cr_src = classifier_lookup(&cls, OVS_VERSION_MAX, &flow_src, NULL,
+                                   NULL);
         if (cr_src) {
             struct ovs_router_entry *p_src = ovs_router_entry_cast(cr_src);
             if (!p_src->local) {
@@ -126,7 +127,7 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr *ip6_dst,
         }
     }
 
-    cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL);
+    cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL, NULL);
     if (cr) {
         struct ovs_router_entry *p = ovs_router_entry_cast(cr);
 

diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
@@ -112,7 +112,7 @@ map_insert(odp_port_t port, struct eth_addr mac, struct in6_addr *addr,
     tnl_port_init_flow(&match.flow, mac, addr, nw_proto, tp_port);
 
     do {
-        cr = classifier_lookup(&cls, OVS_VERSION_MAX, &match.flow, NULL);
+        cr = classifier_lookup(&cls, OVS_VERSION_MAX, &match.flow, NULL, NULL);
         p = tnl_port_cast(cr);
         /* Try again if the rule was released before we get the reference. */
     } while (p && !ovs_refcount_try_ref_rcu(&p->ref_cnt));
@@ -247,7 +247,7 @@ map_delete(struct eth_addr mac, struct in6_addr *addr,
 
     tnl_port_init_flow(&flow, mac, addr, nw_proto, tp_port);
 
-    cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL);
+    cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL, NULL);
     tnl_port_unref(cr);
 }
 
@@ -305,7 +305,7 @@ odp_port_t
 tnl_port_map_lookup(struct flow *flow, struct flow_wildcards *wc)
 {
     const struct cls_rule *cr = classifier_lookup(&cls, OVS_VERSION_MAX, flow,
-                                                  wc);
+                                                  wc, NULL);
 
     return (cr) ? tnl_port_cast(cr)->portno : ODPP_NONE;
 }

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
@@ -229,6 +229,9 @@ struct xlate_ctx {
      * wants actions. */
     struct ofpbuf *odp_actions;
 
+    /* Set of matching conjunctive flows, or NULL. */
+    struct hmapx *conj_flows;
+
     /* Statistics maintained by xlate_table_action().
      *
      * These statistics limit the amount of work that a single flow
@@ -866,6 +869,34 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb)
     }
 }
 
+static void
+xlate_report_conj_matches(const struct xlate_ctx *ctx,
+                          const struct ofputil_port_map *map)
+{
+    struct ds s = DS_EMPTY_INITIALIZER;
+    struct hmapx_node *node;
+    struct cls_rule *rule;
+
+    /* NOTE: The conj flows have meaning in order.  For each flow that is a
+     * component of conj flows, 'k' in 'conjunction(id, k/n)' represents the
+     * dimension.  When there are multiple flows with the same id, it may be
+     * implicitly expected that they would be output in ascending order of 'k'.
+     *
+     * However, because of the use of hmapx strucutre and the fact that the
+     * classifier returns them in arbitrary order, they are output in arbitrary
+     * order here. */
+    HMAPX_FOR_EACH (node, ctx->conj_flows) {
+        ds_clear(&s);
+
+        rule = node->data;
+
+        cls_rule_format(rule, ofproto_get_tun_tab(&ctx->xin->ofproto->up),
+                        map, &s);
+        xlate_report(ctx, OFT_DETAIL, "conj. %s", ds_cstr(&s));
+    }
+
+    ds_destroy(&s);
+}
 
 /* If tracing is enabled in 'ctx', appends a node representing 'rule' (in
  * OpenFlow table 'table_id') to the trace and makes this node the parent for
@@ -882,6 +913,8 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule,
         return;
     }
 
+    struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map);
+
     struct ds s = DS_EMPTY_INITIALIZER;
     ds_put_format(&s, "%2d. ", table_id);
     if (rule == ctx->xin->ofproto->miss_rule) {
@@ -892,8 +925,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule,
         ds_put_cstr(&s, "Packets are IP fragments and "
                     "the fragment handling mode is \"drop\".");
     } else {
-        struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map);
-
         if (ctx->xin->names) {
             struct ofproto_dpif *ofprotop;
             ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name);
@@ -904,8 +935,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule,
                          ofproto_get_tun_tab(&ctx->xin->ofproto->up),
                          &map, &s, OFP_DEFAULT_PRIORITY);
 
-        ofputil_port_map_destroy(&map);
-
         if (ds_last(&s) != ' ') {
             ds_put_cstr(&s, ", ");
         }
@@ -918,6 +947,9 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule,
     ctx->xin->trace = &oftrace_report(ctx->xin->trace, OFT_TABLE,
                                       ds_cstr(&s))->subs;
     ds_destroy(&s);
+
+    xlate_report_conj_matches(ctx, &map);
+    ofputil_port_map_destroy(&map);
 }
 
 /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx'
@@ -4653,7 +4685,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
                                            ctx->xin->resubmit_stats,
                                            &ctx->table_id, in_port,
                                            may_packet_in, honor_table_miss,
-                                           ctx->xin->xcache);
+                                           ctx->xin->xcache, ctx->conj_flows);
         /* Swap back. */
         if (with_ct_orig) {
             tuple_swap(&ctx->xin->flow, ctx->wc);
@@ -4674,6 +4706,11 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
 
             struct ovs_list *old_trace = ctx->xin->trace;
             xlate_report_table(ctx, rule, table_id);
+
+            if (OVS_UNLIKELY(ctx->xin->trace)) {
+                hmapx_clear(ctx->conj_flows);
+            }
+
             xlate_recursively(ctx, rule, table_id <= old_table_id,
                               is_last_action, xlator);
             ctx->xin->trace = old_trace;
@@ -8044,6 +8081,13 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
 
     COVERAGE_INC(xlate_actions);
 
+    ctx.conj_flows = NULL;
+
+    if (OVS_UNLIKELY(xin->trace)) {
+        ctx.conj_flows = xzalloc(sizeof *ctx.conj_flows);
+        hmapx_init(ctx.conj_flows);
+    }
+
     xin->trace = xlate_report(&ctx, OFT_BRIDGE, "bridge(\"%s\")",
                               xbridge->name);
     if (xin->frozen_state) {
@@ -8181,7 +8225,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         ctx.rule = rule_dpif_lookup_from_table(
             ctx.xbridge->ofproto, ctx.xin->tables_version, flow, ctx.wc,
             ctx.xin->resubmit_stats, &ctx.table_id,
-            flow->in_port.ofp_port, true, true, ctx.xin->xcache);
+            flow->in_port.ofp_port, true, true, ctx.xin->xcache,
+            ctx.conj_flows);
         if (ctx.xin->resubmit_stats) {
             rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats, false);
         }
@@ -8194,6 +8239,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         }
 
         xlate_report_table(&ctx, ctx.rule, ctx.table_id);
+
+        if (OVS_UNLIKELY(ctx.xin->trace)) {
+            hmapx_clear(ctx.conj_flows);
+        }
     }
 
     /* Tunnel stats only for not-thawed packets. */
@@ -8375,6 +8424,12 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     ofpbuf_uninit(&scratch_actions);
     ofpbuf_delete(ctx.encap_data);
 
+    /* Clean up 'conj_flows' as it is no longer needed. */
+    if (OVS_UNLIKELY(xin->trace)) {
+        hmapx_destroy(ctx.conj_flows);
+        free(ctx.conj_flows);
+    }
+
     /* Make sure we return a "drop flow" in case of an error. */
     if (ctx.error) {
         xout->slow = 0;

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
@@ -4383,15 +4383,20 @@ ofproto_dpif_get_tables_version(struct ofproto_dpif *ofproto)
  * a reference.
  *
  * 'flow' is non-const to allow for temporary modifications during the lookup.
- * Any changes are restored before returning. */
+ * Any changes are restored before returning.
+ *
+ * 'conj_flows' is an optional parameter.  If it is non-null, the matching
+ * conjunctive flows are inserted. */
 static struct rule_dpif *
 rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, ovs_version_t version,
                           uint8_t table_id, struct flow *flow,
-                          struct flow_wildcards *wc)
+                          struct flow_wildcards *wc,
+                          struct hmapx *conj_flows)
 {
     struct classifier *cls = &ofproto->up.tables[table_id].cls;
     return rule_dpif_cast(rule_from_cls_rule(classifier_lookup(cls, version,
-                                                               flow, wc)));
+                                                               flow, wc,
+                                                               conj_flows)));
 }
 
 void
@@ -4433,15 +4438,19 @@ ofproto_dpif_credit_table_stats(struct ofproto_dpif *ofproto, uint8_t table_id,
  * 'in_port'.  This is needed for resubmit action support.
  *
  * 'flow' is non-const to allow for temporary modifications during the lookup.
- * Any changes are restored before returning. */
+ * Any changes are restored before returning.
+ *
+ * 'conj_flows' is an optional parameter.  If it is non-null, the matching
+ * conjunctive flows are inserted. */
 struct rule_dpif *
 rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
                             ovs_version_t version, struct flow *flow,
                             struct flow_wildcards *wc,
                             const struct dpif_flow_stats *stats,
                             uint8_t *table_id, ofp_port_t in_port,
                             bool may_packet_in, bool honor_table_miss,
-                            struct xlate_cache *xcache)
+                            struct xlate_cache *xcache,
+                            struct hmapx *conj_flows)
 {
     ovs_be16 old_tp_src = flow->tp_src, old_tp_dst = flow->tp_dst;
     ofp_port_t old_in_port = flow->in_port.ofp_port;
@@ -4497,7 +4506,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
          next_id++, next_id += (next_id == TBL_INTERNAL))
     {
         *table_id = next_id;
-        rule = rule_dpif_lookup_in_table(ofproto, version, next_id, flow, wc);
+        rule = rule_dpif_lookup_in_table(ofproto, version, next_id, flow, wc,
+                                         conj_flows);
         if (stats) {
             struct oftable *tbl = &ofproto->up.tables[next_id];
             unsigned long orig;
@@ -6680,7 +6690,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, &match->flow, &match->wc);
+                                     TBL_INTERNAL, &match->flow, &match->wc,
+                                     NULL);
     if (rule) {
         *rulep = &rule->up;
     } else {

diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
@@ -103,7 +103,8 @@ struct rule_dpif *rule_dpif_lookup_from_table(struct ofproto_dpif *,
                                               ofp_port_t in_port,
                                               bool may_packet_in,
                                               bool honor_table_miss,
-                                              struct xlate_cache *);
+                                              struct xlate_cache *,
+                                              struct hmapx *conj_flows);
 
 void rule_dpif_credit_stats(struct rule_dpif *,
                             const struct dpif_flow_stats *, bool);