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 A6DA9A0548; Wed, 12 Oct 2022 00:23:56 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8A33A40691; Wed, 12 Oct 2022 00:23:56 +0200 (CEST) Received: from forward101o.mail.yandex.net (forward101o.mail.yandex.net [37.140.190.181]) by mails.dpdk.org (Postfix) with ESMTP id 7E8854067C for ; Wed, 12 Oct 2022 00:23:54 +0200 (CEST) Received: from forward503o.mail.yandex.net (forward503o.mail.yandex.net [IPv6:2a02:6b8:0:1a2d::613]) by forward101o.mail.yandex.net (Yandex) with ESMTP id 3DE31369FD2C; Wed, 12 Oct 2022 01:21:25 +0300 (MSK) Received: from myt6-654ec0a0ab93.qloud-c.yandex.net (myt6-654ec0a0ab93.qloud-c.yandex.net [IPv6:2a02:6b8:c12:1d80:0:640:654e:c0a0]) by forward503o.mail.yandex.net (Yandex) with ESMTP id BE7F45C43C4; Wed, 12 Oct 2022 01:21:23 +0300 (MSK) Received: by myt6-654ec0a0ab93.qloud-c.yandex.net (smtp/Yandex) with ESMTPSA id mBJPkdx1p6-LKjinuTv; Wed, 12 Oct 2022 01:21:22 +0300 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client certificate not present) X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1665526882; bh=osuQbN9/S6akhX+7NkTfJCt5kn/iErMIePzRCAIT8s8=; h=From:In-Reply-To:Cc:Date:References:To:Subject:Message-ID; b=Dyv00VJXbGdKuat8DKEfuENJeM6sDZqpcKom+IyskjAen05jZg84ZFVlutESgVhDk OXrKWuE0CBqdrFjlTlDrVItyUZ4oqCvxDWa4pF0yhDli90yR3sbcaWDYnmo6v36OIO T3K4czN7Yyku6M8U4vXMubWJTtI/AlHCWXTRD2RY= Authentication-Results: myt6-654ec0a0ab93.qloud-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: <771c16c9-cb84-2d35-5e64-3686b7b5822e@yandex.ru> Date: Tue, 11 Oct 2022 23:21:19 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: =?UTF-8?B?UmU6IOWbnuWkjTogW1BBVENIIHYyIDEvM10gZXRoZGV2OiBhZGQgQVBJ?= =?UTF-8?Q?_for_direct_rearm_mode?= Content-Language: en-US To: Feifei Wang , "thomas@monjalon.net" , Ferruh Yigit , Andrew Rybchenko , Ray Kinsella Cc: "dev@dpdk.org" , nd , Honnappa Nagarahalli , Ruifeng Wang References: <20220927024756.947272-1-feifei.wang2@arm.com> <20220927024756.947272-2-feifei.wang2@arm.com> From: Konstantin Ananyev In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 >>> Add API for enabling direct rearm mode and for mapping RX and TX >>> queues. Currently, the API supports 1:1(txq : rxq) mapping. >>> >>> Furthermore, to avoid Rx load Tx data directly, add API called >>> 'rte_eth_txq_data_get' to get Tx sw_ring and its information. >>> >>> Suggested-by: Honnappa Nagarahalli >>> Suggested-by: Ruifeng Wang >>> Signed-off-by: Feifei Wang >>> Reviewed-by: Ruifeng Wang >>> Reviewed-by: Honnappa Nagarahalli >>> --- >>> lib/ethdev/ethdev_driver.h | 9 ++++ >>> lib/ethdev/ethdev_private.c | 1 + >>> lib/ethdev/rte_ethdev.c | 37 ++++++++++++++ >>> lib/ethdev/rte_ethdev.h | 95 >> ++++++++++++++++++++++++++++++++++++ >>> lib/ethdev/rte_ethdev_core.h | 5 ++ >>> lib/ethdev/version.map | 4 ++ >>> 6 files changed, 151 insertions(+) >>> >>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h >>> index 47a55a419e..14f52907c1 100644 >>> --- a/lib/ethdev/ethdev_driver.h >>> +++ b/lib/ethdev/ethdev_driver.h >>> @@ -58,6 +58,8 @@ struct rte_eth_dev { >>> eth_rx_descriptor_status_t rx_descriptor_status; >>> /** Check the status of a Tx descriptor */ >>> eth_tx_descriptor_status_t tx_descriptor_status; >>> + /** Use Tx mbufs for Rx to rearm */ >>> + eth_rx_direct_rearm_t rx_direct_rearm; >>> >>> /** >>> * Device data that is shared between primary and secondary >>> processes @@ -486,6 +488,11 @@ typedef int >> (*eth_rx_enable_intr_t)(struct rte_eth_dev *dev, >>> typedef int (*eth_rx_disable_intr_t)(struct rte_eth_dev *dev, >>> uint16_t rx_queue_id); >>> >>> +/**< @internal Get Tx information of a transmit queue of an Ethernet >>> +device. */ typedef void (*eth_txq_data_get_t)(struct rte_eth_dev *dev, >>> + uint16_t tx_queue_id, >>> + struct rte_eth_txq_data *txq_data); >>> + >>> /** @internal Release memory resources allocated by given Rx/Tx queue. >> */ >>> typedef void (*eth_queue_release_t)(struct rte_eth_dev *dev, >>> uint16_t queue_id); >>> @@ -1138,6 +1145,8 @@ struct eth_dev_ops { >>> eth_rxq_info_get_t rxq_info_get; >>> /** Retrieve Tx queue information */ >>> eth_txq_info_get_t txq_info_get; >>> + /** Get the address where Tx data is stored */ >>> + eth_txq_data_get_t txq_data_get; >>> eth_burst_mode_get_t rx_burst_mode_get; /**< Get Rx burst >> mode */ >>> eth_burst_mode_get_t tx_burst_mode_get; /**< Get Tx burst >> mode */ >>> eth_fw_version_get_t fw_version_get; /**< Get firmware >> version */ >>> diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c >>> index 48090c879a..bfe16c7d77 100644 >>> --- a/lib/ethdev/ethdev_private.c >>> +++ b/lib/ethdev/ethdev_private.c >>> @@ -276,6 +276,7 @@ eth_dev_fp_ops_setup(struct rte_eth_fp_ops >> *fpo, >>> fpo->rx_queue_count = dev->rx_queue_count; >>> fpo->rx_descriptor_status = dev->rx_descriptor_status; >>> fpo->tx_descriptor_status = dev->tx_descriptor_status; >>> + fpo->rx_direct_rearm = dev->rx_direct_rearm; >>> >>> fpo->rxq.data = dev->data->rx_queues; >>> fpo->rxq.clbk = (void **)(uintptr_t)dev->post_rx_burst_cbs; >>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index >>> 0c2c1088c0..0dccec2e4b 100644 >>> --- a/lib/ethdev/rte_ethdev.c >>> +++ b/lib/ethdev/rte_ethdev.c >>> @@ -1648,6 +1648,43 @@ rte_eth_dev_is_removed(uint16_t port_id) >>> return ret; >>> } >>> >>> +int >>> +rte_eth_tx_queue_data_get(uint16_t port_id, uint16_t queue_id, >>> + struct rte_eth_txq_data *txq_data) { >>> + struct rte_eth_dev *dev; >>> + >>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>> + dev = &rte_eth_devices[port_id]; >>> + >>> + if (queue_id >= dev->data->nb_tx_queues) { >>> + RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u\n", >> queue_id); >>> + return -EINVAL; >>> + } >>> + >>> + if (txq_data == NULL) { >>> + RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u Tx >> queue %u data to NULL\n", >>> + port_id, queue_id); >>> + return -EINVAL; >>> + } >>> + >>> + if (dev->data->tx_queues == NULL || >>> + dev->data->tx_queues[queue_id] == NULL) { >>> + RTE_ETHDEV_LOG(ERR, >>> + "Tx queue %"PRIu16" of device with port_id=%" >>> + PRIu16" has not been setup\n", >>> + queue_id, port_id); >>> + return -EINVAL; >>> + } >>> + >>> + if (*dev->dev_ops->txq_data_get == NULL) >>> + return -ENOTSUP; >>> + >>> + dev->dev_ops->txq_data_get(dev, queue_id, txq_data); >>> + >>> + return 0; >>> +} >>> + >>> static int >>> rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg, >>> uint16_t n_seg, uint32_t *mbp_buf_size, diff --git >>> a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index >>> 2e783536c1..daf7f05d62 100644 >>> --- a/lib/ethdev/rte_ethdev.h >>> +++ b/lib/ethdev/rte_ethdev.h >>> @@ -1949,6 +1949,23 @@ struct rte_eth_txq_info { >>> uint8_t queue_state; /**< one of RTE_ETH_QUEUE_STATE_*. */ >>> } __rte_cache_min_aligned; >>> >>> +/** >>> + * @internal >>> + * Structure used to hold pointers to internal ethdev Tx data. >>> + * The main purpose is to load and store Tx queue data in direct rearm >> mode. >>> + */ >>> +struct rte_eth_txq_data { >>> + uint64_t *offloads; >>> + void *tx_sw_ring; >>> + volatile void *tx_ring; >>> + uint16_t *tx_next_dd; >>> + uint16_t *nb_tx_free; >>> + uint16_t nb_tx_desc; >>> + uint16_t tx_rs_thresh; >>> + uint16_t tx_free_thresh; >>> +} __rte_cache_min_aligned; >>> + >> >> first of all it is not clear why this struct has to be in public header, why it can't >> be in on of ethdev 'private' headers. >> Second it looks like a snippet from private txq fields for some Intel (and alike) >> PMDs (i40e, ice, etc.). >> How it supposed to to be universal and be applicable for any PMD that >> decides to implement this new API? >> >> >>> /* Generic Burst mode flag definition, values can be ORed. */ >>> >>> /** >>> @@ -4718,6 +4735,27 @@ int rte_eth_remove_rx_callback(uint16_t >> port_id, uint16_t queue_id, >>> int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id, >>> const struct rte_eth_rxtx_callback *user_cb); >>> >>> +/** >>> + * Get the address which Tx data is stored. >>> + * >>> + * @param port_id >>> + * The port identifier of the Ethernet device. >>> + * @param queue_id >>> + * The Tx queue on the Ethernet device for which information >>> + * will be retrieved. >>> + * @param txq_data >>> + * A pointer to a structure of type *rte_eth_txq_data* to be filled. >>> + * >>> + * @return >>> + * - 0: Success >>> + * - -ENODEV: If *port_id* is invalid. >>> + * - -ENOTSUP: routine is not supported by the device PMD. >>> + * - -EINVAL: The queue_id is out of range. >>> + */ >>> +__rte_experimental >>> +int rte_eth_tx_queue_data_get(uint16_t port_id, uint16_t queue_id, >>> + struct rte_eth_txq_data *txq_data); >>> + >>> /** >>> * Retrieve information about given port's Rx queue. >>> * >>> @@ -6209,6 +6247,63 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t >> queue_id, >>> return rte_eth_tx_buffer_flush(port_id, queue_id, buffer); >>> } >>> >>> +/** >>> + * @warning >>> + * @b EXPERIMENTAL: this API may change, or be removed, without prior >>> +notice >>> + * >>> + * Put Tx buffers into Rx sw-ring and rearm descs. >>> + * >>> + * @param port_id >>> + * Port identifying the receive side. >>> + * @param queue_id >>> + * The index of the transmit queue identifying the receive side. >>> + * The value must be in the range [0, nb_rx_queue - 1] previously >> supplied >>> + * to rte_eth_dev_configure(). >>> + * @param txq_data >>> + * A pointer to a structure of type *rte_eth_txq_data* to be filled. >>> + * @return >>> + * The number of direct-rearmed buffers. >>> + */ >>> +__rte_experimental >>> +static __rte_always_inline uint16_t >>> +rte_eth_rx_direct_rearm(uint16_t port_id, uint16_t queue_id, >>> + struct rte_eth_txq_data *txq_data) >>> +{ >>> + uint16_t nb_rearm; >>> + struct rte_eth_fp_ops *p; >>> + void *qd; >>> + >>> +#ifdef RTE_ETHDEV_DEBUG_RX >>> + if (port_id >= RTE_MAX_ETHPORTS || >>> + queue_id >= RTE_MAX_QUEUES_PER_PORT) { >>> + RTE_ETHDEV_LOG(ERR, >>> + "Invalid port_id=%u or queue_id=%u\n", >>> + port_id, queue_id); >>> + return 0; >>> + } >>> +#endif >>> + >>> + p = &rte_eth_fp_ops[port_id]; >>> + qd = p->rxq.data[queue_id]; >>> + >>> +#ifdef RTE_ETHDEV_DEBUG_RX >>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0); >>> + >>> + if (qd == NULL) { >>> + RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u for >> port_id=%u\n", >>> + queue_id, port_id); >>> + return 0; >>> + } >>> + >>> + if (!p->rx_direct_rearm) >> >> This check should be done always (unconditionally). >> it is not a mandatory function for the driver (it can safely skip to implement it). >> >>> + return -ENOTSUP; >> >> This function returns uint16_t, why signed integers here? >> >> >>> +#endif >>> + >>> + nb_rearm = p->rx_direct_rearm(qd, txq_data); >> >> So rx_direct_rearm() function knows how to extract data from TX queue? >> As I understand that is possible only in one case: >> rx_direct_rearm() has full knowledge and acess of txq internals, etc. >> That means that rxq and txq have to belong to the same driver and device >> type. >> > Thanks for the comments, and I have some questions for this. > >> First of all, I still think it is not the best design choice. >> If we going ahead with introducing this feature, it better be as generic as >> possible. >> Plus it mixes TX and RX code-paths together, while it would be much better >> to to keep them independent as they are right now. >> >> Another thing with such approach - even for the same PMD, both TXQ and >> RXQ can have different internal data format and behavior logic (depending >> on port/queue configuration). > 1. Here TXQ and RXQ have different internal format means the queue type > and descs can be different, right? If I understand correctly, based on your > first strategy, is it means we will need different 'rearm_func' for different > queue type in the same PMD? Yes, I think so. If let say we have some PMD where depending on the config, there cpuld be 2 different RXQ formats: rxq_a and rxq_b, and 2 different txq formats: txq_c, txq_d. Then assuming PMD would like to support direct-rearm mode for all four combinations, it needs 4 different rearm functions: rearm_txq_c_to_rxq_a() rearm_txq_c_to_rxq_b() rearm_txq_d_to_rxq_a() rearm_txq_d_to_rxq_b() > >> So rx_direct_rearm() function selection have to be done based on both RXQ >> and TXQ config. >> So instead of rte_eth_tx_queue_data_get(), you'll probably need: >> eth_rx_direct_rearm_t rte_eth_get_rx_direct_rearm_func(rx_port, >> rx_queue, tx_port, tx_queue); >> Then, it will be user responsibility to store it somewhere and call periodically: >> >> control_path: >> ... >> rearm_func = rte_eth_get_rx_direct_rearm_func(rxport, rxqueue, >> txport, txqueue); >> data-path: >> while(...) { >> rearm_func(rxport, txport, rxqueue, txqueue); >> rte_eth_rx_burst(rxport, rxqueue, ....); >> rte_eth_tx_burst(txport, txqueue, ....); >> } >> >> >> In that case there seems absolutely no point to introduce struct >> rte_eth_txq_data. rx_direct_rearm() accesses TXQ private data directly >> anyway. > 2. This is a very good proposal and it will be our first choice. Before working on it, > I have a few questions about how to implement 'rearm_func'. > Like you say above, mixed Rx and Tx path code in 'rearm_func' means the hard-code > is mixed like: > rearm_func(...) { > ... > txep = &txq->sw_ring[txq->tx_next_dd - (txq->tx_rs_thresh - 1)]; > for (...) { > rxep[i].mbuf = txep[i].mbuf; > mb0 = txep[i].mbuf; > paddr = mb0->buf_iova + RTE_PKTMBUF_HEADROOM; > dma_addr0 = vdupq_n_u64(paddr); > vst1q_u64((uint64_t *)&rxdp++->read, dma_addr0); > } > } > Is my understanding is right? Sorry, I don't understand the question. Can you probably elaborate a bit? > >> >> Another way - make rte_eth_txq_data totally opaque and allow PMD to >> store there some data that will help it to distinguish expected TXQ format. >> That will allow PMD to keep rx_direct_rearm() the same for all supported >> TXQ formats (it will make decision internally based on data stored in >> txq_data). >> Though in that case you'll probably need one more dev-op to free txq_data. >