From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 63DB541EB1;
	Thu, 16 Mar 2023 14:10:48 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 4281240DF6;
	Thu, 16 Mar 2023 14:10:48 +0100 (CET)
Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188])
 by mails.dpdk.org (Postfix) with ESMTP id 14A5740A89;
 Thu, 16 Mar 2023 14:10:46 +0100 (CET)
Received: from kwepemi500017.china.huawei.com (unknown [172.30.72.53])
 by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Pcnbz4TvGzSkhk;
 Thu, 16 Mar 2023 21:07:27 +0800 (CST)
Received: from [10.67.103.235] (10.67.103.235) by
 kwepemi500017.china.huawei.com (7.221.188.110) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2507.21; Thu, 16 Mar 2023 21:10:43 +0800
Subject: Re: [PATCH 1/5] ethdev: support setting and querying rss algorithm
To: Ivan Malov <ivan.malov@arknetworks.am>
References: <20230315110033.30143-1-liudongdong3@huawei.com>
 <20230315110033.30143-2-liudongdong3@huawei.com>
 <6b1461b1-af9b-8757-47e-8a14722d8b7d@arknetworks.am>
CC: <dev@dpdk.org>, <ferruh.yigit@amd.com>, <thomas@monjalon.net>,
 <andrew.rybchenko@oktetlabs.ru>, <reshma.pattan@intel.com>,
 <stable@dpdk.org>, <yisen.zhuang@huawei.com>, Jie Hai <haijie1@huawei.com>
From: Dongdong Liu <liudongdong3@huawei.com>
Message-ID: <75d1aaae-51bd-d347-6852-5899f0a0c476@huawei.com>
Date: Thu, 16 Mar 2023 21:10:43 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.1
MIME-Version: 1.0
In-Reply-To: <6b1461b1-af9b-8757-47e-8a14722d8b7d@arknetworks.am>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
X-Originating-IP: [10.67.103.235]
X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To
 kwepemi500017.china.huawei.com (7.221.188.110)
X-CFilter-Loop: Reflected
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

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.

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