| Message ID | 20231030050004.52658-1-nmiki@yahoo-corp.jp |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v4,1/1] ofproto-dpif-trace: Improve conjunctive match tracing. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Mon, Oct 30, 2023 at 02:00:04PM +0900, 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 <nmiki@yahoo-corp.jp> Thanks Miki-san, I see that Ilya's review of v3 has been addressed. And we agreed that addressing excessive number of function parameters can be deferred. This one looks good to me. Acked-by: Simon Horman <horms@ovn.org>
On 10/30/23 06:00, 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 <nmiki@yahoo-corp.jp> > --- > v4: > * Fix conj_id matching > * Fix priority matching > * Add a new test > v3: > * Remove struct flow changes. > * Use struct 'cls_rule' instead of struct 'flow'. > * Add priority and id conditionals for 'soft' arrays. > * Use 'minimask' in struct 'cls_rule' as mask. > * Use hmapx instead of ovs_list to store conj flows. > * Passe 'conj_flows' as an argument only when tracing. > v2: > * Reimplemented v1 with a safer and cleaner approach, > since v1 was a messy implementation that rewrote const variables. > --- > lib/classifier.c | 51 +++++++++++++++++++++++++++++++----- > lib/classifier.h | 4 ++- > lib/ovs-router.c | 5 ++-- > lib/tnl-ports.c | 6 ++--- > ofproto/ofproto-dpif-xlate.c | 34 ++++++++++++++++++++++-- > ofproto/ofproto-dpif-xlate.h | 3 +++ > ofproto/ofproto-dpif.c | 25 +++++++++++++----- > ofproto/ofproto-dpif.h | 3 ++- > tests/classifier.at | 29 ++++++++++++++++++++ > tests/test-classifier.c | 8 +++--- > 10 files changed, 142 insertions(+), 26 deletions(-) Thanks for the update! This version mostly looks good to me except for a couple small things. See below. Best regards, Ilya Maximets. > > diff --git a/lib/classifier.c b/lib/classifier.c > index 18dbfc83ad44..8fd056f2d283 100644 > --- 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 Two spaces between sentences is preferred. > + * 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); I was thinking about using i + 1 instead n_soft here. The point is that we break early from the loop, so the later soft matches are not considered for the conjunction match. So, on one hand this would be more accurate to not include later matches. However, all the soft matches were already considered while they were initially discovered and their mask bits are already incorporated into the resulted flow mask. So, repoting the later soft matches makes sense as well. We should keep as is, I think, i.e. have n_soft here, but I just wanted to highlight this aspect. > + } > 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 Two spaces. > + * 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 > index f646a8f7429b..f55a2cba998e 100644 > --- 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 > 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 e243773307b7..bae5d8077b51 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -866,6 +866,28 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb) > } > } > > +static void > +xlate_report_conj_matches(const struct xlate_ctx *ctx) > +{ > + struct hmapx_node *node; > + struct cls_rule *rule; > + struct match match; > + > + /* NOTE: The conj flows have meaning in order. But now they > + * are put into the hmapx structure, so the order may > + * have been rearranged. */ Is order actually meaningful? Slightly, I guess, but we can't predict in which order we're getting soft matches from the classifier. These are rules of the same priority that match the same packet. Classifier may return them in arbitrary order. So, even if the order has a slight meaning in the context of the previous i + 1 vs n_soft comment, it's not very meaningful for the end user and largely unpredictable even if we used an array instead of a hash map here. What do you think? > + HMAPX_FOR_EACH (node, &ctx->xout->conj_flows) { > + struct ds s = DS_EMPTY_INITIALIZER; Nit: We may save some CPU cycles if we move this declaration and the ds_destroy() outside of the loop and use ds_clear() as a first thing in the loop. This way we will reuse the memory instead of re-allocating the dynamic string for each rule. The ds_destroy will need to go outside the loop in this case as well. > + > + rule = node->data; > + minimatch_expand(&rule->match, &match); > + > + match_format(&match, NULL, &s, rule->priority); I just noticed that we have a special function that does exactly what the 2 lines abve do - minimatch_format(). And we actually should use this function because it also populates the tunnel metadata table. Without it we will fail to print out tunnel metadata even if the flow is matching it. For example, we will not print the metadata from a flow like: ip,tun_metadata0=0x1234 actions=conjunction(x/y) Note: you'll need to have some tlv maps configured for tunnel metadata matches to be accepted, e.g. with 'ovs-ofctl add-tlv-map'. For example: $ ovs-vsctl add-br br0 $ ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0,len=4}->tun_metadata0" $ ovs-ofctl add-tlv-map br0 "{class=0xffff,type=1,len=8}->tun_metadata1" $ ovs-ofctl add-flow br0 "conj_id=7,actions=drop" $ ovs-ofctl add-flow br0 "priority=5,tun_metadata0=0x1,actions=conjunction(7,1/2)" $ ovs-ofctl add-flow br0 "priority=5,tun_metadata1=0x2,actions=conjunction(7,2/2)" $ ovs-appctl ofproto/trace br0 "tun_metadata0=0x1,tun_metadata1=0x2,in_port=br0" Flow: <...> bridge("br0") ------------- 0. conj_id=7, priority 32768 -> conj. priority=5,tun_metadata1=0x2 -> conj. priority=5,tun_metadata0=0x1 drop This is how it should look like ^^^ But instead we have: bridge("br0") ------------- 0. conj_id=7, priority 32768 -> conj. priority=5 -> conj. priority=5 drop So, all the metadata is missing. Might be worth converting this into a test case as well, I guess. See the minimatch_format() call in the xlate_report_table() for how it gets the tun_tab, which is a tunnel metadata table. But since we have a cls_rule here, we may just call cls_rule_format() that will call the minimatch_format() for us. These functions also have one more argument which is a port_map. It is used to convert port numbers into port names, in case user provided the --names option. The xlate_report_table() already obtains the port map, so we could preserve it and pass into xlate_report_conj_matches(), so our conjunctive flows will have port names as well. > + xlate_report_debug(ctx, OFT_DETAIL, "conj. %s", ds_cstr(&s)); This should be just xlate_report(), there is no need to print this to the log file. > + > + 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 +940,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 +4677,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); > @@ -7970,6 +7995,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) > *xout = (struct xlate_out) { > .slow = 0, > .recircs = RECIRC_REFS_EMPTY_INITIALIZER, > + .conj_flows = HMAPX_INITIALIZER(&xout->conj_flows), > }; > > struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); > @@ -8181,7 +8207,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.xin->trace ? &ctx.xout->conj_flows : NULL); > if (ctx.xin->resubmit_stats) { > rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats, false); > } > @@ -8375,6 +8402,9 @@ exit: > ofpbuf_uninit(&scratch_actions); > ofpbuf_delete(ctx.encap_data); > > + /* Clean up 'conj_flows' as it is no longer needed. */ > + hmapx_destroy(&xout->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-xlate.h b/ofproto/ofproto-dpif-xlate.h > index 05b46fb26b1c..3d549239dfea 100644 > --- a/ofproto/ofproto-dpif-xlate.h > +++ b/ofproto/ofproto-dpif-xlate.h > @@ -61,6 +61,9 @@ struct xlate_out { > > /* Recirc action IDs on which references are held. */ > struct recirc_refs recircs; > + > + /* Set of matching conjunctive flows. */ > + struct hmapx conj_flows; > }; > > struct xlate_in { > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ba5706f6adcc..8cc0a6506534 100644 > --- 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 Two spaces. > + * 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,7 +4438,10 @@ 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 Ditto. > + * conjunctive flows are inserted. */ > struct rule_dpif * > rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, > ovs_version_t version, struct flow *flow, > @@ -4441,7 +4449,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, > 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 > index d8e0cd37ac5b..1fe22ab41bd9 100644 > --- 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); > diff --git a/tests/classifier.at b/tests/classifier.at > index de2705653e00..257a495985da 100644 > --- a/tests/classifier.at > +++ b/tests/classifier.at > @@ -276,6 +276,13 @@ for src in 0 1 2 3 4 5 6 7; do > AT_CHECK([ovs-appctl ofproto/trace br0 "in_port=1,dl_type=0x0800,nw_src=10.0.0.$src,nw_dst=10.0.0.$dst"], [0], [stdout]) > AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: $out > ]) > + dnl Check detailed output for conjunctive match. > + if test $out = 3; then > + AT_CHECK_UNQUOTED([cat stdout | grep conj\\. | sort], [0], [dnl > + -> conj. priority=100,ip,nw_dst=10.0.0.$dst > + -> conj. priority=100,ip,nw_src=10.0.0.$src > +]) > + fi > done > done > OVS_VSWITCHD_STOP > @@ -418,6 +425,28 @@ ovs-ofctl: "conjunction" actions may be used along with "note" but not any other > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([conjunctive match with same priority]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 2 > +AT_DATA([flows.txt], [dnl > +conj_id=1,actions=2 > +conj_id=2,actions=drop > + > +priority=10,ip,ip_dst=10.0.0.1,actions=conjunction(1,1/2) > +priority=10,ip,ip_src=10.0.0.2,actions=conjunction(1,2/2) > +priority=10,ip,ip_dst=10.0.0.3,actions=conjunction(2,1/2) > +priority=10,ip,in_port=1,actions=conjunction(2,2/2) > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > +# Check that "priority=10,ip,in_port=1,actions=conjunction(2,2/2)" is > +# correctly excluded from the output. > +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port=1,dl_type=0x0800,nw_dst=10.0.0.1,nw_src=10.0.0.2" | grep conj\\. | sort], [0], [dnl > + -> conj. priority=10,ip,nw_dst=10.0.0.1 > + -> conj. priority=10,ip,nw_src=10.0.0.2 > +]) > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > # Flow classifier a packet with excess of padding. > AT_SETUP([flow classifier - packet with extra padding]) > OVS_VSWITCHD_START > diff --git a/tests/test-classifier.c b/tests/test-classifier.c > index cff00c8fa35e..2c1604a01e2e 100644 > --- a/tests/test-classifier.c > +++ b/tests/test-classifier.c > @@ -441,7 +441,7 @@ compare_classifiers(struct classifier *cls, size_t n_invisible_rules, > /* This assertion is here to suppress a GCC 4.9 array-bounds warning */ > ovs_assert(cls->n_tries <= CLS_MAX_TRIES); > > - cr0 = classifier_lookup(cls, version, &flow, &wc); > + cr0 = classifier_lookup(cls, version, &flow, &wc, NULL); > cr1 = tcls_lookup(tcls, &flow); > assert((cr0 == NULL) == (cr1 == NULL)); > if (cr0 != NULL) { > @@ -454,7 +454,7 @@ compare_classifiers(struct classifier *cls, size_t n_invisible_rules, > /* Make sure the rule should have been visible. */ > assert(cls_rule_visible_in_version(cr0, version)); > } > - cr2 = classifier_lookup(cls, version, &flow, NULL); > + cr2 = classifier_lookup(cls, version, &flow, NULL, NULL); > assert(cr2 == cr0); > } > } > @@ -1370,10 +1370,10 @@ lookup_classifier(void *aux_) > if (aux->use_wc) { > flow_wildcards_init_catchall(&wc); > cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x], > - &wc); > + &wc, NULL); > } else { > cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x], > - NULL); > + NULL, NULL); > } > if (cr) { > hits++;
On 2023/11/02 5:39, Ilya Maximets wrote: > On 10/30/23 06:00, 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 <nmiki@yahoo-corp.jp> >> --- >> v4: >> * Fix conj_id matching >> * Fix priority matching >> * Add a new test >> v3: >> * Remove struct flow changes. >> * Use struct 'cls_rule' instead of struct 'flow'. >> * Add priority and id conditionals for 'soft' arrays. >> * Use 'minimask' in struct 'cls_rule' as mask. >> * Use hmapx instead of ovs_list to store conj flows. >> * Passe 'conj_flows' as an argument only when tracing. >> v2: >> * Reimplemented v1 with a safer and cleaner approach, >> since v1 was a messy implementation that rewrote const variables. >> --- >> lib/classifier.c | 51 +++++++++++++++++++++++++++++++----- >> lib/classifier.h | 4 ++- >> lib/ovs-router.c | 5 ++-- >> lib/tnl-ports.c | 6 ++--- >> ofproto/ofproto-dpif-xlate.c | 34 ++++++++++++++++++++++-- >> ofproto/ofproto-dpif-xlate.h | 3 +++ >> ofproto/ofproto-dpif.c | 25 +++++++++++++----- >> ofproto/ofproto-dpif.h | 3 ++- >> tests/classifier.at | 29 ++++++++++++++++++++ >> tests/test-classifier.c | 8 +++--- >> 10 files changed, 142 insertions(+), 26 deletions(-) > > Thanks for the update! This version mostly looks good to me except > for a couple small things. See below. > > Best regards, Ilya Maximets. > Hi Ilya-san and Simon-san, Thanks for your review! I wrote my comments inline. Best Regards, Nobuhiro MIKI >> >> diff --git a/lib/classifier.c b/lib/classifier.c >> index 18dbfc83ad44..8fd056f2d283 100644 >> --- 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 > > Two spaces between sentences is preferred. > Of cource I'll fix it. >> + * 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); > > I was thinking about using i + 1 instead n_soft here. The point > is that we break early from the loop, so the later soft matches > are not considered for the conjunction match. > > So, on one hand this would be more accurate to not include later > matches. However, all the soft matches were already considered > while they were initially discovered and their mask bits are already > incorporated into the resulted flow mask. So, repoting the later > soft matches makes sense as well. > > We should keep as is, I think, i.e. have n_soft here, but I just > wanted to highlight this aspect. > I see, thanks for explaining the situation. I have no objection, so I will leave n_soft as it is. >> + } >> 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 > > Two spaces. > OK. >> + * 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 >> index f646a8f7429b..f55a2cba998e 100644 >> --- 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 >> 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 e243773307b7..bae5d8077b51 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -866,6 +866,28 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb) >> } >> } >> >> +static void >> +xlate_report_conj_matches(const struct xlate_ctx *ctx) >> +{ >> + struct hmapx_node *node; >> + struct cls_rule *rule; >> + struct match match; >> + >> + /* NOTE: The conj flows have meaning in order. But now they >> + * are put into the hmapx structure, so the order may >> + * have been rearranged. */ > > Is order actually meaningful? Slightly, I guess, but we can't > predict in which order we're getting soft matches from the > classifier. These are rules of the same priority that match the > same packet. Classifier may return them in arbitrary order. > So, even if the order has a slight meaning in the context of the > previous i + 1 vs n_soft comment, it's not very meaningful for > the end user and largely unpredictable even if we used an array > instead of a hash map here. What do you think? > I think, 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' and same priority, it may be implicitly expected that they would be output in ascending order of 'k'. However, because the use of hmapx structure and the fact that classifiler returns them in arbitrary order, they are output in arbitrary order here. My understanding is something like this. And I see no problem if these are output in arbitrary order. I would appreciate it if you could correct me if I have misunderstood. >> + HMAPX_FOR_EACH (node, &ctx->xout->conj_flows) { >> + struct ds s = DS_EMPTY_INITIALIZER; > > Nit: We may save some CPU cycles if we move this declaration and > the ds_destroy() outside of the loop and use ds_clear() as a first > thing in the loop. This way we will reuse the memory instead of > re-allocating the dynamic string for each rule. The ds_destroy > will need to go outside the loop in this case as well. > Thanks, I didn't know about ds_clear(). I will fix it. >> + >> + rule = node->data; >> + minimatch_expand(&rule->match, &match); >> + >> + match_format(&match, NULL, &s, rule->priority); > > I just noticed that we have a special function that does exactly > what the 2 lines abve do - minimatch_format(). And we actually > should use this function because it also populates the tunnel > metadata table. Without it we will fail to print out tunnel > metadata even if the flow is matching it. For example, we will > not print the metadata from a flow like: > > ip,tun_metadata0=0x1234 actions=conjunction(x/y) > > Note: you'll need to have some tlv maps configured for tunnel > metadata matches to be accepted, e.g. with 'ovs-ofctl add-tlv-map'. > > For example: > > $ ovs-vsctl add-br br0 > $ ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0,len=4}->tun_metadata0" > $ ovs-ofctl add-tlv-map br0 "{class=0xffff,type=1,len=8}->tun_metadata1" > $ ovs-ofctl add-flow br0 "conj_id=7,actions=drop" > $ ovs-ofctl add-flow br0 "priority=5,tun_metadata0=0x1,actions=conjunction(7,1/2)" > $ ovs-ofctl add-flow br0 "priority=5,tun_metadata1=0x2,actions=conjunction(7,2/2)" > $ ovs-appctl ofproto/trace br0 "tun_metadata0=0x1,tun_metadata1=0x2,in_port=br0" > Flow: <...> > > bridge("br0") > ------------- > 0. conj_id=7, priority 32768 > -> conj. priority=5,tun_metadata1=0x2 > -> conj. priority=5,tun_metadata0=0x1 > drop > > This is how it should look like ^^^ > > But instead we have: > > bridge("br0") > ------------- > 0. conj_id=7, priority 32768 > -> conj. priority=5 > -> conj. priority=5 > drop > > So, all the metadata is missing. Might be worth converting this into > a test case as well, I guess. > > See the minimatch_format() call in the xlate_report_table() for how > it gets the tun_tab, which is a tunnel metadata table. > > But since we have a cls_rule here, we may just call cls_rule_format() > that will call the minimatch_format() for us. > > These functions also have one more argument which is a port_map. It is > used to convert port numbers into port names, in case user provided the > --names option. The xlate_report_table() already obtains the port map, > so we could preserve it and pass into xlate_report_conj_matches(), so > our conjunctive flows will have port names as well. > I reproduced the issue in my environment. Also, I was able to confirm that using cls_rule_format() outputs the tunnel metadata as per your comment. I will fix it and add a test to check the output of the tunnel metadata. Also add a test to check that the port names are output correctly. >> + xlate_report_debug(ctx, OFT_DETAIL, "conj. %s", ds_cstr(&s)); > > This should be just xlate_report(), there is no need to print this > to the log file. > I will fix it. Thanks. >> + >> + 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 +940,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 +4677,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); >> @@ -7970,6 +7995,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) >> *xout = (struct xlate_out) { >> .slow = 0, >> .recircs = RECIRC_REFS_EMPTY_INITIALIZER, >> + .conj_flows = HMAPX_INITIALIZER(&xout->conj_flows), >> }; >> >> struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); >> @@ -8181,7 +8207,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.xin->trace ? &ctx.xout->conj_flows : NULL); >> if (ctx.xin->resubmit_stats) { >> rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats, false); >> } >> @@ -8375,6 +8402,9 @@ exit: >> ofpbuf_uninit(&scratch_actions); >> ofpbuf_delete(ctx.encap_data); >> >> + /* Clean up 'conj_flows' as it is no longer needed. */ >> + hmapx_destroy(&xout->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-xlate.h b/ofproto/ofproto-dpif-xlate.h >> index 05b46fb26b1c..3d549239dfea 100644 >> --- a/ofproto/ofproto-dpif-xlate.h >> +++ b/ofproto/ofproto-dpif-xlate.h >> @@ -61,6 +61,9 @@ struct xlate_out { >> >> /* Recirc action IDs on which references are held. */ >> struct recirc_refs recircs; >> + >> + /* Set of matching conjunctive flows. */ >> + struct hmapx conj_flows; >> }; >> >> struct xlate_in { >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index ba5706f6adcc..8cc0a6506534 100644 >> --- 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 > > Two spaces. > OK. >> + * 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,7 +4438,10 @@ 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 > > Ditto. > OK. >> + * conjunctive flows are inserted. */ >> struct rule_dpif * >> rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, >> ovs_version_t version, struct flow *flow, >> @@ -4441,7 +4449,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, >> 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 >> index d8e0cd37ac5b..1fe22ab41bd9 100644 >> --- 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); >> diff --git a/tests/classifier.at b/tests/classifier.at >> index de2705653e00..257a495985da 100644 >> --- a/tests/classifier.at >> +++ b/tests/classifier.at >> @@ -276,6 +276,13 @@ for src in 0 1 2 3 4 5 6 7; do >> AT_CHECK([ovs-appctl ofproto/trace br0 "in_port=1,dl_type=0x0800,nw_src=10.0.0.$src,nw_dst=10.0.0.$dst"], [0], [stdout]) >> AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: $out >> ]) >> + dnl Check detailed output for conjunctive match. >> + if test $out = 3; then >> + AT_CHECK_UNQUOTED([cat stdout | grep conj\\. | sort], [0], [dnl >> + -> conj. priority=100,ip,nw_dst=10.0.0.$dst >> + -> conj. priority=100,ip,nw_src=10.0.0.$src >> +]) >> + fi >> done >> done >> OVS_VSWITCHD_STOP >> @@ -418,6 +425,28 @@ ovs-ofctl: "conjunction" actions may be used along with "note" but not any other >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([conjunctive match with same priority]) >> +OVS_VSWITCHD_START >> +add_of_ports br0 1 2 >> +AT_DATA([flows.txt], [dnl >> +conj_id=1,actions=2 >> +conj_id=2,actions=drop >> + >> +priority=10,ip,ip_dst=10.0.0.1,actions=conjunction(1,1/2) >> +priority=10,ip,ip_src=10.0.0.2,actions=conjunction(1,2/2) >> +priority=10,ip,ip_dst=10.0.0.3,actions=conjunction(2,1/2) >> +priority=10,ip,in_port=1,actions=conjunction(2,2/2) >> +]) >> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> +# Check that "priority=10,ip,in_port=1,actions=conjunction(2,2/2)" is >> +# correctly excluded from the output. >> +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port=1,dl_type=0x0800,nw_dst=10.0.0.1,nw_src=10.0.0.2" | grep conj\\. | sort], [0], [dnl >> + -> conj. priority=10,ip,nw_dst=10.0.0.1 >> + -> conj. priority=10,ip,nw_src=10.0.0.2 >> +]) >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> # Flow classifier a packet with excess of padding. >> AT_SETUP([flow classifier - packet with extra padding]) >> OVS_VSWITCHD_START >> diff --git a/tests/test-classifier.c b/tests/test-classifier.c >> index cff00c8fa35e..2c1604a01e2e 100644 >> --- a/tests/test-classifier.c >> +++ b/tests/test-classifier.c >> @@ -441,7 +441,7 @@ compare_classifiers(struct classifier *cls, size_t n_invisible_rules, >> /* This assertion is here to suppress a GCC 4.9 array-bounds warning */ >> ovs_assert(cls->n_tries <= CLS_MAX_TRIES); >> >> - cr0 = classifier_lookup(cls, version, &flow, &wc); >> + cr0 = classifier_lookup(cls, version, &flow, &wc, NULL); >> cr1 = tcls_lookup(tcls, &flow); >> assert((cr0 == NULL) == (cr1 == NULL)); >> if (cr0 != NULL) { >> @@ -454,7 +454,7 @@ compare_classifiers(struct classifier *cls, size_t n_invisible_rules, >> /* Make sure the rule should have been visible. */ >> assert(cls_rule_visible_in_version(cr0, version)); >> } >> - cr2 = classifier_lookup(cls, version, &flow, NULL); >> + cr2 = classifier_lookup(cls, version, &flow, NULL, NULL); >> assert(cr2 == cr0); >> } >> } >> @@ -1370,10 +1370,10 @@ lookup_classifier(void *aux_) >> if (aux->use_wc) { >> flow_wildcards_init_catchall(&wc); >> cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x], >> - &wc); >> + &wc, NULL); >> } else { >> cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x], >> - NULL); >> + NULL, NULL); >> } >> if (cr) { >> hits++; >
On 11/7/23 07:56, Nobuhiro MIKI wrote: > On 2023/11/02 5:39, Ilya Maximets wrote: >> On 10/30/23 06:00, 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 <nmiki@yahoo-corp.jp> >>> --- >>> v4: >>> * Fix conj_id matching >>> * Fix priority matching >>> * Add a new test >>> v3: >>> * Remove struct flow changes. >>> * Use struct 'cls_rule' instead of struct 'flow'. >>> * Add priority and id conditionals for 'soft' arrays. >>> * Use 'minimask' in struct 'cls_rule' as mask. >>> * Use hmapx instead of ovs_list to store conj flows. >>> * Passe 'conj_flows' as an argument only when tracing. >>> v2: >>> * Reimplemented v1 with a safer and cleaner approach, >>> since v1 was a messy implementation that rewrote const variables. >>> --- >>> lib/classifier.c | 51 +++++++++++++++++++++++++++++++----- >>> lib/classifier.h | 4 ++- >>> lib/ovs-router.c | 5 ++-- >>> lib/tnl-ports.c | 6 ++--- >>> ofproto/ofproto-dpif-xlate.c | 34 ++++++++++++++++++++++-- >>> ofproto/ofproto-dpif-xlate.h | 3 +++ >>> ofproto/ofproto-dpif.c | 25 +++++++++++++----- >>> ofproto/ofproto-dpif.h | 3 ++- >>> tests/classifier.at | 29 ++++++++++++++++++++ >>> tests/test-classifier.c | 8 +++--- >>> 10 files changed, 142 insertions(+), 26 deletions(-) >> >> Thanks for the update! This version mostly looks good to me except >> for a couple small things. See below. >> >> Best regards, Ilya Maximets. >> > > Hi Ilya-san and Simon-san, > > Thanks for your review! > I wrote my comments inline. > > Best Regards, > Nobuhiro MIKI > >>> >>> diff --git a/lib/classifier.c b/lib/classifier.c >>> index 18dbfc83ad44..8fd056f2d283 100644 >>> --- 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 >> >> Two spaces between sentences is preferred. >> > > Of cource I'll fix it. > >>> + * 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); >> >> I was thinking about using i + 1 instead n_soft here. The point >> is that we break early from the loop, so the later soft matches >> are not considered for the conjunction match. >> >> So, on one hand this would be more accurate to not include later >> matches. However, all the soft matches were already considered >> while they were initially discovered and their mask bits are already >> incorporated into the resulted flow mask. So, repoting the later >> soft matches makes sense as well. >> >> We should keep as is, I think, i.e. have n_soft here, but I just >> wanted to highlight this aspect. >> > > I see, thanks for explaining the situation. > I have no objection, so I will leave n_soft as it is. > >>> + } >>> 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 >> >> Two spaces. >> > > OK. > >>> + * 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 >>> index f646a8f7429b..f55a2cba998e 100644 >>> --- 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 >>> 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 e243773307b7..bae5d8077b51 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -866,6 +866,28 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb) >>> } >>> } >>> >>> +static void >>> +xlate_report_conj_matches(const struct xlate_ctx *ctx) >>> +{ >>> + struct hmapx_node *node; >>> + struct cls_rule *rule; >>> + struct match match; >>> + >>> + /* NOTE: The conj flows have meaning in order. But now they >>> + * are put into the hmapx structure, so the order may >>> + * have been rearranged. */ >> >> Is order actually meaningful? Slightly, I guess, but we can't >> predict in which order we're getting soft matches from the >> classifier. These are rules of the same priority that match the >> same packet. Classifier may return them in arbitrary order. >> So, even if the order has a slight meaning in the context of the >> previous i + 1 vs n_soft comment, it's not very meaningful for >> the end user and largely unpredictable even if we used an array >> instead of a hash map here. What do you think? >> > > I think, 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' and same priority, it may be > implicitly expected that they would be output in ascending order > of 'k'. OK. I see your point here. Makes sense. > > However, because the use of hmapx structure and the fact that > classifiler returns them in arbitrary order, they are output in > arbitrary order here. > > My understanding is something like this. And I see no problem if > these are output in arbitrary order. I would appreciate it if > you could correct me if I have misunderstood. I understand what you meant now and it seems correct to me, thanks! Best regards, Ilya Maximets.
diff --git a/lib/classifier.c b/lib/classifier.c index 18dbfc83ad44..8fd056f2d283 100644 --- 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 index f646a8f7429b..f55a2cba998e 100644 --- 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 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 e243773307b7..bae5d8077b51 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -866,6 +866,28 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb) } } +static void +xlate_report_conj_matches(const struct xlate_ctx *ctx) +{ + struct hmapx_node *node; + struct cls_rule *rule; + struct match match; + + /* NOTE: The conj flows have meaning in order. But now they + * are put into the hmapx structure, so the order may + * have been rearranged. */ + HMAPX_FOR_EACH (node, &ctx->xout->conj_flows) { + struct ds s = DS_EMPTY_INITIALIZER; + + rule = node->data; + minimatch_expand(&rule->match, &match); + + match_format(&match, NULL, &s, rule->priority); + xlate_report_debug(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 @@ -918,6 +940,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 +4677,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); @@ -7970,6 +7995,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) *xout = (struct xlate_out) { .slow = 0, .recircs = RECIRC_REFS_EMPTY_INITIALIZER, + .conj_flows = HMAPX_INITIALIZER(&xout->conj_flows), }; struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); @@ -8181,7 +8207,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.xin->trace ? &ctx.xout->conj_flows : NULL); if (ctx.xin->resubmit_stats) { rule_dpif_credit_stats(ctx.rule, ctx.xin->resubmit_stats, false); } @@ -8375,6 +8402,9 @@ exit: ofpbuf_uninit(&scratch_actions); ofpbuf_delete(ctx.encap_data); + /* Clean up 'conj_flows' as it is no longer needed. */ + hmapx_destroy(&xout->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-xlate.h b/ofproto/ofproto-dpif-xlate.h index 05b46fb26b1c..3d549239dfea 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/ofproto/ofproto-dpif-xlate.h @@ -61,6 +61,9 @@ struct xlate_out { /* Recirc action IDs on which references are held. */ struct recirc_refs recircs; + + /* Set of matching conjunctive flows. */ + struct hmapx conj_flows; }; struct xlate_in { diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index ba5706f6adcc..8cc0a6506534 100644 --- 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,7 +4438,10 @@ 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, @@ -4441,7 +4449,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, 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 index d8e0cd37ac5b..1fe22ab41bd9 100644 --- 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); diff --git a/tests/classifier.at b/tests/classifier.at index de2705653e00..257a495985da 100644 --- a/tests/classifier.at +++ b/tests/classifier.at @@ -276,6 +276,13 @@ for src in 0 1 2 3 4 5 6 7; do AT_CHECK([ovs-appctl ofproto/trace br0 "in_port=1,dl_type=0x0800,nw_src=10.0.0.$src,nw_dst=10.0.0.$dst"], [0], [stdout]) AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: $out ]) + dnl Check detailed output for conjunctive match. + if test $out = 3; then + AT_CHECK_UNQUOTED([cat stdout | grep conj\\. | sort], [0], [dnl + -> conj. priority=100,ip,nw_dst=10.0.0.$dst + -> conj. priority=100,ip,nw_src=10.0.0.$src +]) + fi done done OVS_VSWITCHD_STOP @@ -418,6 +425,28 @@ ovs-ofctl: "conjunction" actions may be used along with "note" but not any other OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([conjunctive match with same priority]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 +AT_DATA([flows.txt], [dnl +conj_id=1,actions=2 +conj_id=2,actions=drop + +priority=10,ip,ip_dst=10.0.0.1,actions=conjunction(1,1/2) +priority=10,ip,ip_src=10.0.0.2,actions=conjunction(1,2/2) +priority=10,ip,ip_dst=10.0.0.3,actions=conjunction(2,1/2) +priority=10,ip,in_port=1,actions=conjunction(2,2/2) +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) +# Check that "priority=10,ip,in_port=1,actions=conjunction(2,2/2)" is +# correctly excluded from the output. +AT_CHECK([ovs-appctl ofproto/trace br0 "in_port=1,dl_type=0x0800,nw_dst=10.0.0.1,nw_src=10.0.0.2" | grep conj\\. | sort], [0], [dnl + -> conj. priority=10,ip,nw_dst=10.0.0.1 + -> conj. priority=10,ip,nw_src=10.0.0.2 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + # Flow classifier a packet with excess of padding. AT_SETUP([flow classifier - packet with extra padding]) OVS_VSWITCHD_START diff --git a/tests/test-classifier.c b/tests/test-classifier.c index cff00c8fa35e..2c1604a01e2e 100644 --- a/tests/test-classifier.c +++ b/tests/test-classifier.c @@ -441,7 +441,7 @@ compare_classifiers(struct classifier *cls, size_t n_invisible_rules, /* This assertion is here to suppress a GCC 4.9 array-bounds warning */ ovs_assert(cls->n_tries <= CLS_MAX_TRIES); - cr0 = classifier_lookup(cls, version, &flow, &wc); + cr0 = classifier_lookup(cls, version, &flow, &wc, NULL); cr1 = tcls_lookup(tcls, &flow); assert((cr0 == NULL) == (cr1 == NULL)); if (cr0 != NULL) { @@ -454,7 +454,7 @@ compare_classifiers(struct classifier *cls, size_t n_invisible_rules, /* Make sure the rule should have been visible. */ assert(cls_rule_visible_in_version(cr0, version)); } - cr2 = classifier_lookup(cls, version, &flow, NULL); + cr2 = classifier_lookup(cls, version, &flow, NULL, NULL); assert(cr2 == cr0); } } @@ -1370,10 +1370,10 @@ lookup_classifier(void *aux_) if (aux->use_wc) { flow_wildcards_init_catchall(&wc); cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x], - &wc); + &wc, NULL); } else { cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x], - NULL); + NULL, NULL); } if (cr) { hits++;
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 <nmiki@yahoo-corp.jp> --- v4: * Fix conj_id matching * Fix priority matching * Add a new test v3: * Remove struct flow changes. * Use struct 'cls_rule' instead of struct 'flow'. * Add priority and id conditionals for 'soft' arrays. * Use 'minimask' in struct 'cls_rule' as mask. * Use hmapx instead of ovs_list to store conj flows. * Passe 'conj_flows' as an argument only when tracing. v2: * Reimplemented v1 with a safer and cleaner approach, since v1 was a messy implementation that rewrote const variables. --- lib/classifier.c | 51 +++++++++++++++++++++++++++++++----- lib/classifier.h | 4 ++- lib/ovs-router.c | 5 ++-- lib/tnl-ports.c | 6 ++--- ofproto/ofproto-dpif-xlate.c | 34 ++++++++++++++++++++++-- ofproto/ofproto-dpif-xlate.h | 3 +++ ofproto/ofproto-dpif.c | 25 +++++++++++++----- ofproto/ofproto-dpif.h | 3 ++- tests/classifier.at | 29 ++++++++++++++++++++ tests/test-classifier.c | 8 +++--- 10 files changed, 142 insertions(+), 26 deletions(-)