[v3] ethdev: add hint when creating async transfer table

Message ID 20220928092425.68214-1-rongweil@nvidia.com (mailing list archive)
State Changes Requested, archived
Delegated to: Andrew Rybchenko
Headers
Series [v3] ethdev: add hint when creating async transfer table |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-x86_64-unit-testing success Testing PASS

Commit Message

Rongwei Liu Sept. 28, 2022, 9:24 a.m. UTC
  The transfer domain rule is able to match traffic wire/vf
origin and it means two directions' underlayer resource.

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

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

It helps to save underlayer memory also on insertion rate, and this
new field doesn't expose any matching criteira.

By default, the transfer domain is to match bi-direction traffic, and
no behavior changed.

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@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>

v2: Move the new field to template table attribute.
---
 app/test-pmd/cmdline_flow.c                 | 26 +++++++++++++++++++++
 doc/guides/prog_guide/rte_flow.rst          |  7 ++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 ++-
 lib/ethdev/rte_flow.h                       |  7 ++++++
 4 files changed, 42 insertions(+), 1 deletion(-)
  

Comments

Andrew Rybchenko Oct. 4, 2022, 8:31 a.m. UTC | #1
On 9/28/22 12:24, Rongwei Liu wrote:
> The transfer domain rule is able to match traffic wire/vf
> origin and it means two directions' underlayer resource.
> 
> In customer deployments, they usually match only one direction
> traffic in single flow table: either from wire or from vf.
> 
> Introduce one new member transfer_mode into rte_flow_template_table_attr
> to indicate the flow table direction property: from wire, from vf
> or bi-direction(default).
> 
> It helps to save underlayer memory also on insertion rate, and this
> new field doesn't expose any matching criteira.
> 
> By default, the transfer domain is to match bi-direction traffic, and
> no behavior changed.
> 
> 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...

Since wire_orig and vf_orig are just optional hints and not
all PMDs are obliged to handle it, it does not impose any
matching criteria. So, example above are misleading and you
need to add pattern items to highlight that corresponding rules
are really wire_orig or vf_orig.

> 
> Signed-off-by: Rongwei Liu <rongweil@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>

[snip]
  
Andrew Rybchenko Nov. 6, 2022, 10:02 a.m. UTC | #2
On 10/4/22 11:31, Andrew Rybchenko wrote:
> On 9/28/22 12:24, Rongwei Liu wrote:
>> The transfer domain rule is able to match traffic wire/vf
>> origin and it means two directions' underlayer resource.
>>
>> In customer deployments, they usually match only one direction
>> traffic in single flow table: either from wire or from vf.
>>
>> Introduce one new member transfer_mode into rte_flow_template_table_attr
>> to indicate the flow table direction property: from wire, from vf
>> or bi-direction(default).
>>
>> It helps to save underlayer memory also on insertion rate, and this
>> new field doesn't expose any matching criteira.
>>
>> By default, the transfer domain is to match bi-direction traffic, and
>> no behavior changed.
>>
>> 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...
> 
> Since wire_orig and vf_orig are just optional hints and not
> all PMDs are obliged to handle it, it does not impose any
> matching criteria. So, example above are misleading and you
> need to add pattern items to highlight that corresponding rules
> are really wire_orig or vf_orig.

I'm sorry, but I still don't see how it is addressed in v4.

> 
>>
>> Signed-off-by: Rongwei Liu <rongweil@nvidia.com>
>> Acked-by: Ori Kam <orika@nvidia.com>
> 
> [snip]
>
  
Rongwei Liu Nov. 7, 2022, 1:58 a.m. UTC | #3
BR
Rongwei

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Sunday, November 6, 2022 18:03
> To: Rongwei Liu <rongweil@nvidia.com>; Matan Azrad <matan@nvidia.com>;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Aman
> Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>; Ferruh Yigit <ferruh.yigit@amd.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> Subject: Re: [PATCH v3] ethdev: add hint when creating async transfer table
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/4/22 11:31, Andrew Rybchenko wrote:
> > On 9/28/22 12:24, Rongwei Liu wrote:
> >> The transfer domain rule is able to match traffic wire/vf origin and
> >> it means two directions' underlayer resource.
> >>
> >> In customer deployments, they usually match only one direction
> >> traffic in single flow table: either from wire or from vf.
> >>
> >> Introduce one new member transfer_mode into
> >> rte_flow_template_table_attr to indicate the flow table direction
> >> property: from wire, from vf or bi-direction(default).
> >>
> >> It helps to save underlayer memory also on insertion rate, and this
> >> new field doesn't expose any matching criteira.
> >>
> >> By default, the transfer domain is to match bi-direction traffic, and
> >> no behavior changed.
> >>
> >> 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...
> >
> > Since wire_orig and vf_orig are just optional hints and not all PMDs
> > are obliged to handle it, it does not impose any matching criteria.
> > So, example above are misleading and you need to add pattern items to
> > highlight that corresponding rules are really wire_orig or vf_orig.
> 
> I'm sorry, but I still don't see how it is addressed in v4.
@Thomas Monjalon Could you share some thoughts? Thanks.
> 
> >
> >>
> >> Signed-off-by: Rongwei Liu <rongweil@nvidia.com>
> >> Acked-by: Ori Kam <orika@nvidia.com>
> >
> > [snip]
> >
  
Thomas Monjalon Nov. 8, 2022, 9:19 a.m. UTC | #4
06/11/2022 11:02, Andrew Rybchenko:
> On 10/4/22 11:31, Andrew Rybchenko wrote:
> > On 9/28/22 12:24, Rongwei Liu wrote:
> >> The transfer domain rule is able to match traffic wire/vf
> >> origin and it means two directions' underlayer resource.
> >>
> >> In customer deployments, they usually match only one direction
> >> traffic in single flow table: either from wire or from vf.

Customer deployment is not an argument.

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

The origin is not a direction.
We should update this sentence.

> >> It helps to save underlayer memory also on insertion rate, and this
> >> new field doesn't expose any matching criteira.

Should be reworded.

> >> By default, the transfer domain is to match bi-direction traffic, and
> >> no behavior changed.

This sentence is confusing, it should be removed.

> >> 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...

This testpmd example needs to be introduced with a sentence.

> > Since wire_orig and vf_orig are just optional hints and not
> > all PMDs are obliged to handle it, it does not impose any
> > matching criteria.

Yes

> > So, example above are misleading and you
> > need to add pattern items to highlight that corresponding rules
> > are really wire_orig or vf_orig.

This is template table creation, so I don't think there is more to add.
What do you have in mind?

> I'm sorry, but I still don't see how it is addressed in v4.

I think the documentation in v4 is pretty clear.
Do you see something in the doc which is confusing?
The commit message needs rewording though.
  
Andrew Rybchenko Nov. 8, 2022, 9:35 a.m. UTC | #5
On 11/8/22 12:19, Thomas Monjalon wrote:
> 06/11/2022 11:02, Andrew Rybchenko:
>> On 10/4/22 11:31, Andrew Rybchenko wrote:
>>> On 9/28/22 12:24, Rongwei Liu wrote:
>>>> The transfer domain rule is able to match traffic wire/vf
>>>> origin and it means two directions' underlayer resource.
>>>>
>>>> In customer deployments, they usually match only one direction
>>>> traffic in single flow table: either from wire or from vf.
> 
> Customer deployment is not an argument.
> 
>>>> Introduce one new member transfer_mode into rte_flow_template_table_attr
>>>> to indicate the flow table direction property: from wire, from vf
>>>> or bi-direction(default).
> 
> The origin is not a direction.
> We should update this sentence.
> 
>>>> It helps to save underlayer memory also on insertion rate, and this
>>>> new field doesn't expose any matching criteira.
> 
> Should be reworded.
> 
>>>> By default, the transfer domain is to match bi-direction traffic, and
>>>> no behavior changed.
> 
> This sentence is confusing, it should be removed.
> 
>>>> 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...
> 
> This testpmd example needs to be introduced with a sentence.
> 
>>> Since wire_orig and vf_orig are just optional hints and not
>>> all PMDs are obliged to handle it, it does not impose any
>>> matching criteria.
> 
> Yes
> 
>>> So, example above are misleading and you
>>> need to add pattern items to highlight that corresponding rules
>>> are really wire_orig or vf_orig.
> 
> This is template table creation, so I don't think there is more to add.
> What do you have in mind?
> 

