Re: [dpdk-dev] [PATCH v7] ethdev: add IPv4 and L4 checksum RSS offload types

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/31/2021 10:52 AM, Alvin Zhang wrote:
> 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 <alvinx.zhang@intel.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Aman Deep Singh <aman.deep.singh@intel.com>
> ---
> 
> v6: rebase to eeedef70, update some note
> v7: fix code style issues
> ---
>  app/test-pmd/cmdline.c                 |  4 +++-
>  app/test-pmd/config.c                  |  2 ++
>  doc/guides/rel_notes/release_21_11.rst |  5 +++++
>  lib/ethdev/rte_ethdev.h                | 24 ++++++++++++++++++++++++
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 82253bc..656a311 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -2252,6 +2252,8 @@ 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, "none"))
>  		rss_conf.rss_hf = 0;
>  	else if (!strcmp(res->value, "level-default")) {
> @@ -2323,7 +2325,7 @@ struct cmd_config_rss {
>  	.help_str = "port config all rss "
>  		"all|default|eth|vlan|ip|tcp|udp|sctp|ether|port|vxlan|geneve|"
>  		"nvgre|vxlan-gpe|l2tpv3|esp|ah|pfcp|ecpri|mpls|none|level-default|"
> -		"level-outer|level-inner|<flowtype_id>",
> +		"level-outer|level-inner|ipv4-chksum|<flowtype_id>",
>  	.tokens = {
>  		(void *)&cmd_config_rss_port,
>  		(void *)&cmd_config_rss_keyword,
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 31d8ba1..ece78f2 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/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index d707a55..fa29b13 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -55,6 +55,11 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +* **Add new RSS offload types for IPv4/L4 checksum in RSS flow.**
> +
> +  Add macros ETH_RSS_IPV4_CHKSUM and ETH_RSS_L4_CHKSUM, now IPv4 and
> +  TCP/UDP/SCTP header checksum field can be used as input set for RSS.
> +
>  
>  Removed Items
>  -------------
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index d2b27c3..e6734df 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -537,6 +537,30 @@ 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)
> +
> +/**
> + * The ETH_RSS_L4_CHKSUM generally refers to a type of checksum field for

what does 'generally' means here? Is there a case it refers to something else?

> + * any L4 header, such as TCP, UDP and SCTP. It is similar to ETH_RSS_PORT,
> + * it does not specify the type of L4 header.
> + * We use this macro to replace below macro for constricting the use of RSS
> + * offload bits:
> + * ETH_RSS_IPV4_TCP_CHKSUM
> + * ETH_RSS_IPV4_UDP_CHKSUM
> + * ETH_RSS_IPV4_SCTP_CHKSUM
> + * ETH_RSS_IPV6_TCP_CHKSUM
> + * ETH_RSS_IPV6_UDP_CHKSUM
> + * ETH_RSS_IPV6_SCTP_CHKSUM

As I get you are listing them here to say the 'ETH_RSS_L4_CHKSUM' replaces
possible usage of above list, but my concern is it may confuse people as those
macros exists (or did exist in the past), so what do you think to remove them?


And just to confirm, we can't use this flag, 'ETH_RSS_L4_CHKSUM' anymore with
'rte_eth_rss_conf.rss_hf', right? Since it will be missing some context for it.
Which means some old APIs (and configuration) won't support this new offload,
but only rte_flow will support it.
If above is correct should it be highlighted in above comment?

> + *
> + * Then how to use this macro? We can use it in RSS flow where the pattern

Can we convert this question to a description just to be a little more formal?

> + * type will specify the L4 header type, for example "flow create 0 ingress \
> + * pattern eth / ipv4 / tcp / end actions rss types l4-chksum  end queues end \
> + * / end"
> + *
> + * For the case that checksum is not used in a UDP header, it takes the
> + * reserved value 0 as input for the hash function.
> + */
> +#define ETH_RSS_L4_CHKSUM	   (1ULL << 35)
>  
>  /*
>   * We use the following macros to combine with above ETH_RSS_* for
>