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

[Jie Hai <haijie1@huawei.com>]

Hi, Thomas

Thanks for your review.

On 2023/8/30 19:46, Thomas Monjalon 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.
> 
Fixed in V3, these API will be affected.
>> --- 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.
> 
Fixed in V3.
>> --- 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.
> 
Fixed in V3, the definations are removed to rte_ethdev.h.
>> + * 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.
> 
Other fields  above the structure 'rte_eth_rss_conf' have comments.
If the new fields 'func' do not have comments, it may be misleading.
Unless all the comments above are removed. I'm not sure whether to
delete them or not.
>>   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.
Fixed in V3.
> 
> How the algorithms support combinations in rss_hf?
> 
The rss_hf defines the input of the RSS hash algorithms.
For example, rss_hf = RTE_ETH_RSS_NONFRAG_IPV4_TCP | RTE_ETH_RSS_IPV4, 
these two kinds of packets can be dispatched to different queues 
according to their tuple field value.
For ipv4-tcp packets, src-ip, dst-ip, src-port, dst-port are used as 
parameters of RSS hash algorithms to compute hash value.
For ipv4 packets src-ip, dst-ip are used.

If rss_hf = RTE_ETH_RSS_NONFRAG_IPV4_TCP | RTE_ETH_RSS_L4_SRC_ONLY, for
ipv4-tcp packets, src-port is used as parameters of RSS hash algorithms 
to compute hash value.

> 
> .

Thanks,
Jie Hai