Re: [PATCH v1] ethdev: add direction info when creating the transfer table

[Ivan Malov <ivan.malov@oktetlabs.ru>]

Hi,

On Wed, 7 Sep 2022, Rongwei Liu wrote:

> The transfer domain rule is able to match traffic wire/vf
> origin and it means two directions' underlayer resource.

The point of fact is that matching traffic coming from
some entity like wire / VF has been long generalised
in the form of representors. So, a flow rule with
attribute "transfer" is able to match traffic
coming from either a REPRESENTED_PORT or from
a PORT_REPRESENTOR (please find these items).

>
> In customer deployments, they usually match only one direction
> traffic in single flow table: either from wire or from vf.

Which customer deployments? Could you please provide detailed examples?

>
> Introduce one new member transfer_mode into rte_flow_attr to
> indicate the flow table direction property: from wire, from vf
> or bi-direction(default).

AFAIK, 'rte_flow_attr' serves both traditional flow rule
insertion and asynchronous (table) approach. The patch
adds the attributes to generic 'rte_flow_attr' but,
for some reason, ignores non-table rules.

For example, the diff below adds the attributes to "table" commands
in testpmd but does not add them to regular (non-table)
commands like "flow create". Why?

>
> It helps to save underlayer memory also on insertion rate.

Which memory? Host memory? NIC memory? Term "underlayer" is vague.
I suggest that the commit message be revised to first explain how
such memory is spent currently, then explain why this is not
optimal and, finally, which way the patch is supposed to
improve that. I.e. be more specific.

>
> By default, the transfer domain is bi-direction, and no behavior changes.
>
> 1. Match wire origin only
>  flow template_table 0 create group 0 priority 0 transfer wire_orig...
> 2. Match vf origin only
>  flow template_table 0 create group 0 priority 0 transfer vf_orig...
>
> Signed-off-by: Rongwei Liu <rongweil at nvidia.com>
> ---
> app/test-pmd/cmdline_flow.c                 | 26 +++++++++++++++++++++
> doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
> lib/ethdev/rte_flow.h                       |  9 ++++++-
> 3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 7f50028eb7..b25b595e82 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -177,6 +177,8 @@ enum index {
> 	TABLE_INGRESS,
> 	TABLE_EGRESS,
> 	TABLE_TRANSFER,
> +	TABLE_TRANSFER_WIRE_ORIG,
> +	TABLE_TRANSFER_VF_ORIG,
> 	TABLE_RULES_NUMBER,
> 	TABLE_PATTERN_TEMPLATE,
> 	TABLE_ACTIONS_TEMPLATE,
> @@ -1141,6 +1143,8 @@ static const enum index next_table_attr[] = {
> 	TABLE_INGRESS,
> 	TABLE_EGRESS,
> 	TABLE_TRANSFER,
> +	TABLE_TRANSFER_WIRE_ORIG,
> +	TABLE_TRANSFER_VF_ORIG,
> 	TABLE_RULES_NUMBER,
> 	TABLE_PATTERN_TEMPLATE,
> 	TABLE_ACTIONS_TEMPLATE,
> @@ -2881,6 +2885,18 @@ static const struct token token_list[] = {
> 		.next = NEXT(next_table_attr),
> 		.call = parse_table,
> 	},
> +	[TABLE_TRANSFER_WIRE_ORIG] = {
> +		.name = "wire_orig",
> +		.help = "affect rule direction to transfer",

This does not explain the "wire" aspect. It's too broad.

> +		.next = NEXT(next_table_attr),
> +		.call = parse_table,
> +	},
> +	[TABLE_TRANSFER_VF_ORIG] = {
> +		.name = "vf_orig",
> +		.help = "affect rule direction to transfer",

This explanation simply duplicates such of the "wire_orig".
It does not explain the "vf" part. Should be more specific.

> +		.next = NEXT(next_table_attr),
> +		.call = parse_table,
> +	},
> 	[TABLE_RULES_NUMBER] = {
> 		.name = "rules_number",
> 		.help = "number of rules in table",
> @@ -8894,6 +8910,16 @@ parse_table(struct context *ctx, const struct token 
> *token,
> 	case TABLE_TRANSFER:
> 		out->args.table.attr.flow_attr.transfer = 1;
> 		return len;
> +	case TABLE_TRANSFER_WIRE_ORIG:
> +		if (!out->args.table.attr.flow_attr.transfer)
> +			return -1;
> +		out->args.table.attr.flow_attr.transfer_mode = 1;
> +		return len;
> +	case TABLE_TRANSFER_VF_ORIG:
> +		if (!out->args.table.attr.flow_attr.transfer)
> +			return -1;
> +		out->args.table.attr.flow_attr.transfer_mode = 2;
> +		return len;
> 	default:
> 		return -1;
> 	}
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst 
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 330e34427d..603b7988dd 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3332,7 +3332,8 @@ It is bound to ``rte_flow_template_table_create()``::
>
>   flow template_table {port_id} create
>       [table_id {id}] [group {group_id}]
> -       [priority {level}] [ingress] [egress] [transfer]
> +       [priority {level}] [ingress] [egress]
> +       [transfer [vf_orig] [wire_orig]]

Is it correct? Shouldn't it rather be
[transfer] [vf_orig] [wire_orig]
?

>       rules_number {number}
>       pattern_template {pattern_template_id}
>       actions_template {actions_template_id}
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index a79f1e7ef0..512b08d817 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -130,7 +130,14 @@ struct rte_flow_attr {
> 	 * through a suitable port. @see rte_flow_pick_transfer_proxy().
> 	 */
> 	uint32_t transfer:1;
> -	uint32_t reserved:29; /**< Reserved, must be zero. */
> +	/**
> +	 * 0 means bidirection,
> +	 * 0x1 origin uplink,

What does "uplink" mean? It's too vague. Hardly a good term.

> +	 * 0x2 origin vport,

What does "origin vport" mean? Hardly a good term as well.

> +	 * N/A both set.

What's this?

> +	 */
> +	uint32_t transfer_mode:2;
> +	uint32_t reserved:27; /**< Reserved, must be zero. */
> };
>
> /**
> -- 
> 2.27.0
>

Since the attributes are added to generic 'struct rte_flow_attr',
non-table (synchronous) flow rules are supposed to support them,
too. If that is indeed the case, then I'm afraid such proposal
does not agree with the existing items PORT_REPRESENTOR and
REPRESENTED_PORT. They do exactly the same thing, but they
are designed to be way more generic. Why not use them?

Ivan