| Message ID | 20231107073048.46298-1-nmiki@yahoo-corp.jp |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v5,1/1] ofproto-dpif-trace: Improve conjunctive match tracing. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 11/7/23 08:30, 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. > > Acked-by: Simon Horman <horms@ovn.org> > Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> > --- > lib/classifier.c | 51 ++++++++++++++++++++++--- > 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 | 72 ++++++++++++++++++++++++++++++++++++ > tests/test-classifier.c | 8 ++-- > 10 files changed, 194 insertions(+), 30 deletions(-) Hi, thanks for the new version! I went through the code and I see that you fixed all the issues I reported. I meant to apply the change, but then noticed one more issue with the handling of the trace, see below. I hope, it's the last one. <snip> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index e243773307b7..36b05803afb8 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -866,6 +866,34 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb) > } > } > > +static void > +xlate_report_conj_matches(const struct xlate_ctx *ctx, > + const struct ofputil_port_map *map) > +{ > + struct ds s = DS_EMPTY_INITIALIZER; > + struct hmapx_node *node; > + struct cls_rule *rule; > + > + /* NOTE: The conj flows have meaning in order. For each flow that is a > + * component of conj flows, 'k' in 'conjunction(id, k/n)' represents the > + * dimension. When there are multiple flows with the same id, it may be > + * implicitly expected that they would be output in ascending order of 'k'. > + * > + * However, because of the use of hmapx strucutre and the fact that the > + * classifier returns them in arbitrary order, they are output in arbitrary > + * order here. */ > + HMAPX_FOR_EACH (node, &ctx->xout->conj_flows) { > + ds_clear(&s); > + > + rule = node->data; > + > + cls_rule_format(rule, ofproto_get_tun_tab(&ctx->xin->ofproto->up), > + map, &s); > + xlate_report(ctx, OFT_DETAIL, "conj. %s", ds_cstr(&s)); > + } > + > + ds_destroy(&s); > +} > > /* If tracing is enabled in 'ctx', appends a node representing 'rule' (in > * OpenFlow table 'table_id') to the trace and makes this node the parent for > @@ -882,6 +910,8 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, > return; > } > > + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); > + > struct ds s = DS_EMPTY_INITIALIZER; > ds_put_format(&s, "%2d. ", table_id); > if (rule == ctx->xin->ofproto->miss_rule) { > @@ -892,8 +922,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, > ds_put_cstr(&s, "Packets are IP fragments and " > "the fragment handling mode is \"drop\"."); > } else { > - struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); > - > if (ctx->xin->names) { > struct ofproto_dpif *ofprotop; > ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name); > @@ -904,8 +932,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, > ofproto_get_tun_tab(&ctx->xin->ofproto->up), > &map, &s, OFP_DEFAULT_PRIORITY); > > - ofputil_port_map_destroy(&map); > - > if (ds_last(&s) != ' ') { > ds_put_cstr(&s, ", "); > } > @@ -918,6 +944,9 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, > ctx->xin->trace = &oftrace_report(ctx->xin->trace, OFT_TABLE, > ds_cstr(&s))->subs; > ds_destroy(&s); > + > + xlate_report_conj_matches(ctx, &map); > + ofputil_port_map_destroy(&map); > } > > /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx' > @@ -4653,7 +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); So, here we should also check that trace is enabled before passing the hash map into a function, otherwise we'll have an unnecessary performance impact when the trace is not enabled. The same as in the xlate_actions() function below. There is one more issue with this code though. We're doing a recursive flow translation for one of the actions that jumps into a different OpenFlow table, but we carry the content of the hash map along and will print those unrelated rules when we will report the result of that recursive flow translation. For example, let's say we have following flow table: conj_id=1,actions=resubmit(,2) priority=10,ip,actions=conjunction(1,1/2) priority=10,in_port=p1,actions=conjunction(1,2/2) priority=10,in_port=p2,actions=conjunction(1,2/2) table=2,conj_id=7,actions=resubmit(,3) table=2,priority=20,ip,actions=conjunction(7,1/2) table=2,priority=20,in_port=p1,actions=conjunction(7,2/2) table=2,priority=20,in_port=p2,actions=conjunction(7,2/2) table=3,actions=drop We have 3 tables and we jump from table 0 to table 2 and to table 3. The trace should look like this: bridge("br0") ------------- 0. conj_id=1, priority 32768 -> conj. priority=10,in_port=p1 -> conj. priority=10,ip resubmit(,2) 2. conj_id=7, priority 32768 -> conj. priority=20,ip -> conj. priority=20,in_port=p1 resubmit(,3) 3. priority 32768 drop We match on the conjunctive flow in table 0, resubmit to table 2, match on a different conjunctive flow in table 2, resubmit to table 3 and match on a simple drop flow. However, with the current implementation, the hash map is carried through the recursive flow translation and we get it growing and printed out for each flow, even not conjunctive ones: bridge("br0") ------------- 0. conj_id=1, priority 32768 -> conj. priority=10,in_port=p1 -> conj. priority=10,ip resubmit(,2) 2. conj_id=7, priority 32768 -> conj. priority=10,in_port=p1 -> conj. priority=20,in_port=p1 -> conj. priority=20,ip -> conj. priority=10,ip resubmit(,3) 3. priority 32768 -> conj. priority=10,in_port=p1 -> conj. priority=20,in_port=p1 -> conj. priority=20,ip -> conj. priority=10,ip drop This is happening because the hash map is only cleared/destroyed at the very end of the flow translation. Instead, we should create it and clear/destroy after each lookup. The 'conj_flows' also doesn't seem to belong in the 'xlate_out' structure. It might be better to have it in the 'xlate_ctx' structure, close to the 'rule', as they are related, or just allocate on stack. One approach might be to do something like this: @@ -4659,6 +4660,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, } if (xlate_resubmit_resource_check(ctx)) { uint8_t old_table_id = ctx->table_id; + struct hmapx *conj_flows = NULL; struct rule_dpif *rule; ctx->table_id = table_id; @@ -4675,6 +4677,13 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, } tuple_swap(&ctx->xin->flow, ctx->wc); } + + + if (ctx->xin->trace) { + conj_flows = xzalloc(sizeof *conj_flows); + hmapx_init(conj_flows); + } + rule = rule_dpif_lookup_from_table(ctx->xbridge->ofproto, ctx->xin->tables_version, &ctx->xin->flow, ctx->wc, @@ -4682,7 +4691,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, &ctx->table_id, in_port, may_packet_in, honor_table_miss, ctx->xin->xcache, - &ctx->xout->conj_flows); + conj_flows); /* Swap back. */ if (with_ct_orig) { tuple_swap(&ctx->xin->flow, ctx->wc); @@ -4702,13 +4711,15 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, } struct ovs_list *old_trace = ctx->xin->trace; - xlate_report_table(ctx, rule, table_id); + xlate_report_table(ctx, rule, table_id, conj_flows); xlate_recursively(ctx, rule, table_id <= old_table_id, is_last_action, xlator); ctx->xin->trace = old_trace; } ctx->table_id = old_table_id; + hmapx_destroy(conj_flows); + free(conj_flows); return; } } --- I.e. just use a temporary hash map, if needed. And do the same in the xlate_actions() function as well. Another way to fix the problem might be to add hmapx_clear() call at the end of xlate_report_conj_matches(), but that would require removing a const qualifier from the ctx pointer in this function. And I'd prefer moving the conj_flows from xlate_out to xlate_ctx in this case, as it is not the translation output, but a temporary context. And we need that info for a very short amount of time actually. What do you think? One small nit below. <snip> > +AT_SETUP([conjunctive match with or without port map]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 2 > +AT_DATA([flows.txt], [dnl > +conj_id=1,actions=drop > +conj_id=2,actions=drop > + > +priority=10,ip,actions=conjunction(1,1/2),conjunction(2,1/2) > +priority=10,in_port=p1,actions=conjunction(1,2/2) > +priority=10,in_port=p2,actions=conjunction(1,2/2) > +]) > +ovs-vsctl show Nit: This call seems unnecessary. Best regards, Ilya Maximets.
On 2023/11/15 5:41, Ilya Maximets wrote: > On 11/7/23 08:30, 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. >> >> Acked-by: Simon Horman <horms@ovn.org> >> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> >> --- >> lib/classifier.c | 51 ++++++++++++++++++++++--- >> 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 | 72 ++++++++++++++++++++++++++++++++++++ >> tests/test-classifier.c | 8 ++-- >> 10 files changed, 194 insertions(+), 30 deletions(-) > > Hi, thanks for the new version! > > I went through the code and I see that you fixed all the issues > I reported. I meant to apply the change, but then noticed one > more issue with the handling of the trace, see below. > I hope, it's the last one. > Hi Ilya, Thank you for pointing this out. Based on your comments I have come up with the following fix. I will prepare v6. Best Regards, Nobuhiro Miki > <snip> > >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index e243773307b7..36b05803afb8 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -866,6 +866,34 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb) >> } >> } >> >> +static void >> +xlate_report_conj_matches(const struct xlate_ctx *ctx, >> + const struct ofputil_port_map *map) >> +{ >> + struct ds s = DS_EMPTY_INITIALIZER; >> + struct hmapx_node *node; >> + struct cls_rule *rule; >> + >> + /* NOTE: The conj flows have meaning in order. For each flow that is a >> + * component of conj flows, 'k' in 'conjunction(id, k/n)' represents the >> + * dimension. When there are multiple flows with the same id, it may be >> + * implicitly expected that they would be output in ascending order of 'k'. >> + * >> + * However, because of the use of hmapx strucutre and the fact that the >> + * classifier returns them in arbitrary order, they are output in arbitrary >> + * order here. */ >> + HMAPX_FOR_EACH (node, &ctx->xout->conj_flows) { >> + ds_clear(&s); >> + >> + rule = node->data; >> + >> + cls_rule_format(rule, ofproto_get_tun_tab(&ctx->xin->ofproto->up), >> + map, &s); >> + xlate_report(ctx, OFT_DETAIL, "conj. %s", ds_cstr(&s)); >> + } >> + >> + ds_destroy(&s); >> +} >> >> /* If tracing is enabled in 'ctx', appends a node representing 'rule' (in >> * OpenFlow table 'table_id') to the trace and makes this node the parent for >> @@ -882,6 +910,8 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, >> return; >> } >> >> + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); >> + >> struct ds s = DS_EMPTY_INITIALIZER; >> ds_put_format(&s, "%2d. ", table_id); >> if (rule == ctx->xin->ofproto->miss_rule) { >> @@ -892,8 +922,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, >> ds_put_cstr(&s, "Packets are IP fragments and " >> "the fragment handling mode is \"drop\"."); >> } else { >> - struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); >> - >> if (ctx->xin->names) { >> struct ofproto_dpif *ofprotop; >> ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name); >> @@ -904,8 +932,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, >> ofproto_get_tun_tab(&ctx->xin->ofproto->up), >> &map, &s, OFP_DEFAULT_PRIORITY); >> >> - ofputil_port_map_destroy(&map); >> - >> if (ds_last(&s) != ' ') { >> ds_put_cstr(&s, ", "); >> } >> @@ -918,6 +944,9 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, >> ctx->xin->trace = &oftrace_report(ctx->xin->trace, OFT_TABLE, >> ds_cstr(&s))->subs; >> ds_destroy(&s); >> + >> + xlate_report_conj_matches(ctx, &map); >> + ofputil_port_map_destroy(&map); >> } >> >> /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx' >> @@ -4653,7 +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); > > So, here we should also check that trace is enabled before passing > the hash map into a function, otherwise we'll have an unnecessary > performance impact when the trace is not enabled. The same as in > the xlate_actions() function below. > > There is one more issue with this code though. We're doing a > recursive flow translation for one of the actions that jumps into > a different OpenFlow table, but we carry the content of the hash map > along and will print those unrelated rules when we will report the > result of that recursive flow translation. > > For example, let's say we have following flow table: > > conj_id=1,actions=resubmit(,2) > priority=10,ip,actions=conjunction(1,1/2) > priority=10,in_port=p1,actions=conjunction(1,2/2) > priority=10,in_port=p2,actions=conjunction(1,2/2) > > table=2,conj_id=7,actions=resubmit(,3) > table=2,priority=20,ip,actions=conjunction(7,1/2) > table=2,priority=20,in_port=p1,actions=conjunction(7,2/2) > table=2,priority=20,in_port=p2,actions=conjunction(7,2/2) > > table=3,actions=drop > > We have 3 tables and we jump from table 0 to table 2 and to table 3. > The trace should look like this: > > bridge("br0") > ------------- > 0. conj_id=1, priority 32768 > -> conj. priority=10,in_port=p1 > -> conj. priority=10,ip > resubmit(,2) > 2. conj_id=7, priority 32768 > -> conj. priority=20,ip > -> conj. priority=20,in_port=p1 > resubmit(,3) > 3. priority 32768 > drop > > We match on the conjunctive flow in table 0, resubmit to table 2, > match on a different conjunctive flow in table 2, resubmit to table > 3 and match on a simple drop flow. > > However, with the current implementation, the hash map is carried > through the recursive flow translation and we get it growing and > printed out for each flow, even not conjunctive ones: > > bridge("br0") > ------------- > 0. conj_id=1, priority 32768 > -> conj. priority=10,in_port=p1 > -> conj. priority=10,ip > resubmit(,2) > 2. conj_id=7, priority 32768 > -> conj. priority=10,in_port=p1 > -> conj. priority=20,in_port=p1 > -> conj. priority=20,ip > -> conj. priority=10,ip > resubmit(,3) > 3. priority 32768 > -> conj. priority=10,in_port=p1 > -> conj. priority=20,in_port=p1 > -> conj. priority=20,ip > -> conj. priority=10,ip > drop > > This is happening because the hash map is only cleared/destroyed > at the very end of the flow translation. > > Instead, we should create it and clear/destroy after each lookup. > The 'conj_flows' also doesn't seem to belong in the 'xlate_out' > structure. It might be better to have it in the 'xlate_ctx' structure, > close to the 'rule', as they are related, or just allocate on stack. > > One approach might be to do something like this: > > @@ -4659,6 +4660,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, > } > if (xlate_resubmit_resource_check(ctx)) { > uint8_t old_table_id = ctx->table_id; > + struct hmapx *conj_flows = NULL; > struct rule_dpif *rule; > > ctx->table_id = table_id; > @@ -4675,6 +4677,13 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, > } > tuple_swap(&ctx->xin->flow, ctx->wc); > } > + > + > + if (ctx->xin->trace) { > + conj_flows = xzalloc(sizeof *conj_flows); > + hmapx_init(conj_flows); > + } > + > rule = rule_dpif_lookup_from_table(ctx->xbridge->ofproto, > ctx->xin->tables_version, > &ctx->xin->flow, ctx->wc, > @@ -4682,7 +4691,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, > &ctx->table_id, in_port, > may_packet_in, honor_table_miss, > ctx->xin->xcache, > - &ctx->xout->conj_flows); > + conj_flows); > /* Swap back. */ > if (with_ct_orig) { > tuple_swap(&ctx->xin->flow, ctx->wc); > @@ -4702,13 +4711,15 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id, > } > > struct ovs_list *old_trace = ctx->xin->trace; > - xlate_report_table(ctx, rule, table_id); > + xlate_report_table(ctx, rule, table_id, conj_flows); > xlate_recursively(ctx, rule, table_id <= old_table_id, > is_last_action, xlator); > ctx->xin->trace = old_trace; > } > > ctx->table_id = old_table_id; > + hmapx_destroy(conj_flows); > + free(conj_flows); > return; > } > } > --- > > I.e. just use a temporary hash map, if needed. And do the same > in the xlate_actions() function as well. > > Another way to fix the problem might be to add hmapx_clear() call at > the end of xlate_report_conj_matches(), but that would require removing > a const qualifier from the ctx pointer in this function. And I'd prefer > moving the conj_flows from xlate_out to xlate_ctx in this case, as it is > not the translation output, but a temporary context. And we need that > info for a very short amount of time actually. > > What do you think? > I agree with moving conj_flows from xlate_out to xlate_ctx. How about doing hmapx_clear for conj_flows on the caller of xlate_report_table? It would be done without removing a const qualifier. xlate_report_conj_matches is only called from xlate_report_table. And xlate_report_table is called from only two places. I wonder if conj_flows should be included in xlate_ctx or if it should be a local variable in the xlate_actions and xlate_table_actionfunctions. Here, I am thinking of including conj_flows in xlate_ctx because I want to reduce the duplication of code fragments, and also because I feel that reducing the number of malloc/free would be more robust. Checking true/false of xin->trace is often carefully tuned using OVS_LIKELY or OVS_UNLIKELY macros. So I will follow that with this patch. In addition, I will add a test case including resubmit flow rule. > One small nit below. > > <snip> > >> +AT_SETUP([conjunctive match with or without port map]) >> +OVS_VSWITCHD_START >> +add_of_ports br0 1 2 >> +AT_DATA([flows.txt], [dnl >> +conj_id=1,actions=drop >> +conj_id=2,actions=drop >> + >> +priority=10,ip,actions=conjunction(1,1/2),conjunction(2,1/2) >> +priority=10,in_port=p1,actions=conjunction(1,2/2) >> +priority=10,in_port=p2,actions=conjunction(1,2/2) >> +]) >> +ovs-vsctl show > > Nit: This call seems unnecessary. > Sorry. I'll remove the line.
diff --git a/lib/classifier.c b/lib/classifier.c index 18dbfc83ad44..0729bd190243 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..36b05803afb8 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -866,6 +866,34 @@ xlate_report_action_set(const struct xlate_ctx *ctx, const char *verb) } } +static void +xlate_report_conj_matches(const struct xlate_ctx *ctx, + const struct ofputil_port_map *map) +{ + struct ds s = DS_EMPTY_INITIALIZER; + struct hmapx_node *node; + struct cls_rule *rule; + + /* NOTE: The conj flows have meaning in order. For each flow that is a + * component of conj flows, 'k' in 'conjunction(id, k/n)' represents the + * dimension. When there are multiple flows with the same id, it may be + * implicitly expected that they would be output in ascending order of 'k'. + * + * However, because of the use of hmapx strucutre and the fact that the + * classifier returns them in arbitrary order, they are output in arbitrary + * order here. */ + HMAPX_FOR_EACH (node, &ctx->xout->conj_flows) { + ds_clear(&s); + + rule = node->data; + + cls_rule_format(rule, ofproto_get_tun_tab(&ctx->xin->ofproto->up), + map, &s); + xlate_report(ctx, OFT_DETAIL, "conj. %s", ds_cstr(&s)); + } + + ds_destroy(&s); +} /* If tracing is enabled in 'ctx', appends a node representing 'rule' (in * OpenFlow table 'table_id') to the trace and makes this node the parent for @@ -882,6 +910,8 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, return; } + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); + struct ds s = DS_EMPTY_INITIALIZER; ds_put_format(&s, "%2d. ", table_id); if (rule == ctx->xin->ofproto->miss_rule) { @@ -892,8 +922,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, ds_put_cstr(&s, "Packets are IP fragments and " "the fragment handling mode is \"drop\"."); } else { - struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); - if (ctx->xin->names) { struct ofproto_dpif *ofprotop; ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name); @@ -904,8 +932,6 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, ofproto_get_tun_tab(&ctx->xin->ofproto->up), &map, &s, OFP_DEFAULT_PRIORITY); - ofputil_port_map_destroy(&map); - if (ds_last(&s) != ' ') { ds_put_cstr(&s, ", "); } @@ -918,6 +944,9 @@ xlate_report_table(const struct xlate_ctx *ctx, struct rule_dpif *rule, ctx->xin->trace = &oftrace_report(ctx->xin->trace, OFT_TABLE, ds_cstr(&s))->subs; ds_destroy(&s); + + xlate_report_conj_matches(ctx, &map); + ofputil_port_map_destroy(&map); } /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx' @@ -4653,7 +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); @@ -7970,6 +8000,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 +8212,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 +8407,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..808a5c23d67e 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..3a1423f56108 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,71 @@ 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 + +AT_SETUP([conjunctive match with metadata]) +OVS_VSWITCHD_START +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0,len=4}->tun_metadata0"]) +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=1,len=8}->tun_metadata1"]) +AT_DATA([flows.txt], [dnl +conj_id=7,actions=drop + +priority=5,tun_metadata0=0x1,actions=conjunction(7,1/2) +priority=5,tun_metadata1=0x2,actions=conjunction(7,2/2) +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) +# Check that tunnel metadata is included in the output. +AT_CHECK([ovs-appctl ofproto/trace br0 "tun_metadata0=0x1,tun_metadata1=0x2,in_port=br0" | grep conj\\. | sort], [0], [dnl + -> conj. priority=5,tun_metadata0=0x1 + -> conj. priority=5,tun_metadata1=0x2 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([conjunctive match with or without port map]) +OVS_VSWITCHD_START +add_of_ports br0 1 2 +AT_DATA([flows.txt], [dnl +conj_id=1,actions=drop +conj_id=2,actions=drop + +priority=10,ip,actions=conjunction(1,1/2),conjunction(2,1/2) +priority=10,in_port=p1,actions=conjunction(1,2/2) +priority=10,in_port=p2,actions=conjunction(1,2/2) +]) +ovs-vsctl show +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) +AT_CHECK([ovs-appctl ofproto/trace br0 "ip,in_port=p1" --names | grep conj\\. | sort], [0], [dnl + -> conj. priority=10,in_port=p1 + -> conj. priority=10,ip +]) +AT_CHECK([ovs-appctl ofproto/trace br0 "ip,in_port=p2" | grep conj\\. | sort], [0], [dnl + -> conj. priority=10,in_port=2 + -> conj. priority=10,ip +]) +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++;