diff mbox series

[v2,2/7] ethdev: add mbuf RSS update as an offload

Message ID 20190821204755.1990-3-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 fail Compilation issues

Commit Message

Pavan Nikhilesh Bhagavatula Aug. 21, 2019, 8:47 p.m. UTC 
  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`.
PMDs notify the validity of `rte_mbuf::hash:rss` to the applcation
by enabling `PKT_RX_RSS_HASH ` flag in `rte_mbuf::ol_flags`.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 doc/guides/nics/features.rst   | 2 ++
 lib/librte_ethdev/rte_ethdev.c | 1 +
 lib/librte_ethdev/rte_ethdev.h | 1 +
 3 files changed, 4 insertions(+)

Comments

Stephen Hemminger Aug. 23, 2019, 1:19 a.m. UTC | #1 
On Thu, 22 Aug 2019 02:17:50 +0530
<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`.
> PMDs notify the validity of `rte_mbuf::hash:rss` to the applcation
> by enabling `PKT_RX_RSS_HASH ` flag in `rte_mbuf::ol_flags`.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Is this really a good idea?
Every bit of hardware that works on Windows with RSS is required to
supply the hash (for software steering).  So if adding an additional
capability, just adds another bit of complexity, code coverage, and one
more thing that won't be tested by everyone.

If hardware has it why not set it? Adding a branch is more expensive than
an unused assignment.
Andrew Rybchenko Aug. 23, 2019, 9:49 a.m. UTC | #2 
On 8/21/19 11:47 PM, 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`.
> PMDs notify the validity of `rte_mbuf::hash:rss` to the applcation
> by enabling `PKT_RX_RSS_HASH ` flag in `rte_mbuf::ol_flags`.
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

LGTM, but release notes are missing.
Andrew Rybchenko Aug. 27, 2019, 1:44 p.m. UTC | #3 
On 8/23/19 4:19 AM, Stephen Hemminger wrote:
> On Thu, 22 Aug 2019 02:17:50 +0530
> <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`.
>> PMDs notify the validity of `rte_mbuf::hash:rss` to the applcation
>> by enabling `PKT_RX_RSS_HASH ` flag in `rte_mbuf::ol_flags`.
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Is this really a good idea?
> Every bit of hardware that works on Windows with RSS is required to
> supply the hash (for software steering).  So if adding an additional
> capability, just adds another bit of complexity, code coverage, and one
> more thing that won't be tested by everyone.
>
> If hardware has it why not set it? Adding a branch is more expensive than
> an unused assignment.

If a PMD sees no gain in disabling the offload and addition branches,
nothing obligates PMD to do. Just keep as is - no problem.
However, it opens the door to skip delivery of the RSS hash
from NIC HW to host CPU if the hash is not required.
Why should it be delivered to CPU and eat PCIe bandwidth if
the information is not required?
diff mbox series

Patch

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.c b/lib/librte_ethdev/rte_ethdev.c
index f529cbe9f..9c5517d5f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -129,6 +129,7 @@  static const struct {
 	RTE_RX_OFFLOAD_BIT2STR(KEEP_CRC),
 	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
 	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
+	RTE_RX_OFFLOAD_BIT2STR(RSS_HASH),
 };
 
 #undef RTE_RX_OFFLOAD_BIT2STR
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 1ab0af4d8..836b30074 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
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \