diff mbox

[dpdk-dev,v2,04/13] ethdev: support of multiple sizes of redirection table

Message ID 1411634427-746-5-git-send-email-helin.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Helin Zhang Sept. 25, 2014, 8:40 a.m. UTC
To support possible different sizes of redirection table,
structures and functions need to be redefined. In detail,
* 'struct rte_eth_rss_reta' has been redefined.
* 'uint16_t reta_size' has been added into
  'struct rte_eth_dev_info'.
* Updating/querying reta have been reimplemented with one
  more parameter of redirection table size.

v2 changes:
* Put changes for supporting multiple sizes of reta in
  ethdev into a single patch.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Reviewed-by: Jijiang Liu <jijiang.liu@intel.com>
Reviewed-by: Cunming Liang <cunming.liang@intel.com>
Reviewed-by: Jingjing Wu <jingjing.wu@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 116 ++++++++++++++++++++++++++----------------
 lib/librte_ether/rte_ethdev.h |  43 ++++++++++------
 2 files changed, 99 insertions(+), 60 deletions(-)

Comments

Thomas Monjalon Oct. 21, 2014, 8:53 p.m. UTC | #1
2014-09-25 16:40, Helin Zhang:
> To support possible different sizes of redirection table,
> structures and functions need to be redefined. In detail,
> * 'struct rte_eth_rss_reta' has been redefined.
> * 'uint16_t reta_size' has been added into
>   'struct rte_eth_dev_info'.
> * Updating/querying reta have been reimplemented with one
>   more parameter of redirection table size.
> 
> v2 changes:
> * Put changes for supporting multiple sizes of reta in
>   ethdev into a single patch.

In order to allow usage of git bisect, compilation must not be broken,
even inside a patchset.
So when refactoring an existing API, you must adapt the dependent code
in the same patch.
To make things easy to review, please try to change API incrementally
with good explanation of why each change is needed.

>  /* Definitions used for redirection table entry size */
> -#define ETH_RSS_RETA_NUM_ENTRIES 128
> -#define ETH_RSS_RETA_MAX_QUEUE   16
> +#define ETH_RSS_RETA_SIZE_64  64
> +#define ETH_RSS_RETA_SIZE_128 128
> +#define ETH_RSS_RETA_SIZE_512 512
> +
> +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))

Are these constants really needed?

>  /**
> - * A structure used to configure Redirection Table of  the Receive Side
> - * Scaling (RSS) feature of an Ethernet port.
> + * A structure used to configure 64 entries of Redirection Table of the
> + * Receive Side Scaling (RSS) feature of an Ethernet port. To configure
> + * more than 64 entries supported by hardware, an array of this structure
> + * is needed.
>   */

Explaining the array of 64 entries could be useful in commit log.
Please don't forget to answer the "why" question in commit logs.

Thanks
Helin Zhang Oct. 28, 2014, 12:33 a.m. UTC | #2
Hi Thomas

Sorry, I should have answer your comments together with my reworking the latest patch set.
Please see my answers for your comments! I really appreciate your comments!

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, October 22, 2014 4:54 AM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> redirection table
> 
> 2014-09-25 16:40, Helin Zhang:
> > To support possible different sizes of redirection table, structures
> > and functions need to be redefined. In detail,
> > * 'struct rte_eth_rss_reta' has been redefined.
> > * 'uint16_t reta_size' has been added into
> >   'struct rte_eth_dev_info'.
> > * Updating/querying reta have been reimplemented with one
> >   more parameter of redirection table size.
> >
> > v2 changes:
> > * Put changes for supporting multiple sizes of reta in
> >   ethdev into a single patch.
> 
> In order to allow usage of git bisect, compilation must not be broken, even inside
> a patchset.
> So when refactoring an existing API, you must adapt the dependent code in the
> same patch.
> To make things easy to review, please try to change API incrementally with good
> explanation of why each change is needed.
OK. The patch set has been reworked to get all patches compiled well.

> 
> >  /* Definitions used for redirection table entry size */ -#define
> > ETH_RSS_RETA_NUM_ENTRIES 128
> > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > +#define ETH_RSS_RETA_SIZE_64  64
> > +#define ETH_RSS_RETA_SIZE_128 128
> > +#define ETH_RSS_RETA_SIZE_512 512
> > +
> > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> 
> Are these constants really needed?
These constants were defined for the third input parameter of
rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End users need
to give the correct reta size listed as above, as other values is not valid. So it would be
better to list the valid reta sizes in macros here.

> 
> >  /**
> > - * A structure used to configure Redirection Table of  the Receive
> > Side
> > - * Scaling (RSS) feature of an Ethernet port.
> > + * A structure used to configure 64 entries of Redirection Table of
> > + the
> > + * Receive Side Scaling (RSS) feature of an Ethernet port. To
> > + configure
> > + * more than 64 entries supported by hardware, an array of this
> > + structure
> > + * is needed.
> >   */
> 
> Explaining the array of 64 entries could be useful in commit log.
> Please don't forget to answer the "why" question in commit logs.
OK. Rework for this has been done in the latest version of patch set.

> 
> Thanks
> --
> Thomas

Thanks,
Helin
Thomas Monjalon Oct. 28, 2014, 10:10 a.m. UTC | #3
2014-10-28 00:33, Zhang, Helin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-09-25 16:40, Helin Zhang:
> > >  /* Definitions used for redirection table entry size */
> > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > +#define ETH_RSS_RETA_SIZE_64  64
> > > +#define ETH_RSS_RETA_SIZE_128 128
> > > +#define ETH_RSS_RETA_SIZE_512 512
> > > +
> > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > 
> > Are these constants really needed?
> 
> These constants were defined for the third input parameter of
> rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End users need
> to give the correct reta size listed as above, as other values is not valid. So it would be
> better to list the valid reta sizes in macros here.

OK, so you should explain that only these values are allowed.
In general, it's something we explain in the comment of the function.

By the way, why only these values are allowed?
Bruce Richardson Oct. 28, 2014, 10:18 a.m. UTC | #4
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, October 28, 2014 10:10 AM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> redirection table
> 
> 2014-10-28 00:33, Zhang, Helin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-09-25 16:40, Helin Zhang:
> > > >  /* Definitions used for redirection table entry size */
> > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > +#define ETH_RSS_RETA_SIZE_64  64
> > > > +#define ETH_RSS_RETA_SIZE_128 128
> > > > +#define ETH_RSS_RETA_SIZE_512 512
> > > > +
> > > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > >
> > > Are these constants really needed?
> >
> > These constants were defined for the third input parameter of
> > rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End users
> need
> > to give the correct reta size listed as above, as other values is not valid. So it
> would be
> > better to list the valid reta sizes in macros here.
> 
If only limited range of values are allowed, would an enum work better than a set of macros? 

> OK, so you should explain that only these values are allowed.
> In general, it's something we explain in the comment of the function.
> 
> By the way, why only these values are allowed?
Helin Zhang Oct. 28, 2014, noon UTC | #5
Hi Thomas

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 28, 2014 6:10 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> redirection table
> 
> 2014-10-28 00:33, Zhang, Helin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-09-25 16:40, Helin Zhang:
> > > >  /* Definitions used for redirection table entry size */ -#define
> > > > ETH_RSS_RETA_NUM_ENTRIES 128
> > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > +#define ETH_RSS_RETA_SIZE_64  64
> > > > +#define ETH_RSS_RETA_SIZE_128 128 #define ETH_RSS_RETA_SIZE_512
> > > > +512
> > > > +
> > > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > >
> > > Are these constants really needed?
> >
> > These constants were defined for the third input parameter of
> > rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End
> > users need to give the correct reta size listed as above, as other
> > values is not valid. So it would be better to list the valid reta sizes in macros
> here.
> 
> OK, so you should explain that only these values are allowed.
> In general, it's something we explain in the comment of the function
It would be better to add comments for the functions.

> 
> By the way, why only these values are allowed?
It depends on hardware, 1G/10G hardware supports 128 reta size only, 40G
hardware supports 512 or 128 depends on hardware configuration, 40G VF
hardware supports 64. If more is introduced in the future, more values can be
added later. It will return with errors if reta size is not supported for specific hardware.

> 
> --
> Thomas

Regards,
Helin
Thomas Monjalon Oct. 28, 2014, 12:13 p.m. UTC | #6
2014-10-28 12:00, Zhang, Helin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-10-28 00:33, Zhang, Helin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2014-09-25 16:40, Helin Zhang:
> > > > >  /* Definitions used for redirection table entry size */
> > > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > > +#define ETH_RSS_RETA_SIZE_64  64
> > > > > +#define ETH_RSS_RETA_SIZE_128 128
> > > > > +#define ETH_RSS_RETA_SIZE_512 512
[...]
> > By the way, why only these values are allowed?
> 
> It depends on hardware, 1G/10G hardware supports 128 reta size only, 40G
> hardware supports 512 or 128 depends on hardware configuration, 40G VF
> hardware supports 64. If more is introduced in the future, more values can be
> added later. It will return with errors if reta size is not supported for specific hardware.

So maybe it should be the responsibility of the driver to use
the right value. This kind of hardware differences should be abstracted
for the application.
If the application need to know the size, a "get" function could
be provided.
Helin Zhang Oct. 28, 2014, 12:36 p.m. UTC | #7
Hi Thomas

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 28, 2014 8:13 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> redirection table
> 
> 2014-10-28 12:00, Zhang, Helin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-10-28 00:33, Zhang, Helin:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2014-09-25 16:40, Helin Zhang:
> > > > > >  /* Definitions used for redirection table entry size */
> > > > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > > > +#define ETH_RSS_RETA_SIZE_64  64 #define
> > > > > > +ETH_RSS_RETA_SIZE_128 128 #define ETH_RSS_RETA_SIZE_512 512
> [...]
> > > By the way, why only these values are allowed?
> >
> > It depends on hardware, 1G/10G hardware supports 128 reta size only,
> > 40G hardware supports 512 or 128 depends on hardware configuration,
> > 40G VF hardware supports 64. If more is introduced in the future, more
> > values can be added later. It will return with errors if reta size is not supported
> for specific hardware.
> 
> So maybe it should be the responsibility of the driver to use the right value. This
> kind of hardware differences should be abstracted for the application.
> If the application need to know the size, a "get" function could be provided.
Yes, the size can be gotten by ops of 'dev_infos_get'. But if end users know the
size of a specific port, he can just use these macros directly without getting it
from somewhere.

> 
> --
> Thomas

Regards,
Helin
Helin Zhang Oct. 28, 2014, 1:20 p.m. UTC | #8
Hi Bruce

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Tuesday, October 28, 2014 6:18 PM
> To: Thomas Monjalon; Zhang, Helin
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> redirection table
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Tuesday, October 28, 2014 10:10 AM
> > To: Zhang, Helin
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple
> > sizes of redirection table
> >
> > 2014-10-28 00:33, Zhang, Helin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2014-09-25 16:40, Helin Zhang:
> > > > >  /* Definitions used for redirection table entry size */
> > > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > > +#define ETH_RSS_RETA_SIZE_64  64 #define ETH_RSS_RETA_SIZE_128
> > > > > +128 #define ETH_RSS_RETA_SIZE_512 512
> > > > > +
> > > > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > > >
> > > > Are these constants really needed?
> > >
> > > These constants were defined for the third input parameter of
> > > rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End
> > > users
> > need
> > > to give the correct reta size listed as above, as other values is
> > > not valid. So it
> > would be
> > > better to list the valid reta sizes in macros here.
> >
> If only limited range of values are allowed, would an enum work better than a set
> of macros?
Good idea! Any other comments for this from other guys?

> 
> > OK, so you should explain that only these values are allowed.
> > In general, it's something we explain in the comment of the function.
> >
> > By the way, why only these values are allowed?

Regards,
Helin
Thomas Monjalon Oct. 28, 2014, 2:22 p.m. UTC | #9
2014-10-28 13:20, Zhang, Helin:
> From: Richardson, Bruce
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > 2014-10-28 00:33, Zhang, Helin:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2014-09-25 16:40, Helin Zhang:
> > > > > >  /* Definitions used for redirection table entry size */
> > > > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > > > +#define ETH_RSS_RETA_SIZE_64  64
> > > > > > +#define ETH_RSS_RETA_SIZE_128 128
> > > > > > +#define ETH_RSS_RETA_SIZE_512 512
> > > > > > +
> > > > > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > > > >
> > > > > Are these constants really needed?
> > > >
> > > > These constants were defined for the third input parameter of
> > > > rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End
> > > > users need
> > > > to give the correct reta size listed as above, as other values is
> > > > not valid. So it would be
> > > > better to list the valid reta sizes in macros here.
> > >
> > If only limited range of values are allowed, would an enum work better than a set
> > of macros?
> 
> Good idea! Any other comments for this from other guys?

I would prefer the API to be independent of the hardware capabilities.
Helin Zhang Oct. 29, 2014, 8:18 a.m. UTC | #10
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, October 28, 2014 10:23 PM
> To: Zhang, Helin
> Cc: Richardson, Bruce; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> redirection table
> 
> 2014-10-28 13:20, Zhang, Helin:
> > From: Richardson, Bruce
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > > 2014-10-28 00:33, Zhang, Helin:
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2014-09-25 16:40, Helin Zhang:
> > > > > > >  /* Definitions used for redirection table entry size */
> > > > > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > > > > +#define ETH_RSS_RETA_SIZE_64  64 #define
> > > > > > > +ETH_RSS_RETA_SIZE_128 128 #define ETH_RSS_RETA_SIZE_512 512
> > > > > > > +
> > > > > > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > > > > >
> > > > > > Are these constants really needed?
> > > > >
> > > > > These constants were defined for the third input parameter of
> > > > > rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query().
> > > > > End users need to give the correct reta size listed as above, as
> > > > > other values is not valid. So it would be better to list the
> > > > > valid reta sizes in macros here.
> > > >
> > > If only limited range of values are allowed, would an enum work
> > > better than a set of macros?
> >
> > Good idea! Any other comments for this from other guys?
> 
> I would prefer the API to be independent of the hardware capabilities.
Let keep it as it is, and add more possible comments somewhere.

> 
> --
> Thomas

Regards,
Helin
Helin Zhang Oct. 29, 2014, 8:24 a.m. UTC | #11
> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, October 28, 2014 8:01 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> redirection table
> 
> Hi Thomas
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Tuesday, October 28, 2014 6:10 PM
> > To: Zhang, Helin
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple
> > sizes of redirection table
> >
> > 2014-10-28 00:33, Zhang, Helin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2014-09-25 16:40, Helin Zhang:
> > > > >  /* Definitions used for redirection table entry size */
> > > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > > +#define ETH_RSS_RETA_SIZE_64  64 #define ETH_RSS_RETA_SIZE_128
> > > > > +128 #define ETH_RSS_RETA_SIZE_512
> > > > > +512
> > > > > +
> > > > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > > >
> > > > Are these constants really needed?
> > >
> > > These constants were defined for the third input parameter of
> > > rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End
> > > users need to give the correct reta size listed as above, as other
> > > values is not valid. So it would be better to list the valid reta
> > > sizes in macros
> > here.
> >
> > OK, so you should explain that only these values are allowed.
> > In general, it's something we explain in the comment of the function
> It would be better to add comments for the functions.
Now do not think explain what value is allowed for the functions in ethdev layer,
as it might be hardware specific.
I think the best way is to comment out end users can get the reta size by
'dev_infos_get' on each port, rather than telling the values directly.

> 
> >
> > By the way, why only these values are allowed?
> It depends on hardware, 1G/10G hardware supports 128 reta size only, 40G
> hardware supports 512 or 128 depends on hardware configuration, 40G VF
> hardware supports 64. If more is introduced in the future, more values can be
> added later. It will return with errors if reta size is not supported for specific
> hardware.
> 
> >
> > --
> > Thomas
> 
> Regards,
> Helin

Regards,
Helin
Thomas Monjalon Oct. 29, 2014, 10 a.m. UTC | #12
2014-10-29 08:24, Zhang, Helin:
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Tuesday, October 28, 2014 8:01 PM
> > To: Thomas Monjalon
> > Cc: dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple sizes of
> > redirection table
> > 
> > Hi Thomas
> > 
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Tuesday, October 28, 2014 6:10 PM
> > > To: Zhang, Helin
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 04/13] ethdev: support of multiple
> > > sizes of redirection table
> > >
> > > 2014-10-28 00:33, Zhang, Helin:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2014-09-25 16:40, Helin Zhang:
> > > > > >  /* Definitions used for redirection table entry size */
> > > > > > -#define ETH_RSS_RETA_NUM_ENTRIES 128
> > > > > > -#define ETH_RSS_RETA_MAX_QUEUE   16
> > > > > > +#define ETH_RSS_RETA_SIZE_64  64 #define ETH_RSS_RETA_SIZE_128
> > > > > > +128 #define ETH_RSS_RETA_SIZE_512
> > > > > > +512
> > > > > > +
> > > > > > +#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
> > > > >
> > > > > Are these constants really needed?
> > > >
> > > > These constants were defined for the third input parameter of
> > > > rte_eth_dev_rss_reta_update() and rte_eth_dev_rss_reta_query(). End
> > > > users need to give the correct reta size listed as above, as other
> > > > values is not valid. So it would be better to list the valid reta
> > > > sizes in macros
> > > here.
> > >
> > > OK, so you should explain that only these values are allowed.
> > > In general, it's something we explain in the comment of the function
> > It would be better to add comments for the functions.
> Now do not think explain what value is allowed for the functions in ethdev layer,
> as it might be hardware specific.
> I think the best way is to comment out end users can get the reta size by
> 'dev_infos_get' on each port, rather than telling the values directly.

Yes, it's better.

Thanks
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index b71b679..8c1cb25 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1917,78 +1917,104 @@  rte_eth_dev_priority_flow_ctrl_set(uint8_t port_id, struct rte_eth_pfc_conf *pfc
 	return (-ENOTSUP);
 }
 
-int
-rte_eth_dev_rss_reta_update(uint8_t port_id, struct rte_eth_rss_reta *reta_conf)
+static inline int
+rte_eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
+			uint16_t reta_size)
 {
-	struct rte_eth_dev *dev;
-	uint16_t max_rxq;
-	uint8_t i,j;
+	uint16_t i, num = reta_size / RTE_BIT_WIDTH_64;
 
-	if (port_id >= nb_ports) {
-		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
-		return (-ENODEV);
-	}
+	if (!reta_conf)
+		return -EINVAL;
 
-	/* Invalid mask bit(s) setting */
-	if ((reta_conf->mask_lo == 0) && (reta_conf->mask_hi == 0)) {
-		PMD_DEBUG_TRACE("Invalid update mask bits for port=%d\n",port_id);
-		return (-EINVAL);
+	for (i = 0; i < num; i++) {
+		if (reta_conf[i].mask)
+			return 0;
 	}
 
-	dev = &rte_eth_devices[port_id];
-	max_rxq = (dev->data->nb_rx_queues <= ETH_RSS_RETA_MAX_QUEUE) ?
-		dev->data->nb_rx_queues : ETH_RSS_RETA_MAX_QUEUE;
-	if (reta_conf->mask_lo != 0) {
-		for (i = 0; i < ETH_RSS_RETA_NUM_ENTRIES/2; i++) {
-			if ((reta_conf->mask_lo & (1ULL << i)) &&
-				(reta_conf->reta[i] >= max_rxq)) {
-				PMD_DEBUG_TRACE("RETA hash index output"
-					"configration for port=%d,invalid"
-					"queue=%d\n",port_id,reta_conf->reta[i]);
+	return -EINVAL;
+}
 
-				return (-EINVAL);
-			}
+static inline int
+rte_eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
+			 uint16_t reta_size,
+			 uint8_t max_rxq)
+{
+	uint16_t i, idx, shift;
+
+	if (!reta_conf)
+		return -EINVAL;
+
+	if (max_rxq == 0) {
+		PMD_DEBUG_TRACE("No receive queue is available\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < reta_size; i++) {
+		idx = i / RTE_BIT_WIDTH_64;
+		shift = i % RTE_BIT_WIDTH_64;
+		if ((reta_conf[idx].mask & (0x1 << shift)) &&
+			(reta_conf[idx].reta[shift] >= max_rxq)) {
+			PMD_DEBUG_TRACE("reta_conf[%u]->reta[%u]: %u exceeds "
+				"the maximum rxq index: %u\n", idx, shift,
+				reta_conf[idx].reta[shift], max_rxq);
+			return -EINVAL;
 		}
 	}
 
-	if (reta_conf->mask_hi != 0) {
-		for (i = 0; i< ETH_RSS_RETA_NUM_ENTRIES/2; i++) {
-			j = (uint8_t)(i + ETH_RSS_RETA_NUM_ENTRIES/2);
+	return 0;
+}
 
-			/* Check if the max entry >= 128 */
-			if ((reta_conf->mask_hi & (1ULL << i)) &&
-				(reta_conf->reta[j] >= max_rxq)) {
-				PMD_DEBUG_TRACE("RETA hash index output"
-					"configration for port=%d,invalid"
-					"queue=%d\n",port_id,reta_conf->reta[j]);
+int
+rte_eth_dev_rss_reta_update(uint8_t port_id,
+			    struct rte_eth_rss_reta_entry64 *reta_conf,
+			    uint16_t reta_size)
+{
+	struct rte_eth_dev *dev;
+	int ret;
 
-				return (-EINVAL);
-			}
-		}
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
 	}
 
+	/* Check mask bits */
+	ret = rte_eth_check_reta_mask(reta_conf, reta_size);
+	if (ret < 0)
+		return ret;
+
+	dev = &rte_eth_devices[port_id];
+
+	/* Check entry value */
+	ret = rte_eth_check_reta_entry(reta_conf, reta_size,
+				dev->data->nb_rx_queues);
+	if (ret < 0)
+		return ret;
+
 	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->reta_update, -ENOTSUP);
-	return (*dev->dev_ops->reta_update)(dev, reta_conf);
+	return (*dev->dev_ops->reta_update)(dev, reta_conf, reta_size);
 }
 
 int
-rte_eth_dev_rss_reta_query(uint8_t port_id, struct rte_eth_rss_reta *reta_conf)
+rte_eth_dev_rss_reta_query(uint8_t port_id,
+			   struct rte_eth_rss_reta_entry64 *reta_conf,
+			   uint16_t reta_size)
 {
 	struct rte_eth_dev *dev;
+	int ret;
 
 	if (port_id >= nb_ports) {
 		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
-		return (-ENODEV);
+		return -ENODEV;
 	}
 
-	if((reta_conf->mask_lo == 0) && (reta_conf->mask_hi == 0)) {
-		PMD_DEBUG_TRACE("Invalid update mask bits for the port=%d\n",port_id);
-		return (-EINVAL);
-	}
+	/* Check mask bits */
+	ret = rte_eth_check_reta_mask(reta_conf, reta_size);
+	if (ret < 0)
+		return ret;
 
 	dev = &rte_eth_devices[port_id];
 	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->reta_query, -ENOTSUP);
-	return (*dev->dev_ops->reta_query)(dev, reta_conf);
+	return (*dev->dev_ops->reta_query)(dev, reta_conf, reta_size);
 }
 
 int
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2c5ab13..9ed4437 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -426,8 +426,11 @@  struct rte_eth_rss_conf {
 		ETH_RSS_L2_PAYLOAD)
 
 /* Definitions used for redirection table entry size */
-#define ETH_RSS_RETA_NUM_ENTRIES 128
-#define ETH_RSS_RETA_MAX_QUEUE   16
+#define ETH_RSS_RETA_SIZE_64  64
+#define ETH_RSS_RETA_SIZE_128 128
+#define ETH_RSS_RETA_SIZE_512 512
+
+#define RTE_BIT_WIDTH_64 (CHAR_BIT * sizeof(uint64_t))
 
 /* Definitions used for VMDQ and DCB functionality */
 #define ETH_VMDQ_MAX_VLAN_FILTERS   64 /**< Maximum nb. of VMDQ vlan filters. */
@@ -491,15 +494,15 @@  struct rte_eth_vmdq_mirror_conf {
 };
 
 /**
- * A structure used to configure Redirection Table of  the Receive Side
- * Scaling (RSS) feature of an Ethernet port.
+ * A structure used to configure 64 entries of Redirection Table of the
+ * Receive Side Scaling (RSS) feature of an Ethernet port. To configure
+ * more than 64 entries supported by hardware, an array of this structure
+ * is needed.
  */
-struct rte_eth_rss_reta {
-	/** First 64 mask bits indicate which entry(s) need to updated/queried. */
-	uint64_t mask_lo;
-	/** Second 64 mask bits indicate which entry(s) need to updated/queried. */
-	uint64_t mask_hi;
-	uint8_t reta[ETH_RSS_RETA_NUM_ENTRIES];  /**< 128 RETA entries*/
+struct rte_eth_rss_reta_entry64 {
+	uint64_t mask;
+	/**< Mask bits indicate which entries need to be updated/queried. */
+	uint8_t reta[RTE_BIT_WIDTH_64]; /**< 64 redirection table entries. */
 };
 
 /**
@@ -906,6 +909,8 @@  struct rte_eth_dev_info {
 	uint16_t max_vmdq_pools; /**< Maximum number of VMDq pools. */
 	uint32_t rx_offload_capa; /**< Device RX offload capabilities. */
 	uint32_t tx_offload_capa; /**< Device TX offload capabilities. */
+	uint16_t reta_size;
+	/**< Device redirection table size, the total number of entries. */
 };
 
 /** Maximum name length for extended statistics counters */
@@ -1180,11 +1185,13 @@  typedef int (*priority_flow_ctrl_set_t)(struct rte_eth_dev *dev,
 /**< @internal Setup priority flow control parameter on an Ethernet device */
 
 typedef int (*reta_update_t)(struct rte_eth_dev *dev,
-				struct rte_eth_rss_reta *reta_conf);
+			     struct rte_eth_rss_reta_entry64 *reta_conf,
+			     uint16_t reta_size);
 /**< @internal Update RSS redirection table on an Ethernet device */
 
 typedef int (*reta_query_t)(struct rte_eth_dev *dev,
-				struct rte_eth_rss_reta *reta_conf);
+			    struct rte_eth_rss_reta_entry64 *reta_conf,
+			    uint16_t reta_size);
 /**< @internal Query RSS redirection table on an Ethernet device */
 
 typedef int (*rss_hash_update_t)(struct rte_eth_dev *dev,
@@ -2896,14 +2903,17 @@  int rte_eth_dev_mac_addr_remove(uint8_t port, struct ether_addr *mac_addr);
  * @param port
  *   The port identifier of the Ethernet device.
  * @param reta_conf
- *    RETA to update.
+ *   RETA to update.
+ * @param reta_size
+ *   Redirection table size.
  * @return
  *   - (0) if successful.
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_dev_rss_reta_update(uint8_t port,
-			struct rte_eth_rss_reta *reta_conf);
+				struct rte_eth_rss_reta_entry64 *reta_conf,
+				uint16_t reta_size);
 
  /**
  * Query Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
@@ -2912,13 +2922,16 @@  int rte_eth_dev_rss_reta_update(uint8_t port,
  *   The port identifier of the Ethernet device.
  * @param reta_conf
  *   RETA to query.
+ * @param reta_size
+ *   Redirection table size.
  * @return
  *   - (0) if successful.
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_dev_rss_reta_query(uint8_t port,
-			struct rte_eth_rss_reta *reta_conf);
+			       struct rte_eth_rss_reta_entry64 *reta_conf,
+			       uint16_t reta_size);
 
  /**
  * Updates unicast hash table for receiving packet with the given destination