diff mbox

[dpdk-dev] lib/librte_ether: add comments RSS flags

Message ID 20180104181506.153697-1-qiming.yang@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Qiming Yang Jan. 4, 2018, 6:15 p.m. UTC 
  Detail the comment for RSS offload types and added comments for each type.

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
---
 lib/librte_ether/rte_ethdev.h | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

Comments

Andrew Rybchenko Jan. 6, 2018, 10:21 a.m. UTC | #1 
On 01/04/2018 09:15 PM, Qiming Yang wrote:
> Detail the comment for RSS offload types and added comments for each type.
>
> Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> ---
>   lib/librte_ether/rte_ethdev.h | 29 +++++++++++++++++++++++++----
>   1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 341c2d6..2091560 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -427,31 +427,52 @@ struct rte_eth_rss_conf {
>   	uint64_t rss_hf;     /**< Hash functions to apply - see below. */
>   };
>   
> -/*
> - * The RSS offload types are defined based on flow types which are defined
> - * in rte_eth_ctrl.h. Different NIC hardwares may support different RSS offload
> - * types. The supported flow types or RSS offload types can be queried by
> +/**
> + * The RSS offload types are related to *rss_hf* field of *rss_conf* structure.
> + * They are defined based on flow types which are defined in rte_eth_ctrl.h.
> + * Different NIC hardwares may support different RSS offload types. The
> + * supported flow types or RSS offload types can be queried by
>    * rte_eth_dev_info_get().
>    */
> +/** Enable RSS offload on IPV4 packets */
>   #define ETH_RSS_IPV4               (1ULL << RTE_ETH_FLOW_IPV4)
> +/** Enable RSS offload on fragmented IPV4 packets */
>   #define ETH_RSS_FRAG_IPV4          (1ULL << RTE_ETH_FLOW_FRAG_IPV4)
> +/** Enable RSS offload on none fragmented IPV4 TCP packets */
>   #define ETH_RSS_NONFRAG_IPV4_TCP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_TCP)
> +/** Enable RSS offload on none fragmented IPV4 UDP packets */
>   #define ETH_RSS_NONFRAG_IPV4_UDP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_UDP)
> +/** Enable RSS offload on none fragmented IPV4 SCTP packets */
>   #define ETH_RSS_NONFRAG_IPV4_SCTP  (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_SCTP)
> +/** Enable RSS offload on other none fragmented IPV4 packets */
>   #define ETH_RSS_NONFRAG_IPV4_OTHER (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_OTHER)
> +/** Enable RSS offload on IPV6 packets */
>   #define ETH_RSS_IPV6               (1ULL << RTE_ETH_FLOW_IPV6)
> +/** Enable RSS offload on fragmented IPV6 packets */
>   #define ETH_RSS_FRAG_IPV6          (1ULL << RTE_ETH_FLOW_FRAG_IPV6)
> +/** Enable RSS offload on none fragmented IPV6 TCP packets */
>   #define ETH_RSS_NONFRAG_IPV6_TCP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_TCP)
> +/** Enable RSS offload on none fragmented IPV6 UDP packets */
>   #define ETH_RSS_NONFRAG_IPV6_UDP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_UDP)
> +/** Enable RSS offload on none fragmented IPV6 SCTP packets */
>   #define ETH_RSS_NONFRAG_IPV6_SCTP  (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_SCTP)
> +/** Enable RSS offload on other none fragmented IPV6 packets */
>   #define ETH_RSS_NONFRAG_IPV6_OTHER (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_OTHER)
> +/** Enable RSS offload on L2 Ethernet packets */
>   #define ETH_RSS_L2_PAYLOAD         (1ULL << RTE_ETH_FLOW_L2_PAYLOAD)
> +/** Enable RSS offload on extended IPV6 packets */
>   #define ETH_RSS_IPV6_EX            (1ULL << RTE_ETH_FLOW_IPV6_EX)
> +/** Enable RSS offload on extended IPV6 TCP packets */
>   #define ETH_RSS_IPV6_TCP_EX        (1ULL << RTE_ETH_FLOW_IPV6_TCP_EX)
> +/** Enable RSS offload on extended IPV6 UDP packets */
>   #define ETH_RSS_IPV6_UDP_EX        (1ULL << RTE_ETH_FLOW_IPV6_UDP_EX)
> +/** Enable RSS offload on all packets from that port */
>   #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)
> +/** Enable RSS offload on VXLAN packets */
>   #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)
> +/** Enable RSS offload on GENEVE packets */
>   #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)
> +/** Enable RSS offload on NVGRE packets */
>   #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)
>   
>   #define ETH_RSS_IP ( \

