Re: [PATCH v4 2/7] ethdev: support setting and querying RSS algorithm

[Jie Hai <haijie1@huawei.com>]

On 2023/9/21 0:39, Ferruh Yigit wrote:
> On 9/8/2023 9:00 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_rss_conf`` is extended by adding a new
>> field "func". This represents the RSS algorithms to apply. The
>> following API will be affected:
>> 	- rte_eth_dev_configure
>> 	- rte_eth_dev_rss_hash_update
>> 	- rte_eth_dev_rss_hash_conf_get
>>
> 
> The problem with adding new configuration fields is, none of the drivers
> are using it.
> 
> I can see you are updating hns3 driver in next patch, but what about
> others, application will set this field and almost all drivers will
> ignore it for now.
> And in near future, some will be supporting it and some won't, still
> frustrating for the application perspective.
> 
> In the past I was gathering list of these items and follow
> implementation of them with drivers, but as the number of drivers
> increased this became very hard to manage.
> 
> Another way to manage this is go and update all drivers, and if the
> configuration requests anything other than
> 'RTE_ETH_HASH_FUNCTION_DEFAULT', return error. So that application can
> know this is not supported by this driver.
> Do you have better solution for this?
> 
I know this may not be reasonable, how about adding new ops to
the query and configuration of the RSS algorithm?
> 
>> If the value of "func" used for configuration is a gibberish
>> value, report the error and return. Do the same for
>> rte_eth_dev_rss_hash_update and rte_eth_dev_configure.
>>
>> To check whether the drivers report the "func" field, it is set
>> to default value before querying.
>>
>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>>   doc/guides/rel_notes/release_23_11.rst |  2 ++
>>   lib/ethdev/rte_ethdev.c                | 17 +++++++++++++++++
>>   lib/ethdev/rte_ethdev.h                | 21 +++++++++++++++++++++
>>   lib/ethdev/rte_flow.c                  |  1 -
>>   lib/ethdev/rte_flow.h                  | 22 ++--------------------
>>   5 files changed, 42 insertions(+), 21 deletions(-)
>>
>> diff --git a/doc/guides/rel_notes/release_23_11.rst b/doc/guides/rel_notes/release_23_11.rst
>> index 333e1d95a283..deb019fbe4c1 100644
>> --- a/doc/guides/rel_notes/release_23_11.rst
>> +++ b/doc/guides/rel_notes/release_23_11.rst
>> @@ -129,6 +129,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.
>>   
>>   Known Issues
>>   ------------
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 0840d2b5942a..4cbcdb344cac 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -1445,6 +1445,14 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>   		goto rollback;
>>   	}
>>   
>> +	if (dev_conf->rx_adv_conf.rss_conf.func >= RTE_ETH_HASH_FUNCTION_MAX) {
>> +		RTE_ETHDEV_LOG(ERR,
>> +			"Ethdev port_id=%u invalid rss hash function (%u)\n",
>> +			port_id, dev_conf->rx_adv_conf.rss_conf.func);
>> +		ret = -EINVAL;
>> +		goto rollback;
>> +	}
>> +
>>   	/* Check if Rx RSS distribution is disabled but RSS hash is enabled. */
>>   	if (((dev_conf->rxmode.mq_mode & RTE_ETH_MQ_RX_RSS_FLAG) == 0) &&
>>   	    (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_RSS_HASH)) {
>> @@ -4630,6 +4638,13 @@ rte_eth_dev_rss_hash_update(uint16_t port_id,
>>   		return -ENOTSUP;
>>   	}
>>   
>> +	if (rss_conf->func >= RTE_ETH_HASH_FUNCTION_MAX) {
>> +		RTE_ETHDEV_LOG(ERR,
>> +			"Ethdev port_id=%u invalid rss hash function (%u)\n",
>> +			port_id, rss_conf->func);
>> +		return -EINVAL;
>> +	}
>> +
>>   	if (*dev->dev_ops->rss_hash_update == NULL)
>>   		return -ENOTSUP;
>>   	ret = eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
>> @@ -4657,6 +4672,8 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>>   		return -EINVAL;
>>   	}
>>   
>> +	rss_conf->func = RTE_ETH_HASH_FUNCTION_DEFAULT;
>> +
>>
> 
> I think setting this in ethdev level is not required if you update all
> drivers to not accept anything other than DEFAULT.
> 
> 
I initialized this field to prevent some drivers from not reporting RSS 
algorithms.
If an improper parameter is transferred before an API is invoked, the 
query result will be incorrect.
>>   	if (*dev->dev_ops->rss_hash_conf_get == NULL)
>>   		return -ENOTSUP;
>>   	ret = eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 40cfbb7efddd..33cec3570f85 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -445,6 +445,26 @@ struct rte_vlan_filter_conf {
>>   	uint64_t ids[64];
>>   };
>>   
>> +/**
>> + * 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.
>> +	 */
>> +	RTE_ETH_HASH_FUNCTION_DEFAULT = 0,
>> +	RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */
>> +	RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */
>> +	/**
>> +	 * Symmetric Toeplitz: src, dst will be replaced by
>> +	 * xor(src, dst). For the case with src/dst only,
>> +	 * src or dst address will xor with zero pair.
>> +	 */
>> +	RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ,
>> +	RTE_ETH_HASH_FUNCTION_MAX,
> 
> As we are moving this to ethdev.h, when we want to add a new algorithm,
> it will cause ABI break because of the 'RTE_ETH_HASH_FUNCTION_MAX', and
> it will need to wait. Is there a way to eliminate this MAX enum?
> 
How about assigning RTE_ETH_HASH_FUNCTION_MAX a larger value, e.g. 255 ?
>> +};
>> +
>>   /**
>>    * A structure used to configure the Receive Side Scaling (RSS) feature
>>    * of an Ethernet port.
>> @@ -464,6 +484,7 @@ struct rte_eth_rss_conf {
>>   	 * Supplying an *rss_hf* equal to zero disables the RSS feature.
>>   	 */
>>   	uint64_t rss_hf;
>> +	enum rte_eth_hash_function func;	/**< Hash algorithm. */
> 
> Can we use 'algorithm' as the field name?
> 
> I know it won't exactly match type name (rte_eth_hash_function) but I
> think this will be more accurate also less confusing for "rss_hf (rss
> hash function)" and "func (function)"?
> 
Considering the comprehensibility, I agree with the proposal.

