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 <[email protected]> >>> --- >>> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
