diff mbox series

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

Message ID 20231011092805.693171-3-haijie1@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series support setting and querying RSS algorithms |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Jie Hai Oct. 11, 2023, 9:27 a.m. UTC 
  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@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                | 27 +++++++++++++++++++++++++
 lib/ethdev/rte_flow.c                  |  1 -
 lib/ethdev/rte_flow.h                  | 28 ++------------------------
 5 files changed, 48 insertions(+), 27 deletions(-)

Comments

Stephen Hemminger Oct. 11, 2023, 5:39 p.m. UTC | #1 
On Wed, 11 Oct 2023 17:27:27 +0800
Jie Hai <haijie1@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@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                | 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.

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.
fengchengwen Oct. 12, 2023, 2:21 a.m. UTC | #2 
On 2023/10/12 1:39, Stephen Hemminger wrote:
> On Wed, 11 Oct 2023 17:27:27 +0800
> Jie Hai <haijie1@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@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                | 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 ?

> 
> 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

> 
> .
>
Stephen Hemminger Oct. 12, 2023, 3:23 p.m. UTC | #3 
On Thu, 12 Oct 2023 10:21:51 +0800
fengchengwen <fengchengwen@huawei.com> wrote:

> > 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 ?

One field is likely to be enough.
I would expect that if driver accepts RSS for rte flow, it would also take same RSS setting for non flow case.
Jie Hai Oct. 24, 2023, 12:54 p.m. UTC | #4 
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@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@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                | 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
> 
>>
>> .
>>
> .
diff mbox series

Patch

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;
+	}
+
 	/* 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)) {
@@ -4673,6 +4681,13 @@  rte_eth_dev_rss_hash_update(uint16_t port_id,
 		return -ENOTSUP;
 	}
 
+	if (rss_conf->algorithm >= RTE_ETH_HASH_FUNCTION_MAX) {
+		RTE_ETHDEV_LOG(ERR,
+			"Ethdev port_id=%u invalid RSS algorithm: 0x%"PRIx64"\n",
+			port_id, rss_conf->algorithm);
+		return -EINVAL;
+	}
+
 	if (*dev->dev_ops->rss_hash_update == NULL)
 		return -ENOTSUP;
 	ret = eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
@@ -4700,6 +4715,8 @@  rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
 		return -EINVAL;
 	}
 
+	rss_conf->algorithm = RTE_ETH_HASH_FUNCTION_DEFAULT;
+
 	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 b9e4e21189d2..42c4250bd509 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -445,6 +445,32 @@  struct rte_vlan_filter_conf {
 	uint64_t ids[64];
 };
 
+/**
+ * Hash function types.
+ */
+enum rte_eth_hash_function {
+	/**
+	 * DEFAULT means driver decides which hash algorithm to pick.
+	 */
+	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,
+	/**
+	 * Symmetric Toeplitz: L3 and L4 fields are sorted prior to
+	 * the hash function.
+	 *  If src_ip > dst_ip, swap src_ip and dst_ip.
+	 *  If src_port > dst_port, swap src_port and dst_port.
+	 */
+	RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ_SORT,
+	RTE_ETH_HASH_FUNCTION_MAX,
+};
+
 /**
  * A structure used to configure the Receive Side Scaling (RSS) feature
  * of an Ethernet port.
@@ -465,6 +491,7 @@  struct rte_eth_rss_conf {
 	 * Setting *rss_hf* to zero disables the RSS feature.
 	 */
 	uint64_t rss_hf;
+	enum rte_eth_hash_function algorithm;	/**< Hash algorithm. */
 };
 
 /*
diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index ba8bf27090fb..deedce08fb0a 100644
--- a/lib/ethdev/rte_flow.c
+++ b/lib/ethdev/rte_flow.c
@@ -13,7 +13,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 5d9e3c68af7b..877a2e1d6ad3 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
@@ -3223,32 +3225,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 driver decides which hash algorithm to pick.
-	 */
-	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,
-	/**
-	 * Symmetric Toeplitz: L3 and L4 fields are sorted prior to
-	 * the hash function.
-	 *  If src_ip > dst_ip, swap src_ip and dst_ip.
-	 *  If src_port > dst_port, swap src_port and dst_port.
-	 */
-	RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ_SORT,
-	RTE_ETH_HASH_FUNCTION_MAX,
-};
-
 /**
  * RTE_FLOW_ACTION_TYPE_RSS
  *