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/9/4 15:45, Thomas Monjalon wrote:
> 04/09/2023 09:10, Jie Hai:
>> On 2023/8/30 19:46, Thomas Monjalon wrote:
>>> 26/08/2023 09:46, Jie Hai:
>   >> + * 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.
> 
> You should explain RTE_ETH_HASH_FUNCTION_DEFAULT in its enum.
> The rest of the explanation can be on the struct field.
> I'm OK to have another patch moving old explanations from the struct
> to the fields.
Fixed in V4, please check it.
> 
>>>>    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.
> 
> I know what is rss_hf.
> My question is about the algorithms.
> Do they all support any combination in rss_hf?
> 
>
I don't know about all vendors' hardware implementations,
so here's just my simple opinion.

Theoretically, I think that all algorithms should support all 
combinations in rss_hf, The reasons are as follows.

1. The rss_hf and algorithms are independent. For different algorithms 
and the same packets and hash key, the input parameters for different 
algorithms should be the same, which depends on implemetation of hardware.

2. As long as hardware and driver support, all packet types defined by
rss_hf could generate the the input parameters for the algorithm to compute.

Thanks,
Jie Hai
> .