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 B6A6DA0032; Fri, 1 Oct 2021 08:50:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 531784067A; Fri, 1 Oct 2021 08:50:07 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 9535B40040 for ; Fri, 1 Oct 2021 08:50:05 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id EE7A27F5FD; Fri, 1 Oct 2021 09:50:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru EE7A27F5FD DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633071005; bh=aDnFEJ7g8gQsdLYMwo4L07dc3LmWyYruwjukDLoZgII=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=lWUJ99M9M44KH8pVZ0ri5/9ss6T6MgwdwBLcIbshCrhrj0oW+y6ghHUmaj64otj5V nTYEOYfFSAzSl9uzg4G5fCd01So45U/wkCAEQdA9j2OqWyTCe5xFKUAaOxyQTtVrQe p3BddQEW8eiY1p7OHsJzEKHAmQB52oomHBPFYq+k= To: Ivan Malov , Ori Kam , "dev@dpdk.org" Cc: Andy Moreton , 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: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Fri, 1 Oct 2021 09:50:04 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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" On 9/30/21 10:07 PM, Ivan Malov wrote: > 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. Thinking overnight about it I'd suggest full "metadata". Yes, it will name a bit longer, but less confusing versus term META already used in flow API. Andrew.