[2/7] ethdev: add mbuf RSS update as a offload
Checks
Commit Message
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Add new Rx offload flag `DEV_RX_OFFLOAD_RSS_HASH` which can be used to
enable/disable PMDs write to `rte_mbuf::hash::rss`.
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
doc/guides/nics/features.rst | 2 ++
lib/librte_ethdev/rte_ethdev.h | 1 +
2 files changed, 3 insertions(+)
Comments
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_RSS_HASH` which can be used to
> enable/disable PMDs write to `rte_mbuf::hash::rss`.
It should be highlighted that presence of the RSS hash is indicated
by PKT_RX_RSS_HASH flag in mbuf anyway. Now applications have
a way to check that RSS hash delivery is supported and should
enable the offload if RSS hash is used. PMD may still provide the
hash even if the offload is not enabled.
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
with above and one note below fixed.
> ---
> doc/guides/nics/features.rst | 2 ++
> lib/librte_ethdev/rte_ethdev.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index d4d55f721..f79b69b38 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -274,6 +274,7 @@ Supports RSS hashing on RX.
>
> * **[uses] user config**: ``dev_conf.rxmode.mq_mode`` = ``ETH_MQ_RX_RSS_FLAG``.
> * **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> * **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
>
> @@ -286,6 +287,7 @@ Inner RSS
> Supports RX RSS hashing on Inner headers.
>
> * **[uses] rte_flow_action_rss**: ``level``.
> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
>
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index f97f0a6e5..889486a11 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1013,6 +1013,7 @@ struct rte_eth_conf {
> #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
> #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000
> #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000
> +#define DEV_RX_OFFLOAD_RSS_HASH 0x00080000
Should be added to rte_rx_offload_names in lib/librte_ethdev/rte_ethdev.c.
>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>Sent: Friday, August 16, 2019 1:18 PM
>To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin
>Jacob Kollanukkaran <jerinj@marvell.com>; ferruh.yigit@intel.com;
>John McNamara <john.mcnamara@intel.com>; Marko Kovacevic
><marko.kovacevic@intel.com>; Thomas Monjalon
><thomas@monjalon.net>
>Cc: dev@dpdk.org
>Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
>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_RSS_HASH` which can be
>used to
>> enable/disable PMDs write to `rte_mbuf::hash::rss`.
>
>It should be highlighted that presence of the RSS hash is indicated
>by PKT_RX_RSS_HASH flag in mbuf anyway. Now applications have
>a way to check that RSS hash delivery is supported and should
>enable the offload if RSS hash is used. PMD may still provide the
>hash even if the offload is not enabled.
>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
>Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>
>with above and one note below fixed.
>
>> ---
>> doc/guides/nics/features.rst | 2 ++
>> lib/librte_ethdev/rte_ethdev.h | 1 +
>> 2 files changed, 3 insertions(+)
>>
>> diff --git a/doc/guides/nics/features.rst
>b/doc/guides/nics/features.rst
>> index d4d55f721..f79b69b38 100644
>> --- a/doc/guides/nics/features.rst
>> +++ b/doc/guides/nics/features.rst
>> @@ -274,6 +274,7 @@ Supports RSS hashing on RX.
>>
>> * **[uses] user config**: ``dev_conf.rxmode.mq_mode`` =
>``ETH_MQ_RX_RSS_FLAG``.
>> * **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
>``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
>> * **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
>``mbuf.rss``.
>>
>> @@ -286,6 +287,7 @@ Inner RSS
>> Supports RX RSS hashing on Inner headers.
>>
>> * **[uses] rte_flow_action_rss**: ``level``.
>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
>``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
>``mbuf.rss``.
>>
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>b/lib/librte_ethdev/rte_ethdev.h
>> index f97f0a6e5..889486a11 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1013,6 +1013,7 @@ struct rte_eth_conf {
>> #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
>> #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000
>> #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000
>> +#define DEV_RX_OFFLOAD_RSS_HASH 0x00080000
>
>Should be added to rte_rx_offload_names in
>lib/librte_ethdev/rte_ethdev.c.
Thanks for the heads up Andrew, will fix in v2.
Pavan
Friday, August 16, 2019 10:48 AM, Andrew Rybchenko:
> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
> 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_RSS_HASH` which can be used
> to
> > enable/disable PMDs write to `rte_mbuf::hash::rss`.
>
> It should be highlighted that presence of the RSS hash is indicated by
> PKT_RX_RSS_HASH flag in mbuf anyway. Now applications have a way to
> check that RSS hash delivery is supported and should enable the offload if
> RSS hash is used. PMD may still provide the hash even if the offload is not
> enabled.
I don't understand how PMDs should act w/ this addition when considering the API breakage to application.
Currently application don't set this flag, and expect to get the RSS hash result on mbuf.
If PMDs will not set the RSS hash result when flag is not present then applications might break.
If they will always set, then there is no meaning for it.
as I understand the motivation to save few cycles on the PMD receive path, if we want to include it we should treat it as API breakage and documents it on the release notes.
My option is that some offload should just be usable (OOB) by the fact user enabled them (e.g. RSS). no need to complicate the user by checking and set this field.
>
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>
> with above and one note below fixed.
>
> > ---
> > doc/guides/nics/features.rst | 2 ++
> > lib/librte_ethdev/rte_ethdev.h | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/doc/guides/nics/features.rst
> > b/doc/guides/nics/features.rst index d4d55f721..f79b69b38 100644
> > --- a/doc/guides/nics/features.rst
> > +++ b/doc/guides/nics/features.rst
> > @@ -274,6 +274,7 @@ Supports RSS hashing on RX.
> >
> > * **[uses] user config**: ``dev_conf.rxmode.mq_mode`` =
> ``ETH_MQ_RX_RSS_FLAG``.
> > * **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
> > +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> > * **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
> > * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
> >
> > @@ -286,6 +287,7 @@ Inner RSS
> > Supports RX RSS hashing on Inner headers.
> >
> > * **[uses] rte_flow_action_rss**: ``level``.
> > +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> > * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
> >
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.h
> > b/lib/librte_ethdev/rte_ethdev.h index f97f0a6e5..889486a11 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1013,6 +1013,7 @@ struct rte_eth_conf {
> > #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
> > #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000
> > #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000
> > +#define DEV_RX_OFFLOAD_RSS_HASH 0x00080000
>
> Should be added to rte_rx_offload_names in
> lib/librte_ethdev/rte_ethdev.c.
On 8/18/19 7:52 AM, Shahaf Shuler wrote:
> Friday, August 16, 2019 10:48 AM, Andrew Rybchenko:
>> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
>> 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_RSS_HASH` which can be used
>> to
>>> enable/disable PMDs write to `rte_mbuf::hash::rss`.
>> It should be highlighted that presence of the RSS hash is indicated by
>> PKT_RX_RSS_HASH flag in mbuf anyway. Now applications have a way to
>> check that RSS hash delivery is supported and should enable the offload if
>> RSS hash is used. PMD may still provide the hash even if the offload is not
>> enabled.
> I don't understand how PMDs should act w/ this addition when considering the API breakage to application.
There is a deprecation notice for it.
I mentioned in my review notes for one of patches in the series
that the change should be highlighted in release notes.
Yes, it is absolutely required if these patches are accepted.
> Currently application don't set this flag, and expect to get the RSS hash result on mbuf.
> If PMDs will not set the RSS hash result when flag is not present then applications might break.
> If they will always set, then there is no meaning for it.
>
> as I understand the motivation to save few cycles on the PMD receive path, if we want to include it we should treat it as API breakage and documents it on the release notes.
> My option is that some offload should just be usable (OOB) by the fact user enabled them (e.g. RSS). no need to complicate the user by checking and set this field.
What I don't understand is why some offloads should just work but
another requires action to enable it. Just because it is the current
state of things - I don't think it is a good motivation. Sorry.
I think more applications use checksum offloads than RSS hash,
but it is still required to enable it. It looks like no single DPDK example
uses RSS hash. So, I guess it not widely used by applications as well.
Anyway these 2 patches for flow action and RSS hash make all Rx
offloads consistent - if you need something, enable it.
And the question is not to save few cycles in the PMD receive path.
It makes is possible to not deliver both from NIC to host.
8 bytes (4 RSS hash and 4 flow mark) are more than 10% for the
smallest packets.
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> with above and one note below fixed.
>>
>>> ---
>>> doc/guides/nics/features.rst | 2 ++
>>> lib/librte_ethdev/rte_ethdev.h | 1 +
>>> 2 files changed, 3 insertions(+)
>>>
>>> diff --git a/doc/guides/nics/features.rst
>>> b/doc/guides/nics/features.rst index d4d55f721..f79b69b38 100644
>>> --- a/doc/guides/nics/features.rst
>>> +++ b/doc/guides/nics/features.rst
>>> @@ -274,6 +274,7 @@ Supports RSS hashing on RX.
>>>
>>> * **[uses] user config**: ``dev_conf.rxmode.mq_mode`` =
>> ``ETH_MQ_RX_RSS_FLAG``.
>>> * **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
>>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
>> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
>>> * **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
>>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
>>>
>>> @@ -286,6 +287,7 @@ Inner RSS
>>> Supports RX RSS hashing on Inner headers.
>>>
>>> * **[uses] rte_flow_action_rss**: ``level``.
>>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
>> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
>>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
>>>
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>> b/lib/librte_ethdev/rte_ethdev.h index f97f0a6e5..889486a11 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1013,6 +1013,7 @@ struct rte_eth_conf {
>>> #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
>>> #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000
>>> #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000
>>> +#define DEV_RX_OFFLOAD_RSS_HASH 0x00080000
>> Should be added to rte_rx_offload_names in
>> lib/librte_ethdev/rte_ethdev.c.
Sunday, August 18, 2019 8:39 AM, Andrew Rybchenko:
> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
> offload
>
> On 8/18/19 7:52 AM, Shahaf Shuler wrote:
> > Friday, August 16, 2019 10:48 AM, Andrew Rybchenko:
> >> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
> >> 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_RSS_HASH` which can be
> used
> >> to
> >>> enable/disable PMDs write to `rte_mbuf::hash::rss`.
> >> It should be highlighted that presence of the RSS hash is indicated
> >> by PKT_RX_RSS_HASH flag in mbuf anyway. Now applications have a way
> >> to check that RSS hash delivery is supported and should enable the
> >> offload if RSS hash is used. PMD may still provide the hash even if
> >> the offload is not enabled.
> > I don't understand how PMDs should act w/ this addition when considering
> the API breakage to application.
>
> There is a deprecation notice for it.
> I mentioned in my review notes for one of patches in the series that the
> change should be highlighted in release notes.
> Yes, it is absolutely required if these patches are accepted.
>
> > Currently application don't set this flag, and expect to get the RSS hash
> result on mbuf.
> > If PMDs will not set the RSS hash result when flag is not present then
> applications might break.
> > If they will always set, then there is no meaning for it.
> >
> > as I understand the motivation to save few cycles on the PMD receive path,
> if we want to include it we should treat it as API breakage and documents it
> on the release notes.
> > My option is that some offload should just be usable (OOB) by the fact user
> enabled them (e.g. RSS). no need to complicate the user by checking and set
> this field.
>
> What I don't understand is why some offloads should just work but another
> requires action to enable it. Just because it is the current state of things - I
> don't think it is a good motivation. Sorry.
Not because it is the current state of things, because it makes user experience much simpler.
You enabled RSS -> you get full RSS behavior
You set a flow rule w/ mark -> you get full flow mark behavior
You set checksum -> you get full csum behavior.
> I think more applications use checksum offloads than RSS hash, but it is still
> required to enable it. It looks like no single DPDK example uses RSS hash. So,
> I guess it not widely used by applications as well.
Well there is at least one called ovs-dpdk, that use the RSS result as the key to access the EMC.
I know of few more, not upstream, ones.
> Anyway these 2 patches for flow action and RSS hash make all Rx offloads
> consistent - if you need something, enable it.
But the user enabled it -
It enabled RSS by setting ETH_MQ_RX_RSS, why does it need to enable another flag?
Same for flow mark.
>
> And the question is not to save few cycles in the PMD receive path.
> It makes is possible to not deliver both from NIC to host.
> 8 bytes (4 RSS hash and 4 flow mark) are more than 10% for the smallest
> packets.
There is always the line between how much tight control we want to provide to user (to save cycles/ to save PCI BW) and how much it will be simple for the user to work on top.
My opinion is that we need to have some basics.
>
> >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>
> >> with above and one note below fixed.
> >>
> >>> ---
> >>> doc/guides/nics/features.rst | 2 ++
> >>> lib/librte_ethdev/rte_ethdev.h | 1 +
> >>> 2 files changed, 3 insertions(+)
> >>>
> >>> diff --git a/doc/guides/nics/features.rst
> >>> b/doc/guides/nics/features.rst index d4d55f721..f79b69b38 100644
> >>> --- a/doc/guides/nics/features.rst
> >>> +++ b/doc/guides/nics/features.rst
> >>> @@ -274,6 +274,7 @@ Supports RSS hashing on RX.
> >>>
> >>> * **[uses] user config**: ``dev_conf.rxmode.mq_mode`` =
> >> ``ETH_MQ_RX_RSS_FLAG``.
> >>> * **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
> >>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> >> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> >>> * **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
> >>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
> ``mbuf.rss``.
> >>>
> >>> @@ -286,6 +287,7 @@ Inner RSS
> >>> Supports RX RSS hashing on Inner headers.
> >>>
> >>> * **[uses] rte_flow_action_rss**: ``level``.
> >>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> >> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> >>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
> ``mbuf.rss``.
> >>>
> >>>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>> b/lib/librte_ethdev/rte_ethdev.h index f97f0a6e5..889486a11 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -1013,6 +1013,7 @@ struct rte_eth_conf {
> >>> #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
> >>> #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000
> >>> #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000
> >>> +#define DEV_RX_OFFLOAD_RSS_HASH 0x00080000
> >> Should be added to rte_rx_offload_names in
> >> lib/librte_ethdev/rte_ethdev.c.
On 8/18/19 9:18 AM, Shahaf Shuler wrote:
> Sunday, August 18, 2019 8:39 AM, Andrew Rybchenko:
>> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
>> offload
>>
>> On 8/18/19 7:52 AM, Shahaf Shuler wrote:
>>> Friday, August 16, 2019 10:48 AM, Andrew Rybchenko:
>>>> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
>>>> 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_RSS_HASH` which can be
>> used
>>>> to
>>>>> enable/disable PMDs write to `rte_mbuf::hash::rss`.
>>>> It should be highlighted that presence of the RSS hash is indicated
>>>> by PKT_RX_RSS_HASH flag in mbuf anyway. Now applications have a way
>>>> to check that RSS hash delivery is supported and should enable the
>>>> offload if RSS hash is used. PMD may still provide the hash even if
>>>> the offload is not enabled.
>>> I don't understand how PMDs should act w/ this addition when considering
>> the API breakage to application.
>>
>> There is a deprecation notice for it.
>> I mentioned in my review notes for one of patches in the series that the
>> change should be highlighted in release notes.
>> Yes, it is absolutely required if these patches are accepted.
>>
>>> Currently application don't set this flag, and expect to get the RSS hash
>> result on mbuf.
>>> If PMDs will not set the RSS hash result when flag is not present then
>> applications might break.
>>> If they will always set, then there is no meaning for it.
>>>
>>> as I understand the motivation to save few cycles on the PMD receive path,
>> if we want to include it we should treat it as API breakage and documents it
>> on the release notes.
>>> My option is that some offload should just be usable (OOB) by the fact user
>> enabled them (e.g. RSS). no need to complicate the user by checking and set
>> this field.
>>
>> What I don't understand is why some offloads should just work but another
>> requires action to enable it. Just because it is the current state of things - I
>> don't think it is a good motivation. Sorry.
> Not because it is the current state of things, because it makes user experience much simpler.
If so, it would be simpler to have no controls at all and always have
everything possible.
> You enabled RSS -> you get full RSS behavior
You enable distribution across many queues here.
RSS hash availability is a side effect here.
> You set a flow rule w/ mark -> you get full flow mark behavior
> You set checksum -> you get full csum behavior.
>
>> I think more applications use checksum offloads than RSS hash, but it is still
>> required to enable it. It looks like no single DPDK example uses RSS hash. So,
>> I guess it not widely used by applications as well.
> Well there is at least one called ovs-dpdk, that use the RSS result as the key to access the EMC.
> I know of few more, not upstream, ones.
>
>> Anyway these 2 patches for flow action and RSS hash make all Rx offloads
>> consistent - if you need something, enable it.
> But the user enabled it -
> It enabled RSS by setting ETH_MQ_RX_RSS, why does it need to enable another flag?
Answered above. If you need distribution it does not mean
that you need RSS hash information. There are really many
examples when you don't really need it.
> Same for flow mark.
>
>> And the question is not to save few cycles in the PMD receive path.
>> It makes is possible to not deliver both from NIC to host.
>> 8 bytes (4 RSS hash and 4 flow mark) are more than 10% for the smallest
>> packets.
> There is always the line between how much tight control we want to provide to user (to save cycles/ to save PCI BW) and how much it will be simple for the user to work on top.
> My opinion is that we need to have some basics.
Many thanks, your arguments make sense. I vote for
consistency and more fine grained control which allows
more optimizations and allow to squeeze more performance
from HW and SW. So, my line is a bit lower. I don't think
that these two patches make user control over-complicated.
>>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>
>>>> with above and one note below fixed.
>>>>
>>>>> ---
>>>>> doc/guides/nics/features.rst | 2 ++
>>>>> lib/librte_ethdev/rte_ethdev.h | 1 +
>>>>> 2 files changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/doc/guides/nics/features.rst
>>>>> b/doc/guides/nics/features.rst index d4d55f721..f79b69b38 100644
>>>>> --- a/doc/guides/nics/features.rst
>>>>> +++ b/doc/guides/nics/features.rst
>>>>> @@ -274,6 +274,7 @@ Supports RSS hashing on RX.
>>>>>
>>>>> * **[uses] user config**: ``dev_conf.rxmode.mq_mode`` =
>>>> ``ETH_MQ_RX_RSS_FLAG``.
>>>>> * **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
>>>>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
>>>> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
>>>>> * **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
>>>>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
>> ``mbuf.rss``.
>>>>> @@ -286,6 +287,7 @@ Inner RSS
>>>>> Supports RX RSS hashing on Inner headers.
>>>>>
>>>>> * **[uses] rte_flow_action_rss**: ``level``.
>>>>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
>>>> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
>>>>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
>> ``mbuf.rss``.
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
>>>>> b/lib/librte_ethdev/rte_ethdev.h index f97f0a6e5..889486a11 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>> @@ -1013,6 +1013,7 @@ struct rte_eth_conf {
>>>>> #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
>>>>> #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000
>>>>> #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000
>>>>> +#define DEV_RX_OFFLOAD_RSS_HASH 0x00080000
>>>> Should be added to rte_rx_offload_names in
>>>> lib/librte_ethdev/rte_ethdev.c.
> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Sunday, August 18, 2019 10:01 AM
> To: Shahaf Shuler <shahafs@mellanox.com>; pbhagavatula@marvell.com;
> jerinj@marvell.com; ferruh.yigit@intel.com; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
> offload
>
> On 8/18/19 9:18 AM, Shahaf Shuler wrote:
> > Sunday, August 18, 2019 8:39 AM, Andrew Rybchenko:
> >> <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a
> >> offload
> >>
> >> On 8/18/19 7:52 AM, Shahaf Shuler wrote:
> >>> Friday, August 16, 2019 10:48 AM, Andrew Rybchenko:
> >>>> Subject: Re: [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as
> >>>> a 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_RSS_HASH` which can be
> >> used
> >>>> to
> >>>>> enable/disable PMDs write to `rte_mbuf::hash::rss`.
> >>>> It should be highlighted that presence of the RSS hash is indicated
> >>>> by PKT_RX_RSS_HASH flag in mbuf anyway. Now applications have a
> way
> >>>> to check that RSS hash delivery is supported and should enable the
> >>>> offload if RSS hash is used. PMD may still provide the hash even if
> >>>> the offload is not enabled.
> >>> I don't understand how PMDs should act w/ this addition when
> >>> considering
> >> the API breakage to application.
> >>
> >> There is a deprecation notice for it.
> >> I mentioned in my review notes for one of patches in the series that
> >> the change should be highlighted in release notes.
> >> Yes, it is absolutely required if these patches are accepted.
> >>
> >>> Currently application don't set this flag, and expect to get the RSS
> >>> hash
> >> result on mbuf.
> >>> If PMDs will not set the RSS hash result when flag is not present
> >>> then
> >> applications might break.
> >>> If they will always set, then there is no meaning for it.
> >>>
> >>> as I understand the motivation to save few cycles on the PMD receive
> >>> path,
> >> if we want to include it we should treat it as API breakage and
> >> documents it on the release notes.
> >>> My option is that some offload should just be usable (OOB) by the
> >>> fact user
> >> enabled them (e.g. RSS). no need to complicate the user by checking
> >> and set this field.
> >>
> >> What I don't understand is why some offloads should just work but
> >> another requires action to enable it. Just because it is the current
> >> state of things - I don't think it is a good motivation. Sorry.
> > Not because it is the current state of things, because it makes user
> experience much simpler.
>
> If so, it would be simpler to have no controls at all and always have
> everything possible.
>
> > You enabled RSS -> you get full RSS behavior
>
> You enable distribution across many queues here.
> RSS hash availability is a side effect here.
I disagree.
There is at least one RSS spec (Microsoft NDIS) that define it part of the RSS action. [1]
Bottom of page " The NIC always passes on the 32-bit hash value."
Am not spec pedantic, just to emphasize that natively drivers expects to have the RSS hash on the packet descriptor.
As for the rest - I think you spread the two different approach correctly.
[1]
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/introduction-to-receive-side-scaling
>
> > You set a flow rule w/ mark -> you get full flow mark behavior You set
> > checksum -> you get full csum behavior.
> >
> >> I think more applications use checksum offloads than RSS hash, but it
> >> is still required to enable it. It looks like no single DPDK example
> >> uses RSS hash. So, I guess it not widely used by applications as well.
> > Well there is at least one called ovs-dpdk, that use the RSS result as the key
> to access the EMC.
> > I know of few more, not upstream, ones.
> >
> >> Anyway these 2 patches for flow action and RSS hash make all Rx
> >> offloads consistent - if you need something, enable it.
> > But the user enabled it -
> > It enabled RSS by setting ETH_MQ_RX_RSS, why does it need to enable
> another flag?
>
> Answered above. If you need distribution it does not mean that you need
> RSS hash information. There are really many examples when you don't really
> need it.
>
> > Same for flow mark.
> >
> >> And the question is not to save few cycles in the PMD receive path.
> >> It makes is possible to not deliver both from NIC to host.
> >> 8 bytes (4 RSS hash and 4 flow mark) are more than 10% for the
> >> smallest packets.
> > There is always the line between how much tight control we want to
> provide to user (to save cycles/ to save PCI BW) and how much it will be
> simple for the user to work on top.
> > My opinion is that we need to have some basics.
>
> Many thanks, your arguments make sense. I vote for consistency and more
> fine grained control which allows more optimizations and allow to squeeze
> more performance from HW and SW. So, my line is a bit lower. I don't think
> that these two patches make user control over-complicated.
>
> >>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>
> >>>> with above and one note below fixed.
> >>>>
> >>>>> ---
> >>>>> doc/guides/nics/features.rst | 2 ++
> >>>>> lib/librte_ethdev/rte_ethdev.h | 1 +
> >>>>> 2 files changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/doc/guides/nics/features.rst
> >>>>> b/doc/guides/nics/features.rst index d4d55f721..f79b69b38 100644
> >>>>> --- a/doc/guides/nics/features.rst
> >>>>> +++ b/doc/guides/nics/features.rst
> >>>>> @@ -274,6 +274,7 @@ Supports RSS hashing on RX.
> >>>>>
> >>>>> * **[uses] user config**: ``dev_conf.rxmode.mq_mode`` =
> >>>> ``ETH_MQ_RX_RSS_FLAG``.
> >>>>> * **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
> >>>>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> >>>> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> >>>>> * **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
> >>>>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
> >> ``mbuf.rss``.
> >>>>> @@ -286,6 +287,7 @@ Inner RSS
> >>>>> Supports RX RSS hashing on Inner headers.
> >>>>>
> >>>>> * **[uses] rte_flow_action_rss**: ``level``.
> >>>>> +* **[uses] rte_eth_rxconf,rte_eth_rxmode**:
> >>>> ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
> >>>>> * **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``,
> >> ``mbuf.rss``.
> >>>>>
> >>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>>>> b/lib/librte_ethdev/rte_ethdev.h index f97f0a6e5..889486a11 100644
> >>>>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>>>> @@ -1013,6 +1013,7 @@ struct rte_eth_conf {
> >>>>> #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
> >>>>> #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000
> >>>>> #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000
> >>>>> +#define DEV_RX_OFFLOAD_RSS_HASH 0x00080000
> >>>> Should be added to rte_rx_offload_names in
> >>>> lib/librte_ethdev/rte_ethdev.c.
@@ -274,6 +274,7 @@ Supports RSS hashing on RX.
* **[uses] user config**: ``dev_conf.rxmode.mq_mode`` = ``ETH_MQ_RX_RSS_FLAG``.
* **[uses] user config**: ``dev_conf.rx_adv_conf.rss_conf``.
+* **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
* **[provides] rte_eth_dev_info**: ``flow_type_rss_offloads``.
* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
@@ -286,6 +287,7 @@ Inner RSS
Supports RX RSS hashing on Inner headers.
* **[uses] rte_flow_action_rss**: ``level``.
+* **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_RSS_HASH``.
* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_RSS_HASH``, ``mbuf.rss``.
@@ -1013,6 +1013,7 @@ struct rte_eth_conf {
#define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000
#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_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
DEV_RX_OFFLOAD_UDP_CKSUM | \