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 B44D5A0A0C; Mon, 5 Jul 2021 11:08:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 91E154068C; Mon, 5 Jul 2021 11:08:16 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 3534F4003C for ; Mon, 5 Jul 2021 11:08:15 +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) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id D674B7F53F; Mon, 5 Jul 2021 12:08:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru D674B7F53F DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1625476094; bh=cY2Rj5yLH9BD2PiNk74tDqkBKx4O0bBw8UXcD5rsKI4=; h=Subject:To:References:From:Date:In-Reply-To; b=abxsuwKM3hOLTqEfEhS5IPFd2aJuZfMF36VpU4UuPUuePBn/3qUd8S8SId9ZQhroE 3WjLb+XVr1KOJkOBsNp+idgXrXAGJ/0ZWUAmT6VascaVzoQ6XWPE+vok2ErnofCC3a LIPPoNwiL8F4kFmNJ+5vi8GdE9juCcF2KKx7KB/4= To: Jiawen Wu , dev@dpdk.org References: <20210617110005.4132926-1-jiawenwu@trustnetic.com> <20210617110005.4132926-13-jiawenwu@trustnetic.com> <6d34d736-eaf6-a69a-47b5-9f42e0c09b5c@oktetlabs.ru> <00a801d77178$d6d645e0$8482d1a0$@trustnetic.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Mon, 5 Jul 2021 12:08:14 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <00a801d77178$d6d645e0$8482d1a0$@trustnetic.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 12/19] net/ngbe: add Rx queue setup and release 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 7/5/21 11:36 AM, Jiawen Wu wrote: > On July 3, 2021 12:36 AM, Andrew Rybchenko wrote: >> On 6/17/21 1:59 PM, Jiawen Wu wrote: >>> Setup device Rx queue and release Rx queue. >>> >>> Signed-off-by: Jiawen Wu >>> --- >>> drivers/net/ngbe/meson.build | 1 + >>> drivers/net/ngbe/ngbe_ethdev.c | 37 +++- >>> drivers/net/ngbe/ngbe_ethdev.h | 16 ++ >>> drivers/net/ngbe/ngbe_rxtx.c | 308 +++++++++++++++++++++++++++++++++ >>> drivers/net/ngbe/ngbe_rxtx.h | 96 ++++++++++ >>> 5 files changed, 457 insertions(+), 1 deletion(-) create mode 100644 >>> drivers/net/ngbe/ngbe_rxtx.c create mode 100644 >>> drivers/net/ngbe/ngbe_rxtx.h >>> >>> diff --git a/drivers/net/ngbe/meson.build >>> b/drivers/net/ngbe/meson.build index 81173fa7f0..9e75b82f1c 100644 >>> --- a/drivers/net/ngbe/meson.build >>> +++ b/drivers/net/ngbe/meson.build >>> @@ -12,6 +12,7 @@ objs = [base_objs] >>> >>> sources = files( >>> 'ngbe_ethdev.c', >>> + 'ngbe_rxtx.c', >>> ) >>> >>> includes += include_directories('base') diff --git >>> a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c >>> index c952023e8b..e73606c5f3 100644 >>> --- a/drivers/net/ngbe/ngbe_ethdev.c >>> +++ b/drivers/net/ngbe/ngbe_ethdev.c >>> @@ -12,6 +12,7 @@ >>> #include "ngbe_logs.h" >>> #include "base/ngbe.h" >>> #include "ngbe_ethdev.h" >>> +#include "ngbe_rxtx.h" >>> >>> static int ngbe_dev_close(struct rte_eth_dev *dev); >>> >>> @@ -37,6 +38,12 @@ static const struct rte_pci_id pci_id_ngbe_map[] = { >>> { .vendor_id = 0, /* sentinel */ }, >>> }; >>> >>> +static const struct rte_eth_desc_lim rx_desc_lim = { >>> + .nb_max = NGBE_RING_DESC_MAX, >>> + .nb_min = NGBE_RING_DESC_MIN, >>> + .nb_align = NGBE_RXD_ALIGN, >>> +}; >>> + >>> static const struct eth_dev_ops ngbe_eth_dev_ops; >>> >>> static inline void >>> @@ -241,12 +248,19 @@ static int >>> ngbe_dev_configure(struct rte_eth_dev *dev) { >>> struct ngbe_interrupt *intr = ngbe_dev_intr(dev); >>> + struct ngbe_adapter *adapter = ngbe_dev_adapter(dev); >>> >>> PMD_INIT_FUNC_TRACE(); >>> >>> /* set flag to update link status after init */ >>> intr->flags |= NGBE_FLAG_NEED_LINK_UPDATE; >>> >>> + /* >>> + * Initialize to TRUE. If any of Rx queues doesn't meet the bulk >>> + * allocation Rx preconditions we will reset it. >>> + */ >>> + adapter->rx_bulk_alloc_allowed = true; >>> + >>> return 0; >>> } >>> >>> @@ -266,11 +280,30 @@ ngbe_dev_close(struct rte_eth_dev *dev) static >>> int ngbe_dev_info_get(struct rte_eth_dev *dev, struct >>> rte_eth_dev_info *dev_info) { >>> - RTE_SET_USED(dev); >>> + struct ngbe_hw *hw = ngbe_dev_hw(dev); >>> + >>> + dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues; >>> + >>> + dev_info->default_rxconf = (struct rte_eth_rxconf) { >>> + .rx_thresh = { >>> + .pthresh = NGBE_DEFAULT_RX_PTHRESH, >>> + .hthresh = NGBE_DEFAULT_RX_HTHRESH, >>> + .wthresh = NGBE_DEFAULT_RX_WTHRESH, >>> + }, >>> + .rx_free_thresh = NGBE_DEFAULT_RX_FREE_THRESH, >>> + .rx_drop_en = 0, >>> + .offloads = 0, >>> + }; >>> + >>> + dev_info->rx_desc_lim = rx_desc_lim; >>> >>> dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_100M | >>> ETH_LINK_SPEED_10M; >>> >>> + /* Driver-preferred Rx/Tx parameters */ >>> + dev_info->default_rxportconf.nb_queues = 1; >>> + dev_info->default_rxportconf.ring_size = 256; >>> + >>> return 0; >>> } >>> >>> @@ -570,6 +603,8 @@ static const struct eth_dev_ops ngbe_eth_dev_ops = >> { >>> .dev_configure = ngbe_dev_configure, >>> .dev_infos_get = ngbe_dev_info_get, >>> .link_update = ngbe_dev_link_update, >>> + .rx_queue_setup = ngbe_dev_rx_queue_setup, >>> + .rx_queue_release = ngbe_dev_rx_queue_release, >>> }; >>> >>> RTE_PMD_REGISTER_PCI(net_ngbe, rte_ngbe_pmd); diff --git >>> a/drivers/net/ngbe/ngbe_ethdev.h b/drivers/net/ngbe/ngbe_ethdev.h >>> index b67508a3de..6580d288c8 100644 >>> --- a/drivers/net/ngbe/ngbe_ethdev.h >>> +++ b/drivers/net/ngbe/ngbe_ethdev.h >>> @@ -30,6 +30,7 @@ struct ngbe_interrupt { struct ngbe_adapter { >>> struct ngbe_hw hw; >>> struct ngbe_interrupt intr; >>> + bool rx_bulk_alloc_allowed; >> >> Shouldn't it be aligned as well as above fields? >> >>> }; >>> >>> static inline struct ngbe_adapter * >>> @@ -58,6 +59,13 @@ ngbe_dev_intr(struct rte_eth_dev *dev) >>> return intr; >>> } >>> >>> +void ngbe_dev_rx_queue_release(void *rxq); >>> + >>> +int ngbe_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id, >>> + uint16_t nb_rx_desc, unsigned int socket_id, >>> + const struct rte_eth_rxconf *rx_conf, >>> + struct rte_mempool *mb_pool); >>> + >>> int >>> ngbe_dev_link_update_share(struct rte_eth_dev *dev, >>> int wait_to_complete); >>> @@ -66,4 +74,12 @@ ngbe_dev_link_update_share(struct rte_eth_dev *dev, >>> #define NGBE_LINK_UP_CHECK_TIMEOUT 1000 /* ms */ >>> #define NGBE_VMDQ_NUM_UC_MAC 4096 /* Maximum nb. of UC MAC addr. */ >>> >>> +/* >>> + * Default values for Rx/Tx configuration */ #define >>> +NGBE_DEFAULT_RX_FREE_THRESH 32 >>> +#define NGBE_DEFAULT_RX_PTHRESH 8 >>> +#define NGBE_DEFAULT_RX_HTHRESH 8 >>> +#define NGBE_DEFAULT_RX_WTHRESH 0 >>> + >>> #endif /* _NGBE_ETHDEV_H_ */ >>> diff --git a/drivers/net/ngbe/ngbe_rxtx.c >>> b/drivers/net/ngbe/ngbe_rxtx.c new file mode 100644 index >>> 0000000000..df0b64dc01 >>> --- /dev/null >>> +++ b/drivers/net/ngbe/ngbe_rxtx.c >>> @@ -0,0 +1,308 @@ >>> +/* SPDX-License-Identifier: BSD-3-Clause >>> + * Copyright(c) 2018-2020 Beijing WangXun Technology Co., Ltd. >>> + * Copyright(c) 2010-2017 Intel Corporation */ >>> + >>> +#include >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "ngbe_logs.h" >>> +#include "base/ngbe.h" >>> +#include "ngbe_ethdev.h" >>> +#include "ngbe_rxtx.h" >>> + >>> +/** >>> + * ngbe_free_sc_cluster - free the not-yet-completed scattered >>> +cluster >>> + * >>> + * The "next" pointer of the last segment of (not-yet-completed) RSC >>> +clusters >>> + * in the sw_sc_ring is not set to NULL but rather points to the next >>> + * mbuf of this RSC aggregation (that has not been completed yet and >>> +still >>> + * resides on the HW ring). So, instead of calling for >>> +rte_pktmbuf_free() we >>> + * will just free first "nb_segs" segments of the cluster explicitly >>> +by calling >>> + * an rte_pktmbuf_free_seg(). >>> + * >>> + * @m scattered cluster head >>> + */ >>> +static void __rte_cold >>> +ngbe_free_sc_cluster(struct rte_mbuf *m) { >>> + uint16_t i, nb_segs = m->nb_segs; >>> + struct rte_mbuf *next_seg; >>> + >>> + for (i = 0; i < nb_segs; i++) { >>> + next_seg = m->next; >>> + rte_pktmbuf_free_seg(m); >>> + m = next_seg; >>> + } >>> +} >>> + >>> +static void __rte_cold >>> +ngbe_rx_queue_release_mbufs(struct ngbe_rx_queue *rxq) { >>> + unsigned int i; >>> + >>> + if (rxq->sw_ring != NULL) { >>> + for (i = 0; i < rxq->nb_rx_desc; i++) { >>> + if (rxq->sw_ring[i].mbuf != NULL) { >>> + rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf); >>> + rxq->sw_ring[i].mbuf = NULL; >>> + } >>> + } >>> + if (rxq->rx_nb_avail) { >> >> Compare vs 0 explicitly. However, it looks like that the check is not required at >> all. Body may be done unconditionally. >> >>> + for (i = 0; i < rxq->rx_nb_avail; ++i) { >>> + struct rte_mbuf *mb; >>> + >>> + mb = rxq->rx_stage[rxq->rx_next_avail + i]; >>> + rte_pktmbuf_free_seg(mb); >>> + } >>> + rxq->rx_nb_avail = 0; >>> + } >>> + } >>> + >>> + if (rxq->sw_sc_ring != NULL) >>> + for (i = 0; i < rxq->nb_rx_desc; i++) >>> + if (rxq->sw_sc_ring[i].fbuf) { >> >> Compare vs NULL >> >>> + ngbe_free_sc_cluster(rxq->sw_sc_ring[i].fbuf); >>> + rxq->sw_sc_ring[i].fbuf = NULL; >>> + } >>> +} >>> + >>> +static void __rte_cold >>> +ngbe_rx_queue_release(struct ngbe_rx_queue *rxq) { >>> + if (rxq != NULL) { >>> + ngbe_rx_queue_release_mbufs(rxq); >>> + rte_free(rxq->sw_ring); >>> + rte_free(rxq->sw_sc_ring); >>> + rte_free(rxq); >>> + } >>> +} >>> + >>> +void __rte_cold >>> +ngbe_dev_rx_queue_release(void *rxq) >>> +{ >>> + ngbe_rx_queue_release(rxq); >>> +} >>> + >>> +/* >>> + * Check if Rx Burst Bulk Alloc function can be used. >>> + * Return >>> + * 0: the preconditions are satisfied and the bulk allocation function >>> + * can be used. >>> + * -EINVAL: the preconditions are NOT satisfied and the default Rx burst >>> + * function must be used. >>> + */ >>> +static inline int __rte_cold >>> +check_rx_burst_bulk_alloc_preconditions(struct ngbe_rx_queue *rxq) { >>> + int ret = 0; >>> + >>> + /* >>> + * Make sure the following pre-conditions are satisfied: >>> + * rxq->rx_free_thresh >= RTE_PMD_NGBE_RX_MAX_BURST >>> + * rxq->rx_free_thresh < rxq->nb_rx_desc >>> + * (rxq->nb_rx_desc % rxq->rx_free_thresh) == 0 >>> + * Scattered packets are not supported. This should be checked >>> + * outside of this function. >>> + */ >>> + if (!(rxq->rx_free_thresh >= RTE_PMD_NGBE_RX_MAX_BURST)) { >> >> Isn't is simpler to read and understand: >> rxq->rx_free_thresh < RTE_PMD_NGBE_RX_MAX_BURST >> >>> + PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions: " >>> + "rxq->rx_free_thresh=%d, " >>> + "RTE_PMD_NGBE_RX_MAX_BURST=%d", >> >> Do not split format string. >> >>> + rxq->rx_free_thresh, RTE_PMD_NGBE_RX_MAX_BURST); >>> + ret = -EINVAL; >>> + } else if (!(rxq->rx_free_thresh < rxq->nb_rx_desc)) { >> >> rxq->rx_free_thresh >= rxq->nb_rx_desc >> is simpler to read >> >>> + PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions: " >>> + "rxq->rx_free_thresh=%d, " >>> + "rxq->nb_rx_desc=%d", >> >> Do not split format string. >> >>> + rxq->rx_free_thresh, rxq->nb_rx_desc); >>> + ret = -EINVAL; >>> + } else if (!((rxq->nb_rx_desc % rxq->rx_free_thresh) == 0)) { >> >> (rxq->nb_rx_desc % rxq->rx_free_thresh) != 0 is easier to read >> >>> + PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions: " >>> + "rxq->nb_rx_desc=%d, " >>> + "rxq->rx_free_thresh=%d", >>> + rxq->nb_rx_desc, rxq->rx_free_thresh); >> >> Do not split format string >> >>> + ret = -EINVAL; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +/* Reset dynamic ngbe_rx_queue fields back to defaults */ static void >>> +__rte_cold ngbe_reset_rx_queue(struct ngbe_adapter *adapter, struct >>> +ngbe_rx_queue *rxq) { >>> + static const struct ngbe_rx_desc zeroed_desc = { >>> + {{0}, {0} }, {{0}, {0} } }; >>> + unsigned int i; >>> + uint16_t len = rxq->nb_rx_desc; >>> + >>> + /* >>> + * By default, the Rx queue setup function allocates enough memory for >>> + * NGBE_RING_DESC_MAX. The Rx Burst bulk allocation function requires >>> + * extra memory at the end of the descriptor ring to be zero'd out. >>> + */ >>> + if (adapter->rx_bulk_alloc_allowed) >>> + /* zero out extra memory */ >>> + len += RTE_PMD_NGBE_RX_MAX_BURST; >>> + >>> + /* >>> + * Zero out HW ring memory. Zero out extra memory at the end of >>> + * the H/W ring so look-ahead logic in Rx Burst bulk alloc function >>> + * reads extra memory as zeros. >>> + */ >>> + for (i = 0; i < len; i++) >>> + rxq->rx_ring[i] = zeroed_desc; >>> + >>> + /* >>> + * initialize extra software ring entries. Space for these extra >>> + * entries is always allocated >>> + */ >>> + memset(&rxq->fake_mbuf, 0x0, sizeof(rxq->fake_mbuf)); >>> + for (i = rxq->nb_rx_desc; i < len; ++i) >>> + rxq->sw_ring[i].mbuf = &rxq->fake_mbuf; >>> + >>> + rxq->rx_nb_avail = 0; >>> + rxq->rx_next_avail = 0; >>> + rxq->rx_free_trigger = (uint16_t)(rxq->rx_free_thresh - 1); >>> + rxq->rx_tail = 0; >>> + rxq->nb_rx_hold = 0; >>> + rxq->pkt_first_seg = NULL; >>> + rxq->pkt_last_seg = NULL; >>> +} >>> + >>> +int __rte_cold >>> +ngbe_dev_rx_queue_setup(struct rte_eth_dev *dev, >>> + uint16_t queue_idx, >>> + uint16_t nb_desc, >>> + unsigned int socket_id, >>> + const struct rte_eth_rxconf *rx_conf, >>> + struct rte_mempool *mp) >>> +{ >>> + const struct rte_memzone *rz; >>> + struct ngbe_rx_queue *rxq; >>> + struct ngbe_hw *hw; >>> + uint16_t len; >>> + struct ngbe_adapter *adapter = ngbe_dev_adapter(dev); >>> + >>> + PMD_INIT_FUNC_TRACE(); >>> + hw = ngbe_dev_hw(dev); >>> + >>> + /* >>> + * Validate number of receive descriptors. >>> + * It must not exceed hardware maximum, and must be multiple >>> + * of NGBE_ALIGN. >>> + */ >>> + if (nb_desc % NGBE_RXD_ALIGN != 0 || >>> + nb_desc > NGBE_RING_DESC_MAX || >>> + nb_desc < NGBE_RING_DESC_MIN) { >>> + return -EINVAL; >>> + } >> >> rte_eth_rx_queue_setup cares about it >> > > I don't quite understand. ethdev does the check based dev_info provided by the driver: if (nb_rx_desc > dev_info.rx_desc_lim.nb_max || nb_rx_desc < dev_info.rx_desc_lim.nb_min || nb_rx_desc % dev_info.rx_desc_lim.nb_align != 0) { RTE_ETHDEV_LOG(ERR, "Invalid value for nb_rx_desc(=%hu), should be: <= %hu, >= %hu, and a product of %hu\n", nb_rx_desc, dev_info.rx_desc_lim.nb_max, dev_info.rx_desc_lim.nb_min, dev_info.rx_desc_lim.nb_align); return -EINVAL; }