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

[Jie Hai <haijie1@huawei.com>]

On 2023/8/31 8:10, Ajit Khaparde wrote:
> On Wed, Aug 30, 2023 at 4:46 AM Thomas Monjalon <thomas@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.
> 
Hi, Ajit Khaparde,

Thanks for your advice. I will update testpmd in next version.

Thanks， Jie Hai
>>
>>