-----Original Message-----
From: Andrew Rybchenko <redacted>
Sent: Tuesday, July 13, 2021 6:24 PM
To: Zhang, AlvinX <redacted>; Zhang, Qi Z
[off-list ref]; ajit.khaparde@broadcom.com
Cc: dev@dpdk.org; Singh, Aman Deep <redacted>
Subject: Re: [PATCH v4] ethdev: add IPv4 and L4 checksum RSS offload types

On 7/13/21 12:38 PM, Zhang, AlvinX wrote:

quoted

quoted

-----Original Message-----
From: Andrew Rybchenko <redacted>
Sent: Tuesday, July 13, 2021 3:55 PM
To: Zhang, AlvinX <redacted>; Zhang, Qi Z
[off-list ref]; ajit.khaparde@broadcom.com
Cc: dev@dpdk.org; Singh, Aman Deep <redacted>
Subject: Re: [PATCH v4] ethdev: add IPv4 and L4 checksum RSS offload
types

On 7/13/21 4:13 AM, Alvin Zhang wrote:

quoted

This patch defines new RSS offload types for IPv4 and
L4(TCP/UDP/SCTP) checksum, which are required when users want to
distribute packets based on the IPv4 or L4 checksum field.

For example "flow create 0 ingress pattern eth / ipv4 / end actions
rss types ipv4-chksum end queues end / end", this flow causes all
matching packets to be distributed to queues on basis of IPv4
checksum.

Signed-off-by: Alvin Zhang <redacted>
Reviewed-by: Andrew Rybchenko <redacted>

I've failed to find mail where I've added the tag. I've replied with
"LGTM" for v2 which has IPv4 checksum only and many comments on L4

checksum added in v3.

quoted

quoted

So, I think it is incorrect to inherit "LGTM" as Reviewed-by.

I'll correct it at next version.

quoted

quoted

Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Acked-by: Aman Deep Singh <redacted>
---

v3: Add L4 checksum RSS offload type
v4: Add doc and help string, update commit log
---
 app/test-pmd/cmdline.c  | 12 +++++++++---
 app/test-pmd/config.c   |  2 ++
 lib/ethdev/rte_ethdev.h |  2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index

0268b18..93543d8 100644

--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c

