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 EE235A0C43; Thu, 30 Sep 2021 21:07:40 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D5876410F2; Thu, 30 Sep 2021 21:07:40 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id B932D410F1 for ; Thu, 30 Sep 2021 21:07:39 +0200 (CEST) Received: from [100.65.5.102] (unknown [5.144.120.126]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 38C7C7F6C1; Thu, 30 Sep 2021 22:07:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 38C7C7F6C1 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633028859; bh=lVQezx2m17VaOKaOmqLgiWVvmSKBLP8wFOsq1P8d8JY=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=Gr5bcBNdyPrfWXk1Cvy4o6vllH50xVEztjduLctMyo8fzpAc08tWTvd+KRDO1iBLj pu0tmliDf0/Qm3JFiFKc2XVeFADd/mJocI3kEise5nKHW7tRuhO9lSv0zQlTuycG0r i5ShVUJrtHS4hE9xQJ7fhODDInj61F6fZ2SEc6eQ= To: Ori Kam , "dev@dpdk.org" Cc: Andy Moreton , Andrew Rybchenko , Ray Kinsella , Jerin Jacob , Wisam Monther , Xiaoyun Li , NBU-Contact-Thomas Monjalon , Ferruh Yigit References: <20210902142359.28138-1-ivan.malov@oktetlabs.ru> <20210923112012.14595-1-ivan.malov@oktetlabs.ru> <20210923112012.14595-2-ivan.malov@oktetlabs.ru> From: Ivan Malov Message-ID: Date: Thu, 30 Sep 2021 22:07:35 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 1/5] ethdev: add API to negotiate delivery of Rx meta data 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 Sender: "dev" Hi Ori, On 30/09/2021 17:59, Ori Kam wrote: > Hi Ivan, > Sorry for jumping in late. No worries. That's OK. > I have a concern that this patch breaks other PMDs. It does no such thing. >>>From the rst file " One should negotiate flag delivery beforehand" > since you only added this function for your PMD all other PMD will fail. > I see that you added exception in the examples, but it doesn't make sense > that applications will also need to add this exception which is not documented. Say, you have an application, and you use it with some specific PMD. Say, that PMD doesn't run into the problem as ours does. In other words, the user can insert a flow with action MARK at any point and get mark delivery working starting from that moment without any problem. Say, this is exactly the way how it works for you at the moment. Now. This new API kicks in. We update the application to invoke it as early as possible. But your PMD in question still doesn't support this API. The comment in the patch says that if the method returns ENOTSUP, the application should ignore that without batting an eyelid. It should just keep on working as it did before the introduction of this API. More specific example: Say, the application doesn't mind using either "RSS + MARK" or tunnel offload. What it does right now is attempt to insert tunnel flows first and, if this fails, fall back to "RSS + MARK". With this API, the application will try to invoke this API with "USER_MARK | TUNNEL_ID" in adapter initialised state. If the PMD says that it can only enable the tunnel offload, then the application will get the knowledge that it doesn't make sense to even try inserting "RSS + MARK" flows. It just can skip useless actions. But if the PMD doesn't support the method, the application will see ENOTSUP and handle this gracefully: it will make no assumptions about what's guaranteed to be supported and what's not and will just keep on its old behaviour: try to insert a flow, fail, fall back to another type of flow. So no breakages with this API. > > Please see more comments inline. > > Thanks, > Ori > >> -----Original Message----- >> From: Ivan Malov >> Sent: Thursday, September 23, 2021 2:20 PM >> Subject: [PATCH v3 1/5] ethdev: add API to negotiate delivery of Rx meta >> data >> >> Delivery of mark, flag and the likes might affect small packet performance. >> If these features are disabled by default, enabling them in started state >> without causing traffic disruption may not always be possible. >> >> Let applications negotiate delivery of Rx meta data beforehand. >> >> Signed-off-by: Ivan Malov >> Reviewed-by: Andrew Rybchenko >> Reviewed-by: Andy Moreton >> Acked-by: Ray Kinsella >> Acked-by: Jerin Jacob >> --- >> app/test-flow-perf/main.c | 21 ++++++++++++ >> app/test-pmd/testpmd.c | 26 +++++++++++++++ >> doc/guides/rel_notes/release_21_11.rst | 9 ++++++ >> lib/ethdev/ethdev_driver.h | 19 +++++++++++ >> lib/ethdev/rte_ethdev.c | 25 ++++++++++++++ >> lib/ethdev/rte_ethdev.h | 45 ++++++++++++++++++++++++++ >> lib/ethdev/rte_flow.h | 12 +++++++ >> lib/ethdev/version.map | 3 ++ >> 8 files changed, 160 insertions(+) >> >> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c index >> 9be8edc31d..48eafffb1d 100644 >> --- a/app/test-flow-perf/main.c >> +++ b/app/test-flow-perf/main.c >> @@ -1760,6 +1760,27 @@ init_port(void) >> rte_exit(EXIT_FAILURE, "Error: can't init mbuf pool\n"); >> >> for (port_id = 0; port_id < nr_ports; port_id++) { >> + uint64_t rx_meta_features = 0; >> + >> + rx_meta_features |= RTE_ETH_RX_META_USER_FLAG; >> + rx_meta_features |= RTE_ETH_RX_META_USER_MARK; >> + >> + ret = rte_eth_rx_meta_negotiate(port_id, >> &rx_meta_features); >> + if (ret == 0) { >> + if (!(rx_meta_features & >> RTE_ETH_RX_META_USER_FLAG)) { >> + printf(":: flow action FLAG will not affect Rx >> mbufs on port=%u\n", >> + port_id); >> + } >> + >> + if (!(rx_meta_features & >> RTE_ETH_RX_META_USER_MARK)) { >> + printf(":: flow action MARK will not affect Rx >> mbufs on port=%u\n", >> + port_id); >> + } >> + } else if (ret != -ENOTSUP) { >> + rte_exit(EXIT_FAILURE, "Error when negotiating Rx >> meta features on port=%u: %s\n", >> + port_id, rte_strerror(-ret)); >> + } >> + >> ret = rte_eth_dev_info_get(port_id, &dev_info); >> if (ret != 0) >> rte_exit(EXIT_FAILURE, >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index >> 97ae52e17e..7a8da3d7ab 100644 >> --- a/app/test-pmd/testpmd.c >> +++ b/app/test-pmd/testpmd.c >> @@ -1485,10 +1485,36 @@ static void >> init_config_port_offloads(portid_t pid, uint32_t socket_id) { >> struct rte_port *port = &ports[pid]; >> + uint64_t rx_meta_features = 0; >> uint16_t data_size; >> int ret; >> int i; >> >> + rx_meta_features |= RTE_ETH_RX_META_USER_FLAG; >> + rx_meta_features |= RTE_ETH_RX_META_USER_MARK; >> + rx_meta_features |= RTE_ETH_RX_META_TUNNEL_ID; >> + >> + ret = rte_eth_rx_meta_negotiate(pid, &rx_meta_features); >> + if (ret == 0) { >> + if (!(rx_meta_features & RTE_ETH_RX_META_USER_FLAG)) { >> + TESTPMD_LOG(INFO, "Flow action FLAG will not >> affect Rx mbufs on port %u\n", >> + pid); >> + } >> + >> + if (!(rx_meta_features & RTE_ETH_RX_META_USER_MARK)) >> { >> + TESTPMD_LOG(INFO, "Flow action MARK will not >> affect Rx mbufs on port %u\n", >> + pid); >> + } >> + >> + if (!(rx_meta_features & RTE_ETH_RX_META_TUNNEL_ID)) { >> + TESTPMD_LOG(INFO, "Flow tunnel offload support >> might be limited or unavailable on port %u\n", >> + pid); >> + } >> + } else if (ret != -ENOTSUP) { >> + rte_exit(EXIT_FAILURE, "Error when negotiating Rx meta >> features on port %u: %s\n", >> + pid, rte_strerror(-ret)); >> + } >> + >> port->dev_conf.txmode = tx_mode; >> port->dev_conf.rxmode = rx_mode; >> >> diff --git a/doc/guides/rel_notes/release_21_11.rst >> b/doc/guides/rel_notes/release_21_11.rst >> index 19356ac53c..6674d4474c 100644 >> --- a/doc/guides/rel_notes/release_21_11.rst >> +++ b/doc/guides/rel_notes/release_21_11.rst >> @@ -106,6 +106,15 @@ New Features >> Added command-line options to specify total number of processes and >> current process ID. Each process owns subset of Rx and Tx queues. >> >> +* **Added an API to negotiate delivery of specific parts of Rx meta >> +data** >> + >> + A new API, ``rte_eth_rx_meta_negotiate()``, was added. >> + The following parts of Rx meta data were defined: >> + >> + * ``RTE_ETH_RX_META_USER_FLAG`` >> + * ``RTE_ETH_RX_META_USER_MARK`` >> + * ``RTE_ETH_RX_META_TUNNEL_ID`` >> + >> >> Removed Items >> ------------- >> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h index >> 40e474aa7e..96e0c60cae 100644 >> --- a/lib/ethdev/ethdev_driver.h >> +++ b/lib/ethdev/ethdev_driver.h >> @@ -789,6 +789,22 @@ typedef int (*eth_get_monitor_addr_t)(void *rxq, >> typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev, >> struct rte_eth_representor_info *info); >> >> +/** >> + * @internal >> + * Negotiate delivery of specific parts of Rx meta data. >> + * >> + * @param dev >> + * Port (ethdev) handle >> + * >> + * @param[inout] features >> + * Feature selection buffer >> + * >> + * @return >> + * Negative errno value on error, zero otherwise >> + */ >> +typedef int (*eth_rx_meta_negotiate_t)(struct rte_eth_dev *dev, >> + uint64_t *features); >> + >> /** >> * @internal A structure containing the functions exported by an Ethernet >> driver. >> */ >> @@ -949,6 +965,9 @@ struct eth_dev_ops { >> >> eth_representor_info_get_t representor_info_get; >> /**< Get representor info. */ >> + >> + eth_rx_meta_negotiate_t rx_meta_negotiate; >> + /**< Negotiate delivery of specific parts of Rx meta data. */ >> }; >> >> /** >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index >> daf5ca9242..49cb84d64c 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -6310,6 +6310,31 @@ rte_eth_representor_info_get(uint16_t port_id, >> return eth_err(port_id, (*dev->dev_ops- >>> representor_info_get)(dev, info)); } >> >> +int >> +rte_eth_rx_meta_negotiate(uint16_t port_id, uint64_t *features) { >> + struct rte_eth_dev *dev; >> + >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> + dev = &rte_eth_devices[port_id]; >> + >> + if (dev->data->dev_configured != 0) { >> + RTE_ETHDEV_LOG(ERR, >> + "The port (id=%"PRIu16") is already configured\n", >> + port_id); >> + return -EBUSY; >> + } >> + >> + if (features == NULL) { >> + RTE_ETHDEV_LOG(ERR, "Invalid features (NULL)\n"); >> + return -EINVAL; >> + } >> + >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_meta_negotiate, >> -ENOTSUP); >> + return eth_err(port_id, >> + (*dev->dev_ops->rx_meta_negotiate)(dev, features)); } >> + >> RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO); >> >> RTE_INIT(ethdev_init_telemetry) >> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index >> 1da37896d8..8467a7a362 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -4888,6 +4888,51 @@ __rte_experimental int >> rte_eth_representor_info_get(uint16_t port_id, >> struct rte_eth_representor_info *info); >> >> +/** The ethdev sees flagged packets if there are flows with action >> +FLAG. */ #define RTE_ETH_RX_META_USER_FLAG (UINT64_C(1) << 0) >> + >> +/** The ethdev sees mark IDs in packets if there are flows with action >> +MARK. */ #define RTE_ETH_RX_META_USER_MARK (UINT64_C(1) << 1) >> + >> +/** The ethdev detects missed packets if there are "tunnel_set" flows >> +in use. */ #define RTE_ETH_RX_META_TUNNEL_ID (UINT64_C(1) << 2) >> + >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice >> + * >> + * Negotiate delivery of specific parts of Rx meta data. >> + * >> + * Invoke this API before the first rte_eth_dev_configure() invocation >> + * to let the PMD make preparations that are inconvenient to do later. >> + * >> + * The negotiation process is as follows: >> + * >> + * - the application requests features intending to use at least some >> +of them; >> + * - the PMD responds with the guaranteed subset of the requested >> +feature set; >> + * - the application can retry negotiation with another set of >> +features; >> + * - the application can pass zero to clear the negotiation result; >> + * - the last negotiated result takes effect upon the ethdev start. >> + * >> + * If this API is unsupported, the application should gracefully ignore that. >> + * >> + * @param port_id >> + * Port (ethdev) identifier >> + * >> + * @param[inout] features >> + * Feature selection buffer >> + * >> + * @return >> + * - (-EBUSY) if the port can't handle this in its current state; >> + * - (-ENOTSUP) if the method itself is not supported by the PMD; >> + * - (-ENODEV) if *port_id* is invalid; >> + * - (-EINVAL) if *features* is NULL; >> + * - (-EIO) if the device is removed; >> + * - (0) on success >> + */ >> +__rte_experimental >> +int rte_eth_rx_meta_negotiate(uint16_t port_id, uint64_t *features); > > I don't think meta is the best name since we also have meta item and the word meta can be used > in other cases. I'm no expert in naming. What could be a better term for this? Personally, I'd rather not perceive "meta" the way you describe. It's not just "meta". It's "rx_meta", and the flags supplied with this API provide enough context to explain what it's all about. > >> + >> #include >> >> /** >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index >> 70f455d47d..6eb7ec0574 100644 >> --- a/lib/ethdev/rte_flow.h >> +++ b/lib/ethdev/rte_flow.h >> @@ -1904,6 +1904,10 @@ enum rte_flow_action_type { >> * PKT_RX_FDIR_ID mbuf flags. >> * >> * See struct rte_flow_action_mark. >> + * >> + * One should negotiate delivery of mark IDs beforehand. >> + * @see rte_eth_rx_meta_negotiate() >> + * @see RTE_ETH_RX_META_USER_MARK >> */ >> RTE_FLOW_ACTION_TYPE_MARK, >> >> @@ -1912,6 +1916,10 @@ enum rte_flow_action_type { >> * sets the PKT_RX_FDIR mbuf flag. >> * >> * No associated configuration structure. >> + * >> + * One should negotiate flag delivery beforehand. >> + * @see rte_eth_rx_meta_negotiate() >> + * @see RTE_ETH_RX_META_USER_FLAG >> */ >> RTE_FLOW_ACTION_TYPE_FLAG, >> >> @@ -4223,6 +4231,10 @@ rte_flow_tunnel_match(uint16_t port_id, >> /** >> * Populate the current packet processing state, if exists, for the given mbuf. >> * >> + * One should negotiate the processing state information delivery >> beforehand. >> + * @see rte_eth_rx_meta_negotiate() >> + * @see RTE_ETH_RX_META_TUNNEL_ID >> + * >> * @param port_id >> * Port identifier of Ethernet device. >> * @param[in] m >> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index >> 904bce6ea1..a8928266a9 100644 >> --- a/lib/ethdev/version.map >> +++ b/lib/ethdev/version.map >> @@ -247,6 +247,9 @@ EXPERIMENTAL { >> rte_mtr_meter_policy_delete; >> rte_mtr_meter_policy_update; >> rte_mtr_meter_policy_validate; >> + >> + # added in 21.11 >> + rte_eth_rx_meta_negotiate; >> }; >> >> INTERNAL { >> -- >> 2.20.1 -- Ivan M