Re: [PATCH v4 1/7] ethdev: recomment some definitions related to RSS

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/8/2023 9:00 AM, Jie Hai wrote:
> 1. Recomment fields of 'rte_eth_rss_conf'.
> 2. Add comments for RTE_ETH_HASH_FUNCTION_DEFAULT.
> 
> Signed-off-by: Jie Hai <haijie1@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.h | 28 +++++++++++++---------------
>  lib/ethdev/rte_flow.h   |  4 ++++
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 04a2564f222a..40cfbb7efddd 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -448,24 +448,22 @@ struct rte_vlan_filter_conf {
>  /**
>   * A structure used to configure the Receive Side Scaling (RSS) feature
>   * of an Ethernet port.
> - * If not NULL, the *rss_key* pointer of the *rss_conf* structure points
> - * to an array holding the RSS key to use for hashing specific header
> - * fields of received packets. The length of this array should be indicated
> - * by *rss_key_len* below. Otherwise, a default random hash key is used by
> - * the device driver.
> - *
> - * The *rss_key_len* field of the *rss_conf* structure indicates the length
> - * in bytes of the array pointed by *rss_key*. To be compatible, this length
> - * will be checked in i40e only. Others assume 40 bytes to be used as before.
> - *
> - * The *rss_hf* field of the *rss_conf* structure indicates the different
> - * types of IPv4/IPv6 packets to which the RSS hashing must be applied.
> - * Supplying an *rss_hf* equal to zero disables the RSS feature.
>   */
>  struct rte_eth_rss_conf {
> -	uint8_t *rss_key;    /**< If not NULL, 40-byte hash key. */
> +	/**
> +	 * If not NULL, 40-byte hash key to use for hashing specific header
> +	 * fields of received packets. The size of rss_key should be indicated
> +	 * by *rss_key_len* below.
>

It says key length is 40 bytes, also says key length is defined by
'rss_key_len'. I wonder how can we clarify this more, what about like:

If not NULL, hash key to use for hashing.
The size of rss_key should be indicated by *rss_key_len* below. Drivers
are free to ignore the *rss_key_len* and assume key length is 40 bytes.


> +	 * Otherwise, a default random hash key is used by the device driver.
> +	 */
> +	uint8_t *rss_key;
>  	uint8_t rss_key_len; /**< hash key length in bytes. */
> -	uint64_t rss_hf;     /**< Hash functions to apply - see below. */
> +	/**
> +	 * The different types of packets to which the RSS hashing must be
> +	 * applied, may be combined with SRC/DST_ONLY, see below.

I find this difficult to understand, can we simplify it?

The name 'hf' (hash function) is confusing on its own, but it is too
much burden to update it, at least we can clarify it with the above
documentation.

rte_flow defines it as 'types' (rte_flow_action_rss.type)
(and it has 'func' for "hash algorithm" to increase confusion).

Can we define something like (please don't use as it is, just thinking):
"
Set part of the packet that hashing will be applied for RSS purposes
(see RTE_ETH_RSS_*).
"

> +	 * Supplying an *rss_hf* equal to zero disables the RSS feature.
> +	 */

s/Supply/Set/ ?
"Setting *rss_hf* to zero disables the RSS feature."

> +	uint64_t rss_hf;
>  };
>  
>  /*
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 2ebb76dbc083..3bd0dc64fbe2 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3187,6 +3187,10 @@ struct rte_flow_query_count {
>   * Hash function types.
>   */
>  enum rte_eth_hash_function {
> +	/**
> +	 * DEFAULT means that conformance to a specific hash algorithm is
> +	 * uncared to the caller. The driver can pick the one it deems optimal.

Not sure about word 'uncared' usage here, what about:

"
DEFAULT means driver decides which hash algorithm to pick.
"