On 2023/09/20 21:46, Ilya Maximets wrote:
> 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.
>
Thanks for the review.
It looks like it could be replaced with cls_rule. I will try it.
>> + 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?
>
If false, it is not necessary.
By the way, when classifier_lookup__() is called for the first time,
allow_conjunctive_matches==true, and for the second and subsequent
recursive calls, allow_conjunctive_matches==false.
>> 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.
>
This is where I was not clear. Thanks for pointing that out!
>> + }
>> 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?
>
The cls_rule contains minimask.
Can we use this as a valid mask? I will try it.
>> + 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.
>
I would change it to pass conj_flows only if xin->trace is non-null
(i.e. only when tracing is enabled). I would like to make this change
without affecting performance.
I will prepare v3.
Best Regards,
Nobuhiro Miki
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
