[v4] ethdev: add special flags when creating async transfer table

Message ID 20221104104439.230147-1-rongweil@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series [v4] ethdev: add special flags when creating async transfer table |

Checks

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

Commit Message

Rongwei Liu Nov. 4, 2022, 10:44 a.m. UTC
  The transfer domain rule is able to match traffic wire/vport
origin which are corresponding to two kinds of underlayer resources.

Wire means traffic arrives from the uplink port while vport means
traffic initiated from VF/SF.

In customer deployments, they usually match only one kind of
traffic in single flow table: either from wire or from vport.
PMD can save significant resources if passing special hint from rte
layer.

There are two possible approaches, using IPv4 as an example:
1. Use pattern item.
   pattern_template: pattern ANY_VPORT / eth / ipv4 is 1.1.1.1 / end
   async flow create: pattern ANY_VPORT / eth / ipv4 is 1.1.1.2 / end
   "ANY_VPORT" needs to be present in each async rule even if it's
   just a hint. No value to match.

2. Add special flags into table_attr. It will be:
   template_table 0 create table_id 0 group 1 transfer vf_orig

Approach 1 needs to specify the pattern in each flow rules which wastes
memory and not end user friendly.
This patch takes the 2nd approach and introduce one new member
specialize into rte_flow_table_attr to indicate async flow table matching
optimization: from wire, from vport.

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

By default, there is no hint, so the behavior of the transfer domain
doesn't change.

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 vport_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.
v4: Mark it as optional and clear the concept.
---
 app/test-pmd/cmdline_flow.c                 | 26 +++++++++++++++++
 doc/guides/prog_guide/rte_flow.rst          | 15 ++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 +-
 lib/ethdev/rte_flow.h                       | 32 +++++++++++++++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)
  

Comments

