DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
* [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
@ 2019-09-09 16:12 Xiaolong Ye
  2019-09-10  4:14 ` Zhang, Qi Z
  0 siblings, 1 reply; 9+ messages in thread
From: Xiaolong Ye @ 2019-09-09 16:12 UTC (permalink / raw)
  To: Ferruh Yigit, Loftus Ciara, Xiaolong Ye, Qi Zhang; +Cc: dev, stable

The kernel only consumes Tx packets if we have some Rx traffic on specified
queue or we have called send(). So we need to issue a send() even when the
allocation fails so that kernel will start to consume packets again.

Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature") breaks
above rule by adding some condition to send, this patch fixes it while
still keeps the need_wakeup feature for Tx.

Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
Cc: stable@dpdk.org

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 41ed5b2af..e496e9aaa 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)
 {
 	struct xsk_umem_info *umem = txq->pair->umem;
 
-#if defined(XDP_USE_NEED_WAKEUP)
-	if (xsk_ring_prod__needs_wakeup(&txq->tx))
-#endif
-		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
-			    0, MSG_DONTWAIT) < 0) {
-			/* some thing unexpected */
-			if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
-				break;
-
-			/* pull from completion queue to leave more space */
-			if (errno == EAGAIN)
-				pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
-		}
+	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
+		    0, MSG_DONTWAIT) < 0) {
+		/* some thing unexpected */
+		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
+			break;
+
+		/* pull from completion queue to leave more space */
+		if (errno == EAGAIN)
+			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
+	}
 	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
 }
 
@@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 
 	xsk_ring_prod__submit(&txq->tx, nb_pkts);
 
-	kick_tx(txq);
+#if defined(XDP_USE_NEED_WAKEUP)
+	if (xsk_ring_prod__needs_wakeup(&txq->tx))
+#endif
+		kick_tx(txq);
 
 	txq->stats.tx_pkts += nb_pkts;
 	txq->stats.tx_bytes += tx_bytes;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-09 16:12 [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets Xiaolong Ye
@ 2019-09-10  4:14 ` Zhang, Qi Z
  2019-09-10 13:53   ` Ye Xiaolong
  0 siblings, 1 reply; 9+ messages in thread
From: Zhang, Qi Z @ 2019-09-10  4:14 UTC (permalink / raw)
  To: Ye, Xiaolong, Yigit, Ferruh, Loftus, Ciara; +Cc: dev, stable



> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, September 10, 2019 12:13 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> 
> The kernel only consumes Tx packets if we have some Rx traffic on specified
> queue or we have called send(). So we need to issue a send() even when the
> allocation fails so that kernel will start to consume packets again.

So "allocation fails" means " xsk_ring_prod__reserve" fail right?
I don't understand when xsk_ring_prod__needs_wakeup is true why kernel will stop Tx packet at this situation 
would you share more insight?

Thanks
Qi

> 
> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature") breaks
> above rule by adding some condition to send, this patch fixes it while still
> keeps the need_wakeup feature for Tx.
> 
> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 41ed5b2af..e496e9aaa 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
>  	struct xsk_umem_info *umem = txq->pair->umem;
> 
> -#if defined(XDP_USE_NEED_WAKEUP)
> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> -#endif
> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> -			    0, MSG_DONTWAIT) < 0) {
> -			/* some thing unexpected */
> -			if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
> -				break;
> -
> -			/* pull from completion queue to leave more space */
> -			if (errno == EAGAIN)
> -				pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
> -		}
> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> +		    0, MSG_DONTWAIT) < 0) {
> +		/* some thing unexpected */
> +		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
> +			break;
> +
> +		/* pull from completion queue to leave more space */
> +		if (errno == EAGAIN)
> +			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
> +	}
>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
> 
> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> 
>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
> 
> -	kick_tx(txq);
> +#if defined(XDP_USE_NEED_WAKEUP)
> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> +#endif
> +		kick_tx(txq);
> 
>  	txq->stats.tx_pkts += nb_pkts;
>  	txq->stats.tx_bytes += tx_bytes;
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-10  4:14 ` Zhang, Qi Z
@ 2019-09-10 13:53   ` Ye Xiaolong
  2019-09-10 14:41     ` Zhang, Qi Z
  2019-09-11  0:12     ` Zhang, Qi Z
  0 siblings, 2 replies; 9+ messages in thread
From: Ye Xiaolong @ 2019-09-10 13:53 UTC (permalink / raw)
  To: Zhang, Qi Z; +Cc: Yigit, Ferruh, Loftus, Ciara, dev, stable, Karlsson, Magnus

On 09/10, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Ye, Xiaolong
>> Sent: Tuesday, September 10, 2019 12:13 AM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
>> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
>> 
>> The kernel only consumes Tx packets if we have some Rx traffic on specified
>> queue or we have called send(). So we need to issue a send() even when the
>> allocation fails so that kernel will start to consume packets again.
>
>So "allocation fails" means " xsk_ring_prod__reserve" fail right?

Yes.

>I don't understand when xsk_ring_prod__needs_wakeup is true why kernel will stop Tx packet at this situation 
>would you share more insight?

Actually, the fail case is xsk_ring_prod__needs_wakeup is false, then we can't
issue a send() when xsk_ring_prod__reserve fails. 

Thanks,
Xiaolong

>
>Thanks
>Qi
>
>> 
>> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature") breaks
>> above rule by adding some condition to send, this patch fixes it while still
>> keeps the need_wakeup feature for Tx.
>> 
>> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
>> Cc: stable@dpdk.org
>> 
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> ---
>>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> index 41ed5b2af..e496e9aaa 100644
>> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
>>  	struct xsk_umem_info *umem = txq->pair->umem;
>> 
>> -#if defined(XDP_USE_NEED_WAKEUP)
>> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
>> -#endif
>> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
>> -			    0, MSG_DONTWAIT) < 0) {
>> -			/* some thing unexpected */
>> -			if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
>> -				break;
>> -
>> -			/* pull from completion queue to leave more space */
>> -			if (errno == EAGAIN)
>> -				pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
>> -		}
>> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
>> +		    0, MSG_DONTWAIT) < 0) {
>> +		/* some thing unexpected */
>> +		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
>> +			break;
>> +
>> +		/* pull from completion queue to leave more space */
>> +		if (errno == EAGAIN)
>> +			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
>> +	}
>>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
>> 
>> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf
>> **bufs, uint16_t nb_pkts)
>> 
>>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
>> 
>> -	kick_tx(txq);
>> +#if defined(XDP_USE_NEED_WAKEUP)
>> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
>> +#endif
>> +		kick_tx(txq);
>> 
>>  	txq->stats.tx_pkts += nb_pkts;
>>  	txq->stats.tx_bytes += tx_bytes;
>> --
>> 2.17.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-10 13:53   ` Ye Xiaolong
@ 2019-09-10 14:41     ` Zhang, Qi Z
  2019-09-10 15:09       ` Ye Xiaolong
  2019-09-11  0:12     ` Zhang, Qi Z
  1 sibling, 1 reply; 9+ messages in thread
From: Zhang, Qi Z @ 2019-09-10 14:41 UTC (permalink / raw)
  To: Ye, Xiaolong; +Cc: Yigit, Ferruh, Loftus, Ciara, dev, stable, Karlsson, Magnus



> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, September 10, 2019 9:54 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
> Magnus <magnus.karlsson@intel.com>
> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> 
> On 09/10, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ye, Xiaolong
> >> Sent: Tuesday, September 10, 2019 12:13 AM
> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> >> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> >> Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> >>
> >> The kernel only consumes Tx packets if we have some Rx traffic on
> >> specified queue or we have called send(). So we need to issue a
> >> send() even when the allocation fails so that kernel will start to consume
> packets again.
> >
> >So "allocation fails" means " xsk_ring_prod__reserve" fail right?
> 
> Yes.
> 
> >I don't understand when xsk_ring_prod__needs_wakeup is true why kernel
> >will stop Tx packet at this situation would you share more insight?
> 
> Actually, the fail case is xsk_ring_prod__needs_wakeup is false, then we
> can't issue a send() when xsk_ring_prod__reserve fails.

Sorry, I think my question should be for the case when xsk_ring_prod__needs_wakeup is false,
I don't understand why we need to handle different at below two situations
1. when xsk_ring_prod__reserve fails
2. normal tx scenario.
My understanding is when xsk_ring_prod__needs_wakeup(tx) is false, which means Tx is ongoing, we don't need to wake up kernel to continue.

> 
> Thanks,
> Xiaolong
> 
> >
> >Thanks
> >Qi
> >
> >>
> >> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> >> breaks above rule by adding some condition to send, this patch fixes
> >> it while still keeps the need_wakeup feature for Tx.
> >>
> >> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >> ---
> >>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28
> >> ++++++++++++++--------------
> >>  1 file changed, 14 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> index 41ed5b2af..e496e9aaa 100644
> >> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
> >>  	struct xsk_umem_info *umem = txq->pair->umem;
> >>
> >> -#if defined(XDP_USE_NEED_WAKEUP)
> >> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> -#endif
> >> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> -			    0, MSG_DONTWAIT) < 0) {
> >> -			/* some thing unexpected */
> >> -			if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
> >> -				break;
> >> -
> >> -			/* pull from completion queue to leave more space */
> >> -			if (errno == EAGAIN)
> >> -				pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
> >> -		}
> >> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> +		    0, MSG_DONTWAIT) < 0) {
> >> +		/* some thing unexpected */
> >> +		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
> >> +			break;
> >> +
> >> +		/* pull from completion queue to leave more space */
> >> +		if (errno == EAGAIN)
> >> +			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
> >> +	}
> >>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
> >>
> >> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf
> >> **bufs, uint16_t nb_pkts)
> >>
> >>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
> >>
> >> -	kick_tx(txq);
> >> +#if defined(XDP_USE_NEED_WAKEUP)
> >> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> +#endif
> >> +		kick_tx(txq);
> >>
> >>  	txq->stats.tx_pkts += nb_pkts;
> >>  	txq->stats.tx_bytes += tx_bytes;
> >> --
> >> 2.17.1
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-10 14:41     ` Zhang, Qi Z
@ 2019-09-10 15:09       ` Ye Xiaolong
  2019-09-11  2:05         ` Zhang, Qi Z
  0 siblings, 1 reply; 9+ messages in thread
From: Ye Xiaolong @ 2019-09-10 15:09 UTC (permalink / raw)
  To: Zhang, Qi Z; +Cc: Yigit, Ferruh, Loftus, Ciara, dev, stable, Karlsson, Magnus

On 09/10, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Ye, Xiaolong
>> Sent: Tuesday, September 10, 2019 9:54 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
>> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
>> Magnus <magnus.karlsson@intel.com>
>> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
>> 
>> On 09/10, Zhang, Qi Z wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Ye, Xiaolong
>> >> Sent: Tuesday, September 10, 2019 12:13 AM
>> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
>> >> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
>> >> Zhang, Qi Z <qi.z.zhang@intel.com>
>> >> Cc: dev@dpdk.org; stable@dpdk.org
>> >> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
>> >>
>> >> The kernel only consumes Tx packets if we have some Rx traffic on
>> >> specified queue or we have called send(). So we need to issue a
>> >> send() even when the allocation fails so that kernel will start to consume
>> packets again.
>> >
>> >So "allocation fails" means " xsk_ring_prod__reserve" fail right?
>> 
>> Yes.
>> 
>> >I don't understand when xsk_ring_prod__needs_wakeup is true why kernel
>> >will stop Tx packet at this situation would you share more insight?
>> 
>> Actually, the fail case is xsk_ring_prod__needs_wakeup is false, then we
>> can't issue a send() when xsk_ring_prod__reserve fails.
>
>Sorry, I think my question should be for the case when xsk_ring_prod__needs_wakeup is false,
>I don't understand why we need to handle different at below two situations
>1. when xsk_ring_prod__reserve fails
>2. normal tx scenario.
>My understanding is when xsk_ring_prod__needs_wakeup(tx) is false, which means Tx is ongoing, we don't need to wake up kernel to continue.
>

The problem is that kernel does not guarantee that all entries are sent for Tx.
There are a number of reasons that this might not happen, but usually some Rx
packet will at some point in time in the very short future trigger further Tx
processing and the packets will be sent. But if you only have Tx processing and
no Rx at all, you have to trigger a sento() again. 

Thanks,
Xiaolong

>> 
>> Thanks,
>> Xiaolong
>> 
>> >
>> >Thanks
>> >Qi
>> >
>> >>
>> >> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
>> >> breaks above rule by adding some condition to send, this patch fixes
>> >> it while still keeps the need_wakeup feature for Tx.
>> >>
>> >> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
>> >> Cc: stable@dpdk.org
>> >>
>> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> >> ---
>> >>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28
>> >> ++++++++++++++--------------
>> >>  1 file changed, 14 insertions(+), 14 deletions(-)
>> >>
>> >> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> index 41ed5b2af..e496e9aaa 100644
>> >> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
>> >>  	struct xsk_umem_info *umem = txq->pair->umem;
>> >>
>> >> -#if defined(XDP_USE_NEED_WAKEUP)
>> >> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
>> >> -#endif
>> >> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
>> >> -			    0, MSG_DONTWAIT) < 0) {
>> >> -			/* some thing unexpected */
>> >> -			if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
>> >> -				break;
>> >> -
>> >> -			/* pull from completion queue to leave more space */
>> >> -			if (errno == EAGAIN)
>> >> -				pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
>> >> -		}
>> >> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
>> >> +		    0, MSG_DONTWAIT) < 0) {
>> >> +		/* some thing unexpected */
>> >> +		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
>> >> +			break;
>> >> +
>> >> +		/* pull from completion queue to leave more space */
>> >> +		if (errno == EAGAIN)
>> >> +			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
>> >> +	}
>> >>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
>> >>
>> >> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf
>> >> **bufs, uint16_t nb_pkts)
>> >>
>> >>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
>> >>
>> >> -	kick_tx(txq);
>> >> +#if defined(XDP_USE_NEED_WAKEUP)
>> >> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
>> >> +#endif
>> >> +		kick_tx(txq);
>> >>
>> >>  	txq->stats.tx_pkts += nb_pkts;
>> >>  	txq->stats.tx_bytes += tx_bytes;
>> >> --
>> >> 2.17.1
>> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-10 13:53   ` Ye Xiaolong
  2019-09-10 14:41     ` Zhang, Qi Z
