From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 References: <20230315110033.30143-1-liudongdong3@huawei.com> <20230315110033.30143-2-liudongdong3@huawei.com> <6b1461b1-af9b-8757-47e-8a14722d8b7d@arknetworks.am> CC: , , , , , , , Jie Hai From: Dongdong Liu 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 >> >> 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 >> Signed-off-by: Dongdong Liu >> --- >> 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. > > . >