Andrew Rybchenko Nov. 8, 2022, 11:39 a.m. UTC | #1
On 11/4/22 13:44, Rongwei Liu wrote:
> The transfer domain rule is able to match traffic wire/vport
> origin which are corresponding to two kinds of underlayer resources.
> 
> Wire means traffic arrives from the uplink port while vport means
> traffic initiated from VF/SF.
> 
> In customer deployments, they usually match only one kind of
> traffic in single flow table: either from wire or from vport.
> PMD can save significant resources if passing special hint from rte
> layer.
> 
> There are two possible approaches, using IPv4 as an example:
> 1. Use pattern item.
>     pattern_template: pattern ANY_VPORT / eth / ipv4 is 1.1.1.1 / end
>     async flow create: pattern ANY_VPORT / eth / ipv4 is 1.1.1.2 / end
>     "ANY_VPORT" needs to be present in each async rule even if it's
>     just a hint. No value to match.
> 
> 2. Add special flags into table_attr. It will be:
>     template_table 0 create table_id 0 group 1 transfer vf_orig
> 
> Approach 1 needs to specify the pattern in each flow rules which wastes
> memory and not end user friendly.
> This patch takes the 2nd approach and introduce one new member
> specialize into rte_flow_table_attr to indicate async flow table matching
> optimization: from wire, from vport.
> 
> It helps to save underlayer memory and also on insertion rate.
> 
> By default, there is no hint, so the behavior of the transfer domain
> doesn't change.
> 
> 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 vport_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.
> v4: Mark it as optional and clear the concept.
> ---
>   app/test-pmd/cmdline_flow.c                 | 26 +++++++++++++++++
>   doc/guides/prog_guide/rte_flow.rst          | 15 ++++++++++
>   doc/guides/testpmd_app_ug/testpmd_funcs.rst |  3 +-
>   lib/ethdev/rte_flow.h                       | 32 +++++++++++++++++++++
>   4 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 88108498e0..15f2af9b40 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -184,6 +184,8 @@ enum index {
>   	TABLE_INGRESS,
>   	TABLE_EGRESS,
>   	TABLE_TRANSFER,
> +	TABLE_TRANSFER_WIRE_ORIG,
> +	TABLE_TRANSFER_VPORT_ORIG,
>   	TABLE_RULES_NUMBER,
>   	TABLE_PATTERN_TEMPLATE,
>   	TABLE_ACTIONS_TEMPLATE,
> @@ -1158,6 +1160,8 @@ static const enum index next_table_attr[] = {
>   	TABLE_INGRESS,
>   	TABLE_EGRESS,
>   	TABLE_TRANSFER,
> +	TABLE_TRANSFER_WIRE_ORIG,
> +	TABLE_TRANSFER_VPORT_ORIG,
>   	TABLE_RULES_NUMBER,
>   	TABLE_PATTERN_TEMPLATE,
>   	TABLE_ACTIONS_TEMPLATE,
> @@ -2933,6 +2937,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_VPORT_ORIG] = {
> +		.name = "vport_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",
> @@ -8993,6 +9009,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.specialize = RTE_FLOW_TRANSFER_WIRE_ORIG;
> +		return len;
> +	case TABLE_TRANSFER_VPORT_ORIG:
> +		if (!out->args.table.attr.flow_attr.transfer)
> +			return -1;
> +		out->args.table.attr.specialize = RTE_FLOW_TRANSFER_VPORT_ORIG;
> +		return len;
>   	default:
>   		return -1;
>   	}
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 3e6242803d..d9ca041ae4 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -3605,6 +3605,21 @@ and pattern and actions templates are created.
>                      &actions_templates, nb_actions_templ,
>                      &error);
>   
> +Table Attribute: Specialize
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Application can help optimizing underlayer resources and insertion rate
> +by specializing template table.
> +Specialization is done by providing hints
> +in the template table attribute ``specialize``.
> +
> +This attribute is not mandatory for each PMD to implement.
> +If a hint is not supported, it will be silently ignored,
> +and no special optimization is done.
> +
> +If a table is specialized, the application should make sure the rules
> +comply with the table attribute.
> +
>   Asynchronous operations
>   -----------------------
>   
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 96c5ae0fe4..b3238415f4 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3145,7 +3145,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 [vport_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 8858b56428..1eab12796f 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t port_id,
>    */
>   struct rte_flow_template_table;
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Special optional flags for template table attribute.
> + * Each bit stands for a table specialization
> + * offering a potential optimization at PMD layer.
> + * PMD can ignore the unsupported bits silently.
> + */
> +enum rte_flow_template_table_specialize {
> +	/**
> +	 * Specialize table for transfer flows which come only from wire.
> +	 * It allows PMD not to allocate resources for non-wire originated traffic.
> +	 * This bit is not a matching criteria, just an optimization hint.
> +	 * Flow rules which match non-wire originated traffic will be missed
> +	 * if the hint is supported.
> +	 */
> +	RTE_FLOW_TRANSFER_WIRE_ORIG = RTE_BIT32(0),
> +	/**
> +	 * Specialize table for transfer flows which come only from vport (e.g. VF, SF).
> +	 * It allows PMD not to allocate resources for non-vport originated traffic.
> +	 * This bit is not a matching criteria, just an optimization hint.
> +	 * Flow rules which match non-vport originated traffic will be missed
> +	 * if the hint is supported.
> +	 */
> +	RTE_FLOW_TRANSFER_VPORT_ORIG = RTE_BIT32(1),
> +};
> +
>   /**
>    * @warning
>    * @b EXPERIMENTAL: this API may change without prior notice.
> @@ -5201,6 +5229,10 @@ struct rte_flow_template_table_attr {
>   	 * Maximum number of flow rules that this table holds.
>   	 */
>   	uint32_t nb_flows;
> +	/**
> +	 * Optional hint flags for PMD optimization.
> +	 */
> +	enum rte_flow_template_table_specialize specialize;


IMHO it is not 100% correct to use enum for flag since
RTE_FLOW_TRANSFER_WIRE_ORIG | RTE_FLOW_TRANSFER_VPORT_ORIG
is not the enum member. uint32_t is a better option here since
bits are defined as RTE_BIT32. enum should be mentioned in the
description.

>   };
>   
>   /**
  
Andrew Rybchenko Nov. 8, 2022, 11:47 a.m. UTC | #2
On 11/8/22 14:39, Andrew Rybchenko wrote:
> On 11/4/22 13:44, Rongwei Liu wrote:
>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>> index 8858b56428..1eab12796f 100644
>> --- a/lib/ethdev/rte_flow.h
>> +++ b/lib/ethdev/rte_flow.h
>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t 
>> port_id,
>>    */
>>   struct rte_flow_template_table;
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Special optional flags for template table attribute.
>> + * Each bit stands for a table specialization
>> + * offering a potential optimization at PMD layer.
>> + * PMD can ignore the unsupported bits silently.
>> + */
>> +enum rte_flow_template_table_specialize {
>> +    /**
>> +     * Specialize table for transfer flows which come only from wire.
>> +     * It allows PMD not to allocate resources for non-wire 
>> originated traffic.
>> +     * This bit is not a matching criteria, just an optimization hint.
>> +     * Flow rules which match non-wire originated traffic will be missed
>> +     * if the hint is supported.

Sorry, but if so, the hint changes behavior.
Let's consider a rule which matches both VF originating and
wire originating traffic. Will the rule be missed (ignored)
regardless if the hint is supported or not?
I.e. it will not apply to wire originated traffic as well.

>> +     */
>> +    RTE_FLOW_TRANSFER_WIRE_ORIG = RTE_BIT32(0),
>> +    /**
>> +     * Specialize table for transfer flows which come only from vport 
>> (e.g. VF, SF).
>> +     * It allows PMD not to allocate resources for non-vport 
>> originated traffic.
>> +     * This bit is not a matching criteria, just an optimization hint.
>> +     * Flow rules which match non-vport originated traffic will be 
>> missed
>> +     * if the hint is supported.
>> +     */
>> +    RTE_FLOW_TRANSFER_VPORT_ORIG = RTE_BIT32(1),
>> +};
>> +
>>   /**
>>    * @warning
>>    * @b EXPERIMENTAL: this API may change without prior notice.
>> @@ -5201,6 +5229,10 @@ struct rte_flow_template_table_attr {
>>        * Maximum number of flow rules that this table holds.
>>        */
>>       uint32_t nb_flows;
>> +    /**
>> +     * Optional hint flags for PMD optimization.
>> +     */
>> +    enum rte_flow_template_table_specialize specialize;
> 
> 
> IMHO it is not 100% correct to use enum for flag since
> RTE_FLOW_TRANSFER_WIRE_ORIG | RTE_FLOW_TRANSFER_VPORT_ORIG
> is not the enum member. uint32_t is a better option here since
> bits are defined as RTE_BIT32. enum should be mentioned in the
> description.
> 
>>   };
>>   /**
>
  
Thomas Monjalon Nov. 8, 2022, 1:29 p.m. UTC | #3
08/11/2022 12:47, Andrew Rybchenko:
> On 11/8/22 14:39, Andrew Rybchenko wrote:
> > On 11/4/22 13:44, Rongwei Liu wrote:
> >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >> index 8858b56428..1eab12796f 100644
> >> --- a/lib/ethdev/rte_flow.h
> >> +++ b/lib/ethdev/rte_flow.h
> >> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t 
> >> port_id,
> >>    */
> >>   struct rte_flow_template_table;
> >> +/**
> >> + * @warning
> >> + * @b EXPERIMENTAL: this API may change without prior notice.
> >> + *
> >> + * Special optional flags for template table attribute.
> >> + * Each bit stands for a table specialization
> >> + * offering a potential optimization at PMD layer.
> >> + * PMD can ignore the unsupported bits silently.
> >> + */
> >> +enum rte_flow_template_table_specialize {
> >> +    /**
> >> +     * Specialize table for transfer flows which come only from wire.
> >> +     * It allows PMD not to allocate resources for non-wire 
> >> originated traffic.
> >> +     * This bit is not a matching criteria, just an optimization hint.
> >> +     * Flow rules which match non-wire originated traffic will be missed
> >> +     * if the hint is supported.
> 
> Sorry, but if so, the hint changes behavior.

Yes the hint may change behaviour.

> Let's consider a rule which matches both VF originating and
> wire originating traffic. Will the rule be missed (ignored)
> regardless if the hint is supported or not?

If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
the PMD may assume the table won't be used for traffic
which is not coming from wire ports.
As a consequence, the table may be implemented on the path
of wire traffic only.
In this case, the traffic coming from virtual ports
won't be affected by this table.
To answer the question, a rule matching both virtual and wire traffic
will be applied in a table affecting only wire traffic,
so it will still apply (not completely ignored).

If you really want to manage both types of traffic in this table,
you must not use such hint.

> I.e. it will not apply to wire originated traffic as well.
> 
> >> +     */
> >> +    RTE_FLOW_TRANSFER_WIRE_ORIG = RTE_BIT32(0),
> >> +    /**
> >> +     * Specialize table for transfer flows which come only from vport 
> >> (e.g. VF, SF).
> >> +     * It allows PMD not to allocate resources for non-vport 
> >> originated traffic.
> >> +     * This bit is not a matching criteria, just an optimization hint.
> >> +     * Flow rules which match non-vport originated traffic will be 
> >> missed
> >> +     * if the hint is supported.
> >> +     */
> >> +    RTE_FLOW_TRANSFER_VPORT_ORIG = RTE_BIT32(1),
> >> +};
> >> +
> >>   /**
> >>    * @warning
> >>    * @b EXPERIMENTAL: this API may change without prior notice.
> >> @@ -5201,6 +5229,10 @@ struct rte_flow_template_table_attr {
> >>        * Maximum number of flow rules that this table holds.
> >>        */
> >>       uint32_t nb_flows;
> >> +    /**
> >> +     * Optional hint flags for PMD optimization.
> >> +     */
> >> +    enum rte_flow_template_table_specialize specialize;
> > 
> > 
> > IMHO it is not 100% correct to use enum for flag since
> > RTE_FLOW_TRANSFER_WIRE_ORIG | RTE_FLOW_TRANSFER_VPORT_ORIG
> > is not the enum member. uint32_t is a better option here since
> > bits are defined as RTE_BIT32. enum should be mentioned in the
> > description.

