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 EA9E846128 for ; Wed, 29 Jan 2025 18:58:17 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CE7A0402E8; Wed, 29 Jan 2025 18:58:17 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 4DCCD4026B for ; Wed, 29 Jan 2025 18:58:16 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1738173495; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eT2AELLTrsruL941L7omLFKiKtlY3S59wOg0N9CwCBM=; b=Kgjms3g01VB/z0rxKLu8JBKNR86mQKANnL7uWha/FHmxOm8oSrI2NCmcRAz3vds+MA0yfI 3auS26ST7SI5dOarez9tX3YBBH0fVaWb/zKOdYLYPmMDZLiOYIATHCQgyApjXzKC0b+r4d tD9FCY3y/cW2OoQ1aw8qV4MoutUjBSg= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-644-sy-ylfLgOmC9St9Vkroihg-1; Wed, 29 Jan 2025 12:58:13 -0500 X-MC-Unique: sy-ylfLgOmC9St9Vkroihg-1 X-Mimecast-MFC-AGG-ID: sy-ylfLgOmC9St9Vkroihg Received: by mail-ed1-f70.google.com with SMTP id 4fb4d7f45d1cf-5dbe8a4165eso1340692a12.0 for ; Wed, 29 Jan 2025 09:58:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738173492; x=1738778292; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=eT2AELLTrsruL941L7omLFKiKtlY3S59wOg0N9CwCBM=; b=lAfUIM/dfbzyS/1CBoiNhEpljbC37ZPBa2Xpok8MWthsmQGGNXJ0/K0jd2HgmeNGqw pTaYXKZNcP4NI6wcAdEA4s8fasSrCs39HN6nFhFEra33/549ZmE52MN27ihE2mNi2aOJ yswPNjiaCYgyi7OG1+V7lqdLynYFzgd1THAcvbex3JfGtIv86CZiz4VnrcgdUC1M9U+x 7my1XEqLSiWFCcpW07rZkRxqpzBssDg+tbxhICOeeQ+yfNSSyms/WxG1zfBo8FM086s5 2QDjOSuA13IJKbJWB1R+WKoOi2GGWYwlaXqV2WaTYkQ3hIjgLcDm8sy/ekt4LqaNX64G RMJA== X-Gm-Message-State: AOJu0YwhJ5lDoAtEmkLp/kKJikT1b5AjYRusBaC2/9qjySwPURC5vqZd 5K8N75lhOZ3KA+5vKItNHLh5NbIRQii6F3RwnpLEueVp7VKAIKWUCXnU4BwOBly1fq8l/tK4G71 PUPU92kJef6SLdg1LlnClUyU6UxMZt9ylgX/dky4yor8o X-Gm-Gg: ASbGncu6ld1KBI3tKnxd+/jfCbO2JXSrBcIGqzlxfxwhh2PCbVYHAYDofjqHXf9z6Hs C8ncbC8tS9WSWgzM84PuWU1gj4erZvQDlUiIT4o5L5zKukgJ8nM4M6neUA4gshMJ2p/h+7vd5cl fZYUMlNHzgJ3aX8E6KCZboTHUwxdzHMlWj2TubVOCrbYJckU37QucR83gCSgmMaerz+lqDFkcGw ENYEkcg+bnLAo2EwliLpcfcbmBg8doCRSKTeiMCEAC/jLccl30y8ZYilo5fVSO7j1mxwK0vWKKU 5SC0ILvTWB8QEh8y0kPWkcNM5RRNRRqPoOxrQbkzwJLtjrKJQiAGQN6XRRcNRTo8daxSrV6KDju b X-Received: by 2002:a05:6402:1d4e:b0:5db:f5e9:6759 with SMTP id 4fb4d7f45d1cf-5dc6f618cfamr226360a12.14.1738173492152; Wed, 29 Jan 2025 09:58:12 -0800 (PST) X-Google-Smtp-Source: AGHT+IEl0qv5shsUClEXKy91sYtHe4EcxBAFE7Gy+S7ce2Ak6vuXorEx9cgSGYoIc3I4uYFr1vLi1Q== X-Received: by 2002:a05:6402:1d4e:b0:5db:f5e9:6759 with SMTP id 4fb4d7f45d1cf-5dc6f618cfamr226344a12.14.1738173491668; Wed, 29 Jan 2025 09:58:11 -0800 (PST) Received: from ?IPV6:2a06:5900:c00c:9000:106a:b694:33e7:1734? ([2a06:5900:c00c:9000:106a:b694:33e7:1734]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5dc1863b714sm9428660a12.45.2025.01.29.09.58.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 29 Jan 2025 09:58:11 -0800 (PST) Message-ID: Date: Wed, 29 Jan 2025 17:58:10 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/2] net/af_xdp: Refactor af_xdp_tx_zc() To: Ariel Otilibili , dev@dpdk.org Cc: stable@dpdk.org, Thomas Monjalon , David Marchand , Stephen Hemminger , Ciara Loftus References: <20250116195640.68885-1-ariel.otilibili@6wind.com> <20250128231152.249497-1-ariel.otilibili@6wind.com> <20250128231152.249497-3-ariel.otilibili@6wind.com> From: Maryam Tahhan In-Reply-To: <20250128231152.249497-3-ariel.otilibili@6wind.com> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 3npaqe8fb2lALjMY0pnhWtTO-EFCRqwmW1xehwC_bXU_1738173492 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org On 28/01/2025 23:11, Ariel Otilibili wrote: > Both legs of the loop share the same logic: either to go out of the > function, or to fall through to the next instructions. > > For that, maybe_cpy_pkt() is introduced. > > Bugzilla ID: 1440 > Signed-off-by: Ariel Otilibili > --- > drivers/net/af_xdp/rte_eth_af_xdp.c | 131 ++++++++++++++++++---------- > 1 file changed, 84 insertions(+), 47 deletions(-) > > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c > index 092bcb73aa0a..efe23283fb52 100644 > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c > @@ -536,13 +536,80 @@ kick_tx(struct pkt_tx_queue *txq, struct xsk_ring_cons *cq) > } > } > > +static inline uint64_t > +update_addr(struct rte_mbuf *mbuf, struct xsk_umem_info *umem) > +{ > + return (uint64_t)mbuf - (uint64_t)umem->buffer > + - umem->mb_pool->header_size; > +} > + > +static inline uint64_t > +update_offset(struct rte_mbuf *mbuf, struct xsk_umem_info *umem) > +{ > + uint64_t offset; > + > + offset = rte_pktmbuf_mtod(mbuf, uint64_t) - (uint64_t)mbuf > + + umem->mb_pool->header_size; > + return offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT; > +} > + > +static struct rte_mbuf * > +maybe_kick_tx(struct pkt_tx_queue *txq, uint32_t *idx_tx, struct rte_mbuf *mbuf) > +{ > + struct rte_mbuf *ret = mbuf; > + > + if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) { > + kick_tx(txq, &txq->pair->cq); > + if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) > + ret = NULL; > + } > + > + return ret; > +} [MT] I don't see why we are passing in mbuf here? > + > +static struct rte_mbuf * > +maybe_create_mbuf(struct pkt_tx_queue *txq, uint32_t *idx_tx, > + struct xsk_umem_info *umem) { > + struct rte_mbuf *local_mbuf = rte_pktmbuf_alloc(umem->mb_pool); > + > + if (local_mbuf == NULL) > + goto out; > + > + if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) { > + rte_pktmbuf_free(local_mbuf); > + local_mbuf = NULL; > + goto out; > + } > + > +out: > + return local_mbuf; > +} > + > +static void > +maybe_cpy_pkt(bool is_mbuf_equal, struct xsk_umem_info *umem, > + uint64_t addr_plus_offset, struct rte_mbuf *mbuf, > + struct xdp_desc *desc) > +{ > + void *pkt; > + > + if(is_mbuf_equal) > + goto out; > + > + pkt = xsk_umem__get_data(umem->buffer, addr_plus_offset); > + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); > + rte_pktmbuf_free(mbuf); > + > +out: > + return; > +} [MT] does this really need to be an inline function? it wasn't common code between the blocks? > + > #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG) > static uint16_t > af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) > { > struct pkt_tx_queue *txq = queue; > struct xsk_umem_info *umem = txq->umem; > - struct rte_mbuf *mbuf; > + struct rte_mbuf *mbuf, *local_mbuf; > unsigned long tx_bytes = 0; > int i; > uint32_t idx_tx; > @@ -551,61 +618,31 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) > uint64_t addr, offset; > struct xsk_ring_cons *cq = &txq->pair->cq; > uint32_t free_thresh = cq->size >> 1; > + bool is_true; > > if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh) > pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq); > > for (i = 0; i < nb_pkts; i++) { > mbuf = bufs[i]; > + is_true = mbuf->pool == umem->mb_pool; > > - if (mbuf->pool == umem->mb_pool) { > - if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) { > - kick_tx(txq, cq); > - if (!xsk_ring_prod__reserve(&txq->tx, 1, > - &idx_tx)) > - goto out; > - } > - desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx); > - desc->len = mbuf->pkt_len; > - addr = (uint64_t)mbuf - (uint64_t)umem->buffer - > - umem->mb_pool->header_size; > - offset = rte_pktmbuf_mtod(mbuf, uint64_t) - > - (uint64_t)mbuf + > - umem->mb_pool->header_size; > - offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT; > - desc->addr = addr | offset; > - tx_bytes += desc->len; > - count++; > - } else { > - struct rte_mbuf *local_mbuf = > - rte_pktmbuf_alloc(umem->mb_pool); > - void *pkt; > - > - if (local_mbuf == NULL) > - goto out; > + if (is_true) > + local_mbuf = maybe_kick_tx(txq, &idx_tx, mbuf); > + else > + local_mbuf = maybe_create_mbuf(txq, &idx_tx, umem); > > - if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) { > - rte_pktmbuf_free(local_mbuf); > - goto out; > - } > + if (!local_mbuf) > + goto out; > > - desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx); > - desc->len = mbuf->pkt_len; > - > - addr = (uint64_t)local_mbuf - (uint64_t)umem->buffer - > - umem->mb_pool->header_size; > - offset = rte_pktmbuf_mtod(local_mbuf, uint64_t) - > - (uint64_t)local_mbuf + > - umem->mb_pool->header_size; > - pkt = xsk_umem__get_data(umem->buffer, addr + offset); > - offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT; > - desc->addr = addr | offset; > - rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), > - desc->len); > - tx_bytes += desc->len; > - rte_pktmbuf_free(mbuf); > - count++; > - } > + desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx); > + desc->len = mbuf->pkt_len; > + addr = update_addr(local_mbuf, umem); > + offset = update_offset(local_mbuf, umem); > + desc->addr = addr | offset; > + maybe_cpy_pkt(is_true, umem, addr + offset, mbuf, desc); > + tx_bytes += desc->len; > + count++; > } > > out: Firstly thank you for your efforts to clean up the code. I believe a simpler cleanup approach would better balance readability + maintainability. This approach would focus on eliminating the repeated code in both branches of the conditional block. For me the main repetition between the branches is the code that reserves and fills the descriptor info, Please find an example below... Note: The following is all untested code (don't even know if it compiles) it's just to give an idea around the cleanup I had in mind: static inline int reserve_and_fill_desc(struct pkt_tx_queue *txq, struct xdp_desc **desc, struct rte_mbuf *mbuf, struct xsk_umem_info *umem, uint32_t *idx_tx) { uint64_t addr, offset; if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) return -1; *desc = xsk_ring_prod__tx_desc(&txq->tx, *idx_tx); (*desc)->len = mbuf->pkt_len; addr = (uint64_t)mbuf - (uint64_t)umem->buffer - umem->mb_pool->header_size; offset = rte_pktmbuf_mtod(mbuf, uint64_t) - (uint64_t)mbuf + umem->mb_pool->header_size; offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT; (*desc)->addr = addr | offset; return 0; } Then the main function would look something like: static uint16_t af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) { struct pkt_tx_queue *txq = queue; struct xsk_umem_info *umem = txq->umem; struct rte_mbuf *mbuf; unsigned long tx_bytes = 0; int i; uint32_t idx_tx; uint16_t count = 0; struct xdp_desc *desc; struct xsk_ring_cons *cq = &txq->pair->cq; uint32_t free_thresh = cq->size >> 1; if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh) pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq); for (i = 0; i < nb_pkts; i++) { mbuf = bufs[i]; if (mbuf->pool == umem->mb_pool) { if (reserve_and_fill_desc(txq, &desc, mbuf, umem, &idx_tx) < 0) { kick_tx(txq, cq); if (reserve_and_fill_desc(txq, &desc, mbuf, umem, &idx_tx) < 0) goto out; } tx_bytes += desc->len; count++; } else { struct rte_mbuf *local_mbuf = rte_pktmbuf_alloc(umem->mb_pool); void *pkt; if (local_mbuf == NULL) goto out; if (reserve_and_fill_desc(txq, &desc, local_mbuf, umem, &idx_tx) < 0) { rte_pktmbuf_free(local_mbuf); goto out; } pkt = xsk_umem__get_data(umem->buffer, (desc->addr & ~0xFFF) + (desc->addr & 0xFFF)); rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len); rte_pktmbuf_free(mbuf); tx_bytes += desc->len; count++; } } out: xsk_ring_prod__submit(&txq->tx, count); kick_tx(txq, cq); txq->stats.tx_pkts += count; txq->stats.tx_bytes += tx_bytes; txq->stats.tx_dropped += nb_pkts - count; return count; } Please let me know your thoughts, and I’d be happy to discuss further