<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Thu, Jan 16, 2025 at 11:26 PM Stephen Hemminger <<a href="mailto:stephen@networkplumber.org">stephen@networkplumber.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, 16 Jan 2025 23:20:06 +0100<br>
Ariel Otilibili <<a href="mailto:ariel.otilibili@6wind.com" target="_blank">ariel.otilibili@6wind.com</a>> wrote:<br>
<br>
> Hi Stephen,<br>
> <br>
> On Thu, Jan 16, 2025 at 10:47 PM Stephen Hemminger <<br>
> <a href="mailto:stephen@networkplumber.org" target="_blank">stephen@networkplumber.org</a>> wrote: <br>
> <br>
> > On Thu, 16 Jan 2025 20:56:39 +0100<br>
> > Ariel Otilibili <<a href="mailto:ariel.otilibili@6wind.com" target="_blank">ariel.otilibili@6wind.com</a>> wrote:<br>
> > <br>
> > > Both branches of the loop share the same logic. Now each one is a<br>
> > > goto dispatcher; either to out (end of function), or to<br>
> > > stats (continuation of the loop).<br>
> > ><br>
> > > Bugzilla ID: 1440<br>
> > > Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")<br>
> > > Signed-off-by: Ariel Otilibili <<a href="mailto:ariel.otilibili@6wind.com" target="_blank">ariel.otilibili@6wind.com</a>><br>
> > > ---<br>
> > > drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++---------------<br>
> > > 1 file changed, 27 insertions(+), 30 deletions(-)<br>
> > ><br>
> > > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c <br>
> > b/drivers/net/af_xdp/rte_eth_af_xdp.c <br>
> > > index 4326a29f7042..8b42704b6d9f 100644<br>
> > > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c<br>
> > > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c<br>
> > > @@ -551,6 +551,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, <br>
> > uint16_t nb_pkts) <br>
> > > uint64_t addr, offset;<br>
> > > struct xsk_ring_cons *cq = &txq->pair->cq;<br>
> > > uint32_t free_thresh = cq->size >> 1;<br>
> > > + struct rte_mbuf *local_mbuf = NULL;<br>
> > ><br>
> > > if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)<br>
> > > pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);<br>
> > > @@ -565,21 +566,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, <br>
> > uint16_t nb_pkts) <br>
> > > &idx_tx))<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>
> > > - tx_bytes += mbuf->pkt_len;<br>
> > > - count++;<br>
> > > +<br>
> > > + goto stats;<br>
> > > } else {<br>
> > > - struct rte_mbuf *local_mbuf =<br>
> > > - rte_pktmbuf_alloc(umem->mb_pool);<br>
> > > - void *pkt;<br>
> > > + local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);<br>
> > ><br>
> > > if (local_mbuf == NULL)<br>
> > > goto out;<br>
> > > @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, <br>
> > uint16_t nb_pkts) <br>
> > > goto out;<br>
> > > }<br>
> > ><br>
> > > - desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);<br>
> > > - desc->len = mbuf->pkt_len;<br>
> > > -<br>
> > > - addr = (uint64_t)local_mbuf - <br>
> > (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 + <br>
> > offset); <br>
> > > - offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;<br>
> > > - desc->addr = addr | offset;<br>
> > > - rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),<br>
> > > - desc->len);<br>
> > > - tx_bytes += mbuf->pkt_len;<br>
> > > - rte_pktmbuf_free(mbuf);<br>
> > > - count++;<br>
> > > + goto stats;<br>
> > > }<br>
> > > +stats:<br>
> > > + struct rte_mbuf *tmp;<br>
> > > + void *pkt;<br>
> > > + tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;<br>
> > > +<br>
> > > + desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);<br>
> > > + desc->len = mbuf->pkt_len;<br>
> > > +<br>
> > > + addr = (uint64_t)tmp - (uint64_t)umem->buffer - <br>
> > umem->mb_pool->header_size; <br>
> > > + offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp + <br>
> > umem->mb_pool->header_size; <br>
> > > + offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;<br>
> > > + desc->addr = addr | offset;<br>
> > > +<br>
> > > + if (mbuf->pool == umem->mb_pool) {<br>
> > > + tx_bytes += mbuf->pkt_len;<br>
> > > + } else {<br>
> > > + pkt = xsk_umem__get_data(umem->buffer, addr + offset);<br>
> > > + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);<br>
> > > + tx_bytes += mbuf->pkt_len;<br>
> > > + rte_pktmbuf_free(mbuf);<br>
> > > + }<br>
> > > + count++;<br>
> > > }<br>
> > ><br>
> > > out: <br>
> ><br>
> > Indentation here is wrong, and looks suspect.<br>
> > Either stats tag should be outside of loop<br>
> > Or stats is inside loop, and both of those goto's are unnecessary<br>
> > <br>
> Thanks for the feedback; I am pushing a new series with an extra tab.<br>
> So it be obvious that stats belongs to the the loop.<br>
<br>
<br>
But the the goto;s aren't needed? Both legs of the If would fall through<br>
to that location.<br></blockquote><div>You are right, Stephen; thanks for the heads up. I am pushing that change, without any goto in each leg, so fall through.</div><div>Bear with me for some time.<br></div></div></div>