I agree, let's not use enum.
Instead we can mention the prefix of the defines in the comments.
  
Andrew Rybchenko Nov. 8, 2022, 2:38 p.m. UTC | #4
On 11/8/22 16:29, Thomas Monjalon wrote:
> 08/11/2022 12:47, Andrew Rybchenko:
>> On 11/8/22 14:39, Andrew Rybchenko wrote:
>>> On 11/4/22 13:44, Rongwei Liu wrote:
>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>>> index 8858b56428..1eab12796f 100644
>>>> --- a/lib/ethdev/rte_flow.h
>>>> +++ b/lib/ethdev/rte_flow.h
>>>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t
>>>> port_id,
>>>>     */
>>>>    struct rte_flow_template_table;
>>>> +/**
>>>> + * @warning
>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>> + *
>>>> + * Special optional flags for template table attribute.
>>>> + * Each bit stands for a table specialization
>>>> + * offering a potential optimization at PMD layer.
>>>> + * PMD can ignore the unsupported bits silently.
>>>> + */
>>>> +enum rte_flow_template_table_specialize {
>>>> +    /**
>>>> +     * Specialize table for transfer flows which come only from wire.
>>>> +     * It allows PMD not to allocate resources for non-wire
>>>> originated traffic.
>>>> +     * This bit is not a matching criteria, just an optimization hint.
>>>> +     * Flow rules which match non-wire originated traffic will be missed
>>>> +     * if the hint is supported.
>>
>> Sorry, but if so, the hint changes behavior.
> 
> Yes the hint may change behaviour.
> 
>> Let's consider a rule which matches both VF originating and
>> wire originating traffic. Will the rule be missed (ignored)
>> regardless if the hint is supported or not?
> 
> If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
> the PMD may assume the table won't be used for traffic
> which is not coming from wire ports.
> As a consequence, the table may be implemented on the path
> of wire traffic only.
> In this case, the traffic coming from virtual ports
> won't be affected by this table.
> To answer the question, a rule matching both virtual and wire traffic
> will be applied in a table affecting only wire traffic,
> so it will still apply (not completely ignored).

If so, it is not a hint. It becomes matching criteria
which should be in pattern as we discussed.

> 
> If you really want to manage both types of traffic in this table,
> you must not use such hint.
> 
>> I.e. it will not apply to wire originated traffic as well.
>>
>>>> +     */
>>>> +    RTE_FLOW_TRANSFER_WIRE_ORIG = RTE_BIT32(0),
>>>> +    /**
>>>> +     * Specialize table for transfer flows which come only from vport
>>>> (e.g. VF, SF).
>>>> +     * It allows PMD not to allocate resources for non-vport
>>>> originated traffic.
>>>> +     * This bit is not a matching criteria, just an optimization hint.
>>>> +     * Flow rules which match non-vport originated traffic will be
>>>> missed
>>>> +     * if the hint is supported.
>>>> +     */
>>>> +    RTE_FLOW_TRANSFER_VPORT_ORIG = RTE_BIT32(1),
>>>> +};
>>>> +
>>>>    /**
>>>>     * @warning
>>>>     * @b EXPERIMENTAL: this API may change without prior notice.
>>>> @@ -5201,6 +5229,10 @@ struct rte_flow_template_table_attr {
>>>>         * Maximum number of flow rules that this table holds.
>>>>         */
>>>>        uint32_t nb_flows;
>>>> +    /**
>>>> +     * Optional hint flags for PMD optimization.
>>>> +     */
>>>> +    enum rte_flow_template_table_specialize specialize;
>>>
>>>
>>> IMHO it is not 100% correct to use enum for flag since
>>> RTE_FLOW_TRANSFER_WIRE_ORIG | RTE_FLOW_TRANSFER_VPORT_ORIG
>>> is not the enum member. uint32_t is a better option here since
>>> bits are defined as RTE_BIT32. enum should be mentioned in the
>>> description.
> 
> I agree, let's not use enum.
> Instead we can mention the prefix of the defines in the comments.
> 
>
  
