diff mbox series

[3/7] ethdev: add flow action type update as an offload

Message ID 20190816055511.2322-4-pbhagavatula@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: add new Rx offload flags |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Pavan Nikhilesh Bhagavatula Aug. 16, 2019, 5:55 a.m. UTC 
  From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be used to
enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 doc/guides/nics/features.rst   | 12 ++++++++++++
 lib/librte_ethdev/rte_ethdev.h |  1 +
 2 files changed, 13 insertions(+)

Comments

Andrew Rybchenko Aug. 16, 2019, 8:05 a.m. UTC | #1 
On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be used to
> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.

Notes similar to RSS hash.

It requires better motivation why. It lets Rx queue know that
it will be used as flow action MARK target and the queue should
be configured to deliver the mark from NIC to PMD and processed
in the driver.

Also I think that flow API action MARK documentation should be
updated to mentioned the offload.

> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>   doc/guides/nics/features.rst   | 12 ++++++++++++
>   lib/librte_ethdev/rte_ethdev.h |  1 +
>   2 files changed, 13 insertions(+)
>
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index f79b69b38..d67430d90 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
>   * **[provides]   mbuf**: ``mbuf.packet_type``.
>   
>   
> +.. _nic_features_flow_action_type_update:

May be  _nic_features_flow_mark ?

> +
> +Flow type update

May be "Flow mark delivery" ?

> +----------------
> +
> +Supports flow action type update to ``mbuf.ol_flags`` and ``mbuf.hash.fdir.hi``.
> +
> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.

DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.



> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``, ``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
> +  ``mbuf.hash.fdir.hi``
> +
> +
>   .. _nic_features_timesync:
>   
>   Timesync
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 889486a11..4a0cff830 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
>   #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>   #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>   #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
>   
>   #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>   				 DEV_RX_OFFLOAD_UDP_CKSUM | \

Add to rte_rx_offload_names
Pavan Nikhilesh Bhagavatula Aug. 17, 2019, 2:23 p.m. UTC | #2 
>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
>
>Notes similar to RSS hash.
>
>It requires better motivation why. It lets Rx queue know that
>it will be used as flow action MARK target and the queue should
>be configured to deliver the mark from NIC to PMD and processed
>in the driver.
>
>Also I think that flow API action MARK documentation should be
>updated to mentioned the offload.

Ack will reword the commit log and update the documentation.
>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> ---
>>   doc/guides/nics/features.rst   | 12 ++++++++++++
>>   lib/librte_ethdev/rte_ethdev.h |  1 +
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/doc/guides/nics/features.rst
>b/doc/guides/nics/features.rst
>> index f79b69b38..d67430d90 100644
>> --- a/doc/guides/nics/features.rst
>> +++ b/doc/guides/nics/features.rst
>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
>>   * **[provides]   mbuf**: ``mbuf.packet_type``.
>>
>>
>> +.. _nic_features_flow_action_type_update:
>
>May be  _nic_features_flow_mark ?
>
>> +
>> +Flow type update
>
>May be "Flow mark delivery" ?

I named it flow action type update because we use the same flag to represent two different
flow types i.e. RTE_FLOW_ACTION_TYPE_FLAG and RTE_FLOW_ACTION_TYPE_MARK.

I'm not so sure about the name RX_OFFLOAD_FLOW_MARK too because mbuf might have ol_flags
marked for ACTION_TYPE_FLAG. Let me know your thoughts on this. 

I will modify the name once we come to an agreement. 

>
>> +----------------
>> +
>> +Supports flow action type update to ``mbuf.ol_flags`` and
>``mbuf.hash.fdir.hi``.
>> +
>> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
>``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
>
>DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.

Ack, will fix in v2.

