[PATCH v8 03/10] ethdev: support setting and querying RSS algorithm

[Jie Hai]

On 2023/11/1 21:36, Ferruh Yigit wrote:
> On 11/1/2023 7:40 AM, Jie Hai wrote:
>> 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_dev_info`` is extended by adding a new
>> field "rss_algo_capa". Drivers are responsible for reporting this
>> capa and configurations of RSS hash algorithm can be verified based
>> on the capability. The default value of "rss_algo_capa" is
>> RTE_ETH_HASH_ALGO_CAPA_MASK(DEFAULT) if drivers do not report it.
>>
>> The structure ``rte_eth_rss_conf`` is extended by adding a new
>> field "algorithm". This represents the RSS algorithms to apply.
>> If the value of "algorithm" used for configuration is a gibberish
>> value, drivers should report the error.
>>
>> To check whether the drivers report valid "algorithm", it is set
>> to default value before querying in rte_eth_dev_rss_hash_conf_get().
>>
>> Signed-off-by: Jie Hai <haijie1 at huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3 at huawei.com>
>> Acked-by: Huisong Li <lihuisong at huawei.com>
>> ---
>>   doc/guides/rel_notes/release_23_11.rst |  5 +++++
>>   lib/ethdev/rte_ethdev.c                | 26 +++++++++++++++++++++++
>>   lib/ethdev/rte_ethdev.h                | 29 ++++++++++++++++++++++++++
>>   lib/ethdev/rte_flow.c                  |  1 -
>>   lib/ethdev/rte_flow.h                  | 26 ++---------------------
>>   5 files changed, 62 insertions(+), 25 deletions(-)
>>
>> diff --git a/doc/guides/rel_notes/release_23_11.rst b/doc/guides/rel_notes/release_23_11.rst
>> index 95db98d098d8..e207786044f9 100644
>> --- a/doc/guides/rel_notes/release_23_11.rst
>> +++ b/doc/guides/rel_notes/release_23_11.rst
>> @@ -372,6 +372,11 @@ ABI Changes
>>   * security: struct ``rte_security_ipsec_sa_options`` was updated
>>     due to inline out-of-place feature addition.
>>   
>> +* ethdev: Added "rss_algo_capa" field to ``rte_eth_dev_info`` structure for
>> +* reporting RSS hash algorithm capability.
>> +
>> +* ethdev: Added "algorithm" field to ``rte_eth_rss_conf`` structure for RSS
>> +  hash algorithm.
>>   
> 
> As well as ABI change, can you also update the "New Features", to
> document getting hash algorithm capability and setting hash algorithm
> support added?
> 
> Also please add an empty line here.
thanks，will add.
> 
>>   Known Issues
>>   ------------
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 07bb35833ba6..f9bd99d07eb1 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -1269,6 +1269,7 @@ int
>>   rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   		      const struct rte_eth_conf *dev_conf)
>>   {
>> +	enum rte_eth_hash_function algorithm;
>>   	struct rte_eth_dev *dev;
>>   	struct rte_eth_dev_info dev_info;
>>   	struct rte_eth_conf orig_conf;
>> @@ -1510,6 +1511,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   		goto rollback;
>>   	}
>>   
>> +	algorithm = dev_conf->rx_adv_conf.rss_conf.algorithm;
>> +	if (RTE_ETH_HASH_ALGO_TO_CAPA(algorithm) == 0 ||
>>
> 
> "RTE_ETH_HASH_ALGO_TO_CAPA(algorithm)" can't be zero for valid "enum
> rte_eth_hash_function" values, I assume above check is for the case
> algorith > 31, as it will result zero.
> My concern is, this is undefined behaviour (shift left >= 32) and some
> compiler can complain about it, instead of relying this can you please
> add explicit "0 <= algorithm < 32" check?
yes, how about associate with "rss_algo_capa"?

+       if (algorithm >= CHAR_BIT * sizeof(dev_info.rss_algo_capa) ||
+           (dev_info.rss_algo_capa & 
RTE_ETH_HASH_ALGO_TO_CAPA(algorithm)) == 0) {

> 
> 
> 
> .