On Wed, Aug 30, 2023 at 4:46 AM Thomas Monjalon <tho...@monjalon.net> wrote:
>
> Hello,
>
> Thanks for bringing a new capability.
>
> 26/08/2023 09:46, Jie Hai:
> > Currently, rte_eth_rss_conf supports configuring and querying
> > rss hash functions, rss key and it's length, but not rss hash
> > algorithm.
> >
> > The structure ``rte_eth_rss_conf`` is extended by adding a new
> > field "func". This represents the RSS algorithms to apply. The
> > following API is affected:
> > - rte_eth_dev_configure
> > - rte_eth_dev_rss_hash_update
> > - rte_eth_dev_rss_hash_conf_get
>
> So far, the RSS algorithm was used only in flow RSS API.
>
> > --- a/doc/guides/rel_notes/release_23_11.rst
> > +++ b/doc/guides/rel_notes/release_23_11.rst
> > @@ -123,6 +123,8 @@ ABI Changes
> > Also, make sure to start the actual text at the margin.
> > =======================================================
> >
> > + * ethdev: Added "func" field to ``rte_eth_rss_conf`` structure for RSS
> > hash
> > + algorithm.
>
> As written above, it should start at the margin.
>
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > +#include "rte_flow.h"
>
> It is strange to include rte_flow.h here.
> It would be better to move the enum.
>
> > + * The *func* field of the *rss_conf* structure indicates the hash
> > algorithm
> > + * applied by the RSS hashing. Passing RTE_ETH_HASH_FUNCTION_DEFAULT allows
> > + * the PMD to use its best-effort algorithm rather than a specific one.
> > */
>
> I don't like commenting a field on top of the structure.
> By the way, the first sentence does not look helpful.
> RTE_ETH_HASH_FUNCTION_DEFAULT may be explained in the enum.
>
> > struct rte_eth_rss_conf {
> > uint8_t *rss_key; /**< If not NULL, 40-byte hash key. */
> > uint8_t rss_key_len; /**< hash key length in bytes. */
> > uint64_t rss_hf; /**< Hash functions to apply - see below. */
> > + enum rte_eth_hash_function func; /**< Hash algorithm to apply.
> > */
>
> You can drop "to apply" words.
>
> How the algorithms support combinations in rss_hf?
I will spend a little more time on this tomorrow.
Can you update testpmd also to display the info as a part of show rss.> >
smime.p7s
Description: S/MIME Cryptographic Signature