These comments just decode the define name and not that useful. What would
be really useful here is specification of which fields of the packet 
headers are
used to calculate hash especially in the case of tunnels.
Shahaf Shuler Jan. 7, 2018, 6:32 a.m. UTC | #2 
Saturday, January 6, 2018 12:22 PM, Andrew Rybchenko:

[..]

> >   #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)

> > +/** Enable RSS offload on VXLAN packets */

> >   #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)

> > +/** Enable RSS offload on GENEVE packets */

> >   #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)

> > +/** Enable RSS offload on NVGRE packets */

> >   #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)

> >

> >   #define ETH_RSS_IP ( \

> 

> These comments just decode the define name and not that useful. What

> would be really useful here is specification of which fields of the packet

> headers are used to calculate hash especially in the case of tunnels.


+1.

Also maybe some more clarifications, for example:
1. What is the expected behavior when, for example, setting the ETH_RSS_IPV6_EX and regular IPv6 packet arrives? is RSS apply on it? 
2. What is the expected behavior from the PMD when not supporting one of the RSS types? 
     For example most of the DPDK examples uses ETH_RSS_IP [1], however very few devices actually supports each and every RSS type.
     Assuming such configuration returns with no error, what should application expect when unsupported packet type arrives. 

#define ETH_RSS_IP ( \                
        ETH_RSS_IPV4 | \              
        ETH_RSS_FRAG_IPV4 | \         
        ETH_RSS_NONFRAG_IPV4_OTHER | \
        ETH_RSS_IPV6 | \              
        ETH_RSS_FRAG_IPV6 | \         
        ETH_RSS_NONFRAG_IPV6_OTHER | \
        ETH_RSS_IPV6_EX)
Thomas Monjalon Jan. 7, 2018, 8:32 p.m. UTC | #3 
07/01/2018 07:32, Shahaf Shuler:
> Saturday, January 6, 2018 12:22 PM, Andrew Rybchenko:
> 
> [..]
> 
> > >   #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)
> > > +/** Enable RSS offload on VXLAN packets */
> > >   #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)
> > > +/** Enable RSS offload on GENEVE packets */
> > >   #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)
> > > +/** Enable RSS offload on NVGRE packets */
> > >   #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)
> > >
> > >   #define ETH_RSS_IP ( \
> > 
> > These comments just decode the define name and not that useful. What
> > would be really useful here is specification of which fields of the packet
> > headers are used to calculate hash especially in the case of tunnels.
> 
> +1.
> 
> Also maybe some more clarifications, for example:
> 1. What is the expected behavior when, for example, setting the ETH_RSS_IPV6_EX and regular IPv6 packet arrives? is RSS apply on it? 
> 2. What is the expected behavior from the PMD when not supporting one of the RSS types? 
>      For example most of the DPDK examples uses ETH_RSS_IP [1], however very few devices actually supports each and every RSS type.
>      Assuming such configuration returns with no error, what should application expect when unsupported packet type arrives. 
> 
> #define ETH_RSS_IP ( \                
>         ETH_RSS_IPV4 | \              
>         ETH_RSS_FRAG_IPV4 | \         
>         ETH_RSS_NONFRAG_IPV4_OTHER | \
>         ETH_RSS_IPV6 | \              
>         ETH_RSS_FRAG_IPV6 | \         
>         ETH_RSS_NONFRAG_IPV6_OTHER | \
>         ETH_RSS_IPV6_EX)              

+1
We really need a detailed documentation of the exact behaviour.

It must be at least as detailed as lib/librte_mbuf/rte_mbuf_ptype.h.
Thanks
Qiming Yang Jan. 9, 2018, 3:20 a.m. UTC | #4 
> -----Original Message-----

> From: Shahaf Shuler [mailto:shahafs@mellanox.com]

> Sent: Sunday, January 7, 2018 2:33 PM

> To: Andrew Rybchenko <arybchenko@solarflare.com>; Yang, Qiming

> <qiming.yang@intel.com>; dev@dpdk.org; Thomas Monjalon

> <thomas@monjalon.net>

> Subject: RE: [dpdk-dev] [DPDK] lib/librte_ether: add comments RSS flags

> 

> Saturday, January 6, 2018 12:22 PM, Andrew Rybchenko:

> 

> [..]

> 

> > >   #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)

> > > +/** Enable RSS offload on VXLAN packets */

> > >   #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)

> > > +/** Enable RSS offload on GENEVE packets */

> > >   #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)

> > > +/** Enable RSS offload on NVGRE packets */

> > >   #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)

> > >

> > >   #define ETH_RSS_IP ( \

> >

> > These comments just decode the define name and not that useful. What

> > would be really useful here is specification of which fields of the

> > packet headers are used to calculate hash especially in the case of tunnels.

> 

> +1.

> 

> Also maybe some more clarifications, for example:

> 1. What is the expected behavior when, for example, setting the

> ETH_RSS_IPV6_EX and regular IPv6 packet arrives? is RSS apply on it?

> 2. What is the expected behavior from the PMD when not supporting one of the

> RSS types?

>      For example most of the DPDK examples uses ETH_RSS_IP [1], however very

> few devices actually supports each and every RSS type.

>      Assuming such configuration returns with no error, what should application

> expect when unsupported packet type arrives.

> 

If the unsupported packet type arrives, RSS offload will not work. I'll detail the comments in the next version patch.
> #define ETH_RSS_IP ( \

>         ETH_RSS_IPV4 | \

>         ETH_RSS_FRAG_IPV4 | \

>         ETH_RSS_NONFRAG_IPV4_OTHER | \

>         ETH_RSS_IPV6 | \

>         ETH_RSS_FRAG_IPV6 | \

>         ETH_RSS_NONFRAG_IPV6_OTHER | \

>         ETH_RSS_IPV6_EX)

> 

> 

>
Qiming Yang Jan. 9, 2018, 3:21 a.m. UTC | #5 
I’ll detail it. Thanks.

From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]

Sent: Saturday, January 6, 2018 6:22 PM
To: Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org; thomas@monjalon.net
Subject: Re: [dpdk-dev] [DPDK] lib/librte_ether: add comments RSS flags

On 01/04/2018 09:15 PM, Qiming Yang wrote:

Detail the comment for RSS offload types and added comments for each type.



Signed-off-by: Qiming Yang <qiming.yang@intel.com><mailto:qiming.yang@intel.com>


---

 lib/librte_ether/rte_ethdev.h | 29 +++++++++++++++++++++++++----

 1 file changed, 25 insertions(+), 4 deletions(-)



diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h

index 341c2d6..2091560 100644

--- a/lib/librte_ether/rte_ethdev.h

+++ b/lib/librte_ether/rte_ethdev.h

@@ -427,31 +427,52 @@ struct rte_eth_rss_conf {

         uint64_t rss_hf;     /**< Hash functions to apply - see below. */

 };



