[PATCH 1/5] ethdev: support setting and querying rss algorithm
Dongdong Liu
liudongdong3 at huawei.com
Thu Mar 16 14:10:43 CET 2023
Hi Ivan
Many thanks for your review.
On 2023/3/15 19:28, Ivan Malov wrote:
> Hi,
>
> On Wed, 15 Mar 2023, Dongdong Liu wrote:
>
>> From: Jie Hai <haijie1 at huawei.com>
>>
>> Currently, rte_eth_rss_conf supports configuring 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 is affected:
>> - rte_eth_dev_configure
>> - rte_eth_dev_rss_hash_update
>> - rte_eth_dev_rss_hash_conf_get
>>
>> To prevent configuration failures caused by incorrect func input, check
>> this parameter in advance. If it's incorrect, a warning is generated
>> and the default value is set. 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 at huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3 at huawei.com>
>> ---
>> doc/guides/rel_notes/release_23_03.rst | 4 ++--
>> lib/ethdev/rte_ethdev.c | 18 ++++++++++++++++++
>> lib/ethdev/rte_ethdev.h | 5 +++++
>> 3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/doc/guides/rel_notes/release_23_03.rst
>> b/doc/guides/rel_notes/release_23_03.rst
>> index af6f37389c..7879567427 100644
>> --- a/doc/guides/rel_notes/release_23_03.rst
>> +++ b/doc/guides/rel_notes/release_23_03.rst
>> @@ -284,8 +284,8 @@ ABI Changes
>> Also, make sure to start the actual text at the margin.
>> =======================================================
>>
>> -* No ABI change that would break compatibility with 22.11.
>> -
>> +* 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 4d03255683..db561026bd 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -1368,6 +1368,15 @@ 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(WARNING,
>> + "Ethdev port_id=%u invalid rss hash function (%u),
>> modified to default value (%u)\n",
>> + port_id, dev_conf->rx_adv_conf.rss_conf.func,
>> + RTE_ETH_HASH_FUNCTION_DEFAULT);
>> + dev->data->dev_conf.rx_adv_conf.rss_conf.func =
>> + RTE_ETH_HASH_FUNCTION_DEFAULT;
>
> I have no strong opinion, but, to me, this behaviour conceals
> programming errors. For example, if an application intends
> to enable hash algorithm A but, due to a programming error,
> passes a gibberish value here, chances are the error will
> end up unnoticed. Especially in case the application
> sets the log level to such that warnings are omitted.
Good point, will fix.
>
> Why not just return the error the standard way?
Aha, The original intention is not to break the ABI,
but I think it could not achieve that.
>
>> + }
>> +
>> /* 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)) {
>> @@ -4553,6 +4562,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(NOTICE,
>> + "Ethdev port_id=%u invalid rss hash function (%u),
>> modified to default value (%u)\n",
>> + port_id, rss_conf->func, RTE_ETH_HASH_FUNCTION_DEFAULT);
>> + rss_conf->func = RTE_ETH_HASH_FUNCTION_DEFAULT;
>> + }
>> +
>> if (*dev->dev_ops->rss_hash_update == NULL)
>> return -ENOTSUP;
>> ret = eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev,
>> @@ -4580,6 +4596,8 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>> return -EINVAL;
>> }
>>
>> + rss_conf->func = 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 99fe9e238b..5abe2cb36d 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -174,6 +174,7 @@ extern "C" {
>>
>> #include "rte_ethdev_trace_fp.h"
>> #include "rte_dev_info.h"
>> +#include "rte_flow.h"
>>
>> extern int rte_eth_dev_logtype;
>>
>> @@ -461,11 +462,15 @@ struct rte_vlan_filter_conf {
>> * 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.
>> + *
>> + * The *func* field of the *rss_conf* structure indicates the different
>> + * types of hash algorithms applied by the RSS hashing.
>
> Consider:
>
> The *func* field of the *rss_conf* structure indicates the algorithm to
> use when computing hash. Passing RTE_ETH_HASH_FUNCTION_DEFAULT allows
> the PMD to use its best-effort algorithm rather than a specific one.
Look at some PMD drivers(i40e, hns3 etc), it seems the
RTE_ETH_HASH_FUNCTION_DEFAULT consider as no rss algorithm is set.
Thanks,
Dongdong
>
>> */
>> struct rte_eth_rss_conf {
>> uint8_t *rss_key; /**< If not NULL, 40-byte hash key. */
>> uint8_t rss_key_len; /**< hash key length in bytes. */
>> uint64_t rss_hf; /**< Hash functions to apply - see below. */
>> + enum rte_eth_hash_function func; /**< Hash algorithm to apply. */
>> };
>>
>> /*
>> --
>> 2.22.0
>>
>>
>
> Thank you.
>
> .
>
More information about the stable
mailing list