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