On 2023/11/1 21:13, Ferruh Yigit wrote:
On 11/1/2023 7:40 AM, Jie Hai wrote:
1. overwrite the comments of fields of 'rte_eth_rss_conf'.
2. Add comments for RTE_ETH_HASH_FUNCTION_DEFAULT.
Signed-off-by: Jie Hai <haij...@huawei.com>
---
lib/ethdev/rte_ethdev.h | 34 +++++++++++++++++++---------------
lib/ethdev/rte_flow.h | 1 +
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index a53dd5a1efec..343a134fdd12 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -448,24 +448,28 @@ struct rte_vlan_filter_conf {
/**
* A structure used to configure the Receive Side Scaling (RSS) feature
* of an Ethernet port.
- * If not NULL, the *rss_key* pointer of the *rss_conf* structure points
- * to an array holding the RSS key to use for hashing specific header
- * fields of received packets. The length of this array should be indicated
- * by *rss_key_len* below. Otherwise, a default random hash key is used by
- * the device driver.
- *
- * The *rss_key_len* field of the *rss_conf* structure indicates the length
- * in bytes of the array pointed by *rss_key*. To be compatible, this length
- * will be checked in i40e only. Others assume 40 bytes to be used as before.
- *
- * The *rss_hf* field of the *rss_conf* structure indicates the different
- * types of IPv4/IPv6 packets to which the RSS hashing must be applied.
- * Supplying an *rss_hf* equal to zero disables the RSS feature.
*/
struct rte_eth_rss_conf {
- uint8_t *rss_key; /**< If not NULL, 40-byte hash key. */
+ /**
+ * In rte_eth_dev_rss_hash_conf_get(), the *rss_key_len* should be
+ * greater than or equal to the *hash_key_size* which get from
+ * rte_eth_dev_info_get() API. And the *rss_key* should contain at least
+ * *hash_key_size* bytes. If not meet these requirements, the query
+ * result is unreliable even if the operation returns success.
+ *
+ * In rte_eth_dev_rss_hash_update() or rte_eth_dev_configure(), if
+ * *rss_key* is not NULL, the *rss_key_len* indicates the length of the
+ * *rss_key* in bytes of the array pointed by *rss_key*,
I think it is sufficient to say "length of the *rss_key* in bytes".
and it should
+ * be equal to *hash_key_size*.
I don't know if we missed something here, first driver reports key size
via 'rte_eth_dev_info_get()::hash_key_size', later other APIs require
'rss_key_len' parameter that should be same as 'hash_key_size', as
driver already know this parameter why we are requesting it back from
the application?
To unify user behavior and verify user behavior at the ethdev level.
Otherwise, drivers are free to use a
+ * random or a default key or to ignore this configuration.
+ */
I guess above clause describes when 'rss_key' is null, can you please
clarify it as following, perhaps with a line break:
"If *rss_key* is NULL, drivers are free to use a random or a default key."
For the "Drivers are free to ignore the *rss_key_len* and assume key
length is 40 bytes." part, as checks in ethdev layer now forces
application to provide 'rss_key_len' as 'hash_key_size', I think we can
remove above, as application will provide 40 bytes when it is the case.
My concern is this check now can break some applications, because
'rss_key_len' wasn't mandatory previously, but it became now.
The testpmd already use 'rte_eth_dev_info_get()::hash_key_size' to
correct the rss_key_len.
If hash_key_size is zero or too big for user's rss_key, it report errors.
Otherwise, it use hash_key_size as rss_key_len.
If all drivers use this app to test rss configuration, the check has no
problem.
The test_link_bonding_rssconf use 40 Bytes rss_key, we know that is
valid for now.
Overall it becomes:
"
In rte_eth_dev_rss_hash_update() or rte_eth_dev_configure(), if
*rss_key* is not NULL, the *rss_key_len* indicates the length of the
*rss_key* in bytes and it should be equal to *hash_key_size*.
If *rss_key* is NULL, drivers are free to use a random or a default key.
"
ok, thanks.
+ uint8_t *rss_key;
uint8_t rss_key_len; /**< hash key length in bytes. */
- uint64_t rss_hf; /**< Hash functions to apply - see below. */
+ /**
+ * Indicates the type of packets or the specific part of packets to
+ * which RSS hashing is to be applied.
+ */
+ uint64_t rss_hf;
};
/*
diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index c16fe8c21f2f..751c29a0f3f3 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -3226,6 +3226,7 @@ struct rte_flow_query_count {
* 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 */
.