Thomas Monjalon Nov. 8, 2022, 3:25 p.m. UTC | #5
08/11/2022 15:38, Andrew Rybchenko:
> On 11/8/22 16:29, Thomas Monjalon wrote:
> > 08/11/2022 12:47, Andrew Rybchenko:
> >> On 11/8/22 14:39, Andrew Rybchenko wrote:
> >>> On 11/4/22 13:44, Rongwei Liu wrote:
> >>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >>>> index 8858b56428..1eab12796f 100644
> >>>> --- a/lib/ethdev/rte_flow.h
> >>>> +++ b/lib/ethdev/rte_flow.h
> >>>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t
> >>>> port_id,
> >>>>     */
> >>>>    struct rte_flow_template_table;
> >>>> +/**
> >>>> + * @warning
> >>>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>>> + *
> >>>> + * Special optional flags for template table attribute.
> >>>> + * Each bit stands for a table specialization
> >>>> + * offering a potential optimization at PMD layer.
> >>>> + * PMD can ignore the unsupported bits silently.
> >>>> + */
> >>>> +enum rte_flow_template_table_specialize {
> >>>> +    /**
> >>>> +     * Specialize table for transfer flows which come only from wire.
> >>>> +     * It allows PMD not to allocate resources for non-wire
> >>>> originated traffic.
> >>>> +     * This bit is not a matching criteria, just an optimization hint.
> >>>> +     * Flow rules which match non-wire originated traffic will be missed
> >>>> +     * if the hint is supported.
> >>
> >> Sorry, but if so, the hint changes behavior.
> > 
> > Yes the hint may change behaviour.
> > 
> >> Let's consider a rule which matches both VF originating and
> >> wire originating traffic. Will the rule be missed (ignored)
> >> regardless if the hint is supported or not?
> > 
> > If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
> > the PMD may assume the table won't be used for traffic
> > which is not coming from wire ports.
> > As a consequence, the table may be implemented on the path
> > of wire traffic only.
> > In this case, the traffic coming from virtual ports
> > won't be affected by this table.
> > To answer the question, a rule matching both virtual and wire traffic
> > will be applied in a table affecting only wire traffic,
> > so it will still apply (not completely ignored).
> 
> If so, it is not a hint. It becomes matching criteria
> which should be in pattern as we discussed.

It is not a strict matching because the PMD is free to support it or not.
  
Andrew Rybchenko Nov. 9, 2022, 8:53 a.m. UTC | #6
On 11/8/22 18:25, Thomas Monjalon wrote:
> 08/11/2022 15:38, Andrew Rybchenko:
>> On 11/8/22 16:29, Thomas Monjalon wrote:
>>> 08/11/2022 12:47, Andrew Rybchenko:
>>>> On 11/8/22 14:39, Andrew Rybchenko wrote:
>>>>> On 11/4/22 13:44, Rongwei Liu wrote:
>>>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>>>>> index 8858b56428..1eab12796f 100644
>>>>>> --- a/lib/ethdev/rte_flow.h
>>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t
>>>>>> port_id,
>>>>>>      */
>>>>>>     struct rte_flow_template_table;
>>>>>> +/**
>>>>>> + * @warning
>>>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>>>> + *
>>>>>> + * Special optional flags for template table attribute.
>>>>>> + * Each bit stands for a table specialization
>>>>>> + * offering a potential optimization at PMD layer.
>>>>>> + * PMD can ignore the unsupported bits silently.
>>>>>> + */
>>>>>> +enum rte_flow_template_table_specialize {
>>>>>> +    /**
>>>>>> +     * Specialize table for transfer flows which come only from wire.
>>>>>> +     * It allows PMD not to allocate resources for non-wire
>>>>>> originated traffic.
>>>>>> +     * This bit is not a matching criteria, just an optimization hint.
>>>>>> +     * Flow rules which match non-wire originated traffic will be missed
>>>>>> +     * if the hint is supported.
>>>>
>>>> Sorry, but if so, the hint changes behavior.
>>>
>>> Yes the hint may change behaviour.
>>>
>>>> Let's consider a rule which matches both VF originating and
>>>> wire originating traffic. Will the rule be missed (ignored)
>>>> regardless if the hint is supported or not?
>>>
>>> If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
>>> the PMD may assume the table won't be used for traffic
>>> which is not coming from wire ports.
>>> As a consequence, the table may be implemented on the path
>>> of wire traffic only.
>>> In this case, the traffic coming from virtual ports
>>> won't be affected by this table.
>>> To answer the question, a rule matching both virtual and wire traffic
>>> will be applied in a table affecting only wire traffic,
>>> so it will still apply (not completely ignored).
>>
>> If so, it is not a hint. It becomes matching criteria
>> which should be in pattern as we discussed.
> 
> It is not a strict matching because the PMD is free to support it or not.

It cannot be optional matching criteria. Matching criteria must
be always mandatory. Otherwise application does not know what
to expect and behaviour may legitimately vary on different
vendors.
  
Thomas Monjalon Nov. 9, 2022, 9:03 a.m. UTC | #7
09/11/2022 09:53, Andrew Rybchenko:
> On 11/8/22 18:25, Thomas Monjalon wrote:
> > 08/11/2022 15:38, Andrew Rybchenko:
> >> On 11/8/22 16:29, Thomas Monjalon wrote:
> >>> 08/11/2022 12:47, Andrew Rybchenko:
> >>>> On 11/8/22 14:39, Andrew Rybchenko wrote:
> >>>>> On 11/4/22 13:44, Rongwei Liu wrote:
> >>>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >>>>>> index 8858b56428..1eab12796f 100644
> >>>>>> --- a/lib/ethdev/rte_flow.h
> >>>>>> +++ b/lib/ethdev/rte_flow.h
> >>>>>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t
> >>>>>> port_id,
> >>>>>>      */
> >>>>>>     struct rte_flow_template_table;
> >>>>>> +/**
> >>>>>> + * @warning
> >>>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>>>>> + *
> >>>>>> + * Special optional flags for template table attribute.
> >>>>>> + * Each bit stands for a table specialization
> >>>>>> + * offering a potential optimization at PMD layer.
> >>>>>> + * PMD can ignore the unsupported bits silently.
> >>>>>> + */
> >>>>>> +enum rte_flow_template_table_specialize {
> >>>>>> +    /**
> >>>>>> +     * Specialize table for transfer flows which come only from wire.
> >>>>>> +     * It allows PMD not to allocate resources for non-wire
> >>>>>> originated traffic.
> >>>>>> +     * This bit is not a matching criteria, just an optimization hint.
> >>>>>> +     * Flow rules which match non-wire originated traffic will be missed
> >>>>>> +     * if the hint is supported.
> >>>>
> >>>> Sorry, but if so, the hint changes behavior.
> >>>
> >>> Yes the hint may change behaviour.
> >>>
> >>>> Let's consider a rule which matches both VF originating and
> >>>> wire originating traffic. Will the rule be missed (ignored)
> >>>> regardless if the hint is supported or not?
> >>>
> >>> If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
> >>> the PMD may assume the table won't be used for traffic
> >>> which is not coming from wire ports.
> >>> As a consequence, the table may be implemented on the path
> >>> of wire traffic only.
> >>> In this case, the traffic coming from virtual ports
> >>> won't be affected by this table.
> >>> To answer the question, a rule matching both virtual and wire traffic
> >>> will be applied in a table affecting only wire traffic,
> >>> so it will still apply (not completely ignored).
> >>
> >> If so, it is not a hint. It becomes matching criteria
> >> which should be in pattern as we discussed.
> > 
> > It is not a strict matching because the PMD is free to support it or not.
> 
> It cannot be optional matching criteria. Matching criteria must
> be always mandatory. Otherwise application does not know what
> to expect and behaviour may legitimately vary on different
> vendors.

I think you take it in the wrong direction.
The idea is not to have it as a criteria.
Let me explain again:

If an application is using a flow table to manage flows
which *always* come from the same type of port (wire or virtual),
then the application can give this information to the driver.
With this assumption coming from the application,
the driver may do some optimizations.

Now about what is explained above:
If the application gives such a hint
but does not respect its own assumption,
then confusion happens.
  
Andrew Rybchenko Nov. 9, 2022, 9:36 a.m. UTC | #8
On 11/9/22 12:03, Thomas Monjalon wrote:
> 09/11/2022 09:53, Andrew Rybchenko:
>> On 11/8/22 18:25, Thomas Monjalon wrote:
>>> 08/11/2022 15:38, Andrew Rybchenko:
>>>> On 11/8/22 16:29, Thomas Monjalon wrote:
>>>>> 08/11/2022 12:47, Andrew Rybchenko:
>>>>>> On 11/8/22 14:39, Andrew Rybchenko wrote:
>>>>>>> On 11/4/22 13:44, Rongwei Liu wrote:
>>>>>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>>>>>>> index 8858b56428..1eab12796f 100644
>>>>>>>> --- a/lib/ethdev/rte_flow.h
>>>>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>>>>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t
>>>>>>>> port_id,
>>>>>>>>       */
>>>>>>>>      struct rte_flow_template_table;
>>>>>>>> +/**
>>>>>>>> + * @warning
>>>>>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>>>>>> + *
>>>>>>>> + * Special optional flags for template table attribute.
>>>>>>>> + * Each bit stands for a table specialization
>>>>>>>> + * offering a potential optimization at PMD layer.
>>>>>>>> + * PMD can ignore the unsupported bits silently.
>>>>>>>> + */
>>>>>>>> +enum rte_flow_template_table_specialize {
>>>>>>>> +    /**
>>>>>>>> +     * Specialize table for transfer flows which come only from wire.
>>>>>>>> +     * It allows PMD not to allocate resources for non-wire
>>>>>>>> originated traffic.
>>>>>>>> +     * This bit is not a matching criteria, just an optimization hint.
>>>>>>>> +     * Flow rules which match non-wire originated traffic will be missed
>>>>>>>> +     * if the hint is supported.
>>>>>>
>>>>>> Sorry, but if so, the hint changes behavior.
>>>>>
>>>>> Yes the hint may change behaviour.
>>>>>
>>>>>> Let's consider a rule which matches both VF originating and
>>>>>> wire originating traffic. Will the rule be missed (ignored)
>>>>>> regardless if the hint is supported or not?
>>>>>
>>>>> If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
>>>>> the PMD may assume the table won't be used for traffic
>>>>> which is not coming from wire ports.
>>>>> As a consequence, the table may be implemented on the path
>>>>> of wire traffic only.
>>>>> In this case, the traffic coming from virtual ports
>>>>> won't be affected by this table.
>>>>> To answer the question, a rule matching both virtual and wire traffic
>>>>> will be applied in a table affecting only wire traffic,
>>>>> so it will still apply (not completely ignored).
>>>>
>>>> If so, it is not a hint. It becomes matching criteria
>>>> which should be in pattern as we discussed.
>>>
>>> It is not a strict matching because the PMD is free to support it or not.
>>
>> It cannot be optional matching criteria. Matching criteria must
>> be always mandatory. Otherwise application does not know what
>> to expect and behaviour may legitimately vary on different
>> vendors.
> 
> I think you take it in the wrong direction.
> The idea is not to have it as a criteria.
> Let me explain again:
> 
> If an application is using a flow table to manage flows
> which *always* come from the same type of port (wire or virtual),

What does guarantee it? Is it used a jump-table and jump rule
must guarantee it? Or has pattern corresponding unit?

It is very thin ice and I'm ready to bet money that finally
it will be used as a matching criteria intentionally or not
intentionally. Simply because it works as matching criteria
on, for example, Mellanox. I.e. if rules from table with
corresponding hint are programmed to HW which applies these
rules on traffic from wire only - effectively it is a matching
criteria. And it will be used this way. And it will be not
portable to other HW which does not support the hint.
So, we're making an API which is very easy to misuse if not
to say more.

You know better if it is OK or not to rely on liable users
in the case of DPDK.

It would be much safer if we do not rely on application in this
case, introduce a new pattern item to specify origin and
require PMD to check that pattern has either a new pattern item
or corresponding  REPRESENTED_PORT/PORT_REPRESENTOR pattern
item.

I realize that my concerns could be not valid and it is just
a paranoia. Just add your ack and let's move forward.

> then the application can give this information to the driver.
> With this assumption coming from the application,
> the driver may do some optimizations.
> 
> Now about what is explained above:
> If the application gives such a hint
> but does not respect its own assumption,
> then confusion happens.
> 
>
  
Thomas Monjalon Nov. 9, 2022, 10:50 a.m. UTC | #9
09/11/2022 10:36, Andrew Rybchenko:
> On 11/9/22 12:03, Thomas Monjalon wrote:
> > 09/11/2022 09:53, Andrew Rybchenko:
> >> On 11/8/22 18:25, Thomas Monjalon wrote:
> >>> 08/11/2022 15:38, Andrew Rybchenko:
> >>>> On 11/8/22 16:29, Thomas Monjalon wrote:
> >>>>> 08/11/2022 12:47, Andrew Rybchenko:
> >>>>>> On 11/8/22 14:39, Andrew Rybchenko wrote:
> >>>>>>> On 11/4/22 13:44, Rongwei Liu wrote:
> >>>>>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >>>>>>>> index 8858b56428..1eab12796f 100644
> >>>>>>>> --- a/lib/ethdev/rte_flow.h
> >>>>>>>> +++ b/lib/ethdev/rte_flow.h
> >>>>>>>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t
> >>>>>>>> port_id,
> >>>>>>>>       */
> >>>>>>>>      struct rte_flow_template_table;
> >>>>>>>> +/**
> >>>>>>>> + * @warning
> >>>>>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>>>>>>> + *
> >>>>>>>> + * Special optional flags for template table attribute.
> >>>>>>>> + * Each bit stands for a table specialization
> >>>>>>>> + * offering a potential optimization at PMD layer.
> >>>>>>>> + * PMD can ignore the unsupported bits silently.
> >>>>>>>> + */
> >>>>>>>> +enum rte_flow_template_table_specialize {
> >>>>>>>> +    /**
> >>>>>>>> +     * Specialize table for transfer flows which come only from wire.
> >>>>>>>> +     * It allows PMD not to allocate resources for non-wire
> >>>>>>>> originated traffic.
> >>>>>>>> +     * This bit is not a matching criteria, just an optimization hint.
> >>>>>>>> +     * Flow rules which match non-wire originated traffic will be missed
> >>>>>>>> +     * if the hint is supported.
> >>>>>>
> >>>>>> Sorry, but if so, the hint changes behavior.
> >>>>>
> >>>>> Yes the hint may change behaviour.
> >>>>>
> >>>>>> Let's consider a rule which matches both VF originating and
> >>>>>> wire originating traffic. Will the rule be missed (ignored)
> >>>>>> regardless if the hint is supported or not?
> >>>>>
> >>>>> If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
> >>>>> the PMD may assume the table won't be used for traffic
> >>>>> which is not coming from wire ports.
> >>>>> As a consequence, the table may be implemented on the path
> >>>>> of wire traffic only.
> >>>>> In this case, the traffic coming from virtual ports
> >>>>> won't be affected by this table.
> >>>>> To answer the question, a rule matching both virtual and wire traffic
> >>>>> will be applied in a table affecting only wire traffic,
> >>>>> so it will still apply (not completely ignored).
> >>>>
> >>>> If so, it is not a hint. It becomes matching criteria
> >>>> which should be in pattern as we discussed.
> >>>
> >>> It is not a strict matching because the PMD is free to support it or not.
> >>
> >> It cannot be optional matching criteria. Matching criteria must
> >> be always mandatory. Otherwise application does not know what
> >> to expect and behaviour may legitimately vary on different
> >> vendors.
> > 
> > I think you take it in the wrong direction.
> > The idea is not to have it as a criteria.
> > Let me explain again:
> > 
> > If an application is using a flow table to manage flows
> > which *always* come from the same type of port (wire or virtual),
> 
> What does guarantee it? Is it used a jump-table and jump rule
> must guarantee it? Or has pattern corresponding unit?
> 
> It is very thin ice and I'm ready to bet money that finally
> it will be used as a matching criteria intentionally or not
> intentionally. Simply because it works as matching criteria
> on, for example, Mellanox. I.e. if rules from table with
> corresponding hint are programmed to HW which applies these
> rules on traffic from wire only - effectively it is a matching
> criteria. And it will be used this way. And it will be not
> portable to other HW which does not support the hint.
> So, we're making an API which is very easy to misuse if not
> to say more.

I completely understand your concern (I have same).
In other words, if the application misuse the hint,
it will become not portable.
That's why I made sure to highlight such misue consequence
in the API comments.

> You know better if it is OK or not to rely on liable users
> in the case of DPDK.

I do not rely on users, and I don't want to block innovation.
That's why I want to make sure all is explained and clear,
so freedom comes with responsibility.

> It would be much safer if we do not rely on application in this
> case, introduce a new pattern item to specify origin and
> require PMD to check that pattern has either a new pattern item
> or corresponding  REPRESENTED_PORT/PORT_REPRESENTOR pattern
> item.

Safer is not often compatible with fastest :)

> I realize that my concerns could be not valid and it is just
> a paranoia. Just add your ack and let's move forward.

Let's wait for other opinions.

> > then the application can give this information to the driver.
> > With this assumption coming from the application,
> > the driver may do some optimizations.
> > 
> > Now about what is explained above:
> > If the application gives such a hint
> > but does not respect its own assumption,
> > then confusion happens.
  

Patch

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 88108498e0..15f2af9b40 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -184,6 +184,8 @@  enum index {
 	TABLE_INGRESS,
 	TABLE_EGRESS,
 	TABLE_TRANSFER,
+	TABLE_TRANSFER_WIRE_ORIG,
+	TABLE_TRANSFER_VPORT_ORIG,
 	TABLE_RULES_NUMBER,
 	TABLE_PATTERN_TEMPLATE,
 	TABLE_ACTIONS_TEMPLATE,
@@ -1158,6 +1160,8 @@  static const enum index next_table_attr[] = {
 	TABLE_INGRESS,
 	TABLE_EGRESS,
 	TABLE_TRANSFER,
+	TABLE_TRANSFER_WIRE_ORIG,
+	TABLE_TRANSFER_VPORT_ORIG,
 	TABLE_RULES_NUMBER,
 	TABLE_PATTERN_TEMPLATE,
 	TABLE_ACTIONS_TEMPLATE,
@@ -2933,6 +2937,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_VPORT_ORIG] = {
+		.name = "vport_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",
@@ -8993,6 +9009,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.specialize = RTE_FLOW_TRANSFER_WIRE_ORIG;
+		return len;
+	case TABLE_TRANSFER_VPORT_ORIG:
+		if (!out->args.table.attr.flow_attr.transfer)
+			return -1;
+		out->args.table.attr.specialize = RTE_FLOW_TRANSFER_VPORT_ORIG;
+		return len;
 	default:
 		return -1;
 	}
diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 3e6242803d..d9ca041ae4 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -3605,6 +3605,21 @@  and pattern and actions templates are created.
                    &actions_templates, nb_actions_templ,
                    &error);
 
+Table Attribute: Specialize
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Application can help optimizing underlayer resources and insertion rate
+by specializing template table.
+Specialization is done by providing hints
+in the template table attribute ``specialize``.
+
+This attribute is not mandatory for each PMD to implement.
+If a hint is not supported, it will be silently ignored,
+and no special optimization is done.
+
+If a table is specialized, the application should make sure the rules
+comply with the table attribute.
+
 Asynchronous operations
 -----------------------
 
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index 96c5ae0fe4..b3238415f4 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -3145,7 +3145,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 [vport_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 8858b56428..1eab12796f 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -5186,6 +5186,34 @@  rte_flow_actions_template_destroy(uint16_t port_id,
  */
 struct rte_flow_template_table;
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Special optional flags for template table attribute.
+ * Each bit stands for a table specialization
+ * offering a potential optimization at PMD layer.
+ * PMD can ignore the unsupported bits silently.
+ */
+enum rte_flow_template_table_specialize {
+	/**
+	 * Specialize table for transfer flows which come only from wire.
+	 * It allows PMD not to allocate resources for non-wire originated traffic.
+	 * This bit is not a matching criteria, just an optimization hint.
+	 * Flow rules which match non-wire originated traffic will be missed
+	 * if the hint is supported.
+	 */
+	RTE_FLOW_TRANSFER_WIRE_ORIG = RTE_BIT32(0),
+	/**
+	 * Specialize table for transfer flows which come only from vport (e.g. VF, SF).
+	 * It allows PMD not to allocate resources for non-vport originated traffic.
+	 * This bit is not a matching criteria, just an optimization hint.
+	 * Flow rules which match non-vport originated traffic will be missed
+	 * if the hint is supported.
+	 */
+	RTE_FLOW_TRANSFER_VPORT_ORIG = RTE_BIT32(1),
+};
+
 /**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
@@ -5201,6 +5229,10 @@  struct rte_flow_template_table_attr {
 	 * Maximum number of flow rules that this table holds.
 	 */
 	uint32_t nb_flows;
+	/**
+	 * Optional hint flags for PMD optimization.
+	 */
+	enum rte_flow_template_table_specialize specialize;
 };
 
 /**