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