<div dir="auto"><div><br><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Sat 1 Feb 2025, 10:03 Ariel Otilibili, <<a href="mailto:ariel.otilibili@6wind.com">ariel.otilibili@6wind.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Both legs of the loop share the same logic: the common parts are about<br>
reserving and filling both address and length into the description.<br>
<br>
This is moved into reserve_and_fill().<br>
<br>
Bugzilla ID: 1440<br>
Suggested-by: Maryam Tahhan <<a href="mailto:mtahhan@redhat.com" target="_blank" rel="noreferrer">mtahhan@redhat.com</a>><br>
Signed-off-by: Ariel Otilibili <<a href="mailto:ariel.otilibili@6wind.com" target="_blank" rel="noreferrer">ariel.otilibili@6wind.com</a>><br>
---<br>
drivers/net/af_xdp/rte_eth_af_xdp.c | 62 ++++++++++++++++-------------<br>
1 file changed, 34 insertions(+), 28 deletions(-)<br>
<br>
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c<br>
index 092bcb73aa0a..840a12dbf508 100644<br>
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c<br>
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c<br>
@@ -536,6 +536,31 @@ kick_tx(struct pkt_tx_queue *txq, struct xsk_ring_cons *cq)<br>
}<br>
}<br>
<br>
+static inline struct xdp_desc *<br>
+reserve_and_fill(struct pkt_tx_queue *txq, struct rte_mbuf *mbuf,<br>
+ struct xsk_umem_info *umem)<br>
+{<br>
+ struct xdp_desc *desc = NULL;<br>
+ uint32_t *idx_tx = NULL;<br>
+ uint64_t addr, offset;<br>
+<br>
+ if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx))<br>
+ goto out;<br>
+<br>
+ desc = xsk_ring_prod__tx_desc(&txq->tx, *idx_tx);<br>
+ desc->len = mbuf->pkt_len;<br>
+<br>
+ addr = (uint64_t)mbuf - (uint64_t)umem->buffer<br>
+ - umem->mb_pool->header_size;<br>
+ offset = rte_pktmbuf_mtod(mbuf, uint64_t) - (uint64_t)mbuf<br>
+ + umem->mb_pool->header_size;<br>
+ offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;<br>
+ desc->addr = addr | offset;<br>
+<br>
+out:<br>
+ return desc;<br>
+}<br>
+<br>
#if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)<br>
static uint16_t<br>
af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)<br>
@@ -545,10 +570,8 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)<br>
struct rte_mbuf *mbuf;<br>
unsigned long tx_bytes = 0;<br>
int i;<br>
- uint32_t idx_tx;<br>
uint16_t count = 0;<br>
struct xdp_desc *desc;<br>
- uint64_t addr, offset;<br>
struct xsk_ring_cons *cq = &txq->pair->cq;<br>
uint32_t free_thresh = cq->size >> 1;<br>
<br>
@@ -559,21 +582,12 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)<br>
mbuf = bufs[i];<br>
<br>
if (mbuf->pool == umem->mb_pool) {<br>
- if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {<br>
+ if (!(desc = reserve_and_fill(txq, mbuf, umem))) {<br>
kick_tx(txq, cq);<br>
- if (!xsk_ring_prod__reserve(&txq->tx, 1,<br>
- &idx_tx))<br>
+ if (!(desc = reserve_and_fill(txq, mbuf, umem)))<br>
goto out;<br>
}<br>
- desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);<br>
- desc->len = mbuf->pkt_len;<br>
- addr = (uint64_t)mbuf - (uint64_t)umem->buffer -<br>
- umem->mb_pool->header_size;<br>
- offset = rte_pktmbuf_mtod(mbuf, uint64_t) -<br>
- (uint64_t)mbuf +<br>
- umem->mb_pool->header_size;<br>
- offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;<br>
- desc->addr = addr | offset;<br>
+<br>
tx_bytes += desc->len;<br>
count++;<br>
} else {<br>
@@ -584,26 +598,18 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)<br>
if (local_mbuf == NULL)<br>
goto out;<br>
<br>
- if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {<br>
+ if (!(desc = reserve_and_fill(txq, local_mbuf, umem))) {<br>
rte_pktmbuf_free(local_mbuf);<br>
goto out;<br>
}<br>
<br>
- desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);<br>
- desc->len = mbuf->pkt_len;<br>
-<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">So I think i spotted one issue, you might need override desc->len after the call to the reserve_and_fill function so as it's not taken supposed to be from local_mbuf here.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote gmail_quote_container"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- addr = (uint64_t)local_mbuf - (uint64_t)umem->buffer -<br>
- umem->mb_pool->header_size;<br>
- offset = rte_pktmbuf_mtod(local_mbuf, uint64_t) -<br>
- (uint64_t)local_mbuf +<br>
- umem->mb_pool->header_size;<br>
- pkt = xsk_umem__get_data(umem->buffer, addr + offset);<br>
- offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;<br>
- desc->addr = addr | offset;<br>
+ pkt = xsk_umem__get_data(umem->buffer,<br>
+ (desc->addr & ~0xFFF)<br>
+ + (desc->addr & 0xFFF));<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Would prefer to move this pkt assignment to reserve_and_fill()</div><div dir="auto">What if we passed a void **ppkt to reserve_and_fill() then kept the original logic just wrapped in a NULL check?</div><div dir="auto"><br></div><div dir="auto"><div dir="auto"> if (ppkt) {</div><div dir="auto"> *ppkt = xsk_umem__get_data(umem->buffer, addr + offset);</div><div dir="auto"> }</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">Before the offset and desc->addr </div><div dir="auto"> offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;</div><div dir="auto"> desc->addr = addr | offset;</div><div dir="auto"><br></div></div><div dir="auto">WDYT? </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote gmail_quote_container"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),<br>
- desc->len);<br>
- tx_bytes += desc->len;<br>
+ desc->len);<br>
rte_pktmbuf_free(mbuf);<br>
+ tx_bytes += desc->len;<br>
count++;<br>
}<br>
}<br>
-- <br>
2.30.2<br>
<br>
</blockquote></div></div></div>