@ 2019-09-11  0:12     ` Zhang, Qi Z
  1 sibling, 0 replies; 9+ messages in thread
From: Zhang, Qi Z @ 2019-09-11  0:12 UTC (permalink / raw)
  To: Ye, Xiaolong; +Cc: Yigit, Ferruh, Loftus, Ciara, dev, stable, Karlsson, Magnus



> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, September 10, 2019 9:54 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
> Magnus <magnus.karlsson@intel.com>
> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> 
> On 09/10, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ye, Xiaolong
> >> Sent: Tuesday, September 10, 2019 12:13 AM
> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> >> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> >> Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> >>
> >> The kernel only consumes Tx packets if we have some Rx traffic on
> >> specified queue or we have called send(). So we need to issue a
> >> send() even when the allocation fails so that kernel will start to consume
> packets again.
> >
> >So "allocation fails" means " xsk_ring_prod__reserve" fail right?
> 
> Yes.
> 
> >I don't understand when xsk_ring_prod__needs_wakeup is true why kernel
> >will stop Tx packet at this situation would you share more insight?
> 
> Actually, the fail case is xsk_ring_prod__needs_wakeup is false, then we
> can't issue a send() when xsk_ring_prod__reserve fails.

Sorry, I think my question should be for the case when xsk_ring_prod__needs_wakeup is false,
I don't understand why we need to handle different at below two situations
1. when xsk_ring_prod__reserve fails
2. normal tx scenario.
My understanding is when xsk_ring_prod__needs_wakeup(tx) is false, which means Tx is ongoing, we don't need to wake up kernel to continue.

> 
> Thanks,
> Xiaolong
> 
> >
> >Thanks
> >Qi
> >
> >>
> >> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> >> breaks above rule by adding some condition to send, this patch fixes
> >> it while still keeps the need_wakeup feature for Tx.
> >>
> >> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >> ---
> >>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28
> >> ++++++++++++++--------------
> >>  1 file changed, 14 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> index 41ed5b2af..e496e9aaa 100644
> >> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
> >>  	struct xsk_umem_info *umem = txq->pair->umem;
> >>
> >> -#if defined(XDP_USE_NEED_WAKEUP)
> >> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> -#endif
> >> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> -			    0, MSG_DONTWAIT) < 0) {
> >> -			/* some thing unexpected */
> >> -			if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
> >> -				break;
> >> -
> >> -			/* pull from completion queue to leave more space */
> >> -			if (errno == EAGAIN)
> >> -				pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
> >> -		}
> >> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> +		    0, MSG_DONTWAIT) < 0) {
> >> +		/* some thing unexpected */
> >> +		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
> >> +			break;
> >> +
> >> +		/* pull from completion queue to leave more space */
> >> +		if (errno == EAGAIN)
> >> +			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
> >> +	}
> >>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
> >>
> >> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf
> >> **bufs, uint16_t nb_pkts)
> >>
> >>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
> >>
> >> -	kick_tx(txq);
> >> +#if defined(XDP_USE_NEED_WAKEUP)
> >> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> +#endif
> >> +		kick_tx(txq);
> >>
> >>  	txq->stats.tx_pkts += nb_pkts;
> >>  	txq->stats.tx_bytes += tx_bytes;
> >> --
> >> 2.17.1
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-10 15:09       ` Ye Xiaolong
@ 2019-09-11  2:05         ` Zhang, Qi Z
  2019-09-11 12:56           ` Ye Xiaolong
  0 siblings, 1 reply; 9+ messages in thread
From: Zhang, Qi Z @ 2019-09-11  2:05 UTC (permalink / raw)
  To: Ye, Xiaolong; +Cc: Yigit, Ferruh, Loftus, Ciara, dev, stable, Karlsson, Magnus



> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, September 10, 2019 11:09 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
> Magnus <magnus.karlsson@intel.com>
> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> 
> On 09/10, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ye, Xiaolong
> >> Sent: Tuesday, September 10, 2019 9:54 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> >> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
> >> Magnus <magnus.karlsson@intel.com>
> >> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> >>
> >> On 09/10, Zhang, Qi Z wrote:
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Ye, Xiaolong
> >> >> Sent: Tuesday, September 10, 2019 12:13 AM
> >> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> >> >> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> >> >> Zhang, Qi Z <qi.z.zhang@intel.com>
> >> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> >> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> >> >>
> >> >> The kernel only consumes Tx packets if we have some Rx traffic on
> >> >> specified queue or we have called send(). So we need to issue a
> >> >> send() even when the allocation fails so that kernel will start to
> >> >> consume
> >> packets again.
> >> >
> >> >So "allocation fails" means " xsk_ring_prod__reserve" fail right?
> >>
> >> Yes.
> >>
> >> >I don't understand when xsk_ring_prod__needs_wakeup is true why
> >> >kernel will stop Tx packet at this situation would you share more
> insight?
> >>
> >> Actually, the fail case is xsk_ring_prod__needs_wakeup is false, then
> >> we can't issue a send() when xsk_ring_prod__reserve fails.
> >
> >Sorry, I think my question should be for the case when
> >xsk_ring_prod__needs_wakeup is false, I don't understand why we need to
> >handle different at below two situations 1. when xsk_ring_prod__reserve
> >fails 2. normal tx scenario.
> >My understanding is when xsk_ring_prod__needs_wakeup(tx) is false,
> which means Tx is ongoing, we don't need to wake up kernel to continue.
> >
> 
> The problem is that kernel does not guarantee that all entries are sent for Tx.
> There are a number of reasons that this might not happen, but usually some
> Rx packet will at some point in time in the very short future trigger further Tx
> processing and the packets will be sent. But if you only have Tx processing
> and no Rx at all, you have to trigger a sento() again.

Ok , so the question is why we have below code.
#if defined(XDP_USE_NEED_WAKEUP)
if (xsk_ring_prod__needs_wakeup(&txq->tx))
#endif
	kick_tx(txq);

Here, when xsk_ring_prod__needs_wakeup is false, we can skip kick_tx (send), but why same "if check" can't be applied to the case when xsk_ring_prod__reserve failed?

Btw, think about below case
when xsk_ring_prod_reserve failed, if we don't kick_tx, and no following rx happens, 
does that mean the remain packets in tx queue will never get chance be transmitted?, what happen if the last tx_burst is never be called?

> 
> Thanks,
> Xiaolong
> 
> >>
> >> Thanks,
> >> Xiaolong
> >>
> >> >
> >> >Thanks
> >> >Qi
> >> >
> >> >>
> >> >> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> >> >> breaks above rule by adding some condition to send, this patch
> >> >> fixes it while still keeps the need_wakeup feature for Tx.
> >> >>
> >> >> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> >> >> Cc: stable@dpdk.org
> >> >>
> >> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >> >> ---
> >> >>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28
> >> >> ++++++++++++++--------------
> >> >>  1 file changed, 14 insertions(+), 14 deletions(-)
> >> >>
> >> >> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> index 41ed5b2af..e496e9aaa 100644
> >> >> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
> >> >>  	struct xsk_umem_info *umem = txq->pair->umem;
> >> >>
> >> >> -#if defined(XDP_USE_NEED_WAKEUP)
> >> >> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> >> -#endif
> >> >> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> >> -			    0, MSG_DONTWAIT) < 0) {
> >> >> -			/* some thing unexpected */
> >> >> -			if (errno != EBUSY && errno != EAGAIN && errno !=
> EINTR)
> >> >> -				break;
> >> >> -
> >> >> -			/* pull from completion queue to leave more space */
> >> >> -			if (errno == EAGAIN)
> >> >> -				pull_umem_cq(umem,
> ETH_AF_XDP_TX_BATCH_SIZE);
> >> >> -		}
> >> >> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> >> +		    0, MSG_DONTWAIT) < 0) {
> >> >> +		/* some thing unexpected */
> >> >> +		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
> >> >> +			break;
> >> >> +
> >> >> +		/* pull from completion queue to leave more space */
> >> >> +		if (errno == EAGAIN)
> >> >> +			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
> >> >> +	}
> >> >>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
> >> >>
> >> >> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf
> >> >> **bufs, uint16_t nb_pkts)
> >> >>
> >> >>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
> >> >>
> >> >> -	kick_tx(txq);
> >> >> +#if defined(XDP_USE_NEED_WAKEUP)
> >> >> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> >> +#endif
> >> >> +		kick_tx(txq);
> >> >>
> >> >>  	txq->stats.tx_pkts += nb_pkts;
> >> >>  	txq->stats.tx_bytes += tx_bytes;
> >> >> --
> >> >> 2.17.1
> >> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-11  2:05         ` Zhang, Qi Z
@ 2019-09-11 12:56           ` Ye Xiaolong
  2019-09-11 23:30             ` Zhang, Qi Z
  0 siblings, 1 reply; 9+ messages in thread
From: Ye Xiaolong @ 2019-09-11 12:56 UTC (permalink / raw)
  To: Zhang, Qi Z; +Cc: Yigit, Ferruh, Loftus, Ciara, dev, stable, Karlsson, Magnus

On 09/11, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Ye, Xiaolong
>> Sent: Tuesday, September 10, 2019 11:09 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
>> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
>> Magnus <magnus.karlsson@intel.com>
>> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
>> 
>> On 09/10, Zhang, Qi Z wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Ye, Xiaolong
>> >> Sent: Tuesday, September 10, 2019 9:54 PM
>> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
>> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
>> >> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
>> >> Magnus <magnus.karlsson@intel.com>
>> >> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
>> >>
>> >> On 09/10, Zhang, Qi Z wrote:
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Ye, Xiaolong
>> >> >> Sent: Tuesday, September 10, 2019 12:13 AM
>> >> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
>> >> >> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
>> >> >> Zhang, Qi Z <qi.z.zhang@intel.com>
>> >> >> Cc: dev@dpdk.org; stable@dpdk.org
>> >> >> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
>> >> >>
>> >> >> The kernel only consumes Tx packets if we have some Rx traffic on
>> >> >> specified queue or we have called send(). So we need to issue a
>> >> >> send() even when the allocation fails so that kernel will start to
>> >> >> consume
>> >> packets again.
>> >> >
>> >> >So "allocation fails" means " xsk_ring_prod__reserve" fail right?
>> >>
>> >> Yes.
>> >>
>> >> >I don't understand when xsk_ring_prod__needs_wakeup is true why
>> >> >kernel will stop Tx packet at this situation would you share more
>> insight?
>> >>
>> >> Actually, the fail case is xsk_ring_prod__needs_wakeup is false, then
>> >> we can't issue a send() when xsk_ring_prod__reserve fails.
>> >
>> >Sorry, I think my question should be for the case when
>> >xsk_ring_prod__needs_wakeup is false, I don't understand why we need to
>> >handle different at below two situations 1. when xsk_ring_prod__reserve
>> >fails 2. normal tx scenario.
>> >My understanding is when xsk_ring_prod__needs_wakeup(tx) is false,
>> which means Tx is ongoing, we don't need to wake up kernel to continue.
>> >
>> 
>> The problem is that kernel does not guarantee that all entries are sent for Tx.
>> There are a number of reasons that this might not happen, but usually some
>> Rx packet will at some point in time in the very short future trigger further Tx
>> processing and the packets will be sent. But if you only have Tx processing
>> and no Rx at all, you have to trigger a sento() again.
>
>Ok , so the question is why we have below code.
>#if defined(XDP_USE_NEED_WAKEUP)
>if (xsk_ring_prod__needs_wakeup(&txq->tx))
>#endif
>	kick_tx(txq);
>
>Here, when xsk_ring_prod__needs_wakeup is false, we can skip kick_tx (send), but why same "if check" can't be applied to the case when xsk_ring_prod__reserve failed?

When the system is running out of Tx entries, it needs some explicit action to
trigger kernel consumes the Tx buffers.

>
>Btw, think about below case
>when xsk_ring_prod_reserve failed, if we don't kick_tx, and no following rx happens, 
>does that mean the remain packets in tx queue will never get chance be transmitted?, what happen if the last tx_burst is never be called?

This is exactly the issue this patch try to fix, in this case, xsk_ring_prod__reserve
failure means there is no more available entries in tx queue, if we don't call
send/sendto or there is no rx traffic, Tx just halts.

Thanks,
Xiaolong

>
>> 
>> Thanks,
>> Xiaolong
>> 
>> >>
>> >> Thanks,
>> >> Xiaolong
>> >>
>> >> >
>> >> >Thanks
>> >> >Qi
>> >> >
>> >> >>
>> >> >> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
>> >> >> breaks above rule by adding some condition to send, this patch
>> >> >> fixes it while still keeps the need_wakeup feature for Tx.
>> >> >>
>> >> >> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
>> >> >> Cc: stable@dpdk.org
>> >> >>
>> >> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> >> >> ---
>> >> >>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28
>> >> >> ++++++++++++++--------------
>> >> >>  1 file changed, 14 insertions(+), 14 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> >> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> >> index 41ed5b2af..e496e9aaa 100644
>> >> >> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> >> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> >> >> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
>> >> >>  	struct xsk_umem_info *umem = txq->pair->umem;
>> >> >>
>> >> >> -#if defined(XDP_USE_NEED_WAKEUP)
>> >> >> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
>> >> >> -#endif
>> >> >> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
>> >> >> -			    0, MSG_DONTWAIT) < 0) {
>> >> >> -			/* some thing unexpected */
>> >> >> -			if (errno != EBUSY && errno != EAGAIN && errno !=
>> EINTR)
>> >> >> -				break;
>> >> >> -
>> >> >> -			/* pull from completion queue to leave more space */
>> >> >> -			if (errno == EAGAIN)
>> >> >> -				pull_umem_cq(umem,
>> ETH_AF_XDP_TX_BATCH_SIZE);
>> >> >> -		}
>> >> >> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
>> >> >> +		    0, MSG_DONTWAIT) < 0) {
>> >> >> +		/* some thing unexpected */
>> >> >> +		if (errno != EBUSY && errno != EAGAIN && errno != EINTR)
>> >> >> +			break;
>> >> >> +
>> >> >> +		/* pull from completion queue to leave more space */
>> >> >> +		if (errno == EAGAIN)
>> >> >> +			pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);
>> >> >> +	}
>> >> >>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
>> >> >>
>> >> >> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf
>> >> >> **bufs, uint16_t nb_pkts)
>> >> >>
>> >> >>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
>> >> >>
>> >> >> -	kick_tx(txq);
>> >> >> +#if defined(XDP_USE_NEED_WAKEUP)
>> >> >> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
>> >> >> +#endif
>> >> >> +		kick_tx(txq);
>> >> >>
>> >> >>  	txq->stats.tx_pkts += nb_pkts;
>> >> >>  	txq->stats.tx_bytes += tx_bytes;
>> >> >> --
>> >> >> 2.17.1
>> >> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets
  2019-09-11 12:56           ` Ye Xiaolong
@ 2019-09-11 23:30             ` Zhang, Qi Z
  0 siblings, 0 replies; 9+ messages in thread
From: Zhang, Qi Z @ 2019-09-11 23:30 UTC (permalink / raw)
  To: Ye, Xiaolong; +Cc: Yigit, Ferruh, Loftus, Ciara, dev, stable, Karlsson, Magnus



> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Wednesday, September 11, 2019 8:56 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
> Magnus <magnus.karlsson@intel.com>
> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> 
> On 09/11, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ye, Xiaolong
> >> Sent: Tuesday, September 10, 2019 11:09 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> >> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
> >> Magnus <magnus.karlsson@intel.com>
> >> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> >>
> >> On 09/10, Zhang, Qi Z wrote:
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Ye, Xiaolong
> >> >> Sent: Tuesday, September 10, 2019 9:54 PM
> >> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> >> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> >> >> <ciara.loftus@intel.com>; dev@dpdk.org; stable@dpdk.org; Karlsson,
> >> >> Magnus <magnus.karlsson@intel.com>
> >> >> Subject: Re: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> >> >>
> >> >> On 09/10, Zhang, Qi Z wrote:
> >> >> >
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Ye, Xiaolong
> >> >> >> Sent: Tuesday, September 10, 2019 12:13 AM
> >> >> >> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Loftus, Ciara
> >> >> >> <ciara.loftus@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> >> >> >> Zhang, Qi Z <qi.z.zhang@intel.com>
> >> >> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> >> >> Subject: [PATCH] net/af_xdp: fix Tx halt when no recv packets
> >> >> >>
> >> >> >> The kernel only consumes Tx packets if we have some Rx traffic
> >> >> >> on specified queue or we have called send(). So we need to
> >> >> >> issue a
> >> >> >> send() even when the allocation fails so that kernel will start
> >> >> >> to consume
> >> >> packets again.
> >> >> >
> >> >> >So "allocation fails" means " xsk_ring_prod__reserve" fail right?
> >> >>
> >> >> Yes.
> >> >>
> >> >> >I don't understand when xsk_ring_prod__needs_wakeup is true why
> >> >> >kernel will stop Tx packet at this situation would you share more
> >> insight?
> >> >>
> >> >> Actually, the fail case is xsk_ring_prod__needs_wakeup is false,
> >> >> then we can't issue a send() when xsk_ring_prod__reserve fails.
> >> >
> >> >Sorry, I think my question should be for the case when
> >> >xsk_ring_prod__needs_wakeup is false, I don't understand why we need
> >> >to handle different at below two situations 1. when
> >> >xsk_ring_prod__reserve fails 2. normal tx scenario.
> >> >My understanding is when xsk_ring_prod__needs_wakeup(tx) is false,
> >> which means Tx is ongoing, we don't need to wake up kernel to continue.
> >> >
> >>
> >> The problem is that kernel does not guarantee that all entries are sent for
> Tx.
> >> There are a number of reasons that this might not happen, but usually
> >> some Rx packet will at some point in time in the very short future
> >> trigger further Tx processing and the packets will be sent. But if
> >> you only have Tx processing and no Rx at all, you have to trigger a sento()
> again.
> >
> >Ok , so the question is why we have below code.
> >#if defined(XDP_USE_NEED_WAKEUP)
> >if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >#endif
> >	kick_tx(txq);
> >
> >Here, when xsk_ring_prod__needs_wakeup is false, we can skip kick_tx
> (send), but why same "if check" can't be applied to the case when
> xsk_ring_prod__reserve failed?
> 
> When the system is running out of Tx entries, it needs some explicit action to
> trigger kernel consumes the Tx buffers.
> 
> >
> >Btw, think about below case
> >when xsk_ring_prod_reserve failed, if we don't kick_tx, and no
> >following rx happens, does that mean the remain packets in tx queue will
> never get chance be transmitted?, what happen if the last tx_burst is never
> be called?
> 
> This is exactly the issue this patch try to fix, in this case,
> xsk_ring_prod__reserve failure means there is no more available entries in tx
> queue, if we don't call send/sendto or there is no rx traffic, Tx just halts.

the problem I saw is if a packet is transmitted in a tx_burst, it may get chance not be transmitted immediately by kernel (wait for a trigger), it may rely on a "send" call from next tx_burst.(that's the patch does). But this does not looks like a correct driver behavior, since we can't assume application will do tx_burst all the time.
I think in every tx_burst we need to guarantee kernel tx should be activated immediately, trigger a Tx for a previous tx_burst should not be the case.

> 
> Thanks,
> Xiaolong
> 
> >
> >>
> >> Thanks,
> >> Xiaolong
> >>
> >> >>
> >> >> Thanks,
> >> >> Xiaolong
> >> >>
> >> >> >
> >> >> >Thanks
> >> >> >Qi
> >> >> >
> >> >> >>
> >> >> >> Commit 45bba02c95b0 ("net/af_xdp: support need wakeup
> feature")
> >> >> >> breaks above rule by adding some condition to send, this patch
> >> >> >> fixes it while still keeps the need_wakeup feature for Tx.
> >> >> >>
> >> >> >> Fixes: 45bba02c95b0 ("net/af_xdp: support need wakeup feature")
> >> >> >> Cc: stable@dpdk.org
> >> >> >>
> >> >> >> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >> >> >> ---
> >> >> >>  drivers/net/af_xdp/rte_eth_af_xdp.c | 28
> >> >> >> ++++++++++++++--------------
> >> >> >>  1 file changed, 14 insertions(+), 14 deletions(-)
> >> >> >>
> >> >> >> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> >> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> >> index 41ed5b2af..e496e9aaa 100644
> >> >> >> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> >> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >> >> >> @@ -286,19 +286,16 @@ kick_tx(struct pkt_tx_queue *txq)  {
> >> >> >>  	struct xsk_umem_info *umem = txq->pair->umem;
> >> >> >>
> >> >> >> -#if defined(XDP_USE_NEED_WAKEUP)
> >> >> >> -	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> >> >> -#endif
> >> >> >> -		while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> >> >> -			    0, MSG_DONTWAIT) < 0) {
> >> >> >> -			/* some thing unexpected */
> >> >> >> -			if (errno != EBUSY && errno != EAGAIN && errno !=
> >> EINTR)
> >> >> >> -				break;
> >> >> >> -
> >> >> >> -			/* pull from completion queue to leave more space
> */
> >> >> >> -			if (errno == EAGAIN)
> >> >> >> -				pull_umem_cq(umem,
> >> ETH_AF_XDP_TX_BATCH_SIZE);
> >> >> >> -		}
> >> >> >> +	while (send(xsk_socket__fd(txq->pair->xsk), NULL,
> >> >> >> +		    0, MSG_DONTWAIT) < 0) {
> >> >> >> +		/* some thing unexpected */
> >> >> >> +		if (errno != EBUSY && errno != EAGAIN && errno !=
> EINTR)
> >> >> >> +			break;
> >> >> >> +
> >> >> >> +		/* pull from completion queue to leave more space */
> >> >> >> +		if (errno == EAGAIN)
> >> >> >> +			pull_umem_cq(umem,
> ETH_AF_XDP_TX_BATCH_SIZE);
> >> >> >> +	}
> >> >> >>  	pull_umem_cq(umem, ETH_AF_XDP_TX_BATCH_SIZE);  }
> >> >> >>
> >> >> >> @@ -367,7 +364,10 @@ eth_af_xdp_tx(void *queue, struct
> rte_mbuf
> >> >> >> **bufs, uint16_t nb_pkts)
> >> >> >>
> >> >> >>  	xsk_ring_prod__submit(&txq->tx, nb_pkts);
> >> >> >>
> >> >> >> -	kick_tx(txq);
> >> >> >> +#if defined(XDP_USE_NEED_WAKEUP)
> >> >> >> +	if (xsk_ring_prod__needs_wakeup(&txq->tx))
> >> >> >> +#endif
> >> >> >> +		kick_tx(txq);
> >> >> >>
> >> >> >>  	txq->stats.tx_pkts += nb_pkts;
> >> >> >>  	txq->stats.tx_bytes += tx_bytes;
> >> >> >> --
> >> >> >> 2.17.1
> >> >> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-09 16:12 [dpdk-dev] [PATCH] net/af_xdp: fix Tx halt when no recv packets Xiaolong Ye
2019-09-10  4:14 ` Zhang, Qi Z
2019-09-10 13:53   ` Ye Xiaolong
2019-09-10 14:41     ` Zhang, Qi Z
2019-09-10 15:09       ` Ye Xiaolong
2019-09-11  2:05         ` Zhang, Qi Z
2019-09-11 12:56           ` Ye Xiaolong
2019-09-11 23:30             ` Zhang, Qi Z
2019-09-11  0:12     ` Zhang, Qi Z

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox