diff mbox series

[2/2] net/af_xdp: Refactor af_xdp_tx_zc()

Message ID 20250116195640.68885-3-ariel.otilibili@6wind.com (mailing list archive)
State Changes Requested
Delegated to: Stephen Hemminger
Headers
Series Fix use after free, and refactor af_xdp_tx_zc() |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build fail github build: failed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-unit-amd64-testing warning Testing issues
ci/iol-sample-apps-testing warning Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing warning Testing issues
ci/iol-unit-arm64-testing warning Testing issues
ci/iol-compile-arm64-testing fail Testing issues
ci/iol-compile-amd64-testing fail Testing issues

Commit Message

Ariel Otilibili Jan. 16, 2025, 7:56 p.m. UTC 
Both branches of the loop share the same logic. Now each one is a
goto dispatcher; either to out (end of function), or to
stats (continuation of the loop).

Bugzilla ID: 1440
Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")
Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++---------------
 1 file changed, 27 insertions(+), 30 deletions(-)

Comments

Stephen Hemminger Jan. 16, 2025, 9:47 p.m. UTC | #1 
On Thu, 16 Jan 2025 20:56:39 +0100
Ariel Otilibili <ariel.otilibili@6wind.com> wrote:

> Both branches of the loop share the same logic. Now each one is a
> goto dispatcher; either to out (end of function), or to
> stats (continuation of the loop).
> 
> Bugzilla ID: 1440
> Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")
> Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> ---
>  drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++---------------
>  1 file changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 4326a29f7042..8b42704b6d9f 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -551,6 +551,7 @@ 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;
> +	struct rte_mbuf *local_mbuf = NULL;
>  
>  	if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
>  		pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
> @@ -565,21 +566,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  							    &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 += mbuf->pkt_len;
> -			count++;
> +
> +			goto stats;
>  		} else {
> -			struct rte_mbuf *local_mbuf =
> -					rte_pktmbuf_alloc(umem->mb_pool);
> -			void *pkt;
> +			local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
>  
>  			if (local_mbuf == NULL)
>  				goto out;
> @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  				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 += mbuf->pkt_len;
> -			rte_pktmbuf_free(mbuf);
> -			count++;
> +			goto stats;
>  		}
> +stats:
> +	struct rte_mbuf *tmp;
> +	void *pkt;
> +	tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;
> +
> +	desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> +	desc->len = mbuf->pkt_len;
> +
> +	addr = (uint64_t)tmp - (uint64_t)umem->buffer - umem->mb_pool->header_size;
> +	offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp + umem->mb_pool->header_size;
> +	offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> +	desc->addr = addr | offset;
> +
> +	if (mbuf->pool == umem->mb_pool) {
> +		tx_bytes += mbuf->pkt_len;
> +	} else {
> +		pkt = xsk_umem__get_data(umem->buffer, addr + offset);
> +		rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
> +		tx_bytes += mbuf->pkt_len;
> +		rte_pktmbuf_free(mbuf);
> +	}
> +	count++;
>  	}
>  
>  out:

Indentation here is wrong, and looks suspect.
Either stats tag should be outside of loop
Or stats is inside loop, and both of those goto's are unnecessary
Ariel Otilibili Jan. 16, 2025, 10:20 p.m. UTC | #2 
Hi Stephen,

On Thu, Jan 16, 2025 at 10:47 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Thu, 16 Jan 2025 20:56:39 +0100
> Ariel Otilibili <ariel.otilibili@6wind.com> wrote:
>
> > Both branches of the loop share the same logic. Now each one is a
> > goto dispatcher; either to out (end of function), or to
> > stats (continuation of the loop).
> >
> > Bugzilla ID: 1440
> > Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")
> > Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> > ---
> >  drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++---------------
> >  1 file changed, 27 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > index 4326a29f7042..8b42704b6d9f 100644
> > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > @@ -551,6 +551,7 @@ 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;
> > +     struct rte_mbuf *local_mbuf = NULL;
> >
> >       if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
> >               pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
> > @@ -565,21 +566,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
> >                                                           &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 += mbuf->pkt_len;
> > -                     count++;
> > +
> > +                     goto stats;
> >               } else {
> > -                     struct rte_mbuf *local_mbuf =
> > -                                     rte_pktmbuf_alloc(umem->mb_pool);
> > -                     void *pkt;
> > +                     local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
> >
> >                       if (local_mbuf == NULL)
> >                               goto out;
> > @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
> >                               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 += mbuf->pkt_len;
> > -                     rte_pktmbuf_free(mbuf);
> > -                     count++;
> > +                     goto stats;
> >               }
> > +stats:
> > +     struct rte_mbuf *tmp;
> > +     void *pkt;
> > +     tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;
> > +
> > +     desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> > +     desc->len = mbuf->pkt_len;
> > +
> > +     addr = (uint64_t)tmp - (uint64_t)umem->buffer -
> umem->mb_pool->header_size;
> > +     offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp +
> umem->mb_pool->header_size;
> > +     offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> > +     desc->addr = addr | offset;
> > +
> > +     if (mbuf->pool == umem->mb_pool) {
> > +             tx_bytes += mbuf->pkt_len;
> > +     } else {
> > +             pkt = xsk_umem__get_data(umem->buffer, addr + offset);
> > +             rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
> > +             tx_bytes += mbuf->pkt_len;
> > +             rte_pktmbuf_free(mbuf);
> > +     }
> > +     count++;
> >       }
> >
> >  out:
>
> Indentation here is wrong, and looks suspect.
> Either stats tag should be outside of loop
> Or stats is inside loop, and both of those goto's are unnecessary
>
Thanks for the feedback; I am pushing a new series with an extra tab.
So it be obvious that stats belongs to the the loop.
Stephen Hemminger Jan. 16, 2025, 10:26 p.m. UTC | #3 
On Thu, 16 Jan 2025 23:20:06 +0100
Ariel Otilibili <ariel.otilibili@6wind.com> wrote:

