[PATCH v5 02/40] ethdev: support setting and querying RSS algorithm

[Jie Hai]

On 2023/10/12 10:21, fengchengwen wrote:
> 
> 
> On 2023/10/12 1:39, Stephen Hemminger wrote:
>> On Wed, 11 Oct 2023 17:27:27 +0800
>> Jie Hai <haijie1 at huawei.com> 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 "algorithm". 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
>>>
>>> If the value of "algorithm" 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 valid "algorithm", it is set
>>> to default value before querying.
>>>
>>> Signed-off-by: Jie Hai <haijie1 at huawei.com>
>>> Signed-off-by: Dongdong Liu <liudongdong3 at huawei.com>
>>> ---
>>>   doc/guides/rel_notes/release_23_11.rst |  2 ++
>>>   lib/ethdev/rte_ethdev.c                | 17 ++++++++++++++++
>>>   lib/ethdev/rte_ethdev.h                | 27 +++++++++++++++++++++++++
>>>   lib/ethdev/rte_flow.c                  |  1 -
>>>   lib/ethdev/rte_flow.h                  | 28 ++------------------------
>>>   5 files changed, 48 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/release_23_11.rst b/doc/guides/rel_notes/release_23_11.rst
>>> index e13d57728071..92a445ab2ed3 100644
>>> --- a/doc/guides/rel_notes/release_23_11.rst
>>> +++ b/doc/guides/rel_notes/release_23_11.rst
>>> @@ -197,6 +197,8 @@ ABI Changes
>>>     fields, to move ``rxq`` and ``txq`` fields, to change the size of
>>>     ``reserved1`` and ``reserved2`` fields.
>>>   
>>> +* ethdev: Added "algorithm" 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 18a4b950b184..2eda1b8072e5 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -1464,6 +1464,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.algorithm >= RTE_ETH_HASH_FUNCTION_MAX) {
>>> +		RTE_ETHDEV_LOG(ERR,
>>> +			"Ethdev port_id=%u invalid RSS algorithm: 0x%"PRIx64"\n",
>>> +			port_id, dev_conf->rx_adv_conf.rss_conf.algorithm);
>>> +		ret = -EINVAL;
>>> +		goto rollback;
>>> +	}
>>> +
>>
>> Rather than having every driver check the algorithm, why not handle this like
>> other features in DPDK (which may mean API/ABI changes).
>>
>> Add a field rss_algo_capa which is bit field of available RSS functions.
>> Then the check for algorithm can be done in generic code. There a couple
>> of reserved fields that could be used.
> 
> +1 for add a field
> 
> But there are two ways to config rss: ethdev-ops, ethdev-rteflow-ops, should distinguish them ? or just define for ethdev-ops ?
> 
The rte_flow API does not distinguish RSS rule and FDIR rule,
the actual implementation of rss configuration depends on drivers.
I think if we had this "rss_algo_capa ", it can be just for ethdev-ops 
for ethdev API level use.
>>
>> It would mean updating all the drivers once with the capa field but
>> would provide way for application to know what fields are possible.
>>
>> It has proved to be a problem in later ABI changes if a maximum value
>> is exposed. I.e don't expose RTE_ETH_HASH_FUNCTION_MAX.
> 
> +1
> 
>>
>> .
>>
> .