@@ -2254,6 +2254,10 @@ struct cmd_config_rss {
 		rss_conf.rss_hf = ETH_RSS_ECPRI;
 	else if (!strcmp(res->value, "mpls"))
 		rss_conf.rss_hf = ETH_RSS_MPLS;
+	else if (!strcmp(res->value, "ipv4-chksum"))
+		rss_conf.rss_hf = ETH_RSS_IPV4_CHKSUM;
+	else if (!strcmp(res->value, "l4-chksum"))
+		rss_conf.rss_hf = ETH_RSS_L4_CHKSUM;
 	else if (!strcmp(res->value, "none"))
 		rss_conf.rss_hf = 0;
 	else if (!strcmp(res->value, "level-default")) { @@ -2325,7
+2329,7 @@ struct cmd_config_rss {
 	.help_str = "port config all rss "

	"all|default|eth|vlan|ip|tcp|udp|sctp|ether|port|vxlan|geneve|"

quoted

quoted

quoted

	"nvgre|vxlan-gpe|l2tpv3|esp|ah|pfcp|ecpri|mpls|none|level-default|"

quoted

-		"level-outer|level-inner|<flowtype_id>",
+		"level-outer|level-inner|ipv4-chksum|l4-chksum|<flowtype_id>",
 	.tokens = {
 		(void *)&cmd_config_rss_port,
 		(void *)&cmd_config_rss_keyword,

@@ -2438,7 +2442,8 @@ struct cmd_config_rss_hash_key {
 				 "ipv6-tcp-ex#ipv6-udp-ex#"
 				 "l3-src-only#l3-dst-only#l4-src-only#l4-dst-only#"
 				 "l2-src-only#l2-dst-only#s-vlan#c-vlan#"
-				 "l2tpv3#esp#ah#pfcp#pppoe#gtpu#ecpri#mpls");
+				 "l2tpv3#esp#ah#pfcp#pppoe#gtpu#ecpri#mpls#"
+				 "ipv4-chksum#l4-chksum");
 cmdline_parse_token_string_t cmd_config_rss_hash_key_value =
 	TOKEN_STRING_INITIALIZER(struct cmd_config_rss_hash_key, key,

NULL);

@@ -2451,7 +2456,8 @@ struct cmd_config_rss_hash_key {
 		"l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex|"
 		"l3-src-only|l3-dst-only|l4-src-only|l4-dst-only|"
 		"l2-src-only|l2-dst-only|s-vlan|c-vlan|"
-		"l2tpv3|esp|ah|pfcp|pppoe|gtpu|ecpri|mpls "
+		"l2tpv3|esp|ah|pfcp|pppoe|gtpu|ecpri|mpls|"
+		"ipv4-chksum|l4-chksum "
 		"<string of hex digits (variable length, NIC dependent)>",
 	.tokens = {
 		(void *)&cmd_config_rss_hash_key_port, diff --git

a/app/test-pmd/config.c b/app/test-pmd/config.c index
43c79b5..14968bf
100644

--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c

@@ -140,6 +140,8 @@
 	{ "gtpu", ETH_RSS_GTPU },
 	{ "ecpri", ETH_RSS_ECPRI },
 	{ "mpls", ETH_RSS_MPLS },
+	{ "ipv4-chksum", ETH_RSS_IPV4_CHKSUM },
+	{ "l4-chksum", ETH_RSS_L4_CHKSUM },
 	{ NULL, 0 },
 };

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index

faf3bd9..63b0321 100644

--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h

@@ -537,6 +537,8 @@ struct rte_eth_rss_conf {
 #define ETH_RSS_PPPOE		   (1ULL << 31)
 #define ETH_RSS_ECPRI		   (1ULL << 32)
 #define ETH_RSS_MPLS		   (1ULL << 33)
+#define ETH_RSS_IPV4_CHKSUM	   (1ULL << 34)
+#define ETH_RSS_L4_CHKSUM	   (1ULL << 35)	/* TCP/UDP/SCTP */

It does not reply on my questions at all.

Above list of Layer 4 protocols does not say if the flag may be reported when all
above protocols are supported or some protocols are supported only.

Also I had a question about UDP checksum 0 since it could be treated in a
different ways since logically it is no checksum at all.

I'll add comment for UDP checksum in next patch.   
Thanks.   

quoted

You have said:

quoted

quoted

quoted

I don't think so. IMHO, it should report all RSS capabilities
regardless generic vs flow API RSS action.

May be it is OK to report subset in
dev_info->flow_type_rss_offloads, but I'm very uncomfortable with the
approach. Superset sounds more logical to me, but has drawbacks as
well.

Here I have another question:
There are flow type definition and RSS offload type definition,
#define RTE_ETH_FLOW_RAW                 1
#define RTE_ETH_FLOW_IPV4                2
#define RTE_ETH_FLOW_FRAG_IPV4           3
#define RTE_ETH_FLOW_NONFRAG_IPV4_TCP    4

#define ETH_RSS_IPV4               (1ULL << 2)
#define ETH_RSS_FRAG_IPV4          (1ULL << 3)
#define ETH_RSS_NONFRAG_IPV4_TCP   (1ULL << 4)

are they the different expressions of the same concept?

Sorry, but I don't understand the question.

quoted

If yes, why they have been decoupled by the commit: fce6b66893.

Do you mean that motivation in the changeset description is not clear?

quoted

Then what the flow type of ETH_RSS_PORT, it's UDP, TCP, or SCTP?

I have no answer to the question as well. That's why I'm trying to avoid it in the
patch.

IMHO, the flow type is different from RSS Offload type.  
That's why they have been decoupled in commit fce6b66893   
    ethdev: decouple flow types and RSS offload types   

    This patch decouples RTE_ETH_FLOW_* and ETH_RSS_*. The former defines   
    flow types and the latter defines RSS offload types.   

There are total 25 flow types, but 30+ RSS offload types.  
The PMDs report flow type in dev_info->flow_type_rss_offloads, but they don't report RSS offload type.   

quoted

If not, the PMDs can report supported RSS flow type by

dev_info->flow_type_rss_offloads,

quoted

but have no way to report supported RSS offload types.

Sorry, don't understand.

quoted

quoted

Nack