>>   };
>>   
>>   /*
>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
>> index 271d854f7807..d3f2d466d841 100644
>> --- a/lib/ethdev/rte_flow.c
>> +++ b/lib/ethdev/rte_flow.c
>> @@ -12,7 +12,6 @@
>>   #include <rte_branch_prediction.h>
>>   #include <rte_string_fns.h>
>>   #include <rte_mbuf_dyn.h>
>> -#include "rte_ethdev.h"
>>   #include "rte_flow_driver.h"
>>   #include "rte_flow.h"
>>   
>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>> index 3bd0dc64fbe2..a7578b1d2eff 100644
>> --- a/lib/ethdev/rte_flow.h
>> +++ b/lib/ethdev/rte_flow.h
>> @@ -40,6 +40,8 @@
>>   #include <rte_macsec.h>
>>   #include <rte_ib.h>
>>   
>> +#include "rte_ethdev.h"
>> +
>>   #ifdef __cplusplus
>>   extern "C" {
>>   #endif
>> @@ -3183,26 +3185,6 @@ struct rte_flow_query_count {
>>   	uint64_t bytes; /**< Number of bytes through this rule [out]. */
>>   };
>>   
>> -/**
>> - * 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.
>> -	 */
>> -	RTE_ETH_HASH_FUNCTION_DEFAULT = 0,
>> -	RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */
>> -	RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */
>> -	/**
>> -	 * Symmetric Toeplitz: src, dst will be replaced by
>> -	 * xor(src, dst). For the case with src/dst only,
>> -	 * src or dst address will xor with zero pair.
>> -	 */
>> -	RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ,
>> -	RTE_ETH_HASH_FUNCTION_MAX,
>> -};
>> -
>>   /**
>>    * RTE_FLOW_ACTION_TYPE_RSS
>>    *
> 
> .