[dpdk-dev] [PATCH] net/af_xdp: fix zero copy Tx queue drain

[dpdk-dev] [PATCH] net/af_xdp: fix zero copy Tx queue drain

[Baruch Siach]

Call xsk_ring_prod__submit() before kick_tx() so that the kernel
consumer sees the updated state of Tx ring. Otherwise, Tx packets are
stuck in the ring until the next call to af_xdp_tx_zc().

Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
Cc: stable@dpdk.org

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 74ffa4511284..81998d86b4aa 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -502,10 +502,11 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 		if (mbuf->pool == umem->mb_pool) {
 			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
+				xsk_ring_prod__submit(&txq->tx, count);
 				kick_tx(txq, cq);
 				if (!xsk_ring_prod__reserve(&txq->tx, 1,
 							    &idx_tx))
-					goto out;
+					goto out_skip_tx;
 			}
 			desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
 			desc->len = mbuf->pkt_len;
@@ -527,7 +528,6 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
 				rte_pktmbuf_free(local_mbuf);
-				kick_tx(txq, cq);
 				goto out;
 			}
 
@@ -551,11 +551,11 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		tx_bytes += mbuf->pkt_len;
 	}
 
-	kick_tx(txq, cq);
-
 out:
 	xsk_ring_prod__submit(&txq->tx, count);
+	kick_tx(txq, cq);
 
+out_skip_tx:
 	txq->stats.tx_pkts += count;
 	txq->stats.tx_bytes += tx_bytes;
 	txq->stats.tx_dropped += nb_pkts - count;
-- 
2.32.0

Re: [dpdk-dev] [PATCH] net/af_xdp: fix zero copy Tx queue drain

[Loftus, Ciara]

> 
> Call xsk_ring_prod__submit() before kick_tx() so that the kernel
> consumer sees the updated state of Tx ring. Otherwise, Tx packets are
> stuck in the ring until the next call to af_xdp_tx_zc().
> 
> Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/net/af_xdp/rte_eth_af_xdp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 74ffa4511284..81998d86b4aa 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -502,10 +502,11 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> 
>  		if (mbuf->pool == umem->mb_pool) {
>  			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
> +				xsk_ring_prod__submit(&txq->tx, count);

We cannot submit 'count' to the tx ring both here and at 'out'. We could end up submitting too many.

>  				kick_tx(txq, cq);

The purpose of this kick is not necessarily to transmit new descriptors but to drain the completion queue. No space in the completion queue may be what is preventing the kernel from transmitting existing tx buffers and then in userspace causing the the xsk_ring_prod__reserve to fail.
We are not trying to transmit new descriptors here.

>  				if (!xsk_ring_prod__reserve(&txq->tx, 1,
>  							    &idx_tx))
> -					goto out;
> +					goto out_skip_tx;
>  			}
>  			desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
>  			desc->len = mbuf->pkt_len;
> @@ -527,7 +528,6 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
> 
>  			if (!xsk_ring_prod__reserve(&txq->tx, 1, &idx_tx)) {
>  				rte_pktmbuf_free(local_mbuf);
> -				kick_tx(txq, cq);
>  				goto out;
>  			}
> 
> @@ -551,11 +551,11 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
>  		tx_bytes += mbuf->pkt_len;
>  	}
> 
> -	kick_tx(txq, cq);
> -
>  out:
>  	xsk_ring_prod__submit(&txq->tx, count);
> +	kick_tx(txq, cq);

I think this change is valid. We should kick here after the submit.
Thanks for the patch. Could you please spin a v2 if you agree with the above?

Thanks,
Ciara

> 
> +out_skip_tx:
>  	txq->stats.tx_pkts += count;
>  	txq->stats.tx_bytes += tx_bytes;
>  	txq->stats.tx_dropped += nb_pkts - count;
> --
> 2.32.0