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 6A80041E9F; Wed, 15 Mar 2023 12:28:16 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4A7C940A7A; Wed, 15 Mar 2023 12:28:16 +0100 (CET) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id 0C47D40141; Wed, 15 Mar 2023 12:28:15 +0100 (CET) Received: from debian (unknown [78.109.71.129]) (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 173C0E0BA5; Wed, 15 Mar 2023 15:28:14 +0400 (+04) Date: Wed, 15 Mar 2023 15:28:33 +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: <20230315110033.30143-2-liudongdong3@huawei.com> Message-ID: <6b1461b1-af9b-8757-47e-8a14722d8b7d@arknetworks.am> References: <20230315110033.30143-1-liudongdong3@huawei.com> <20230315110033.30143-2-liudongdong3@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed 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, 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. Why not just return the error the standard way? > + } > + > /* 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. > */ > 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.