-/*

- * The RSS offload types are defined based on flow types which are defined

- * in rte_eth_ctrl.h. Different NIC hardwares may support different RSS offload

- * types. The supported flow types or RSS offload types can be queried by

+/**

+ * The RSS offload types are related to *rss_hf* field of *rss_conf* structure.

+ * They are defined based on flow types which are defined in rte_eth_ctrl.h.

+ * Different NIC hardwares may support different RSS offload types. The

+ * supported flow types or RSS offload types can be queried by

  * rte_eth_dev_info_get().

  */

+/** Enable RSS offload on IPV4 packets */

 #define ETH_RSS_IPV4               (1ULL << RTE_ETH_FLOW_IPV4)

+/** Enable RSS offload on fragmented IPV4 packets */

 #define ETH_RSS_FRAG_IPV4          (1ULL << RTE_ETH_FLOW_FRAG_IPV4)

+/** Enable RSS offload on none fragmented IPV4 TCP packets */

 #define ETH_RSS_NONFRAG_IPV4_TCP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_TCP)

+/** Enable RSS offload on none fragmented IPV4 UDP packets */

 #define ETH_RSS_NONFRAG_IPV4_UDP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_UDP)

+/** Enable RSS offload on none fragmented IPV4 SCTP packets */

 #define ETH_RSS_NONFRAG_IPV4_SCTP  (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_SCTP)

+/** Enable RSS offload on other none fragmented IPV4 packets */

 #define ETH_RSS_NONFRAG_IPV4_OTHER (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_OTHER)

+/** Enable RSS offload on IPV6 packets */

 #define ETH_RSS_IPV6               (1ULL << RTE_ETH_FLOW_IPV6)

+/** Enable RSS offload on fragmented IPV6 packets */

 #define ETH_RSS_FRAG_IPV6          (1ULL << RTE_ETH_FLOW_FRAG_IPV6)

+/** Enable RSS offload on none fragmented IPV6 TCP packets */

 #define ETH_RSS_NONFRAG_IPV6_TCP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_TCP)

+/** Enable RSS offload on none fragmented IPV6 UDP packets */

 #define ETH_RSS_NONFRAG_IPV6_UDP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_UDP)

+/** Enable RSS offload on none fragmented IPV6 SCTP packets */

 #define ETH_RSS_NONFRAG_IPV6_SCTP  (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_SCTP)

+/** Enable RSS offload on other none fragmented IPV6 packets */

 #define ETH_RSS_NONFRAG_IPV6_OTHER (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_OTHER)

+/** Enable RSS offload on L2 Ethernet packets */

 #define ETH_RSS_L2_PAYLOAD         (1ULL << RTE_ETH_FLOW_L2_PAYLOAD)

+/** Enable RSS offload on extended IPV6 packets */

 #define ETH_RSS_IPV6_EX            (1ULL << RTE_ETH_FLOW_IPV6_EX)

+/** Enable RSS offload on extended IPV6 TCP packets */

 #define ETH_RSS_IPV6_TCP_EX        (1ULL << RTE_ETH_FLOW_IPV6_TCP_EX)

+/** Enable RSS offload on extended IPV6 UDP packets */

 #define ETH_RSS_IPV6_UDP_EX        (1ULL << RTE_ETH_FLOW_IPV6_UDP_EX)

+/** Enable RSS offload on all packets from that port */

 #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)

+/** Enable RSS offload on VXLAN packets */

 #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)

+/** Enable RSS offload on GENEVE packets */

 #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)

+/** Enable RSS offload on NVGRE packets */

 #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)



 #define ETH_RSS_IP ( \

These comments just decode the define name and not that useful. What would
be really useful here is specification of which fields of the packet headers are
used to calculate hash especially in the case of tunnels.
Qiming Yang Jan. 10, 2018, 2:15 a.m. UTC | #6 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, January 8, 2018 4:33 AM
> To: Yang, Qiming <qiming.yang@intel.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [DPDK] lib/librte_ether: add comments RSS flags
> 
> 07/01/2018 07:32, Shahaf Shuler:
> > Saturday, January 6, 2018 12:22 PM, Andrew Rybchenko:
> >
> > [..]
> >
> > > >   #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)
> > > > +/** Enable RSS offload on VXLAN packets */
> > > >   #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)
> > > > +/** Enable RSS offload on GENEVE packets */
> > > >   #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)
> > > > +/** Enable RSS offload on NVGRE packets */
> > > >   #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)
> > > >
> > > >   #define ETH_RSS_IP ( \
> > >
> > > These comments just decode the define name and not that useful. What
> > > would be really useful here is specification of which fields of the
> > > packet headers are used to calculate hash especially in the case of tunnels.
> >
> > +1.
> >
> > Also maybe some more clarifications, for example:
> > 1. What is the expected behavior when, for example, setting the
> ETH_RSS_IPV6_EX and regular IPv6 packet arrives? is RSS apply on it?
> > 2. What is the expected behavior from the PMD when not supporting one of
> the RSS types?
> >      For example most of the DPDK examples uses ETH_RSS_IP [1], however
> very few devices actually supports each and every RSS type.
> >      Assuming such configuration returns with no error, what should application
> expect when unsupported packet type arrives.
> >
> > #define ETH_RSS_IP ( \
> >         ETH_RSS_IPV4 | \
> >         ETH_RSS_FRAG_IPV4 | \
> >         ETH_RSS_NONFRAG_IPV4_OTHER | \
> >         ETH_RSS_IPV6 | \
> >         ETH_RSS_FRAG_IPV6 | \
> >         ETH_RSS_NONFRAG_IPV6_OTHER | \
> >         ETH_RSS_IPV6_EX)
> 
> +1
> We really need a detailed documentation of the.
> 

I agree with your comments, but the same RSS configuration may have different behavior(RSS offload and input set) in different driver. Adding driver specific information in rte_ethdev.h  is not suitable.
So I think the best scheme is to update driver's document to detail the exact behavior and add reference doc here.

> It must be at least as detailed as lib/librte_mbuf/rte_mbuf_ptype.h.
> Thanks
Thomas Monjalon Jan. 10, 2018, 7:38 a.m. UTC | #7 
10/01/2018 03:15, Yang, Qiming:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 07/01/2018 07:32, Shahaf Shuler:
> > > Saturday, January 6, 2018 12:22 PM, Andrew Rybchenko:
> > >
> > > [..]
> > >
> > > > >   #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)
> > > > > +/** Enable RSS offload on VXLAN packets */
> > > > >   #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)
> > > > > +/** Enable RSS offload on GENEVE packets */
> > > > >   #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)
> > > > > +/** Enable RSS offload on NVGRE packets */
> > > > >   #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)
> > > > >
> > > > >   #define ETH_RSS_IP ( \
> > > >
> > > > These comments just decode the define name and not that useful. What
> > > > would be really useful here is specification of which fields of the
> > > > packet headers are used to calculate hash especially in the case of tunnels.
> > >
> > > +1.
> > >
> > > Also maybe some more clarifications, for example:
> > > 1. What is the expected behavior when, for example, setting the
> > ETH_RSS_IPV6_EX and regular IPv6 packet arrives? is RSS apply on it?
> > > 2. What is the expected behavior from the PMD when not supporting one of
> > the RSS types?
> > >      For example most of the DPDK examples uses ETH_RSS_IP [1], however
> > very few devices actually supports each and every RSS type.
> > >      Assuming such configuration returns with no error, what should application
> > expect when unsupported packet type arrives.
> > >
> > > #define ETH_RSS_IP ( \
> > >         ETH_RSS_IPV4 | \
> > >         ETH_RSS_FRAG_IPV4 | \
> > >         ETH_RSS_NONFRAG_IPV4_OTHER | \
> > >         ETH_RSS_IPV6 | \
> > >         ETH_RSS_FRAG_IPV6 | \
> > >         ETH_RSS_NONFRAG_IPV6_OTHER | \
> > >         ETH_RSS_IPV6_EX)
> > 
> > +1
> > We really need a detailed documentation of the.
> > 
> 
> I agree with your comments, but the same RSS configuration may have different behavior(RSS offload and input set) in different driver.

