On 9/15/23 05:02, Nobuhiro MIKI wrote:
> 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.
>
> Signed-off-by: Nobuhiro MIKI <[email protected]>
Thanks for a patch! Looks like a useful feature to have.
Not a full review, but see some comments below.
> ---
> lib/classifier.c | 38 ++++++++++++++++++++++++-----
> lib/classifier.h | 4 ++-
> lib/ovs-router.c | 5 ++--
> lib/tnl-ports.c | 6 ++---
> ofproto/ofproto-dpif-xlate.c | 47 +++++++++++++++++++++++++++++++-----
> ofproto/ofproto-dpif-xlate.h | 3 +++
> ofproto/ofproto-dpif.c | 25 +++++++++++++------
> ofproto/ofproto-dpif.h | 3 ++-
> tests/classifier.at | 11 +++++++++
> tests/test-classifier.c | 8 +++---
> 10 files changed, 120 insertions(+), 30 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 18dbfc83ad44..e43bd6e39d80 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -853,6 +853,20 @@ trie_ctx_init(struct trie_ctx *ctx, const struct
> cls_trie *trie)
> ctx->lookup_done = false;
> }
>
> +static void
> +append_conj_flows(struct ovs_list *conj_flows,
> + struct cls_conjunction_set **soft, size_t n_soft)
> +{
> + struct flow *f;
> + int i;
> +
> + for (i = 0; i < n_soft; i++) {
> + f = xmalloc(sizeof *f);
> + miniflow_expand(&soft[i]->match->flow, f);
Can we just get a pointer for a cls_rule instead of copying the whole thing?
The pointer should be good within RCU period.
> + ovs_list_push_back(conj_flows, &f->list_node);
> + }
> +}
> +
> struct conjunctive_match {
> struct hmap_node hmap_node;
> uint32_t id;
> @@ -933,11 +947,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 appended to the list. */
> 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 ovs_list *conj_flows)
> {
> struct trie_ctx trie_ctx[CLS_MAX_TRIES];
> const struct cls_match *match;
> @@ -1097,10 +1115,14 @@ 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,
> + conj_flows);
This is called with allow_conjunctive_matches == false. Do we need
to pass conj_flows there?
> flow->conj_id = saved_conj_id;
>
> if (rule) {
> + if (conj_flows) {
> + append_conj_flows(conj_flows, soft, n_soft);
This doesn't seem right. IIUC, 'soft' array may contain lower priority
rules that are not related to the current lookup. Only soft matches
with the same priority and id should be taken into account.
> + }
> free_conjunctive_matches(&matches,
> cm_stubs, ARRAY_SIZE(cm_stubs));
> if (soft != soft_stub) {
> @@ -1161,12 +1183,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 appended to the list. */
> 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 ovs_list *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
> index f646a8f7429b..d398f42ae310 100644
> --- a/lib/classifier.h
> +++ b/lib/classifier.h
> @@ -299,6 +299,7 @@
> * parallel to the rule's removal. */
>
> #include "cmap.h"
> +#include "openvswitch/list.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 ovs_list *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
> index 7c04bb0e6b14..ca014d80ed31 100644
> --- 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
> index f16409a0bf08..bb0b0b0c55f4 100644
> --- 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
> index 9cdddb1ff8cb..11bf48adbf09 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -866,6 +866,33 @@ xlate_report_action_set(const struct xlate_ctx *ctx,
> const char *verb)
> }
> }
>
> +static void
> +xlate_report_conj_matches(const struct xlate_ctx *ctx)
> +{
> + struct match match;
> + struct flow *f;
> + int count;
> + int i = 0;
> +
> + count = ovs_list_size(&ctx->xout->conj_flows);
> +
> + LIST_FOR_EACH (f, list_node, &ctx->xout->conj_flows) {
> + match_init(&match, f, ctx->xin->wc);
> +
> + /* Hide unnecessary fields. */
> + match.wc.masks.conj_id = 0;
> + match.wc.masks.recirc_id = 0;
> + match.wc.masks.in_port.ofp_port = 0;
> +
> + struct ds s = DS_EMPTY_INITIALIZER;
> +
> + match_format(&match, NULL, &s, OFP_DEFAULT_PRIORITY);
Not having a mask may be confusing if conjunctive matches are masked.
In your tests we also see all-zeroes be reported as a match that might
be confusing as well.
Is there a way to get proper masks? Maybe by having a cls_rule pointer?
> + xlate_report_debug(ctx, OFT_DETAIL, "conj(%d/%d). %s",
> + ++i, count, 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
> @@ -918,6 +945,8 @@ 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);
> }
>
> /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx'
> @@ -4653,7 +4682,8 @@ 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->xout->conj_flows);
> /* Swap back. */
> if (with_ct_orig) {
> tuple_swap(&ctx->xin->flow, ctx->wc);
> @@ -7967,10 +7997,9 @@ xlate_optimize_odp_actions(struct xlate_in *xin)
> enum xlate_error
> xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> {
> - *xout = (struct xlate_out) {
> - .slow = 0,
> - .recircs = RECIRC_REFS_EMPTY_INITIALIZER,
> - };
> + xout->slow = 0;
> + xout->recircs = RECIRC_REFS_EMPTY_INITIALIZER;
> + ovs_list_init(&xout->conj_flows);
>
> struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
> struct xbridge *xbridge = xbridge_lookup(xcfg, xin->ofproto);
> @@ -8181,7 +8210,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.xout->conj_flows);
We should probably pass conj_flows only if tracing is enabled,
otherwise we'll get an unnecessary and potentially significant
performance hit.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
