Re: [PATCH v2 1/5] ethdev: support setting and querying RSS algorithm

[Thomas Monjalon <thomas@monjalon.net>]

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?