This is a bug!
The API must have the same meaning for all drivers.

> Adding driver specific information in rte_ethdev.h  is not suitable.
> So I think the best scheme is to update driver's document to detail the exact behavior and add reference doc here.
> 
> > It must be at least as detailed as lib/librte_mbuf/rte_mbuf_ptype.h.
> > Thanks
Andrew Rybchenko Jan. 10, 2018, 7:55 a.m. UTC | #8 
On 01/10/2018 10:38 AM, Thomas Monjalon wrote:
> 10/01/2018 03:15, Yang, Qiming:
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>> 07/01/2018 07:32, Shahaf Shuler:
>>>> Saturday, January 6, 2018 12:22 PM, Andrew Rybchenko:
>>>>
>>>> [..]
>>>>
>>>>>>    #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)
>>>>>> +/** Enable RSS offload on VXLAN packets */
>>>>>>    #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)
>>>>>> +/** Enable RSS offload on GENEVE packets */
>>>>>>    #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)
>>>>>> +/** Enable RSS offload on NVGRE packets */
>>>>>>    #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)
>>>>>>
>>>>>>    #define ETH_RSS_IP ( \
>>>>> These comments just decode the define name and not that useful. What
>>>>> would be really useful here is specification of which fields of the
>>>>> packet headers are used to calculate hash especially in the case of tunnels.
>>>> +1.
>>>>
>>>> Also maybe some more clarifications, for example:
>>>> 1. What is the expected behavior when, for example, setting the
>>> ETH_RSS_IPV6_EX and regular IPv6 packet arrives? is RSS apply on it?
>>>> 2. What is the expected behavior from the PMD when not supporting one of
>>> the RSS types?
>>>>       For example most of the DPDK examples uses ETH_RSS_IP [1], however
>>> very few devices actually supports each and every RSS type.
>>>>       Assuming such configuration returns with no error, what should application
>>> expect when unsupported packet type arrives.
>>>> #define ETH_RSS_IP ( \
>>>>          ETH_RSS_IPV4 | \
>>>>          ETH_RSS_FRAG_IPV4 | \
>>>>          ETH_RSS_NONFRAG_IPV4_OTHER | \
>>>>          ETH_RSS_IPV6 | \
>>>>          ETH_RSS_FRAG_IPV6 | \
>>>>          ETH_RSS_NONFRAG_IPV6_OTHER | \
>>>>          ETH_RSS_IPV6_EX)
>>> +1
>>> We really need a detailed documentation of the.
>>>
>> I agree with your comments, but the same RSS configuration may have different behavior(RSS offload and input set) in different driver.
> This is a bug!
> The API must have the same meaning for all drivers.

+1 yes, that's what applications expect
Of course, different hardware/drivers can and have specifics
which should be described in the driver documentation, but
default behaviour should be defined.

>> Adding driver specific information in rte_ethdev.h  is not suitable.
>> So I think the best scheme is to update driver's document to detail the exact behavior and add reference doc here.
>>
>>> It must be at least as detailed as lib/librte_mbuf/rte_mbuf_ptype.h.
>>> Thanks
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 341c2d6..2091560 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -427,31 +427,52 @@  struct rte_eth_rss_conf {
 	uint64_t rss_hf;     /**< Hash functions to apply - see below. */
 };
 
-/*
- * The RSS offload types are defined based on flow types which are defined
- * in rte_eth_ctrl.h. Different NIC hardwares may support different RSS offload
- * types. The supported flow types or RSS offload types can be queried by
+/**
+ * The RSS offload types are related to *rss_hf* field of *rss_conf* structure.
+ * They are defined based on flow types which are defined in rte_eth_ctrl.h.
+ * Different NIC hardwares may support different RSS offload types. The
+ * supported flow types or RSS offload types can be queried by
  * rte_eth_dev_info_get().
  */
+/** Enable RSS offload on IPV4 packets */
 #define ETH_RSS_IPV4               (1ULL << RTE_ETH_FLOW_IPV4)
+/** Enable RSS offload on fragmented IPV4 packets */
 #define ETH_RSS_FRAG_IPV4          (1ULL << RTE_ETH_FLOW_FRAG_IPV4)
+/** Enable RSS offload on none fragmented IPV4 TCP packets */
 #define ETH_RSS_NONFRAG_IPV4_TCP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_TCP)
+/** Enable RSS offload on none fragmented IPV4 UDP packets */
 #define ETH_RSS_NONFRAG_IPV4_UDP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_UDP)
+/** Enable RSS offload on none fragmented IPV4 SCTP packets */
 #define ETH_RSS_NONFRAG_IPV4_SCTP  (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_SCTP)
+/** Enable RSS offload on other none fragmented IPV4 packets */
 #define ETH_RSS_NONFRAG_IPV4_OTHER (1ULL << RTE_ETH_FLOW_NONFRAG_IPV4_OTHER)
+/** Enable RSS offload on IPV6 packets */
 #define ETH_RSS_IPV6               (1ULL << RTE_ETH_FLOW_IPV6)
+/** Enable RSS offload on fragmented IPV6 packets */
 #define ETH_RSS_FRAG_IPV6          (1ULL << RTE_ETH_FLOW_FRAG_IPV6)
+/** Enable RSS offload on none fragmented IPV6 TCP packets */
 #define ETH_RSS_NONFRAG_IPV6_TCP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_TCP)
+/** Enable RSS offload on none fragmented IPV6 UDP packets */
 #define ETH_RSS_NONFRAG_IPV6_UDP   (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_UDP)
+/** Enable RSS offload on none fragmented IPV6 SCTP packets */
 #define ETH_RSS_NONFRAG_IPV6_SCTP  (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_SCTP)
+/** Enable RSS offload on other none fragmented IPV6 packets */
 #define ETH_RSS_NONFRAG_IPV6_OTHER (1ULL << RTE_ETH_FLOW_NONFRAG_IPV6_OTHER)
+/** Enable RSS offload on L2 Ethernet packets */
 #define ETH_RSS_L2_PAYLOAD         (1ULL << RTE_ETH_FLOW_L2_PAYLOAD)
+/** Enable RSS offload on extended IPV6 packets */
 #define ETH_RSS_IPV6_EX            (1ULL << RTE_ETH_FLOW_IPV6_EX)
+/** Enable RSS offload on extended IPV6 TCP packets */
 #define ETH_RSS_IPV6_TCP_EX        (1ULL << RTE_ETH_FLOW_IPV6_TCP_EX)
+/** Enable RSS offload on extended IPV6 UDP packets */
 #define ETH_RSS_IPV6_UDP_EX        (1ULL << RTE_ETH_FLOW_IPV6_UDP_EX)
+/** Enable RSS offload on all packets from that port */
 #define ETH_RSS_PORT               (1ULL << RTE_ETH_FLOW_PORT)
+/** Enable RSS offload on VXLAN packets */
 #define ETH_RSS_VXLAN              (1ULL << RTE_ETH_FLOW_VXLAN)
+/** Enable RSS offload on GENEVE packets */
 #define ETH_RSS_GENEVE             (1ULL << RTE_ETH_FLOW_GENEVE)
+/** Enable RSS offload on NVGRE packets */
 #define ETH_RSS_NVGRE              (1ULL << RTE_ETH_FLOW_NVGRE)
 
 #define ETH_RSS_IP ( \