Re: [dpdk-dev] [PATCH] ethdev: fix RSS flow expansion in case of mismatch

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 7/27/2020 1:57 PM, Dekel Peled wrote:
> Function rte_flow_expand_rss() is used to expand a flow rule with
> partial pattern into several rules, to ensure all relevant packets
> are matched.
> It uses utility function rte_flow_expand_rss_item_complete(), to check
> if the last valid item in the flow rule pattern needs to be completed.
> For example the pattern "eth / ipv4 proto is 17 / end" will be completed
> with a "udp" item.

better to add the "actions rss types udp" part of the rule to clarify why the
original rule will be completed with 'udp'.

> This function returns "void" item in two cases:

Can you please clarify what does function returning 'void' mean, so people won't
need to dig the code to find out.

> 1) The last item has empty spec, for example "eth / ipv4 / end".
> 2) The last itme has spec that can't be expanded for RSS.
>    For example the pattern "eth / ipv4 proto is 47 / end" ends with IPv4
>    item that has next protocol GRE.
> 
> In both cases the flow rule may be expanded, but in the second case such
> expansion may create rules with invalid pattern.
> For example "eth / ipv4 proto is 47 / udp / end".
> In such a case the flow rule should not be expanded.

OK, got the problem.

> 
> This patch updates function rte_flow_expand_rss_item_complete().
> Return value RTE_FLOW_ITEM_TYPE_END is used to indicate the flow rule
> should not be expanded.

But there is only limited number of check in
'rte_flow_expand_rss_item_complete()', like for ipv4 it checks the proto
udp/tcp/ip. What if other proto is part of rule, won't sending END cause the
initial problem of missing pattern.

I mean, for following rule,
... pattern eth / ipv4 proto is 47 / end actions rss type gre end ...

With your update, rule is not expanded and the 'gre' pattern is not added, won't
this cause not matching the rule for rss?


I may be missing something here, or if not I am sure you can fix it,
BUT I am not sure if this is right way to proceed.

This expansion operation is complex, it is hard to verify it without debugging,
and I can see it has been fixed a few times already.
It is trying to construct the correct pattern by trying to understand the user's
intention.
Overall it has two problems I think,
1) It is hard to make it correct, and not sure if we can rely on the user input
to create *correct* patterns.
2) Only mlx is using this function  (and mlx developed and acked it), so other
vendors doesn't try to expand the rules, so same rule may work different for
different PMDs (assuming with same HW capabilities).
Like just giving 'ip' pattern and request 'udp' rss will fail on Intel but will
work on mlx because of this expansion.

What about other two alternatives:
a) If the rule doesn't work, return error. stop. user should fix it. Don't try
to expand or fix the rule implicitly.

b) If mlx is strong on having the expansion, what about moving it to the PMD. It
won't cause more defects for others, cleans the common code and highlights that
this behavior is unique to a vendor.
Unless there are any other vendor willing to use it. Or is it part of the
rte_flow contract already, @Ori, do you know if this expansion behavior
documented anywhere?


Thanks,
ferruh


> In such a case, rte_flow_expand_rss() will return with the original flow
> rule only, without any expansion.
> 
> Fixes: fc2dd8dd492f ("ethdev: fix expand RSS flows")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> Acked-by: Xiaoyu Min <jackmin@mellanox.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  lib/librte_ethdev/rte_flow.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index f8fdd68..59a386d 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -247,6 +247,8 @@ struct rte_flow_desc_data {
>  			ret = RTE_FLOW_ITEM_TYPE_IPV6;
>  		else if (rte_be_to_cpu_16(ether_type) == RTE_ETHER_TYPE_VLAN)
>  			ret = RTE_FLOW_ITEM_TYPE_VLAN;
> +		else
> +			ret = RTE_FLOW_ITEM_TYPE_END;
>  		break;
>  	case RTE_FLOW_ITEM_TYPE_VLAN:
>  		if (item->mask)
> @@ -264,6 +266,8 @@ struct rte_flow_desc_data {
>  			ret = RTE_FLOW_ITEM_TYPE_IPV6;
>  		else if (rte_be_to_cpu_16(ether_type) == RTE_ETHER_TYPE_VLAN)
>  			ret = RTE_FLOW_ITEM_TYPE_VLAN;
> +		else
> +			ret = RTE_FLOW_ITEM_TYPE_END;
>  		break;
>  	case RTE_FLOW_ITEM_TYPE_IPV4:
>  		if (item->mask)
> @@ -284,6 +288,8 @@ struct rte_flow_desc_data {
>  			ret = RTE_FLOW_ITEM_TYPE_IPV4;
>  		else if (ip_next_proto == IPPROTO_IPV6)
>  			ret = RTE_FLOW_ITEM_TYPE_IPV6;
> +		else
> +			ret = RTE_FLOW_ITEM_TYPE_END;
>  		break;
>  	case RTE_FLOW_ITEM_TYPE_IPV6:
>  		if (item->mask)
> @@ -304,6 +310,8 @@ struct rte_flow_desc_data {
>  			ret = RTE_FLOW_ITEM_TYPE_IPV4;
>  		else if (ip_next_proto == IPPROTO_IPV6)
>  			ret = RTE_FLOW_ITEM_TYPE_IPV6;
> +		else
> +			ret = RTE_FLOW_ITEM_TYPE_END;
>  		break;
>  	default:
>  		ret = RTE_FLOW_ITEM_TYPE_VOID;
> @@ -1110,10 +1118,14 @@ enum rte_flow_conv_item_spec_type {
>  	memset(flow_items, 0, sizeof(flow_items));
>  	user_pattern_size -= sizeof(*item);
>  	/*
> -	 * Check if the last valid item has spec set
> -	 * and need complete pattern.
> +	 * Check if the last valid item has spec set, need complete pattern,
> +	 * and the pattern can be used for expansion.
>  	 */
>  	missed_item.type = rte_flow_expand_rss_item_complete(last_item);
> +	if (missed_item.type == RTE_FLOW_ITEM_TYPE_END) {
> +		/* Item type END indicates expansion is not required. */
> +		return lsize;
> +	}
>  	if (missed_item.type != RTE_FLOW_ITEM_TYPE_VOID) {
>  		next = NULL;
>  		missed = 1;
>