Since origin is just a hint which does not impose any matching
criteria it must be highlighted in example that corresponding
rules must have some pattern items defining corresponding
origin.

>> I'm sorry, but I still don't see how it is addressed in v4.
> 
> I think the documentation in v4 is pretty clear.
> Do you see something in the doc which is confusing?
> The commit message needs rewording though.
> 
>
  
Thomas Monjalon Nov. 8, 2022, 11:18 a.m. UTC | #6
08/11/2022 10:35, Andrew Rybchenko:
> On 11/8/22 12:19, Thomas Monjalon wrote:
> > 06/11/2022 11:02, Andrew Rybchenko:
> >> On 10/4/22 11:31, Andrew Rybchenko wrote:
> >>> On 9/28/22 12:24, Rongwei Liu wrote:
> >>>> The transfer domain rule is able to match traffic wire/vf
> >>>> origin and it means two directions' underlayer resource.
> >>>>
> >>>> In customer deployments, they usually match only one direction
> >>>> traffic in single flow table: either from wire or from vf.
> > 
> > Customer deployment is not an argument.
> > 
> >>>> Introduce one new member transfer_mode into rte_flow_template_table_attr
> >>>> to indicate the flow table direction property: from wire, from vf
> >>>> or bi-direction(default).
> > 
> > The origin is not a direction.
> > We should update this sentence.
> > 
> >>>> It helps to save underlayer memory also on insertion rate, and this
> >>>> new field doesn't expose any matching criteira.
> > 
> > Should be reworded.
> > 
> >>>> By default, the transfer domain is to match bi-direction traffic, and
> >>>> no behavior changed.
> > 
> > This sentence is confusing, it should be removed.
> > 
> >>>> 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...
> > 
> > This testpmd example needs to be introduced with a sentence.
> > 
> >>> Since wire_orig and vf_orig are just optional hints and not
> >>> all PMDs are obliged to handle it, it does not impose any
> >>> matching criteria.
> > 
> > Yes
> > 
> >>> So, example above are misleading and you
> >>> need to add pattern items to highlight that corresponding rules
> >>> are really wire_orig or vf_orig.
> > 
> > This is template table creation, so I don't think there is more to add.
> > What do you have in mind?
> > 
> 
> Since origin is just a hint which does not impose any matching
> criteria it must be highlighted in example that corresponding
> rules must have some pattern items defining corresponding
> origin.

Yes we could talk about corresponding rules in the commit message.

What do you think of the explanations in the doc?

> >> I'm sorry, but I still don't see how it is addressed in v4.
> > 
> > I think the documentation in v4 is pretty clear.
> > Do you see something in the doc which is confusing?
> > The commit message needs rewording though.
  
Andrew Rybchenko Nov. 8, 2022, 11:48 a.m. UTC | #7
On 11/8/22 14:18, Thomas Monjalon wrote:
> 08/11/2022 10:35, Andrew Rybchenko:
>> On 11/8/22 12:19, Thomas Monjalon wrote:
>>> 06/11/2022 11:02, Andrew Rybchenko:
>>>> On 10/4/22 11:31, Andrew Rybchenko wrote:
>>>>> On 9/28/22 12:24, Rongwei Liu wrote:
>>>>>> The transfer domain rule is able to match traffic wire/vf
>>>>>> origin and it means two directions' underlayer resource.
>>>>>>
>>>>>> In customer deployments, they usually match only one direction
>>>>>> traffic in single flow table: either from wire or from vf.
>>>
>>> Customer deployment is not an argument.
>>>
>>>>>> Introduce one new member transfer_mode into rte_flow_template_table_attr
>>>>>> to indicate the flow table direction property: from wire, from vf
>>>>>> or bi-direction(default).
>>>
>>> The origin is not a direction.
>>> We should update this sentence.
>>>
>>>>>> It helps to save underlayer memory also on insertion rate, and this
>>>>>> new field doesn't expose any matching criteira.
>>>
>>> Should be reworded.
>>>
>>>>>> By default, the transfer domain is to match bi-direction traffic, and
>>>>>> no behavior changed.
>>>
>>> This sentence is confusing, it should be removed.
>>>
>>>>>> 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...
>>>
>>> This testpmd example needs to be introduced with a sentence.
>>>
>>>>> Since wire_orig and vf_orig are just optional hints and not
>>>>> all PMDs are obliged to handle it, it does not impose any
>>>>> matching criteria.
>>>
>>> Yes
>>>
>>>>> So, example above are misleading and you
>>>>> need to add pattern items to highlight that corresponding rules
>>>>> are really wire_orig or vf_orig.
>>>
>>> This is template table creation, so I don't think there is more to add.
>>> What do you have in mind?
>>>
>>
>> Since origin is just a hint which does not impose any matching
>> criteria it must be highlighted in example that corresponding
>> rules must have some pattern items defining corresponding
>> origin.
> 
> Yes we could talk about corresponding rules in the commit message.
> 
> What do you think of the explanations in the doc?

I've replied on v4.

> 
>>>> I'm sorry, but I still don't see how it is addressed in v4.
>>>
>>> I think the documentation in v4 is pretty clear.
>>> Do you see something in the doc which is confusing?
>>> The commit message needs rewording though.
> 
> 
>
  

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index d97be6fe98..beacb0a802 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -178,6 +178,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,
@@ -1142,6 +1144,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,
@@ -2882,6 +2886,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",
+		.next = NEXT(next_table_attr),
+		.call = parse_table,
+	},
+	[TABLE_TRANSFER_VF_ORIG] = {
+		.name = "vf_orig",
+		.help = "affect rule direction to transfer",
+		.next = NEXT(next_table_attr),
+		.call = parse_table,
+	},
 	[TABLE_RULES_NUMBER] = {
 		.name = "rules_number",
 		.help = "number of rules in table",
@@ -8895,6 +8911,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.transfer_mode = 1;
+		return len;
+	case TABLE_TRANSFER_VF_ORIG:
+		if (!out->args.table.attr.flow_attr.transfer)
+			return -1;
+		out->args.table.attr.transfer_mode = 2;
+		return len;
 	default:
 		return -1;
 	}
diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 588914b231..de8d6836d6 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -3748,6 +3748,13 @@  the maximum number of flow rules is defined at table creation time.
 Any flow rule creation beyond the maximum table size is rejected.
 Application may create another table to accommodate more rules in this case.
 
+Attribute: transfer_mode
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+This is an optional table attribute and meaningless if `Attribute: Transfer`
+is not specified. It doesn't expose any matching criteria but just as a hint
+to indicate PMD where to bound the rules.
+
 .. code-block:: c
 
    struct rte_flow_template_table *
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 5fbec06c35..9a97c5f513 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3125,7 +3125,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]]
        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 017f690798..a546e9a72b 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -5233,6 +5233,13 @@  struct rte_flow_template_table_attr {
 	 * Maximum number of flow rules that this table holds.
 	 */
 	uint32_t nb_flows;
+	/**
+	 * 0 means bidirection,
+	 * 0x1 origin uplink,
+	 * 0x2 origin vport,
+	 * N/A both set.
+	 */
+	uint32_t transfer_mode:2;
 };
 
 /**