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 74A8D42643; Tue, 26 Sep 2023 15:23:15 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0329B402AA; Tue, 26 Sep 2023 15:23:15 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 9D41440271 for ; Tue, 26 Sep 2023 15:23:13 +0200 (CEST) Received: from kwepemi500020.china.huawei.com (unknown [172.30.72.55]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Rw0h960RKzNnsZ; Tue, 26 Sep 2023 21:19:21 +0800 (CST) Received: from [10.67.121.175] (10.67.121.175) by kwepemi500020.china.huawei.com (7.221.188.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Tue, 26 Sep 2023 21:23:10 +0800 Message-ID: <09832b92-1278-15eb-e153-0de226b2687e@huawei.com> Date: Tue, 26 Sep 2023 21:23:10 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH v4 2/7] ethdev: support setting and querying RSS algorithm To: Ferruh Yigit , , Thomas Monjalon , Andrew Rybchenko , Ori Kam CC: References: <20230904072851.7384-1-haijie1@huawei.com> <20230908080030.3837515-1-haijie1@huawei.com> <20230908080030.3837515-3-haijie1@huawei.com> <785537ac-1b25-472a-b114-53620373da69@amd.com> From: Jie Hai In-Reply-To: <785537ac-1b25-472a-b114-53620373da69@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.175] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemi500020.china.huawei.com (7.221.188.8) 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 On 2023/9/21 0:39, Ferruh Yigit wrote: > On 9/8/2023 9:00 AM, Jie Hai wrote: >> Currently, rte_eth_rss_conf supports configuring and querying >> 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 will be affected: >> - rte_eth_dev_configure >> - rte_eth_dev_rss_hash_update >> - rte_eth_dev_rss_hash_conf_get >> > > The problem with adding new configuration fields is, none of the drivers > are using it. > > I can see you are updating hns3 driver in next patch, but what about > others, application will set this field and almost all drivers will > ignore it for now. > And in near future, some will be supporting it and some won't, still > frustrating for the application perspective. > > In the past I was gathering list of these items and follow > implementation of them with drivers, but as the number of drivers > increased this became very hard to manage. > > Another way to manage this is go and update all drivers, and if the > configuration requests anything other than > 'RTE_ETH_HASH_FUNCTION_DEFAULT', return error. So that application can > know this is not supported by this driver. > Do you have better solution for this? > I know this may not be reasonable, how about adding new ops to the query and configuration of the RSS algorithm? > >> If the value of "func" used for configuration is a gibberish >> value, report the error and return. 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_11.rst | 2 ++ >> lib/ethdev/rte_ethdev.c | 17 +++++++++++++++++ >> lib/ethdev/rte_ethdev.h | 21 +++++++++++++++++++++ >> lib/ethdev/rte_flow.c | 1 - >> lib/ethdev/rte_flow.h | 22 ++-------------------- >> 5 files changed, 42 insertions(+), 21 deletions(-) >> >> diff --git a/doc/guides/rel_notes/release_23_11.rst b/doc/guides/rel_notes/release_23_11.rst >> index 333e1d95a283..deb019fbe4c1 100644 >> --- a/doc/guides/rel_notes/release_23_11.rst >> +++ b/doc/guides/rel_notes/release_23_11.rst >> @@ -129,6 +129,8 @@ ABI Changes >> Also, make sure to start the actual text at the margin. >> ======================================================= >> >> +* 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 0840d2b5942a..4cbcdb344cac 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -1445,6 +1445,14 @@ 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(ERR, >> + "Ethdev port_id=%u invalid rss hash function (%u)\n", >> + port_id, dev_conf->rx_adv_conf.rss_conf.func); >> + ret = -EINVAL; >> + goto rollback; >> + } >> + >> /* 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)) { >> @@ -4630,6 +4638,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(ERR, >> + "Ethdev port_id=%u invalid rss hash function (%u)\n", >> + port_id, rss_conf->func); >> + return -EINVAL; >> + } >> + >> if (*dev->dev_ops->rss_hash_update == NULL) >> return -ENOTSUP; >> ret = eth_err(port_id, (*dev->dev_ops->rss_hash_update)(dev, >> @@ -4657,6 +4672,8 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id, >> return -EINVAL; >> } >> >> + rss_conf->func = RTE_ETH_HASH_FUNCTION_DEFAULT; >> + >> > > I think setting this in ethdev level is not required if you update all > drivers to not accept anything other than DEFAULT. > > I initialized this field to prevent some drivers from not reporting RSS algorithms. If an improper parameter is transferred before an API is invoked, the query result will be incorrect. >> 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 40cfbb7efddd..33cec3570f85 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -445,6 +445,26 @@ struct rte_vlan_filter_conf { >> uint64_t ids[64]; >> }; >> >> +/** >> + * Hash function types. >> + */ >> +enum rte_eth_hash_function { >> + /** >> + * DEFAULT means that conformance to a specific hash algorithm is >> + * uncared to the caller. The driver can pick the one it deems optimal. >> + */ >> + RTE_ETH_HASH_FUNCTION_DEFAULT = 0, >> + RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */ >> + RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */ >> + /** >> + * Symmetric Toeplitz: src, dst will be replaced by >> + * xor(src, dst). For the case with src/dst only, >> + * src or dst address will xor with zero pair. >> + */ >> + RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ, >> + RTE_ETH_HASH_FUNCTION_MAX, > > As we are moving this to ethdev.h, when we want to add a new algorithm, > it will cause ABI break because of the 'RTE_ETH_HASH_FUNCTION_MAX', and > it will need to wait. Is there a way to eliminate this MAX enum? > How about assigning RTE_ETH_HASH_FUNCTION_MAX a larger value, e.g. 255 ? >> +}; >> + >> /** >> * A structure used to configure the Receive Side Scaling (RSS) feature >> * of an Ethernet port. >> @@ -464,6 +484,7 @@ struct rte_eth_rss_conf { >> * Supplying an *rss_hf* equal to zero disables the RSS feature. >> */ >> uint64_t rss_hf; >> + enum rte_eth_hash_function func; /**< Hash algorithm. */ > > Can we use 'algorithm' as the field name? > > I know it won't exactly match type name (rte_eth_hash_function) but I > think this will be more accurate also less confusing for "rss_hf (rss > hash function)" and "func (function)"? > Considering the comprehensibility, I agree with the proposal. >> }; >> >> /* >> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c >> index 271d854f7807..d3f2d466d841 100644 >> --- a/lib/ethdev/rte_flow.c >> +++ b/lib/ethdev/rte_flow.c >> @@ -12,7 +12,6 @@ >> #include >> #include >> #include >> -#include "rte_ethdev.h" >> #include "rte_flow_driver.h" >> #include "rte_flow.h" >> >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h >> index 3bd0dc64fbe2..a7578b1d2eff 100644 >> --- a/lib/ethdev/rte_flow.h >> +++ b/lib/ethdev/rte_flow.h >> @@ -40,6 +40,8 @@ >> #include >> #include >> >> +#include "rte_ethdev.h" >> + >> #ifdef __cplusplus >> extern "C" { >> #endif >> @@ -3183,26 +3185,6 @@ struct rte_flow_query_count { >> uint64_t bytes; /**< Number of bytes through this rule [out]. */ >> }; >> >> -/** >> - * Hash function types. >> - */ >> -enum rte_eth_hash_function { >> - /** >> - * DEFAULT means that conformance to a specific hash algorithm is >> - * uncared to the caller. The driver can pick the one it deems optimal. >> - */ >> - RTE_ETH_HASH_FUNCTION_DEFAULT = 0, >> - RTE_ETH_HASH_FUNCTION_TOEPLITZ, /**< Toeplitz */ >> - RTE_ETH_HASH_FUNCTION_SIMPLE_XOR, /**< Simple XOR */ >> - /** >> - * Symmetric Toeplitz: src, dst will be replaced by >> - * xor(src, dst). For the case with src/dst only, >> - * src or dst address will xor with zero pair. >> - */ >> - RTE_ETH_HASH_FUNCTION_SYMMETRIC_TOEPLITZ, >> - RTE_ETH_HASH_FUNCTION_MAX, >> -}; >> - >> /** >> * RTE_FLOW_ACTION_TYPE_RSS >> * > > .