> Hi Stephen,
> 
> On Thu, Jan 16, 2025 at 10:47 PM Stephen Hemminger <
> stephen@networkplumber.org> wrote:  
> 
> > On Thu, 16 Jan 2025 20:56:39 +0100
> > Ariel Otilibili <ariel.otilibili@6wind.com> wrote:
> >  
> > > Both branches of the loop share the same logic. Now each one is a
> > > goto dispatcher; either to out (end of function), or to
> > > stats (continuation of the loop).
> > >
> > > Bugzilla ID: 1440
> > > Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")
> > > Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> > > ---
> > >  drivers/net/af_xdp/rte_eth_af_xdp.c | 57 ++++++++++++++---------------
> > >  1 file changed, 27 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c  
> > b/drivers/net/af_xdp/rte_eth_af_xdp.c  
> > > index 4326a29f7042..8b42704b6d9f 100644
> > > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> > > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > > @@ -551,6 +551,7 @@ 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;
> > > +     struct rte_mbuf *local_mbuf = NULL;
> > >
> > >       if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
> > >               pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
> > > @@ -565,21 +566,10 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs,  
> > uint16_t nb_pkts)  
> > >                                                           &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 += mbuf->pkt_len;
> > > -                     count++;
> > > +
> > > +                     goto stats;
> > >               } else {
> > > -                     struct rte_mbuf *local_mbuf =
> > > -                                     rte_pktmbuf_alloc(umem->mb_pool);
> > > -                     void *pkt;
> > > +                     local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
> > >
> > >                       if (local_mbuf == NULL)
> > >                               goto out;
> > > @@ -589,23 +579,30 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs,  
> > uint16_t nb_pkts)  
> > >                               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 += mbuf->pkt_len;
> > > -                     rte_pktmbuf_free(mbuf);
> > > -                     count++;
> > > +                     goto stats;
> > >               }
> > > +stats:
> > > +     struct rte_mbuf *tmp;
> > > +     void *pkt;
> > > +     tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;
> > > +
> > > +     desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> > > +     desc->len = mbuf->pkt_len;
> > > +
> > > +     addr = (uint64_t)tmp - (uint64_t)umem->buffer -  
> > umem->mb_pool->header_size;  
> > > +     offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp +  
> > umem->mb_pool->header_size;  
> > > +     offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> > > +     desc->addr = addr | offset;
> > > +
> > > +     if (mbuf->pool == umem->mb_pool) {
> > > +             tx_bytes += mbuf->pkt_len;
> > > +     } else {
> > > +             pkt = xsk_umem__get_data(umem->buffer, addr + offset);
> > > +             rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
> > > +             tx_bytes += mbuf->pkt_len;
> > > +             rte_pktmbuf_free(mbuf);
> > > +     }
> > > +     count++;
> > >       }
> > >
> > >  out:  
> >
> > Indentation here is wrong, and looks suspect.
> > Either stats tag should be outside of loop
> > Or stats is inside loop, and both of those goto's are unnecessary
> >  
> Thanks for the feedback; I am pushing a new series with an extra tab.
> So it be obvious that stats belongs to the the loop.


But the the goto;s aren't needed? Both legs of the If would fall through
to that location.
Ariel Otilibili Jan. 16, 2025, 10:36 p.m. UTC | #4 
On Thu, Jan 16, 2025 at 11:26 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

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

Patch

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 4326a29f7042..8b42704b6d9f 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -551,6 +551,7 @@  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;
+	struct rte_mbuf *local_mbuf = NULL;
 
 	if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
 		pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
@@ -565,21 +566,10 @@  af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 							    &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 += mbuf->pkt_len;
-			count++;
+
+			goto stats;
 		} else {
-			struct rte_mbuf *local_mbuf =
-					rte_pktmbuf_alloc(umem->mb_pool);
-			void *pkt;
+			local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
 
 			if (local_mbuf == NULL)
 				goto out;
@@ -589,23 +579,30 @@  af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 				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 += mbuf->pkt_len;
-			rte_pktmbuf_free(mbuf);
-			count++;
+			goto stats;
 		}
+stats:
+	struct rte_mbuf *tmp;
+	void *pkt;
+	tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;
+
+	desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
+	desc->len = mbuf->pkt_len;
+
+	addr = (uint64_t)tmp - (uint64_t)umem->buffer - umem->mb_pool->header_size;
+	offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp + umem->mb_pool->header_size;
+	offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
+	desc->addr = addr | offset;
+
+	if (mbuf->pool == umem->mb_pool) {
+		tx_bytes += mbuf->pkt_len;
+	} else {
+		pkt = xsk_umem__get_data(umem->buffer, addr + offset);
+		rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
+		tx_bytes += mbuf->pkt_len;
+		rte_pktmbuf_free(mbuf);
+	}
+	count++;
 	}
 
 out: