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

[Zhang, Helin]

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 at 6wind.com]
> Sent: Wednesday, October 22, 2014 4:54 AM
> To: Zhang, Helin
> Cc: dev at 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