<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>