>
>
>
>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
>``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
>> +  ``mbuf.hash.fdir.hi``
>> +
>> +
>>   .. _nic_features_timesync:
>>
>>   Timesync
>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>b/lib/librte_ethdev/rte_ethdev.h
>> index 889486a11..4a0cff830 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
>>   #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>>   #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>>   #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
>> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
>>
>>   #define DEV_RX_OFFLOAD_CHECKSUM
>(DEV_RX_OFFLOAD_IPV4_CKSUM | \
>>   				 DEV_RX_OFFLOAD_UDP_CKSUM | \
>
>Add to rte_rx_offload_names
Shahaf Shuler Aug. 18, 2019, 4:59 a.m. UTC | #3 
Friday, August 16, 2019 11:05 AM, Andrew Rybchenko:
> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as
> an offload
> 
> On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote:
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be
> used to
> > enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
> 
> Notes similar to RSS hash.
> 
> It requires better motivation why. It lets Rx queue know that it will be used
> as flow action MARK target and the queue should be configured to deliver
> the mark from NIC to PMD and processed in the driver.

This one is even worse than the RSS (sorry). 

First - the API breakage exists also here and if we want to include such patch we should have proper doc on RN. 
Second - the user explicitly inserted a rte_flow rule w/ action mark. Its expectation is to receive his mark on the mbuf (otherwise why would it set this action?).  it is not expected from the user to set another offload flag just to enable the mark set on the mbuf. It makes the user experience very convoluted.   
Third - so far we never reported rte_flow capabilities, and there is a good reason for it - the cap matrix is too big. For rte_flow the chosen approach was trail and error. If we start w/ the flow mark, the amount of cap bits the application will need to monitor will be huge. 

My suggestion here, for PMD that wants to optimize their datapath is to check if flow w/ mark action was inserted on the queue. So long there is no such flow they can disable the set of the mark.


> 
> Also I think that flow API action MARK documentation should be updated to
> mentioned the offload.
> 
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > ---
> >   doc/guides/nics/features.rst   | 12 ++++++++++++
> >   lib/librte_ethdev/rte_ethdev.h |  1 +
> >   2 files changed, 13 insertions(+)
> >
> > diff --git a/doc/guides/nics/features.rst
> > b/doc/guides/nics/features.rst index f79b69b38..d67430d90 100644
> > --- a/doc/guides/nics/features.rst
> > +++ b/doc/guides/nics/features.rst
> > @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
> >   * **[provides]   mbuf**: ``mbuf.packet_type``.
> >
> >
> > +.. _nic_features_flow_action_type_update:
> 
> May be  _nic_features_flow_mark ?
> 
> > +
> > +Flow type update
> 
> May be "Flow mark delivery" ?
> 
> > +----------------
> > +
> > +Supports flow action type update to ``mbuf.ol_flags`` and
> ``mbuf.hash.fdir.hi``.
> > +
> > +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
> 
> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.
> 
> 
> 
> > +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
> > +``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
> > +  ``mbuf.hash.fdir.hi``
> > +
> > +
> >   .. _nic_features_timesync:
> >
> >   Timesync
> > diff --git a/lib/librte_ethdev/rte_ethdev.h
> > b/lib/librte_ethdev/rte_ethdev.h index 889486a11..4a0cff830 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
> >   #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >   #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >   #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> > +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
> >
> >   #define DEV_RX_OFFLOAD_CHECKSUM
> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >   				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> 
> Add to rte_rx_offload_names
Andrew Rybchenko Aug. 18, 2019, 5:57 a.m. UTC | #4 
On 8/18/19 7:59 AM, Shahaf Shuler wrote:
> Friday, August 16, 2019 11:05 AM, Andrew Rybchenko:
>> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as
>> an offload
>>
>> On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote:
>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>
>>> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be
>> used to
>>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
>> Notes similar to RSS hash.
>>
>> It requires better motivation why. It lets Rx queue know that it will be used
>> as flow action MARK target and the queue should be configured to deliver
>> the mark from NIC to PMD and processed in the driver.
> This one is even worse than the RSS (sorry).
>
> First - the API breakage exists also here and if we want to include such patch we should have proper doc on RN.

Yes, there is a deprecation notice for it in v19.08 and I already
mentioned in review notes for the patch [1/7] that release notes
are required.

> Second - the user explicitly inserted a rte_flow rule w/ action mark. Its expectation is to receive his mark on the mbuf (otherwise why would it set this action?).  it is not expected from the user to set another offload flag just to enable the mark set on the mbuf. It makes the user experience very convoluted.
> Third - so far we never reported rte_flow capabilities, and there is a good reason for it - the cap matrix is too big. For rte_flow the chosen approach was trail and error. If we start w/ the flow mark, the amount of cap bits the application will need to monitor will be huge.

The feature differs a lot from other rte_flow features since it
adds Rx meta information.
And yes, rte_flow rule to set mark should fail with appropriate
diagnostics if the offload is supported by not enabled or not
supported at all. So, application will be informed and I think
it is less worse than RSS hash.

> My suggestion here, for PMD that wants to optimize their datapath is to check if flow w/ mark action was inserted on the queue. So long there is no such flow they can disable the set of the mark.

Unfortunately the information is required on Rx queue setup
stage and rte_rule insertion is too late.

Anyway, the important point here is all Rx offloads consistency:
want something - enable it. The only remaining exception is
packet type which default behaviour is preserved since really
flexible solution suggested by Konstantin.

>> Also I think that flow API action MARK documentation should be updated to
>> mentioned the offload.
>>
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> ---
>>>    doc/guides/nics/features.rst   | 12 ++++++++++++
>>>    lib/librte_ethdev/rte_ethdev.h |  1 +
>>>    2 files changed, 13 insertions(+)
>>>
>>> diff --git a/doc/guides/nics/features.rst
>>> b/doc/guides/nics/features.rst index f79b69b38..d67430d90 100644
>>> --- a/doc/guides/nics/features.rst
>>> +++ b/doc/guides/nics/features.rst
>>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
>>>    * **[provides]   mbuf**: ``mbuf.packet_type``.
>>>
>>>
>>> +.. _nic_features_flow_action_type_update:
>> May be  _nic_features_flow_mark ?
>>
>>> +
>>> +Flow type update
>> May be "Flow mark delivery" ?
>>
>>> +----------------
>>> +
>>> +Supports flow action type update to ``mbuf.ol_flags`` and
>> ``mbuf.hash.fdir.hi``.
>>> +
>>> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
>> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
>>
>> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.
>>
>>
>>
>>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
>>> +``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
>>> +  ``mbuf.hash.fdir.hi``
>>> +
>>> +
>>>    .. _nic_features_timesync:
>>>
>>>    Timesync
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>> b/lib/librte_ethdev/rte_ethdev.h index 889486a11..4a0cff830 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
>>>    #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>>>    #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>>>    #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
>>> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
>>>
>>>    #define DEV_RX_OFFLOAD_CHECKSUM
>> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>>>    				 DEV_RX_OFFLOAD_UDP_CKSUM | \
>> Add to rte_rx_offload_names
Shahaf Shuler Aug. 18, 2019, 6:20 a.m. UTC | #5 
Sunday, August 18, 2019 8:57 AM, Andrew Rybchenko:
> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as
> an offload
> 
> On 8/18/19 7:59 AM, Shahaf Shuler wrote:
> > Friday, August 16, 2019 11:05 AM, Andrew Rybchenko:
> >> <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type
> >> update as an offload
> >>
> >> On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote:
> >>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>>
> >>> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be
> >> used to
> >>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
> >> Notes similar to RSS hash.
> >>
> >> It requires better motivation why. It lets Rx queue know that it will
> >> be used as flow action MARK target and the queue should be configured
> >> to deliver the mark from NIC to PMD and processed in the driver.
> > This one is even worse than the RSS (sorry).
> >
> > First - the API breakage exists also here and if we want to include such
> patch we should have proper doc on RN.
> 
> Yes, there is a deprecation notice for it in v19.08 and I already mentioned in
> review notes for the patch [1/7] that release notes are required.
> 
> > Second - the user explicitly inserted a rte_flow rule w/ action mark. Its
> expectation is to receive his mark on the mbuf (otherwise why would it set
> this action?).  it is not expected from the user to set another offload flag just
> to enable the mark set on the mbuf. It makes the user experience very
> convoluted.
> > Third - so far we never reported rte_flow capabilities, and there is a good
> reason for it - the cap matrix is too big. For rte_flow the chosen approach was
> trail and error. If we start w/ the flow mark, the amount of cap bits the
> application will need to monitor will be huge.
> 
> The feature differs a lot from other rte_flow features since it adds Rx meta
> information.
> And yes, rte_flow rule to set mark should fail with appropriate diagnostics if
> the offload is supported by not enabled or not supported at all. So,
> application will be informed and I think it is less worse than RSS hash.
> 
> > My suggestion here, for PMD that wants to optimize their datapath is to
> check if flow w/ mark action was inserted on the queue. So long there is no
> such flow they can disable the set of the mark.
> 
> Unfortunately the information is required on Rx queue setup stage and
> rte_rule insertion is too late.

See my comments on the RSS patch. Same point of discussion.

The main question we need to answer -
User set flow mark action by rte_flow (that was accepted). Why does it need to set more flag? 

> 
> Anyway, the important point here is all Rx offloads consistency:
> want something - enable it. The only remaining exception is packet type
> which default behaviour is preserved since really flexible solution suggested
> by Konstantin.
> 
> >> Also I think that flow API action MARK documentation should be
> >> updated to mentioned the offload.
> >>
> >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>> ---
> >>>    doc/guides/nics/features.rst   | 12 ++++++++++++
> >>>    lib/librte_ethdev/rte_ethdev.h |  1 +
> >>>    2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/doc/guides/nics/features.rst
> >>> b/doc/guides/nics/features.rst index f79b69b38..d67430d90 100644
> >>> --- a/doc/guides/nics/features.rst
> >>> +++ b/doc/guides/nics/features.rst
> >>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
> >>>    * **[provides]   mbuf**: ``mbuf.packet_type``.
> >>>
> >>>
> >>> +.. _nic_features_flow_action_type_update:
> >> May be  _nic_features_flow_mark ?
> >>
> >>> +
> >>> +Flow type update
> >> May be "Flow mark delivery" ?
> >>
> >>> +----------------
> >>> +
> >>> +Supports flow action type update to ``mbuf.ol_flags`` and
> >> ``mbuf.hash.fdir.hi``.
> >>> +
> >>> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
> >> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
> >>
> >> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.
> >>
> >>
> >>
> >>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
> >>> +``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
> >>> +  ``mbuf.hash.fdir.hi``
> >>> +
> >>> +
> >>>    .. _nic_features_timesync:
> >>>
> >>>    Timesync
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>> b/lib/librte_ethdev/rte_ethdev.h index 889486a11..4a0cff830 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
> >>>    #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >>>    #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >>>    #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> >>> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
> >>>
> >>>    #define DEV_RX_OFFLOAD_CHECKSUM
> >> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >>>    				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> >> Add to rte_rx_offload_names
Andrew Rybchenko Aug. 18, 2019, 7:08 a.m. UTC | #6 
On 8/18/19 9:20 AM, Shahaf Shuler wrote:
> Sunday, August 18, 2019 8:57 AM, Andrew Rybchenko:
>> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as
>> an offload
>>
>> On 8/18/19 7:59 AM, Shahaf Shuler wrote:
>>> Friday, August 16, 2019 11:05 AM, Andrew Rybchenko:
>>>> <marko.kovacevic@intel.com>; Thomas Monjalon
>> <thomas@monjalon.net>
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type
>>>> update as an offload
>>>>
>>>> On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote:
>>>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>>
>>>>> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be
>>>> used to
>>>>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
>>>> Notes similar to RSS hash.
>>>>
>>>> It requires better motivation why. It lets Rx queue know that it will
>>>> be used as flow action MARK target and the queue should be configured
>>>> to deliver the mark from NIC to PMD and processed in the driver.
>>> This one is even worse than the RSS (sorry).
>>>
>>> First - the API breakage exists also here and if we want to include such
>> patch we should have proper doc on RN.
>>
>> Yes, there is a deprecation notice for it in v19.08 and I already mentioned in
>> review notes for the patch [1/7] that release notes are required.
>>
>>> Second - the user explicitly inserted a rte_flow rule w/ action mark. Its
>> expectation is to receive his mark on the mbuf (otherwise why would it set
>> this action?).  it is not expected from the user to set another offload flag just
>> to enable the mark set on the mbuf. It makes the user experience very
>> convoluted.
>>> Third - so far we never reported rte_flow capabilities, and there is a good
>> reason for it - the cap matrix is too big. For rte_flow the chosen approach was
>> trail and error. If we start w/ the flow mark, the amount of cap bits the
>> application will need to monitor will be huge.
>>
>> The feature differs a lot from other rte_flow features since it adds Rx meta
>> information.
>> And yes, rte_flow rule to set mark should fail with appropriate diagnostics if
>> the offload is supported by not enabled or not supported at all. So,
>> application will be informed and I think it is less worse than RSS hash.
>>
>>> My suggestion here, for PMD that wants to optimize their datapath is to
>> check if flow w/ mark action was inserted on the queue. So long there is no
>> such flow they can disable the set of the mark.
>>
>> Unfortunately the information is required on Rx queue setup stage and
>> rte_rule insertion is too late.
> See my comments on the RSS patch. Same point of discussion.
>
> The main question we need to answer -
> User set flow mark action by rte_flow (that was accepted). Why does it need to set more flag?

I think all arguments are on the table.
Mine:
  - possibility to squeeze more performance from HW and SW
  - consistency (at least as I see it) in filling in of the information 
in mbuf
    (direct using corresponding offloads)
Your:
  - more complexity in control (which is less painful in the case of 
flow mark,
    since it should be an error on flow rule setup, and a bit more 
painful in
    in the case of RSS hash since there is simply no RSS hash if not 
enabled)
  - API breakage (but there is deprecation notice in v19.08)

Many thanks for the discussion.

>> Anyway, the important point here is all Rx offloads consistency:
>> want something - enable it. The only remaining exception is packet type
>> which default behaviour is preserved since really flexible solution suggested
>> by Konstantin.
>>
>>>> Also I think that flow API action MARK documentation should be
>>>> updated to mentioned the offload.
>>>>
>>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>> ---
>>>>>     doc/guides/nics/features.rst   | 12 ++++++++++++
>>>>>     lib/librte_ethdev/rte_ethdev.h |  1 +
>>>>>     2 files changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/doc/guides/nics/features.rst
>>>>> b/doc/guides/nics/features.rst index f79b69b38..d67430d90 100644
>>>>> --- a/doc/guides/nics/features.rst
>>>>> +++ b/doc/guides/nics/features.rst
>>>>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
>>>>>     * **[provides]   mbuf**: ``mbuf.packet_type``.
>>>>>
>>>>>
>>>>> +.. _nic_features_flow_action_type_update:
>>>> May be  _nic_features_flow_mark ?
>>>>
>>>>> +
>>>>> +Flow type update
>>>> May be "Flow mark delivery" ?
>>>>
>>>>> +----------------
>>>>> +
>>>>> +Supports flow action type update to ``mbuf.ol_flags`` and
>>>> ``mbuf.hash.fdir.hi``.
>>>>> +
>>>>> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
>>>> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
>>>>
>>>> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.
>>>>
>>>>
>>>>
>>>>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
>>>>> +``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
>>>>> +  ``mbuf.hash.fdir.hi``
>>>>> +
>>>>> +
>>>>>     .. _nic_features_timesync:
>>>>>
>>>>>     Timesync
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>>> b/lib/librte_ethdev/rte_ethdev.h index 889486a11..4a0cff830 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
>>>>>     #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>>>>>     #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
>>>>>     #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
>>>>> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
>>>>>
>>>>>     #define DEV_RX_OFFLOAD_CHECKSUM
>>>> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>>>>>     				 DEV_RX_OFFLOAD_UDP_CKSUM | \
>>>> Add to rte_rx_offload_names
Andrew Rybchenko Aug. 18, 2019, 9:46 a.m. UTC | #7 
On 8/17/19 5:23 PM, Pavan Nikhilesh Bhagavatula wrote:
>>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
>> Notes similar to RSS hash.
>>
>> It requires better motivation why. It lets Rx queue know that
>> it will be used as flow action MARK target and the queue should
>> be configured to deliver the mark from NIC to PMD and processed
>> in the driver.
>>
>> Also I think that flow API action MARK documentation should be
>> updated to mentioned the offload.
> Ack will reword the commit log and update the documentation.
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> ---
>>>    doc/guides/nics/features.rst   | 12 ++++++++++++
>>>    lib/librte_ethdev/rte_ethdev.h |  1 +
>>>    2 files changed, 13 insertions(+)
>>>
>>> diff --git a/doc/guides/nics/features.rst
>> b/doc/guides/nics/features.rst
>>> index f79b69b38..d67430d90 100644
>>> --- a/doc/guides/nics/features.rst
>>> +++ b/doc/guides/nics/features.rst
>>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
>>>    * **[provides]   mbuf**: ``mbuf.packet_type``.
>>>
>>>
>>> +.. _nic_features_flow_action_type_update:
>> May be  _nic_features_flow_mark ?
>>
>>> +
>>> +Flow type update
>> May be "Flow mark delivery" ?
> I named it flow action type update because we use the same flag to represent two different
> flow types i.e. RTE_FLOW_ACTION_TYPE_FLAG and RTE_FLOW_ACTION_TYPE_MARK.
>
> I'm not so sure about the name RX_OFFLOAD_FLOW_MARK too because mbuf might have ol_flags
> marked for ACTION_TYPE_FLAG. Let me know your thoughts on this.

I forgot about RTE_FLOW_ACTION_TYPE_FLAG and really care about mark only
since it is 32-bit vs one. Anyway may be it is more correct to care 
about both.
If so,  may be _nic_features_flow_flag_mark and "Flow flag/mark update"?

> I will modify the name once we come to an agreement.
>
>>> +----------------
>>> +
>>> +Supports flow action type update to ``mbuf.ol_flags`` and
>> ``mbuf.hash.fdir.hi``.
>>> +
>>> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
>> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
>>
>> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.
> Ack, will fix in v2.
>
>>
>>
>>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
>> ``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
>>> +  ``mbuf.hash.fdir.hi``
>>> +
>>> +
>>>    .. _nic_features_timesync:
>>>
>>>    Timesync
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>> b/lib/librte_ethdev/rte_ethdev.h
>>> index 889486a11..4a0cff830 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
>>>    #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
>>>    #define DEV_RX_OFFLOAD_OUTER_Udrivers/net/sfc/base/rhead_ev.c.origDP_CKSUM  0x00040000
>>>    #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
>>> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
>>>
>>>    #define DEV_RX_OFFLOAD_CHECKSUM
>> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>>>    				 DEV_RX_OFFLOAD_UDP_CKSUM | \
>> Add to rte_rx_offload_names
diff mbox series

Patch

diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index f79b69b38..d67430d90 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -594,6 +594,18 @@  application to set ptypes it is interested in.
 * **[provides]   mbuf**: ``mbuf.packet_type``.
 
 
+.. _nic_features_flow_action_type_update:
+
+Flow type update
+----------------
+
+Supports flow action type update to ``mbuf.ol_flags`` and ``mbuf.hash.fdir.hi``.
+
+* **[uses]     rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
+* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``, ``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
+  ``mbuf.hash.fdir.hi``
+
+
 .. _nic_features_timesync:
 
 Timesync
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 889486a11..4a0cff830 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1014,6 +1014,7 @@  struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
 #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
 #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
+#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \