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 5EF5841EB1 for ; Thu, 16 Mar 2023 15:31:26 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4F1C642D4B; Thu, 16 Mar 2023 15:31:26 +0100 (CET) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id C49FD40DDC; Thu, 16 Mar 2023 15:31:23 +0100 (CET) Received: from debian (unknown [78.109.71.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by agw.arknetworks.am (Postfix) with ESMTPSA id D8541E09FC; Thu, 16 Mar 2023 18:31:22 +0400 (+04) Date: Thu, 16 Mar 2023 18:31:42 +0400 (+04) From: Ivan Malov To: Dongdong Liu 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 Subject: Re: [PATCH 1/5] ethdev: support setting and querying rss algorithm In-Reply-To: <75d1aaae-51bd-d347-6852-5899f0a0c476@huawei.com> Message-ID: <57cc597a-e45-789c-2471-c22111dd49f@arknetworks.am> References: <20230315110033.30143-1-liudongdong3@huawei.com> <20230315110033.30143-2-liudongdong3@huawei.com> <6b1461b1-af9b-8757-47e-8a14722d8b7d@arknetworks.am> <75d1aaae-51bd-d347-6852-5899f0a0c476@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org 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 >>> >>> 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. 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.