Re: [PATCH 1/5] ethdev: support setting and querying rss algorithm

[Ivan Malov <ivan.malov@arknetworks.am>]

Hi,

Thanks for responding and PSB.

On Thu, 16 Mar 2023, Dongdong Liu wrote:

> 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@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@huawei.com>
>>> Signed-off-by: Dongdong Liu <liudongdong3@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.

This does not seem to contradict the suggested description.

If they, however, treat this as "no RSS at all", then
perhaps it is a mistake, because if the user requests
Rx MQ mode "RSS" and selects algorithm DEFAULT, this
is clearly not the same as "no RSS". Not by a long
shot. Because for "no RSS" the user would have
passed MQ mode choice "NONE", I take it.

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

Thank you.