[v3] ethdev: add IPv4 and L4 checksum RSS offload types

  This patch defines new RSS offload types for IPv4 and L4 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>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---

v3: Add L4 checksum RSS offload type
---
 app/test-pmd/cmdline.c  | 4 ++++
 app/test-pmd/config.c   | 2 ++
 lib/ethdev/rte_ethdev.h | 2 ++
 3 files changed, 8 insertions(+)
  

On Tue, Jun 15, 2021 at 1:50 PM Alvin Zhang <alvinx.zhang@intel.com> wrote:
>
> This patch defines new RSS offload types for IPv4 and L4 checksum,
> which are required when users want to distribute packets based on the
> IPv4 or L4 checksum field.

What is the usecase for distribution based on L4/IPv4 checksum?
Is it something like HW has the feature so expose it or there is some
real use case for this application?



> 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>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>
> v3: Add L4 checksum RSS offload type
> ---
>  app/test-pmd/cmdline.c  | 4 ++++
>  app/test-pmd/config.c   | 2 ++
>  lib/ethdev/rte_ethdev.h | 2 ++
>  3 files changed, 8 insertions(+)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0268b18..6148d84 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")) {
> 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..1268729 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)
>
>  /*
>   * We use the following macros to combine with above ETH_RSS_* for
> --
> 1.8.3.1
>
  

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, June 15, 2021 4:26 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
> offload types
> 
> On Tue, Jun 15, 2021 at 1:50 PM Alvin Zhang <alvinx.zhang@intel.com>
> wrote:
> >
> > This patch defines new RSS offload types for IPv4 and L4 checksum,
> > which are required when users want to distribute packets based on the
> > IPv4 or L4 checksum field.
> 
> What is the usecase for distribution based on L4/IPv4 checksum?
> Is it something like HW has the feature so expose it or there is some real use
> case for this application?

This is for real use case, some research by using TCP checksum for FDIR on ixgbe.
https://hsadok.com/papers/sprayer-hotnets18.pdf

and we are looking for similar solution in ice, and checksum RSS is the feature we need to have.

> 
> 
> 
> > 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>
> > Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > ---
> >
> > v3: Add L4 checksum RSS offload type
> > ---
> >  app/test-pmd/cmdline.c  | 4 ++++
> >  app/test-pmd/config.c   | 2 ++
> >  lib/ethdev/rte_ethdev.h | 2 ++
> >  3 files changed, 8 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 0268b18..6148d84 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")) { 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..1268729 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)
> >
> >  /*
> >   * We use the following macros to combine with above ETH_RSS_* for
> > --
> > 1.8.3.1
> >
  

Acked-by: Aman Deep Singh <aman.deep.singh@intel.com>
  

On 6/15/21 11:19 AM, Alvin Zhang wrote:
> This patch defines new RSS offload types for IPv4 and L4 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>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> 
> v3: Add L4 checksum RSS offload type
> ---
>  app/test-pmd/cmdline.c  | 4 ++++
>  app/test-pmd/config.c   | 2 ++
>  lib/ethdev/rte_ethdev.h | 2 ++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0268b18..6148d84 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")) {
> 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..1268729 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)

What does efine which L4 protocols are supported? How user will
know?
  

> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> > faf3bd9..1268729 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)
> 
> What does efine which L4 protocols are supported? How user will know?

I think TCP/UDP/SCTP can be supported, but this is determined by PMD, here we only provide a general interface.

BRs,
Alvin
  

> > @@ -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)
> 
> What does efine which L4 protocols are supported? How user will know?

I think if we want to support L4 checksum RSS by using below command
port config all rss (all|default|eth|vlan|...)

We must define TCP/UDP/SCTP checksum RSS separately: 
#define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
#define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
#deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)

Here 3 bits are occupied, this is not good for there are not many bits available.

If we only want to using it in flows, we only need to define ETH_RSS_L4_CHKSUM, 
because the flow pattern pointed out the L4 protocol type.
flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss types l4-chksum end queues end / end

So what's your opinions?

Thanks
Alvin
  

> -----Original Message-----
> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> Sent: Tuesday, July 6, 2021 3:06 PM
> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
> 
> > > @@ -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)
> >
> > What does efine which L4 protocols are supported? How user will know?
> 
> I think if we want to support L4 checksum RSS by using below command port
> config all rss (all|default|eth|vlan|...)
> 
> We must define TCP/UDP/SCTP checksum RSS separately:
> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
> 
> Here 3 bits are occupied, this is not good for there are not many bits available.
> 
> If we only want to using it in flows, we only need to define
> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4 protocol
> type.
> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss types l4-chksum
> end queues end / end

+1, the pattern already give the hint to avoid the ambiguity and I think we already have ETH_RSS_LEVEL to figure out inner or outer.
 
> 
> So what's your opinions?
> 
> Thanks
> Alvin
  

On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
>> Sent: Tuesday, July 6, 2021 3:06 PM
>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
>> Cc: dev@dpdk.org
>> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
>>
>>>> @@ -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)
>>>
>>> What does efine which L4 protocols are supported? How user will know?
>>
>> I think if we want to support L4 checksum RSS by using below command port
>> config all rss (all|default|eth|vlan|...)
>>
>> We must define TCP/UDP/SCTP checksum RSS separately:
>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
>>
>> Here 3 bits are occupied, this is not good for there are not many bits available.
>>
>> If we only want to using it in flows, we only need to define
>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4 protocol
>> type.
>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss types l4-chksum
>> end queues end / end
> 
> +1, the pattern already give the hint to avoid the ambiguity and I think we already have ETH_RSS_LEVEL to figure out inner or outer.

The problem that it may be used in generic RSS flags which
has no the context. Also even in the case of flow API
context could have no L4 protocol at all.

Is UDP checksum 0 treated as no checksum and go to default
queue or treated as a regular checksum with value equal to 0?

I tend to agree that 3 flags is too much for the feature,
but one flag without properly defined meaning is not good
as well.

I just want rules to be defined and documented.
  

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, July 6, 2021 4:05 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
> 
> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> >> Sent: Tuesday, July 6, 2021 3:06 PM
> >> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
> >> Cc: dev@dpdk.org
> >> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload
> >> types
> >>
> >>>> @@ -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)
> >>>
> >>> What does efine which L4 protocols are supported? How user will know?
> >>
> >> I think if we want to support L4 checksum RSS by using below command
> >> port config all rss (all|default|eth|vlan|...)
> >>
> >> We must define TCP/UDP/SCTP checksum RSS separately:
> >> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
> >> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
> >> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
> >>
> >> Here 3 bits are occupied, this is not good for there are not many bits
> available.
> >>
> >> If we only want to using it in flows, we only need to define
> >> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
> >> protocol type.
> >> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
> >> types l4-chksum end queues end / end
> >
> > +1, the pattern already give the hint to avoid the ambiguity and I think we
> already have ETH_RSS_LEVEL to figure out inner or outer.
> 
> The problem that it may be used in generic RSS flags which has no the context.
> Also even in the case of flow API context could have no L4 protocol at all.

For generic case, it can simply assume it cover all L4 checksum cases and I'm not sure if any user intend to use it as generic RSS, pmd can simply reject it if it's not necessary to support.
In flow API, if no l4 protocol in pattern , the PMD should return failure (or maybe some default behavior), and I think this is not a new question as it happens all the cases
e.g.:
pattern eth / vlan / end action rss type ipv4 .

> 
> Is UDP checksum 0 treated as no checksum and go to default queue or treated
> as a regular checksum with value equal to 0?

I think we can treat it as value 0, as least our hardware behavior like this, is this any issue?

> 
> I tend to agree that 3 flags is too much for the feature, but one flag without
> properly defined meaning is not good as well.
> 
> I just want rules to be defined and documented.'

Agree, we need more document for this. if you agree above proposal.
  

On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, July 6, 2021 4:05 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
>>
>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
>>>> Sent: Tuesday, July 6, 2021 3:06 PM
>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>> Cc: dev@dpdk.org
>>>> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload
>>>> types
>>>>
>>>>>> @@ -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)
>>>>>
>>>>> What does efine which L4 protocols are supported? How user will know?
>>>>
>>>> I think if we want to support L4 checksum RSS by using below command
>>>> port config all rss (all|default|eth|vlan|...)
>>>>
>>>> We must define TCP/UDP/SCTP checksum RSS separately:
>>>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
>>>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
>>>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
>>>>
>>>> Here 3 bits are occupied, this is not good for there are not many bits
>> available.
>>>>
>>>> If we only want to using it in flows, we only need to define
>>>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
>>>> protocol type.
>>>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
>>>> types l4-chksum end queues end / end
>>>
>>> +1, the pattern already give the hint to avoid the ambiguity and I think we
>> already have ETH_RSS_LEVEL to figure out inner or outer.
>>
>> The problem that it may be used in generic RSS flags which has no the context.
>> Also even in the case of flow API context could have no L4 protocol at all.
> 
> For generic case, it can simply assume it cover all L4 checksum cases and I'm not sure if any user intend to use it as generic RSS, pmd can simply reject it if it's not necessary to support.

Try to look at it from an application point of view which does
not know any specifics of the driver.

 * Get dev_info and see ETH_RSS_L4_CHKSUM, good!, would like to
   use it.

 * If I try to use it in default RSS config, but the request
   fail, it could be very confusing.

 * Will it distribute TCP packets? UDP packets? SCTP packets?
   Or should I care about RSS for some of them based on other
   supported fields? E.g. if SCTP is not supported by the NIC,
   I need to install RSS flow rule for the IP protocol to do
   RSS based on IPv4/IPv6 addresses. But if SCTP is supported,
   I'm happy to use ETH_RSS_L4_CHKSUM for it as well.

> In flow API, if no l4 protocol in pattern , the PMD should return failure (or maybe some default behavior), and I think this is not a new question as it happens all the cases
> e.g.:
> pattern eth / vlan / end action rss type ipv4 .

IMHO, it would be pretty logical to apply RSS to IPv4 packets
only and send everything else to default queue.

>>
>> Is UDP checksum 0 treated as no checksum and go to default queue or treated
>> as a regular checksum with value equal to 0?
> 
> I think we can treat it as value 0, as least our hardware behavior like this, is this any issue?

OK, no problem. Just document it.

>>
>> I tend to agree that 3 flags is too much for the feature, but one flag without
>> properly defined meaning is not good as well.
>>
>> I just want rules to be defined and documented.'
> 
> Agree, we need more document for this. if you agree above proposal.
>
  

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Wednesday, July 7, 2021 5:49 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
> 
> On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Tuesday, July 6, 2021 4:05 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
> >> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload
> >> types
> >>
> >> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> >>>> Sent: Tuesday, July 6, 2021 3:06 PM
> >>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
> >>>> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
> >>>> Cc: dev@dpdk.org
> >>>> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
> >>>> offload types
> >>>>
> >>>>>> @@ -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)
> >>>>>
> >>>>> What does efine which L4 protocols are supported? How user will know?
> >>>>
> >>>> I think if we want to support L4 checksum RSS by using below
> >>>> command port config all rss (all|default|eth|vlan|...)
> >>>>
> >>>> We must define TCP/UDP/SCTP checksum RSS separately:
> >>>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
> >>>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
> >>>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
> >>>>
> >>>> Here 3 bits are occupied, this is not good for there are not many
> >>>> bits
> >> available.
> >>>>
> >>>> If we only want to using it in flows, we only need to define
> >>>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
> >>>> protocol type.
> >>>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
> >>>> types l4-chksum end queues end / end
> >>>
> >>> +1, the pattern already give the hint to avoid the ambiguity and I
> >>> +think we
> >> already have ETH_RSS_LEVEL to figure out inner or outer.
> >>
> >> The problem that it may be used in generic RSS flags which has no the
> context.
> >> Also even in the case of flow API context could have no L4 protocol at all.
> >
> > For generic case, it can simply assume it cover all L4 checksum cases and I'm
> not sure if any user intend to use it as generic RSS, pmd can simply reject it if
> it's not necessary to support.
> 
> Try to look at it from an application point of view which does not know any
> specifics of the driver.
> 
>  * Get dev_info and see ETH_RSS_L4_CHKSUM, good!, would like to
>    use it.


The PMD should not expose it if it don't want to (or not able to) support all l4 checksum from generic RSS configure

And we should assume this is only apply for generic RSS configure but not for flow API.

Because the rte_flow_validate is the recommended method to check if a RSS action is supported in flow API or not.

> 
>  * If I try to use it in default RSS config, but the request
>    fail, it could be very confusing.
> 
>  * Will it distribute TCP packets? UDP packets? SCTP packets?
>    Or should I care about RSS for some of them based on other
>    supported fields? E.g. if SCTP is not supported by the NIC,
>    I need to install RSS flow rule for the IP protocol to do
>    RSS based on IPv4/IPv6 addresses. But if SCTP is supported,
>    I'm happy to use ETH_RSS_L4_CHKSUM for it as well.
> 
> > In flow API, if no l4 protocol in pattern , the PMD should return
> > failure (or maybe some default behavior), and I think this is not a
> > new question as it happens all the cases
> > e.g.:
> > pattern eth / vlan / end action rss type ipv4 .
> 
> IMHO, it would be pretty logical to apply RSS to IPv4 packets only and send
> everything else to default queue.

Yes, this also make sense to me, but I think PMD's flow parser still can have more strict check, as it does not drop any feature that the NIC can support.

> 
> >>
> >> Is UDP checksum 0 treated as no checksum and go to default queue or
> >> treated as a regular checksum with value equal to 0?
> >
> > I think we can treat it as value 0, as least our hardware behavior like this, is
> this any issue?
> 
> OK, no problem. Just document it.
> 
> >>
> >> I tend to agree that 3 flags is too much for the feature, but one
> >> flag without properly defined meaning is not good as well.
> >>
> >> I just want rules to be defined and documented.'
> >
> > Agree, we need more document for this. if you agree above proposal.
> >
  

On 7/7/21 4:00 PM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Wednesday, July 7, 2021 5:49 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
>>
>> On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Tuesday, July 6, 2021 4:05 PM
>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>>>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload
>>>> types
>>>>
>>>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
>>>>>> Sent: Tuesday, July 6, 2021 3:06 PM
>>>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
>>>>>> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>>>> Cc: dev@dpdk.org
>>>>>> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
>>>>>> offload types
>>>>>>
>>>>>>>> @@ -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)
>>>>>>>
>>>>>>> What does efine which L4 protocols are supported? How user will know?
>>>>>>
>>>>>> I think if we want to support L4 checksum RSS by using below
>>>>>> command port config all rss (all|default|eth|vlan|...)
>>>>>>
>>>>>> We must define TCP/UDP/SCTP checksum RSS separately:
>>>>>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
>>>>>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
>>>>>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
>>>>>>
>>>>>> Here 3 bits are occupied, this is not good for there are not many
>>>>>> bits
>>>> available.
>>>>>>
>>>>>> If we only want to using it in flows, we only need to define
>>>>>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
>>>>>> protocol type.
>>>>>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
>>>>>> types l4-chksum end queues end / end
>>>>>
>>>>> +1, the pattern already give the hint to avoid the ambiguity and I
>>>>> +think we
>>>> already have ETH_RSS_LEVEL to figure out inner or outer.
>>>>
>>>> The problem that it may be used in generic RSS flags which has no the
>> context.
>>>> Also even in the case of flow API context could have no L4 protocol at all.
>>>
>>> For generic case, it can simply assume it cover all L4 checksum cases and I'm
>> not sure if any user intend to use it as generic RSS, pmd can simply reject it if
>> it's not necessary to support.
>>
>> Try to look at it from an application point of view which does not know any
>> specifics of the driver.
>>
>>  * Get dev_info and see ETH_RSS_L4_CHKSUM, good!, would like to
>>    use it.
> 
> 
> The PMD should not expose it if it don't want to (or not able to) support all l4 checksum from generic RSS configure

Document what is "all L4".

> 
> And we should assume this is only apply for generic RSS configure but not for flow API.

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

It is just RSS capabilities reporting w/o any context.

> 
> Because the rte_flow_validate is the recommended method to check if a RSS action is supported in flow API or not.

It could restrict the subset. But superset should be reported
in caps.

> 
>>
>>  * If I try to use it in default RSS config, but the request
>>    fail, it could be very confusing.
>>
>>  * Will it distribute TCP packets? UDP packets? SCTP packets?
>>    Or should I care about RSS for some of them based on other
>>    supported fields? E.g. if SCTP is not supported by the NIC,
>>    I need to install RSS flow rule for the IP protocol to do
>>    RSS based on IPv4/IPv6 addresses. But if SCTP is supported,
>>    I'm happy to use ETH_RSS_L4_CHKSUM for it as well.
>>
>>> In flow API, if no l4 protocol in pattern , the PMD should return
>>> failure (or maybe some default behavior), and I think this is not a
>>> new question as it happens all the cases
>>> e.g.:
>>> pattern eth / vlan / end action rss type ipv4 .
>>
>> IMHO, it would be pretty logical to apply RSS to IPv4 packets only and send
>> everything else to default queue.
> 
> Yes, this also make sense to me, but I think PMD's flow parser still can have more strict check, as it does not drop any feature that the NIC can support.
> 
>>
>>>>
>>>> Is UDP checksum 0 treated as no checksum and go to default queue or
>>>> treated as a regular checksum with value equal to 0?
>>>
>>> I think we can treat it as value 0, as least our hardware behavior like this, is
>> this any issue?
>>
>> OK, no problem. Just document it.
>>
>>>>
>>>> I tend to agree that 3 flags is too much for the feature, but one
>>>> flag without properly defined meaning is not good as well.
>>>>
>>>> I just want rules to be defined and documented.'
>>>
>>> Agree, we need more document for this. if you agree above proposal.
>>>
>
  

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Wednesday, July 7, 2021 9:11 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
> 
> On 7/7/21 4:00 PM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Wednesday, July 7, 2021 5:49 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
> >> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload
> >> types
> >>
> >> On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> Sent: Tuesday, July 6, 2021 4:05 PM
> >>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
> >>>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
> >>>> Cc: dev@dpdk.org
> >>>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
> >>>> offload types
> >>>>
> >>>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> >>>>>> Sent: Tuesday, July 6, 2021 3:06 PM
> >>>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
> >>>>>> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
> >>>>>> Cc: dev@dpdk.org
> >>>>>> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
> >>>>>> offload types
> >>>>>>
> >>>>>>>> @@ -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)
> >>>>>>>
> >>>>>>> What does efine which L4 protocols are supported? How user will
> know?
> >>>>>>
> >>>>>> I think if we want to support L4 checksum RSS by using below
> >>>>>> command port config all rss (all|default|eth|vlan|...)
> >>>>>>
> >>>>>> We must define TCP/UDP/SCTP checksum RSS separately:
> >>>>>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
> >>>>>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
> >>>>>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
> >>>>>>
> >>>>>> Here 3 bits are occupied, this is not good for there are not many
> >>>>>> bits
> >>>> available.
> >>>>>>
> >>>>>> If we only want to using it in flows, we only need to define
> >>>>>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
> >>>>>> protocol type.
> >>>>>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
> >>>>>> types l4-chksum end queues end / end
> >>>>>
> >>>>> +1, the pattern already give the hint to avoid the ambiguity and I
> >>>>> +think we
> >>>> already have ETH_RSS_LEVEL to figure out inner or outer.
> >>>>
> >>>> The problem that it may be used in generic RSS flags which has no
> >>>> the
> >> context.
> >>>> Also even in the case of flow API context could have no L4 protocol at all.
> >>>
> >>> For generic case, it can simply assume it cover all L4 checksum
> >>> cases and I'm
> >> not sure if any user intend to use it as generic RSS, pmd can simply
> >> reject it if it's not necessary to support.
> >>
> >> Try to look at it from an application point of view which does not
> >> know any specifics of the driver.
> >>
> >>  * Get dev_info and see ETH_RSS_L4_CHKSUM, good!, would like to
> >>    use it.
> >
> >
> > The PMD should not expose it if it don't want to (or not able to)
> > support all l4 checksum from generic RSS configure
> 
> Document what is "all L4".
> 
> >
> > And we should assume this is only apply for generic RSS configure but not for
> flow API.
> 
> I don't think so. IMHO, it should report all RSS capabilities regardless generic vs
> flow API RSS action.


The RSS action in flow API could cover lots of possibility.
for example an ETH_RSS_IPV4 can be applied on a GTPU flow for inner but may not work for a VxLan flow's inner l3 at the same time.
it's difficult to accurately describe all of these by a 64 bits capability, it's more practice to just rely on rte_flow_validation.
Otherwise it will always leading the confusing you mentioned in previous mail.

It is more reasonable for me, the driver just expose some basic RSS bit that everybody can easiely understand,(e.g.: 5 tuple.), and left all the complexity capability probe to flow API.

> 
> It is just RSS capabilities reporting w/o any context.




> 
> >
> > Because the rte_flow_validate is the recommended method to check if a RSS
> action is supported in flow API or not.
> 
> It could restrict the subset. But superset should be reported in caps.
> 
> >
> >>
> >>  * If I try to use it in default RSS config, but the request
> >>    fail, it could be very confusing.
> >>
> >>  * Will it distribute TCP packets? UDP packets? SCTP packets?
> >>    Or should I care about RSS for some of them based on other
> >>    supported fields? E.g. if SCTP is not supported by the NIC,
> >>    I need to install RSS flow rule for the IP protocol to do
> >>    RSS based on IPv4/IPv6 addresses. But if SCTP is supported,
> >>    I'm happy to use ETH_RSS_L4_CHKSUM for it as well.
> >>
> >>> In flow API, if no l4 protocol in pattern , the PMD should return
> >>> failure (or maybe some default behavior), and I think this is not a
> >>> new question as it happens all the cases
> >>> e.g.:
> >>> pattern eth / vlan / end action rss type ipv4 .
> >>
> >> IMHO, it would be pretty logical to apply RSS to IPv4 packets only
> >> and send everything else to default queue.
> >
> > Yes, this also make sense to me, but I think PMD's flow parser still can have
> more strict check, as it does not drop any feature that the NIC can support.
> >
> >>
> >>>>
> >>>> Is UDP checksum 0 treated as no checksum and go to default queue or
> >>>> treated as a regular checksum with value equal to 0?
> >>>
> >>> I think we can treat it as value 0, as least our hardware behavior
> >>> like this, is
> >> this any issue?
> >>
> >> OK, no problem. Just document it.
> >>
> >>>>
> >>>> I tend to agree that 3 flags is too much for the feature, but one
> >>>> flag without properly defined meaning is not good as well.
> >>>>
> >>>> I just want rules to be defined and documented.'
> >>>
> >>> Agree, we need more document for this. if you agree above proposal.
> >>>
> >
  

@Thomas, @Ferruh, @Ori I need your opinion on the discussion.

On 7/8/21 4:07 AM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Wednesday, July 7, 2021 9:11 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload types
>>
>> On 7/7/21 4:00 PM, Zhang, Qi Z wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Wednesday, July 7, 2021 5:49 PM
>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>>>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>> Cc: dev@dpdk.org
>>>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS offload
>>>> types
>>>>
>>>> On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Sent: Tuesday, July 6, 2021 4:05 PM
>>>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhang, AlvinX
>>>>>> <alvinx.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>>>> Cc: dev@dpdk.org
>>>>>> Subject: Re: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
>>>>>> offload types
>>>>>>
>>>>>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
>>>>>>>> Sent: Tuesday, July 6, 2021 3:06 PM
>>>>>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Zhang, Qi Z
>>>>>>>> <qi.z.zhang@intel.com>; ajit.khaparde@broadcom.com
>>>>>>>> Cc: dev@dpdk.org
>>>>>>>> Subject: RE: [PATCH v3] ethdev: add IPv4 and L4 checksum RSS
>>>>>>>> offload types
>>>>>>>>
>>>>>>>>>> @@ -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)
>>>>>>>>>
>>>>>>>>> What does efine which L4 protocols are supported? How user will
>> know?
>>>>>>>>
>>>>>>>> I think if we want to support L4 checksum RSS by using below
>>>>>>>> command port config all rss (all|default|eth|vlan|...)
>>>>>>>>
>>>>>>>> We must define TCP/UDP/SCTP checksum RSS separately:
>>>>>>>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
>>>>>>>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
>>>>>>>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
>>>>>>>>
>>>>>>>> Here 3 bits are occupied, this is not good for there are not many
>>>>>>>> bits
>>>>>> available.
>>>>>>>>
>>>>>>>> If we only want to using it in flows, we only need to define
>>>>>>>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
>>>>>>>> protocol type.
>>>>>>>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
>>>>>>>> types l4-chksum end queues end / end
>>>>>>>
>>>>>>> +1, the pattern already give the hint to avoid the ambiguity and I
>>>>>>> +think we
>>>>>> already have ETH_RSS_LEVEL to figure out inner or outer.
>>>>>>
>>>>>> The problem that it may be used in generic RSS flags which has no
>>>>>> the
>>>> context.
>>>>>> Also even in the case of flow API context could have no L4 protocol at all.
>>>>>
>>>>> For generic case, it can simply assume it cover all L4 checksum
>>>>> cases and I'm
>>>> not sure if any user intend to use it as generic RSS, pmd can simply
>>>> reject it if it's not necessary to support.
>>>>
>>>> Try to look at it from an application point of view which does not
>>>> know any specifics of the driver.
>>>>
>>>>  * Get dev_info and see ETH_RSS_L4_CHKSUM, good!, would like to
>>>>    use it.
>>>
>>>
>>> The PMD should not expose it if it don't want to (or not able to)
>>> support all l4 checksum from generic RSS configure
>>
>> Document what is "all L4".
>>
>>>
>>> And we should assume this is only apply for generic RSS configure but not for
>> flow API.
>>
>> I don't think so. IMHO, it should report all RSS capabilities regardless generic vs
>> flow API RSS action.
> 
> 
> The RSS action in flow API could cover lots of possibility.
> for example an ETH_RSS_IPV4 can be applied on a GTPU flow for inner but may not work for a VxLan flow's inner l3 at the same time.
> it's difficult to accurately describe all of these by a 64 bits capability, it's more practice to just rely on rte_flow_validation.
> Otherwise it will always leading the confusing you mentioned in previous mail.
> 
> It is more reasonable for me, the driver just expose some basic RSS bit that everybody can easiely understand,(e.g.: 5 tuple.), and left all the complexity capability probe to flow API.

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.

> 
>>
>> It is just RSS capabilities reporting w/o any context.
> 
> 
> 
> 
>>
>>>
>>> Because the rte_flow_validate is the recommended method to check if a RSS
>> action is supported in flow API or not.
>>
>> It could restrict the subset. But superset should be reported in caps.
>>
>>>
>>>>
>>>>  * If I try to use it in default RSS config, but the request
>>>>    fail, it could be very confusing.
>>>>
>>>>  * Will it distribute TCP packets? UDP packets? SCTP packets?
>>>>    Or should I care about RSS for some of them based on other
>>>>    supported fields? E.g. if SCTP is not supported by the NIC,
>>>>    I need to install RSS flow rule for the IP protocol to do
>>>>    RSS based on IPv4/IPv6 addresses. But if SCTP is supported,
>>>>    I'm happy to use ETH_RSS_L4_CHKSUM for it as well.
>>>>
>>>>> In flow API, if no l4 protocol in pattern , the PMD should return
>>>>> failure (or maybe some default behavior), and I think this is not a
>>>>> new question as it happens all the cases
>>>>> e.g.:
>>>>> pattern eth / vlan / end action rss type ipv4 .
>>>>
>>>> IMHO, it would be pretty logical to apply RSS to IPv4 packets only
>>>> and send everything else to default queue.
>>>
>>> Yes, this also make sense to me, but I think PMD's flow parser still can have
>> more strict check, as it does not drop any feature that the NIC can support.
>>>
>>>>
>>>>>>
>>>>>> Is UDP checksum 0 treated as no checksum and go to default queue or
>>>>>> treated as a regular checksum with value equal to 0?
>>>>>
>>>>> I think we can treat it as value 0, as least our hardware behavior
>>>>> like this, is
>>>> this any issue?
>>>>
>>>> OK, no problem. Just document it.
>>>>
>>>>>>
>>>>>> I tend to agree that 3 flags is too much for the feature, but one
>>>>>> flag without properly defined meaning is not good as well.
>>>>>>
>>>>>> I just want rules to be defined and documented.'
>>>>>
>>>>> Agree, we need more document for this. if you agree above proposal.
>>>>>
>>>
>
  

08/07/2021 09:45, Andrew Rybchenko:
> @Thomas, @Ferruh, @Ori I need your opinion on the discussion.
> 
> On 7/8/21 4:07 AM, Zhang, Qi Z wrote:
> > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> On 7/7/21 6:23 AM, Zhang, Qi Z wrote:
> >>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>>>> On 7/6/21 10:18 AM, Zhang, Qi Z wrote:
> >>>>>>> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> >>>>>>>>
> >>>>>>>>>> @@ -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)
> >>>>>>>>>
> >>>>>>>>> What does efine which L4 protocols are supported? How user will
> >> know?
> >>>>>>>>
> >>>>>>>> I think if we want to support L4 checksum RSS by using below
> >>>>>>>> command port config all rss (all|default|eth|vlan|...)
> >>>>>>>>
> >>>>>>>> We must define TCP/UDP/SCTP checksum RSS separately:
> >>>>>>>> #define ETH_RSS_TCP_CHKSUM	(1ULL << 35)
> >>>>>>>> #define ETH_RSS_UDP_CHKSUM	(1ULL << 36)
> >>>>>>>> #deifne ETH_RSS_SCTP_CHKSUM	(1ULL << 37)
> >>>>>>>>
> >>>>>>>> Here 3 bits are occupied, this is not good for there are not many
> >>>>>>>> bits
> >>>>>> available.
> >>>>>>>>
> >>>>>>>> If we only want to using it in flows, we only need to define
> >>>>>>>> ETH_RSS_L4_CHKSUM, because the flow pattern pointed out the L4
> >>>>>>>> protocol type.
> >>>>>>>> flow create 0 ingress pattern eth / ipv4 / tcp / end actions rss
> >>>>>>>> types l4-chksum end queues end / end
> >>>>>>>
> >>>>>>> +1, the pattern already give the hint to avoid the ambiguity and I
> >>>>>>> +think we
> >>>>>> already have ETH_RSS_LEVEL to figure out inner or outer.
> >>>>>>
> >>>>>> The problem that it may be used in generic RSS flags which has no
> >>>>>> the
> >>>> context.
> >>>>>> Also even in the case of flow API context could have no L4 protocol at all.
> >>>>>
> >>>>> For generic case, it can simply assume it cover all L4 checksum
> >>>>> cases and I'm
> >>>> not sure if any user intend to use it as generic RSS, pmd can simply
> >>>> reject it if it's not necessary to support.
> >>>>
> >>>> Try to look at it from an application point of view which does not
> >>>> know any specifics of the driver.
> >>>>
> >>>>  * Get dev_info and see ETH_RSS_L4_CHKSUM, good!, would like to
> >>>>    use it.
> >>>
> >>>
> >>> The PMD should not expose it if it don't want to (or not able to)
> >>> support all l4 checksum from generic RSS configure

That's restrictive to allow only full-support,
but I'm fine with the trade-off to avoid wasting bits.

> >>
> >> Document what is "all L4".

List of L4 protocols should be explicit.


> >>> And we should assume this is only apply for generic RSS configure but not for
> >> flow API.
> >>
> >> I don't think so. IMHO, it should report all RSS capabilities regardless generic vs
> >> flow API RSS action.
> > 
> > 
> > The RSS action in flow API could cover lots of possibility.
> > for example an ETH_RSS_IPV4 can be applied on a GTPU flow for inner but may not work for a VxLan flow's inner l3 at the same time.
> > it's difficult to accurately describe all of these by a 64 bits capability, it's more practice to just rely on rte_flow_validation.
> > Otherwise it will always leading the confusing you mentioned in previous mail.
> > 
> > It is more reasonable for me, the driver just expose some basic RSS bit that everybody can easiely understand,(e.g.: 5 tuple.), and left all the complexity capability probe to flow API.
> 
> 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.

Yes superset should be reported, this is the meaning of capabilities:
the driver is capable but there are some limitations
which cannot be advertised, so rte_flow_validate checks the limitations
in the dynamic context.


> >> It is just RSS capabilities reporting w/o any context.
> > 
> >>>
> >>> Because the rte_flow_validate is the recommended method to check if a RSS
> >> action is supported in flow API or not.
> >>
> >> It could restrict the subset. But superset should be reported in caps.
> >>
> >>>
> >>>>
> >>>>  * If I try to use it in default RSS config, but the request
> >>>>    fail, it could be very confusing.
> >>>>
> >>>>  * Will it distribute TCP packets? UDP packets? SCTP packets?
> >>>>    Or should I care about RSS for some of them based on other
> >>>>    supported fields? E.g. if SCTP is not supported by the NIC,
> >>>>    I need to install RSS flow rule for the IP protocol to do
> >>>>    RSS based on IPv4/IPv6 addresses. But if SCTP is supported,
> >>>>    I'm happy to use ETH_RSS_L4_CHKSUM for it as well.
> >>>>
> >>>>> In flow API, if no l4 protocol in pattern , the PMD should return
> >>>>> failure (or maybe some default behavior), and I think this is not a
> >>>>> new question as it happens all the cases
> >>>>> e.g.:
> >>>>> pattern eth / vlan / end action rss type ipv4 .
> >>>>
> >>>> IMHO, it would be pretty logical to apply RSS to IPv4 packets only
> >>>> and send everything else to default queue.
> >>>
> >>> Yes, this also make sense to me, but I think PMD's flow parser still can have
> >> more strict check, as it does not drop any feature that the NIC can support.
> >>>
> >>>>
> >>>>>>
> >>>>>> Is UDP checksum 0 treated as no checksum and go to default queue or
> >>>>>> treated as a regular checksum with value equal to 0?
> >>>>>
> >>>>> I think we can treat it as value 0, as least our hardware behavior
> >>>>> like this, is
> >>>> this any issue?
> >>>>
> >>>> OK, no problem. Just document it.
> >>>>
> >>>>>>
> >>>>>> I tend to agree that 3 flags is too much for the feature, but one
> >>>>>> flag without properly defined meaning is not good as well.
> >>>>>>
> >>>>>> I just want rules to be defined and documented.'
> >>>>>
> >>>>> Agree, we need more document for this. if you agree above proposal.

Yes please do not add a new flag in rte_ethdev.h without doc.