[PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[Maciej Fijalkowski]

v2:
- add likely for internal redirect return codes in ice and ixgbe
  (Jesper)
- do not drop the buffer that head pointed to at full XSK RQ (Maxim)
- terminate Rx loop only when need_wakeup feature is enabled (Maxim)
- reword from 'stop softirq processing' to 'stop NAPI Rx processing'
- s/ENXIO/EINVAL in mlx5 and stmmac's ndo_xsk_wakeup to keep it
  consitent with Intel's drivers (Maxim)
- include Jesper's Acks

Hi!

This is a revival of Bjorn's idea [0] to break NAPI loop when XSK Rx
queue gets full which in turn makes it impossible to keep on
successfully producing descriptors to XSK Rx ring. By breaking out of
the driver side immediately we will give the user space opportunity for
consuming descriptors from XSK Rx ring and therefore provide room in the
ring so that HW Rx -> XSK Rx redirection can be done.

Maxim asked and Jesper agreed on simplifying Bjorn's original API used
for detecting the event of interest, so let's just simply check for
-ENOBUFS within Intel's ZC drivers after an attempt to redirect a buffer
to XSK Rx. No real need for redirect API extension.

One might ask why it is still relevant even after having proper busy
poll support in place - here is the justification.

For xdpsock that was:
- run for l2fwd scenario,
- app/driver processing took place on the same core in busy poll
  with 2048 budget,
- HW ring sizes Tx 256, Rx 2048,

this work improved throughput by 78% and reduced Rx queue full statistic
bump by 99%.

For testing ice, make sure that you have [1] present on your side.

This set, besides the work described above, carries also improvements
around return codes in various XSK paths and lastly a minor optimization
for xskq_cons_has_entries(), a helper that might be used when XSK Rx
batching would make it to the kernel.

Link to v1 and discussion around it is at [2].

Thanks!
MF

[0]: https://lore.kernel.org/bpf/20200904135332.60259-1-bjorn.topel@gmail.com/
[1]: https://lore.kernel.org/netdev/20220317175727.340251-1-maciej.fijalkowski@intel.com/
[2]: https://lore.kernel.org/bpf/5864171b-1e08-1b51-026e-5f09b8945076@nvidia.com/T/

Björn Töpel (1):
  xsk: improve xdp_do_redirect() error codes

Maciej Fijalkowski (13):
  xsk: diversify return codes in xsk_rcv_check()
  ice: xsk: decorate ICE_XDP_REDIR with likely()
  ixgbe: xsk: decorate IXGBE_XDP_REDIR with likely()
  ice: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
  i40e: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
  ixgbe: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
  ice: xsk: diversify return values from xsk_wakeup call paths
  i40e: xsk: diversify return values from xsk_wakeup call paths
  ixgbe: xsk: diversify return values from xsk_wakeup call paths
  mlx5: xsk: diversify return values from xsk_wakeup call paths
  stmmac: xsk: diversify return values from xsk_wakeup call paths
  ice: xsk: avoid refilling single Rx descriptors
  xsk: drop ternary operator from xskq_cons_has_entries

 .../ethernet/intel/i40e/i40e_txrx_common.h    |  1 +
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 38 ++++++++-----
 drivers/net/ethernet/intel/ice/ice_txrx.h     |  1 +
 drivers/net/ethernet/intel/ice/ice_xsk.c      | 53 ++++++++++++-------
 .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 52 ++++++++++--------
 .../ethernet/mellanox/mlx5/core/en/xsk/tx.c   |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  4 +-
 net/xdp/xsk.c                                 |  4 +-
 net/xdp/xsk_queue.h                           |  4 +-
 10 files changed, 99 insertions(+), 61 deletions(-)

-- 
2.33.1

[PATCH v2 bpf-next 01/14] xsk: improve xdp_do_redirect() error codes

[Maciej Fijalkowski]

From: Björn Töpel <bjorn@kernel.org>

The error codes returned by xdp_do_redirect() when redirecting a frame
to an AF_XDP socket has not been very useful. A driver could not
distinguish between different errors. Prior this change the following
codes where used:

Socket not bound or incorrect queue/netdev: EINVAL
XDP frame/AF_XDP buffer size mismatch: ENOSPC
Could not allocate buffer (copy mode): ENOSPC
AF_XDP Rx buffer full: ENOSPC

After this change:

Socket not bound or incorrect queue/netdev: EINVAL
XDP frame/AF_XDP buffer size mismatch: ENOSPC
Could not allocate buffer (copy mode): ENOMEM
AF_XDP Rx buffer full: ENOBUFS

An AF_XDP zero-copy driver can now potentially determine if the
failure was due to a full Rx buffer, and if so stop processing more
frames, yielding to the userland AF_XDP application.

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Björn Töpel <bjorn@kernel.org>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 net/xdp/xsk.c       | 2 +-
 net/xdp/xsk_queue.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 2c34caee0fd1..f75e121073e7 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -184,7 +184,7 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
 	xsk_xdp = xsk_buff_alloc(xs->pool);
 	if (!xsk_xdp) {
 		xs->rx_dropped++;
-		return -ENOSPC;
+		return -ENOMEM;
 	}
 
 	xsk_copy_xdp(xsk_xdp, xdp, len);
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 801cda5d1938..644479e65578 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -382,7 +382,7 @@ static inline int xskq_prod_reserve_desc(struct xsk_queue *q,
 	u32 idx;
 
 	if (xskq_prod_is_full(q))
-		return -ENOSPC;
+		return -ENOBUFS;
 
 	/* A, matches D */
 	idx = q->cached_prod++ & q->ring_mask;
-- 
2.33.1

[PATCH v2 bpf-next 02/14] xsk: diversify return codes in xsk_rcv_check()

[Maciej Fijalkowski]

Inspired by patch that made xdp_do_redirect() return values for XSKMAP
more meaningful, return -ENXIO instead of -EINVAL for socket being
unbound in xsk_rcv_check() as this is the usual value that is returned
for such event. In turn, it is now possible to easily distinguish what
went wrong, which is a bit harder when for both cases checked, -EINVAL
was returned.

Return codes can be counted in a nice way via bpftrace oneliner that
Jesper has shown:

bpftrace -e 'tracepoint:xdp:xdp_redirect* {@err[-args->err] = count();}'

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 net/xdp/xsk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index f75e121073e7..040c73345b7c 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -217,7 +217,7 @@ static bool xsk_is_bound(struct xdp_sock *xs)
 static int xsk_rcv_check(struct xdp_sock *xs, struct xdp_buff *xdp)
 {
 	if (!xsk_is_bound(xs))
-		return -EINVAL;
+		return -ENXIO;
 
 	if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
 		return -EINVAL;
-- 
2.33.1

[PATCH v2 bpf-next 03/14] ice: xsk: decorate ICE_XDP_REDIR with likely()

[Maciej Fijalkowski]

ice_run_xdp_zc() suggests to compiler that XDP_REDIRECT is the most
probable action returned from BPF program that AF_XDP has in its
pipeline. Let's also bring this suggestion up to the callsite of
ice_run_xdp_zc() so that compiler will be able to generate more
optimized code which in turn will make branch predictor happy.

Suggested-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 866ee4df9671..e9ff05de0084 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -629,18 +629,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		xsk_buff_dma_sync_for_cpu(xdp, rx_ring->xsk_pool);
 
 		xdp_res = ice_run_xdp_zc(rx_ring, xdp, xdp_prog, xdp_ring);
-		if (xdp_res) {
-			if (xdp_res & (ICE_XDP_TX | ICE_XDP_REDIR))
-				xdp_xmit |= xdp_res;
-			else
-				xsk_buff_free(xdp);
+		if (likely(xdp_res & (ICE_XDP_TX | ICE_XDP_REDIR)))
+			xdp_xmit |= xdp_res;
+		else if (xdp_res == ICE_XDP_CONSUMED)
+			xsk_buff_free(xdp);
+		else
+			goto construct_skb;
 
-			total_rx_bytes += size;
-			total_rx_packets++;
+		total_rx_bytes += size;
+		total_rx_packets++;
+
+		ice_bump_ntc(rx_ring);
+		continue;
 
-			ice_bump_ntc(rx_ring);
-			continue;
-		}
 construct_skb:
 		/* XDP_PASS path */
 		skb = ice_construct_skb_zc(rx_ring, xdp);
-- 
2.33.1

[PATCH v2 bpf-next 04/14] ixgbe: xsk: decorate IXGBE_XDP_REDIR with likely()

[Maciej Fijalkowski]

ixgbe_run_xdp_zc() suggests to compiler that XDP_REDIRECT is the most
probable action returned from BPF program that AF_XDP has in its
pipeline. Let's also bring this suggestion up to the callsite of
ixgbe_run_xdp_zc() so that compiler will be able to generate more
optimized code which in turn will make branch predictor happy.

Suggested-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 25 ++++++++++----------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index dd7ff66d422f..85497bf10624 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -303,21 +303,22 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector,
 		xsk_buff_dma_sync_for_cpu(bi->xdp, rx_ring->xsk_pool);
 		xdp_res = ixgbe_run_xdp_zc(adapter, rx_ring, bi->xdp);
 
-		if (xdp_res) {
-			if (xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR))
-				xdp_xmit |= xdp_res;
-			else
-				xsk_buff_free(bi->xdp);
+		if (likely(xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)))
+			xdp_xmit |= xdp_res;
+		else if (xdp_res == IXGBE_XDP_CONSUMED)
+			xsk_buff_free(bi->xdp);
+		else
+			goto construct_skb;
 
-			bi->xdp = NULL;
-			total_rx_packets++;
-			total_rx_bytes += size;
+		bi->xdp = NULL;
+		total_rx_packets++;
+		total_rx_bytes += size;
 
-			cleaned_count++;
-			ixgbe_inc_ntc(rx_ring);
-			continue;
-		}
+		cleaned_count++;
+		ixgbe_inc_ntc(rx_ring);
+		continue;
 
+construct_skb:
 		/* XDP_PASS path */
 		skb = ixgbe_construct_skb_zc(rx_ring, bi->xdp);
 		if (!skb) {
-- 
2.33.1

[PATCH v2 bpf-next 05/14] ice: xsk: terminate Rx side of NAPI when XSK Rx queue gets full

[Maciej Fijalkowski]

When XSK pool uses need_wakeup feature, correlate -ENOBUFS that was
returned from xdp_do_redirect() with a XSK Rx queue being full. In such
case, terminate the Rx processing that is being done on the current HW
Rx ring and let the user space consume descriptors from XSK Rx queue so
that there is room that driver can use later on.

Introduce new internal return code ICE_XDP_EXIT that will indicate case
described above.

Note that it does not affect Tx processing that is bound to the same
NAPI context, nor the other Rx rings.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_txrx.h |  1 +
 drivers/net/ethernet/intel/ice/ice_xsk.c  | 29 +++++++++++++++--------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index cead3eb149bd..f5a906c03669 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -133,6 +133,7 @@ static inline int ice_skb_pad(void)
 #define ICE_XDP_CONSUMED	BIT(0)
 #define ICE_XDP_TX		BIT(1)
 #define ICE_XDP_REDIR		BIT(2)
+#define ICE_XDP_EXIT		BIT(3)
 
 #define ICE_RX_DMA_ATTR \
 	(DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING)
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index e9ff05de0084..99bfa21c3938 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -540,9 +540,13 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
 
 	if (likely(act == XDP_REDIRECT)) {
 		err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
-		if (err)
-			goto out_failure;
-		return ICE_XDP_REDIR;
+		if (!err)
+			return ICE_XDP_REDIR;
+		if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
+			result = ICE_XDP_EXIT;
+		else
+			result = ICE_XDP_CONSUMED;
+		goto out_failure;
 	}
 
 	switch (act) {
@@ -553,15 +557,16 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
 		if (result == ICE_XDP_CONSUMED)
 			goto out_failure;
 		break;
+	case XDP_DROP:
+		result = ICE_XDP_CONSUMED;
+		break;
 	default:
 		bpf_warn_invalid_xdp_action(rx_ring->netdev, xdp_prog, act);
 		fallthrough;
 	case XDP_ABORTED:
+		result = ICE_XDP_CONSUMED;
 out_failure:
 		trace_xdp_exception(rx_ring->netdev, xdp_prog, act);
-		fallthrough;
-	case XDP_DROP:
-		result = ICE_XDP_CONSUMED;
 		break;
 	}
 
@@ -629,12 +634,16 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		xsk_buff_dma_sync_for_cpu(xdp, rx_ring->xsk_pool);
 
 		xdp_res = ice_run_xdp_zc(rx_ring, xdp, xdp_prog, xdp_ring);
-		if (likely(xdp_res & (ICE_XDP_TX | ICE_XDP_REDIR)))
+		if (likely(xdp_res & (ICE_XDP_TX | ICE_XDP_REDIR))) {
 			xdp_xmit |= xdp_res;
-		else if (xdp_res == ICE_XDP_CONSUMED)
+		} else if (xdp_res == ICE_XDP_EXIT) {
+			failure = true;
+			break;
+		} else if (xdp_res == ICE_XDP_CONSUMED) {
 			xsk_buff_free(xdp);
-		else
+		} else if (xdp_res == ICE_XDP_PASS) {
 			goto construct_skb;
+		}
 
 		total_rx_bytes += size;
 		total_rx_packets++;
@@ -669,7 +678,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		ice_receive_skb(rx_ring, skb, vlan_tag);
 	}
 
-	failure = !ice_alloc_rx_bufs_zc(rx_ring, ICE_DESC_UNUSED(rx_ring));
+	failure |= !ice_alloc_rx_bufs_zc(rx_ring, ICE_DESC_UNUSED(rx_ring));
 
 	ice_finalize_xdp_rx(xdp_ring, xdp_xmit);
 	ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
-- 
2.33.1

[PATCH v2 bpf-next 06/14] i40e: xsk: terminate Rx side of NAPI when XSK Rx queue gets full

[Maciej Fijalkowski]

When XSK pool uses need_wakeup feature, correlate -ENOBUFS that was
returned from xdp_do_redirect() with a XSK Rx queue being full. In such
case, terminate the Rx processing that is being done on the current HW
Rx ring and let the user space consume descriptors from XSK Rx queue so
that there is room that driver can use later on.

Introduce new internal return code I40E_XDP_EXIT that will indicate case
described above.

Note that it does not affect Tx processing that is bound to the same
NAPI context, nor the other Rx rings.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 .../ethernet/intel/i40e/i40e_txrx_common.h    |  1 +
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 32 +++++++++++++------
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h b/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h
index 19da3b22160f..8c5118c8baaf 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h
@@ -20,6 +20,7 @@ void i40e_release_rx_desc(struct i40e_ring *rx_ring, u32 val);
 #define I40E_XDP_CONSUMED	BIT(0)
 #define I40E_XDP_TX		BIT(1)
 #define I40E_XDP_REDIR		BIT(2)
+#define I40E_XDP_EXIT		BIT(3)
 
 /*
  * build_ctob - Builds the Tx descriptor (cmd, offset and type) qword
diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index c1d25b0b0ca2..94ea8288859a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -161,9 +161,13 @@ static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp)
 
 	if (likely(act == XDP_REDIRECT)) {
 		err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
-		if (err)
-			goto out_failure;
-		return I40E_XDP_REDIR;
+		if (!err)
+			return I40E_XDP_REDIR;
+		if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
+			result = I40E_XDP_EXIT;
+		else
+			result = I40E_XDP_CONSUMED;
+		goto out_failure;
 	}
 
 	switch (act) {
@@ -175,16 +179,17 @@ static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct xdp_buff *xdp)
 		if (result == I40E_XDP_CONSUMED)
 			goto out_failure;
 		break;
+	case XDP_DROP:
+		result = I40E_XDP_CONSUMED;
+		break;
 	default:
 		bpf_warn_invalid_xdp_action(rx_ring->netdev, xdp_prog, act);
 		fallthrough;
 	case XDP_ABORTED:
+		result = I40E_XDP_CONSUMED;
 out_failure:
 		trace_xdp_exception(rx_ring->netdev, xdp_prog, act);
 		fallthrough; /* handle aborts by dropping packet */
-	case XDP_DROP:
-		result = I40E_XDP_CONSUMED;
-		break;
 	}
 	return result;
 }
@@ -271,7 +276,8 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
 				      unsigned int *rx_packets,
 				      unsigned int *rx_bytes,
 				      unsigned int size,
-				      unsigned int xdp_res)
+				      unsigned int xdp_res,
+				      bool *failure)
 {
 	struct sk_buff *skb;
 
@@ -281,11 +287,15 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
 	if (likely(xdp_res == I40E_XDP_REDIR) || xdp_res == I40E_XDP_TX)
 		return;
 
+	if (xdp_res == I40E_XDP_EXIT) {
+		*failure = true;
+		return;
+	}
+
 	if (xdp_res == I40E_XDP_CONSUMED) {
 		xsk_buff_free(xdp_buff);
 		return;
 	}
-
 	if (xdp_res == I40E_XDP_PASS) {
 		/* NB! We are not checking for errors using
 		 * i40e_test_staterr with
@@ -371,7 +381,9 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 
 		xdp_res = i40e_run_xdp_zc(rx_ring, bi);
 		i40e_handle_xdp_result_zc(rx_ring, bi, rx_desc, &rx_packets,
-					  &rx_bytes, size, xdp_res);
+					  &rx_bytes, size, xdp_res, &failure);
+		if (failure)
+			break;
 		total_rx_packets += rx_packets;
 		total_rx_bytes += rx_bytes;
 		xdp_xmit |= xdp_res & (I40E_XDP_TX | I40E_XDP_REDIR);
@@ -382,7 +394,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 	cleaned_count = (next_to_clean - rx_ring->next_to_use - 1) & count_mask;
 
 	if (cleaned_count >= I40E_RX_BUFFER_WRITE)
-		failure = !i40e_alloc_rx_buffers_zc(rx_ring, cleaned_count);
+		failure |= !i40e_alloc_rx_buffers_zc(rx_ring, cleaned_count);
 
 	i40e_finalize_xdp_rx(rx_ring, xdp_xmit);
 	i40e_update_rx_stats(rx_ring, total_rx_bytes, total_rx_packets);
-- 
2.33.1

[PATCH v2 bpf-next 07/14] ixgbe: xsk: terminate Rx side of NAPI when XSK Rx queue gets full

[Maciej Fijalkowski]

When XSK pool uses need_wakeup feature, correlate -ENOBUFS that was
returned from xdp_do_redirect() with a XSK Rx queue being full. In such
case, terminate the Rx processing that is being done on the current HW
Rx ring and let the user space consume descriptors from XSK Rx queue so
that there is room that driver can use later on.

Introduce new internal return code IXGBE_XDP_EXIT that will indicate case
described above.

Note that it does not affect Tx processing that is bound to the same
NAPI context, nor the other Rx rings.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 27 ++++++++++++-------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
index bba3feaf3318..f1f69ce67420 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
@@ -8,6 +8,7 @@
 #define IXGBE_XDP_CONSUMED	BIT(0)
 #define IXGBE_XDP_TX		BIT(1)
 #define IXGBE_XDP_REDIR		BIT(2)
+#define IXGBE_XDP_EXIT		BIT(3)
 
 #define IXGBE_TXD_CMD (IXGBE_TXD_CMD_EOP | \
 		       IXGBE_TXD_CMD_RS)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index 85497bf10624..bdd70b85a787 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -109,9 +109,13 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter,
 
 	if (likely(act == XDP_REDIRECT)) {
 		err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
-		if (err)
-			goto out_failure;
-		return IXGBE_XDP_REDIR;
+		if (!err)
+			return IXGBE_XDP_REDIR;
+		if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
+			result = IXGBE_XDP_EXIT;
+		else
+			result = IXGBE_XDP_CONSUMED;
+		goto out_failure;
 	}
 
 	switch (act) {
@@ -130,16 +134,17 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter,
 		if (result == IXGBE_XDP_CONSUMED)
 			goto out_failure;
 		break;
+	case XDP_DROP:
+		result = IXGBE_XDP_CONSUMED;
+		break;
 	default:
 		bpf_warn_invalid_xdp_action(rx_ring->netdev, xdp_prog, act);
 		fallthrough;
 	case XDP_ABORTED:
+		result = IXGBE_XDP_CONSUMED;
 out_failure:
 		trace_xdp_exception(rx_ring->netdev, xdp_prog, act);
 		fallthrough; /* handle aborts by dropping packet */
-	case XDP_DROP:
-		result = IXGBE_XDP_CONSUMED;
-		break;
 	}
 	return result;
 }
@@ -303,12 +308,16 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector,
 		xsk_buff_dma_sync_for_cpu(bi->xdp, rx_ring->xsk_pool);
 		xdp_res = ixgbe_run_xdp_zc(adapter, rx_ring, bi->xdp);
 
-		if (likely(xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)))
+		if (likely(xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR))) {
 			xdp_xmit |= xdp_res;
-		else if (xdp_res == IXGBE_XDP_CONSUMED)
+		} else if (xdp_res == IXGBE_XDP_EXIT) {
+			failure = true;
+			break;
+		} else if (xdp_res == IXGBE_XDP_CONSUMED) {
 			xsk_buff_free(bi->xdp);
-		else
+		} else if (xdp_res == IXGBE_XDP_PASS) {
 			goto construct_skb;
+		}
 
 		bi->xdp = NULL;
 		total_rx_packets++;
-- 
2.33.1

[PATCH v2 bpf-next 08/14] ice: xsk: diversify return values from xsk_wakeup call paths

[Maciej Fijalkowski]

Currently, when debugging AF_XDP workloads, one can correlate the -ENXIO
return code as the case that XSK is not in the bound state. Returning
same code from ndo_xsk_wakeup can be misleading and simply makes it
harder to follow what is going on.

Change ENXIOs in ice's ndo_xsk_wakeup() implementation to EINVALs, so
that when probing it is clear that something is wrong on the driver
side, not the xsk_{recv,send}msg.

There is a -ENETDOWN that can happen from both kernel/driver sides
though, but I don't have a correct replacement for this on one of the
sides, so let's keep it that way.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 99bfa21c3938..53c299eebd99 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -934,13 +934,13 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
 		return -ENETDOWN;
 
 	if (!ice_is_xdp_ena_vsi(vsi))
-		return -ENXIO;
+		return -EINVAL;
 
 	if (queue_id >= vsi->num_txq)
-		return -ENXIO;
+		return -EINVAL;
 
 	if (!vsi->xdp_rings[queue_id]->xsk_pool)
-		return -ENXIO;
+		return -EINVAL;
 
 	ring = vsi->xdp_rings[queue_id];
 
-- 
2.33.1

[PATCH v2 bpf-next 09/14] i40e: xsk: diversify return values from xsk_wakeup call paths

[Maciej Fijalkowski]

Currently, when debugging AF_XDP workloads, one can correlate the -ENXIO
return code as the case that XSK is not in the bound state. Returning
same code from ndo_xsk_wakeup can be misleading and simply makes it
harder to follow what is going on.

Change ENXIOs in i40e's ndo_xsk_wakeup() implementation to EINVALs, so
that when probing it is clear that something is wrong on the driver
side, not the xsk_{recv,send}msg.

There is a -ENETDOWN that can happen from both kernel/driver sides
though, but I don't have a correct replacement for this on one of the
sides, so let's keep it that way.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 94ea8288859a..050280fd10c1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -606,13 +606,13 @@ int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
 		return -ENETDOWN;
 
 	if (!i40e_enabled_xdp_vsi(vsi))
-		return -ENXIO;
+		return -EINVAL;
 
 	if (queue_id >= vsi->num_queue_pairs)
-		return -ENXIO;
+		return -EINVAL;
 
 	if (!vsi->xdp_rings[queue_id]->xsk_pool)
-		return -ENXIO;
+		return -EINVAL;
 
 	ring = vsi->xdp_rings[queue_id];
 
-- 
2.33.1

[PATCH v2 bpf-next 10/14] ixgbe: xsk: diversify return values from xsk_wakeup call paths

[Maciej Fijalkowski]

Currently, when debugging AF_XDP workloads, one can correlate the -ENXIO
return code as the case that XSK is not in the bound state. Returning
same code from ndo_xsk_wakeup can be misleading and simply makes it
harder to follow what is going on.

Change ENXIOs in ixgbe's ndo_xsk_wakeup() implementation to EINVALs, so
that when probing it is clear that something is wrong on the driver
side, not the xsk_{recv,send}msg.

There is a -ENETDOWN that can happen from both kernel/driver sides
though, but I don't have a correct replacement for this on one of the
sides, so let's keep it that way.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index bdd70b85a787..68532cffd453 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -526,10 +526,10 @@ int ixgbe_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
 		return -ENETDOWN;
 
 	if (!READ_ONCE(adapter->xdp_prog))
-		return -ENXIO;
+		return -EINVAL;
 
 	if (qid >= adapter->num_xdp_queues)
-		return -ENXIO;
+		return -EINVAL;
 
 	ring = adapter->xdp_ring[qid];
 
@@ -537,7 +537,7 @@ int ixgbe_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
 		return -ENETDOWN;
 
 	if (!ring->xsk_pool)
-		return -ENXIO;
+		return -EINVAL;
 
 	if (!napi_if_scheduled_mark_missed(&ring->q_vector->napi)) {
 		u64 eics = BIT_ULL(ring->q_vector->v_idx);
-- 
2.33.1

[PATCH v2 bpf-next 11/14] mlx5: xsk: diversify return values from xsk_wakeup call paths

[Maciej Fijalkowski]

Currently, when debugging AF_XDP workloads, one can correlate the -ENXIO
return code as the case that XSK is not in the bound state. Returning
same code from ndo_xsk_wakeup can be misleading and simply makes it
harder to follow what is going on.

Change ENXIO in mlx5's ndo_xsk_wakeup() implementation to EINVAL, so
that when probing it is clear that something is wrong on the driver
side, not the xsk_{recv,send}msg.

There is a -ENETDOWN that can happen from both kernel/driver sides
though, but I don't have a correct replacement for this on one of the
sides, so let's keep it that way.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
index 3ec0c17db010..4902ef74fedf 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
@@ -23,7 +23,7 @@ int mlx5e_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
 	c = priv->channels.c[ix];
 
 	if (unlikely(!test_bit(MLX5E_CHANNEL_STATE_XSK, c->state)))
-		return -ENXIO;
+		return -EINVAL;
 
 	if (!napi_if_scheduled_mark_missed(&c->napi)) {
 		/* To avoid WQE overrun, don't post a NOP if async_icosq is not
-- 
2.33.1

[PATCH v2 bpf-next 12/14] stmmac: xsk: diversify return values from xsk_wakeup call paths

[Maciej Fijalkowski]

Currently, when debugging AF_XDP workloads, one can correlate the -ENXIO
return code as the case that XSK is not in the bound state. Returning
same code from ndo_xsk_wakeup can be misleading and simply makes it
harder to follow what is going on.

Change ENXIOs in stmmac's ndo_xsk_wakeup() implementation to EINVALs, so
that when probing it is clear that something is wrong on the driver
side, not the xsk_{recv,send}msg.

There is a -ENETDOWN that can happen from both kernel/driver sides
though, but I don't have a correct replacement for this on one of the
sides, so let's keep it that way.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 4a4b3651ab3e..c9e077b2a56e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6565,7 +6565,7 @@ int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags)
 		return -ENETDOWN;
 
 	if (!stmmac_xdp_is_enabled(priv))
-		return -ENXIO;
+		return -EINVAL;
 
 	if (queue >= priv->plat->rx_queues_to_use ||
 	    queue >= priv->plat->tx_queues_to_use)
@@ -6576,7 +6576,7 @@ int stmmac_xsk_wakeup(struct net_device *dev, u32 queue, u32 flags)
 	ch = &priv->channel[queue];
 
 	if (!rx_q->xsk_pool && !tx_q->xsk_pool)
-		return -ENXIO;
+		return -EINVAL;
 
 	if (!napi_if_scheduled_mark_missed(&ch->rxtx_napi)) {
 		/* EQoS does not have per-DMA channel SW interrupt,
-- 
2.33.1

[PATCH v2 bpf-next 13/14] ice: xsk: avoid refilling single Rx descriptors

[Maciej Fijalkowski]

Call alloc Rx routine for ZC driver only when the amount of unallocated
descriptors exceeds given threshold.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 53c299eebd99..c26b408ba419 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -587,6 +587,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 	unsigned int xdp_xmit = 0;
 	struct bpf_prog *xdp_prog;
 	bool failure = false;
+	int entries_to_alloc;
 
 	/* ZC patch is enabled only when XDP program is set,
 	 * so here it can not be NULL
@@ -678,7 +679,9 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		ice_receive_skb(rx_ring, skb, vlan_tag);
 	}
 
-	failure |= !ice_alloc_rx_bufs_zc(rx_ring, ICE_DESC_UNUSED(rx_ring));
+	entries_to_alloc = ICE_DESC_UNUSED(rx_ring);
+	if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
+		failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
 
 	ice_finalize_xdp_rx(xdp_ring, xdp_xmit);
 	ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
-- 
2.33.1

[PATCH v2 bpf-next 14/14] xsk: drop ternary operator from xskq_cons_has_entries

[Maciej Fijalkowski]

Simplify the mentioned helper function by removing ternary operator. The
expression that is there outputs the boolean value by itself.

This helper might be used in the hot path so this simplification can
also be considered as micro optimization.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 net/xdp/xsk_queue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 644479e65578..a794410989cc 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -263,7 +263,7 @@ static inline u32 xskq_cons_nb_entries(struct xsk_queue *q, u32 max)
 
 static inline bool xskq_cons_has_entries(struct xsk_queue *q, u32 cnt)
 {
-	return xskq_cons_nb_entries(q, cnt) >= cnt ? true : false;
+	return xskq_cons_nb_entries(q, cnt) >= cnt;
 }
 
 static inline bool xskq_cons_peek_addr_unchecked(struct xsk_queue *q, u64 *addr)
-- 
2.33.1

Re: [PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Wed, 13 Apr 2022 17:30:01 +0200 you wrote:
> v2:
> - add likely for internal redirect return codes in ice and ixgbe
>   (Jesper)
> - do not drop the buffer that head pointed to at full XSK RQ (Maxim)
> - terminate Rx loop only when need_wakeup feature is enabled (Maxim)
> - reword from 'stop softirq processing' to 'stop NAPI Rx processing'
> - s/ENXIO/EINVAL in mlx5 and stmmac's ndo_xsk_wakeup to keep it
>   consitent with Intel's drivers (Maxim)
> - include Jesper's Acks
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next,01/14] xsk: improve xdp_do_redirect() error codes
    https://git.kernel.org/bpf/bpf-next/c/c6c1f11b691e
  - [v2,bpf-next,02/14] xsk: diversify return codes in xsk_rcv_check()
    https://git.kernel.org/bpf/bpf-next/c/2be4a677ccb2
  - [v2,bpf-next,03/14] ice: xsk: decorate ICE_XDP_REDIR with likely()
    https://git.kernel.org/bpf/bpf-next/c/0bd5ab511e30
  - [v2,bpf-next,04/14] ixgbe: xsk: decorate IXGBE_XDP_REDIR with likely()
    https://git.kernel.org/bpf/bpf-next/c/d090c885860f
  - [v2,bpf-next,05/14] ice: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
    https://git.kernel.org/bpf/bpf-next/c/50ae06648073
  - [v2,bpf-next,06/14] i40e: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
    https://git.kernel.org/bpf/bpf-next/c/b8aef650e549
  - [v2,bpf-next,07/14] ixgbe: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
    https://git.kernel.org/bpf/bpf-next/c/c7dd09fd4628
  - [v2,bpf-next,08/14] ice: xsk: diversify return values from xsk_wakeup call paths
    https://git.kernel.org/bpf/bpf-next/c/ed8a6bc60f9e
  - [v2,bpf-next,09/14] i40e: xsk: diversify return values from xsk_wakeup call paths
    https://git.kernel.org/bpf/bpf-next/c/ed7ae2d62217
  - [v2,bpf-next,10/14] ixgbe: xsk: diversify return values from xsk_wakeup call paths
    https://git.kernel.org/bpf/bpf-next/c/0f8bf018899e
  - [v2,bpf-next,11/14] mlx5: xsk: diversify return values from xsk_wakeup call paths
    https://git.kernel.org/bpf/bpf-next/c/7b7f2f273d87
  - [v2,bpf-next,12/14] stmmac: xsk: diversify return values from xsk_wakeup call paths
    https://git.kernel.org/bpf/bpf-next/c/a817ead4154d
  - [v2,bpf-next,13/14] ice: xsk: avoid refilling single Rx descriptors
    https://git.kernel.org/bpf/bpf-next/c/4efad196163f
  - [v2,bpf-next,14/14] xsk: drop ternary operator from xskq_cons_has_entries
    https://git.kernel.org/bpf/bpf-next/c/0fb53aabc5fc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[Maxim Mikityanskiy]

Hi Maciej,

On Wed, 2022-04-13 at 17:30 +0200, Maciej Fijalkowski wrote:
> v2:
> - add likely for internal redirect return codes in ice and ixgbe
>   (Jesper)
> - do not drop the buffer that head pointed to at full XSK RQ (Maxim)

I found an issue with this approach. If you don't drop that packet,
you'll need to run the XDP program for the same packet again. If the
XDP program is anything more complex than "redirect-everything-to-XSK",
it will get confused, for example, if it tracks any state or counts
anything.

We can't check whether there is enough space in the XSK RX ring before
running the XDP program, as we don't know in advance which XSK socket
will be selected.

We can't store bpf_redirect_info across NAPI calls to avoid running the
XDP program second time, as bpf_redirect_info is protected by RCU and
the assumption that the whole XDP_REDIRECT operation happens within one
NAPI cycle.

I see the following options:

1. Drop the packet when an overflow happens. The problem is that it
will happen systematically, but it's still better than the old behavior
(drop mulitple packets when an overflow happens and hog the CPU).

2. Make this feature opt-in. If the application opts in, it guarantees
to take measures to handle duplicate packets in XDP properly.

3. Require the XDP program to indicate it supports being called
multiple times for the same packet and pass a flag to it if it's a
repeated run. Drop the packet in the driver if the XDP program doesn't
indicate this support. The indication can be done similar to how it's
implemented for XDP multi buffer.

Thoughts? Other options?

Thanks,
Max

> - terminate Rx loop only when need_wakeup feature is enabled (Maxim)
> - reword from 'stop softirq processing' to 'stop NAPI Rx processing'
> - s/ENXIO/EINVAL in mlx5 and stmmac's ndo_xsk_wakeup to keep it
>   consitent with Intel's drivers (Maxim)
> - include Jesper's Acks
> 
> Hi!
> 
> This is a revival of Bjorn's idea [0] to break NAPI loop when XSK Rx
> queue gets full which in turn makes it impossible to keep on
> successfully producing descriptors to XSK Rx ring. By breaking out of
> the driver side immediately we will give the user space opportunity for
> consuming descriptors from XSK Rx ring and therefore provide room in the
> ring so that HW Rx -> XSK Rx redirection can be done.
> 
> Maxim asked and Jesper agreed on simplifying Bjorn's original API used
> for detecting the event of interest, so let's just simply check for
> -ENOBUFS within Intel's ZC drivers after an attempt to redirect a buffer
> to XSK Rx. No real need for redirect API extension.
> 
> One might ask why it is still relevant even after having proper busy
> poll support in place - here is the justification.
> 
> For xdpsock that was:
> - run for l2fwd scenario,
> - app/driver processing took place on the same core in busy poll
>   with 2048 budget,
> - HW ring sizes Tx 256, Rx 2048,
> 
> this work improved throughput by 78% and reduced Rx queue full statistic
> bump by 99%.
> 
> For testing ice, make sure that you have [1] present on your side.
> 
> This set, besides the work described above, carries also improvements
> around return codes in various XSK paths and lastly a minor optimization
> for xskq_cons_has_entries(), a helper that might be used when XSK Rx
> batching would make it to the kernel.
> 
> Link to v1 and discussion around it is at [2].
> 
> Thanks!
> MF
> 
> [0]: https://lore.kernel.org/bpf/20200904135332.60259-1-bjorn.topel@gmail.com/
> [1]: https://lore.kernel.org/netdev/20220317175727.340251-1-maciej.fijalkowski@intel.com/
> [2]: https://lore.kernel.org/bpf/5864171b-1e08-1b51-026e-5f09b8945076@nvidia.com/T/
> 
> Björn Töpel (1):
>   xsk: improve xdp_do_redirect() error codes
> 
> Maciej Fijalkowski (13):
>   xsk: diversify return codes in xsk_rcv_check()
>   ice: xsk: decorate ICE_XDP_REDIR with likely()
>   ixgbe: xsk: decorate IXGBE_XDP_REDIR with likely()
>   ice: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
>   i40e: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
>   ixgbe: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
>   ice: xsk: diversify return values from xsk_wakeup call paths
>   i40e: xsk: diversify return values from xsk_wakeup call paths
>   ixgbe: xsk: diversify return values from xsk_wakeup call paths
>   mlx5: xsk: diversify return values from xsk_wakeup call paths
>   stmmac: xsk: diversify return values from xsk_wakeup call paths
>   ice: xsk: avoid refilling single Rx descriptors
>   xsk: drop ternary operator from xskq_cons_has_entries
> 
>  .../ethernet/intel/i40e/i40e_txrx_common.h    |  1 +
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 38 ++++++++-----
>  drivers/net/ethernet/intel/ice/ice_txrx.h     |  1 +
>  drivers/net/ethernet/intel/ice/ice_xsk.c      | 53 ++++++++++++-------
>  .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 52 ++++++++++--------
>  .../ethernet/mellanox/mlx5/core/en/xsk/tx.c   |  2 +-
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  4 +-
>  net/xdp/xsk.c                                 |  4 +-
>  net/xdp/xsk_queue.h                           |  4 +-
>  10 files changed, 99 insertions(+), 61 deletions(-)
> 

Re: [PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[Maxim Mikityanskiy]

Anyone from Intel? Maciej, Björn, Magnus?

Does anyone else have anything to say? IMO, calling XDP for the same
packet multiple times is a bug, we should agree on some sane solution.

On Thu, 2022-08-18 at 14:26 +0300, Maxim Mikityanskiy wrote:
> Hi Maciej,
> 
> On Wed, 2022-04-13 at 17:30 +0200, Maciej Fijalkowski wrote:
> > v2:
> > - add likely for internal redirect return codes in ice and ixgbe
> >   (Jesper)
> > - do not drop the buffer that head pointed to at full XSK RQ
> > (Maxim)
> 
> I found an issue with this approach. If you don't drop that packet,
> you'll need to run the XDP program for the same packet again. If the
> XDP program is anything more complex than "redirect-everything-to-
> XSK",
> it will get confused, for example, if it tracks any state or counts
> anything.
> 
> We can't check whether there is enough space in the XSK RX ring
> before
> running the XDP program, as we don't know in advance which XSK socket
> will be selected.
> 
> We can't store bpf_redirect_info across NAPI calls to avoid running
> the
> XDP program second time, as bpf_redirect_info is protected by RCU and
> the assumption that the whole XDP_REDIRECT operation happens within
> one
> NAPI cycle.
> 
> I see the following options:
> 
> 1. Drop the packet when an overflow happens. The problem is that it
> will happen systematically, but it's still better than the old
> behavior
> (drop mulitple packets when an overflow happens and hog the CPU).
> 
> 2. Make this feature opt-in. If the application opts in, it
> guarantees
> to take measures to handle duplicate packets in XDP properly.
> 
> 3. Require the XDP program to indicate it supports being called
> multiple times for the same packet and pass a flag to it if it's a
> repeated run. Drop the packet in the driver if the XDP program
> doesn't
> indicate this support. The indication can be done similar to how it's
> implemented for XDP multi buffer.
> 
> Thoughts? Other options?
> 
> Thanks,
> Max
> 
> > - terminate Rx loop only when need_wakeup feature is enabled
> > (Maxim)
> > - reword from 'stop softirq processing' to 'stop NAPI Rx
> > processing'
> > - s/ENXIO/EINVAL in mlx5 and stmmac's ndo_xsk_wakeup to keep it
> >   consitent with Intel's drivers (Maxim)
> > - include Jesper's Acks
> > 
> > Hi!
> > 
> > This is a revival of Bjorn's idea [0] to break NAPI loop when XSK
> > Rx
> > queue gets full which in turn makes it impossible to keep on
> > successfully producing descriptors to XSK Rx ring. By breaking out
> > of
> > the driver side immediately we will give the user space opportunity
> > for
> > consuming descriptors from XSK Rx ring and therefore provide room
> > in the
> > ring so that HW Rx -> XSK Rx redirection can be done.
> > 
> > Maxim asked and Jesper agreed on simplifying Bjorn's original API
> > used
> > for detecting the event of interest, so let's just simply check for
> > -ENOBUFS within Intel's ZC drivers after an attempt to redirect a
> > buffer
> > to XSK Rx. No real need for redirect API extension.
> > 
> > One might ask why it is still relevant even after having proper
> > busy
> > poll support in place - here is the justification.
> > 
> > For xdpsock that was:
> > - run for l2fwd scenario,
> > - app/driver processing took place on the same core in busy poll
> >   with 2048 budget,
> > - HW ring sizes Tx 256, Rx 2048,
> > 
> > this work improved throughput by 78% and reduced Rx queue full
> > statistic
> > bump by 99%.
> > 
> > For testing ice, make sure that you have [1] present on your side.
> > 
> > This set, besides the work described above, carries also
> > improvements
> > around return codes in various XSK paths and lastly a minor
> > optimization
> > for xskq_cons_has_entries(), a helper that might be used when XSK
> > Rx
> > batching would make it to the kernel.
> > 
> > Link to v1 and discussion around it is at [2].
> > 
> > Thanks!
> > MF
> > 
> > [0]:
> > https://lore.kernel.org/bpf/20200904135332.60259-1-bjorn.topel@gmail.com/
> > [1]:
> > https://lore.kernel.org/netdev/20220317175727.340251-1-maciej.fijalkowski@intel.com/
> > [2]:
> > https://lore.kernel.org/bpf/5864171b-1e08-1b51-026e-5f09b8945076@nvidia.com/T/
> > 
> > Björn Töpel (1):
> >   xsk: improve xdp_do_redirect() error codes
> > 
> > Maciej Fijalkowski (13):
> >   xsk: diversify return codes in xsk_rcv_check()
> >   ice: xsk: decorate ICE_XDP_REDIR with likely()
> >   ixgbe: xsk: decorate IXGBE_XDP_REDIR with likely()
> >   ice: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> >   i40e: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> >   ixgbe: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> >   ice: xsk: diversify return values from xsk_wakeup call paths
> >   i40e: xsk: diversify return values from xsk_wakeup call paths
> >   ixgbe: xsk: diversify return values from xsk_wakeup call paths
> >   mlx5: xsk: diversify return values from xsk_wakeup call paths
> >   stmmac: xsk: diversify return values from xsk_wakeup call paths
> >   ice: xsk: avoid refilling single Rx descriptors
> >   xsk: drop ternary operator from xskq_cons_has_entries
> > 
> >  .../ethernet/intel/i40e/i40e_txrx_common.h    |  1 +
> >  drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 38 ++++++++-----
> >  drivers/net/ethernet/intel/ice/ice_txrx.h     |  1 +
> >  drivers/net/ethernet/intel/ice/ice_xsk.c      | 53 ++++++++++++---
> > ----
> >  .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  1 +
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 52 ++++++++++-----
> > ---
> >  .../ethernet/mellanox/mlx5/core/en/xsk/tx.c   |  2 +-
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  4 +-
> >  net/xdp/xsk.c                                 |  4 +-
> >  net/xdp/xsk_queue.h                           |  4 +-
> >  10 files changed, 99 insertions(+), 61 deletions(-)
> > 
> 

Re: [PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[Maciej Fijalkowski]

On Tue, Aug 23, 2022 at 09:49:43AM +0000, Maxim Mikityanskiy wrote:
> Anyone from Intel? Maciej, Björn, Magnus?

Hey Maxim,

how about keeping it simple and going with option 1? This behavior was
even proposed in the v1 submission of the patch set we're talking about.

> 
> Does anyone else have anything to say? IMO, calling XDP for the same
> packet multiple times is a bug, we should agree on some sane solution.
> 
> On Thu, 2022-08-18 at 14:26 +0300, Maxim Mikityanskiy wrote:
> > Hi Maciej,
> > 
> > On Wed, 2022-04-13 at 17:30 +0200, Maciej Fijalkowski wrote:
> > > v2:
> > > - add likely for internal redirect return codes in ice and ixgbe
> > >   (Jesper)
> > > - do not drop the buffer that head pointed to at full XSK RQ
> > > (Maxim)
> > 
> > I found an issue with this approach. If you don't drop that packet,
> > you'll need to run the XDP program for the same packet again. If the
> > XDP program is anything more complex than "redirect-everything-to-
> > XSK",
> > it will get confused, for example, if it tracks any state or counts
> > anything.
> > 
> > We can't check whether there is enough space in the XSK RX ring
> > before
> > running the XDP program, as we don't know in advance which XSK socket
> > will be selected.
> > 
> > We can't store bpf_redirect_info across NAPI calls to avoid running
> > the
> > XDP program second time, as bpf_redirect_info is protected by RCU and
> > the assumption that the whole XDP_REDIRECT operation happens within
> > one
> > NAPI cycle.
> > 
> > I see the following options:
> > 
> > 1. Drop the packet when an overflow happens. The problem is that it
> > will happen systematically, but it's still better than the old
> > behavior
> > (drop mulitple packets when an overflow happens and hog the CPU).
> > 
> > 2. Make this feature opt-in. If the application opts in, it
> > guarantees
> > to take measures to handle duplicate packets in XDP properly.
> > 
> > 3. Require the XDP program to indicate it supports being called
> > multiple times for the same packet and pass a flag to it if it's a
> > repeated run. Drop the packet in the driver if the XDP program
> > doesn't
> > indicate this support. The indication can be done similar to how it's
> > implemented for XDP multi buffer.
> > 
> > Thoughts? Other options?
> > 
> > Thanks,
> > Max
> > 
> > > - terminate Rx loop only when need_wakeup feature is enabled
> > > (Maxim)
> > > - reword from 'stop softirq processing' to 'stop NAPI Rx
> > > processing'
> > > - s/ENXIO/EINVAL in mlx5 and stmmac's ndo_xsk_wakeup to keep it
> > >   consitent with Intel's drivers (Maxim)
> > > - include Jesper's Acks
> > > 
> > > Hi!
> > > 
> > > This is a revival of Bjorn's idea [0] to break NAPI loop when XSK
> > > Rx
> > > queue gets full which in turn makes it impossible to keep on
> > > successfully producing descriptors to XSK Rx ring. By breaking out
> > > of
> > > the driver side immediately we will give the user space opportunity
> > > for
> > > consuming descriptors from XSK Rx ring and therefore provide room
> > > in the
> > > ring so that HW Rx -> XSK Rx redirection can be done.
> > > 
> > > Maxim asked and Jesper agreed on simplifying Bjorn's original API
> > > used
> > > for detecting the event of interest, so let's just simply check for
> > > -ENOBUFS within Intel's ZC drivers after an attempt to redirect a
> > > buffer
> > > to XSK Rx. No real need for redirect API extension.
> > > 
> > > One might ask why it is still relevant even after having proper
> > > busy
> > > poll support in place - here is the justification.
> > > 
> > > For xdpsock that was:
> > > - run for l2fwd scenario,
> > > - app/driver processing took place on the same core in busy poll
> > >   with 2048 budget,
> > > - HW ring sizes Tx 256, Rx 2048,
> > > 
> > > this work improved throughput by 78% and reduced Rx queue full
> > > statistic
> > > bump by 99%.
> > > 
> > > For testing ice, make sure that you have [1] present on your side.
> > > 
> > > This set, besides the work described above, carries also
> > > improvements
> > > around return codes in various XSK paths and lastly a minor
> > > optimization
> > > for xskq_cons_has_entries(), a helper that might be used when XSK
> > > Rx
> > > batching would make it to the kernel.
> > > 
> > > Link to v1 and discussion around it is at [2].
> > > 
> > > Thanks!
> > > MF
> > > 
> > > [0]:
> > > https://lore.kernel.org/bpf/20200904135332.60259-1-bjorn.topel@gmail.com/
> > > [1]:
> > > https://lore.kernel.org/netdev/20220317175727.340251-1-maciej.fijalkowski@intel.com/
> > > [2]:
> > > https://lore.kernel.org/bpf/5864171b-1e08-1b51-026e-5f09b8945076@nvidia.com/T/
> > > 
> > > Björn Töpel (1):
> > >   xsk: improve xdp_do_redirect() error codes
> > > 
> > > Maciej Fijalkowski (13):
> > >   xsk: diversify return codes in xsk_rcv_check()
> > >   ice: xsk: decorate ICE_XDP_REDIR with likely()
> > >   ixgbe: xsk: decorate IXGBE_XDP_REDIR with likely()
> > >   ice: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> > >   i40e: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> > >   ixgbe: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> > >   ice: xsk: diversify return values from xsk_wakeup call paths
> > >   i40e: xsk: diversify return values from xsk_wakeup call paths
> > >   ixgbe: xsk: diversify return values from xsk_wakeup call paths
> > >   mlx5: xsk: diversify return values from xsk_wakeup call paths
> > >   stmmac: xsk: diversify return values from xsk_wakeup call paths
> > >   ice: xsk: avoid refilling single Rx descriptors
> > >   xsk: drop ternary operator from xskq_cons_has_entries
> > > 
> > >  .../ethernet/intel/i40e/i40e_txrx_common.h    |  1 +
> > >  drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 38 ++++++++-----
> > >  drivers/net/ethernet/intel/ice/ice_txrx.h     |  1 +
> > >  drivers/net/ethernet/intel/ice/ice_xsk.c      | 53 ++++++++++++---
> > > ----
> > >  .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  1 +
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 52 ++++++++++-----
> > > ---
> > >  .../ethernet/mellanox/mlx5/core/en/xsk/tx.c   |  2 +-
> > >  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  4 +-
> > >  net/xdp/xsk.c                                 |  4 +-
> > >  net/xdp/xsk_queue.h                           |  4 +-
> > >  10 files changed, 99 insertions(+), 61 deletions(-)
> > > 
> > 
> 

Re: [PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[Maxim Mikityanskiy]

On Tue, 2022-08-23 at 13:24 +0200, Maciej Fijalkowski wrote:
> On Tue, Aug 23, 2022 at 09:49:43AM +0000, Maxim Mikityanskiy wrote:
> > Anyone from Intel? Maciej, Björn, Magnus?
> 
> Hey Maxim,
> 
> how about keeping it simple and going with option 1? This behavior was
> even proposed in the v1 submission of the patch set we're talking about.

Yeah, I know it was the behavior in v1. It was me who suggested not
dropping that packet, and I didn't realize back then that it had this
undesired side effect - sorry for that!

Option 1 sounds good to me as the first remedy, we can start with that.

However, it's not perfect: when NAPI and the application are pinned to
the same core, if the fill ring is bigger than the RX ring (which makes
sense in case of multiple sockets on the same UMEM), the driver will
constantly get into this condition, drop one packet, yield to
userspace, the application will of course clean up the RX ring, but
then the process will repeat.

That means, we'll always have a small percentage of packets dropped,
which may trigger the congestion control algorithms on the other side,
slowing down the TX to unacceptable speeds (because packet drops won't
disappear after slowing down just a little).

Given the above, we may need a more complex solution for the long term.
What do you think?

Also, if the application uses poll(), this whole logic (either v1 or
v2) seems not needed, because poll() returns to the application when
something becomes available in the RX ring, but I guess the reason for
adding it was that fantastic 78% performance improvement mentioned in
the cover letter?

> 
> > 
> > Does anyone else have anything to say? IMO, calling XDP for the same
> > packet multiple times is a bug, we should agree on some sane solution.
> > 
> > On Thu, 2022-08-18 at 14:26 +0300, Maxim Mikityanskiy wrote:
> > > Hi Maciej,
> > > 
> > > On Wed, 2022-04-13 at 17:30 +0200, Maciej Fijalkowski wrote:
> > > > v2:
> > > > - add likely for internal redirect return codes in ice and ixgbe
> > > >   (Jesper)
> > > > - do not drop the buffer that head pointed to at full XSK RQ
> > > > (Maxim)
> > > 
> > > I found an issue with this approach. If you don't drop that packet,
> > > you'll need to run the XDP program for the same packet again. If the
> > > XDP program is anything more complex than "redirect-everything-to-
> > > XSK",
> > > it will get confused, for example, if it tracks any state or counts
> > > anything.
> > > 
> > > We can't check whether there is enough space in the XSK RX ring
> > > before
> > > running the XDP program, as we don't know in advance which XSK socket
> > > will be selected.
> > > 
> > > We can't store bpf_redirect_info across NAPI calls to avoid running
> > > the
> > > XDP program second time, as bpf_redirect_info is protected by RCU and
> > > the assumption that the whole XDP_REDIRECT operation happens within
> > > one
> > > NAPI cycle.
> > > 
> > > I see the following options:
> > > 
> > > 1. Drop the packet when an overflow happens. The problem is that it
> > > will happen systematically, but it's still better than the old
> > > behavior
> > > (drop mulitple packets when an overflow happens and hog the CPU).
> > > 
> > > 2. Make this feature opt-in. If the application opts in, it
> > > guarantees
> > > to take measures to handle duplicate packets in XDP properly.
> > > 
> > > 3. Require the XDP program to indicate it supports being called
> > > multiple times for the same packet and pass a flag to it if it's a
> > > repeated run. Drop the packet in the driver if the XDP program
> > > doesn't
> > > indicate this support. The indication can be done similar to how it's
> > > implemented for XDP multi buffer.
> > > 
> > > Thoughts? Other options?
> > > 
> > > Thanks,
> > > Max
> > > 
> > > > - terminate Rx loop only when need_wakeup feature is enabled
> > > > (Maxim)
> > > > - reword from 'stop softirq processing' to 'stop NAPI Rx
> > > > processing'
> > > > - s/ENXIO/EINVAL in mlx5 and stmmac's ndo_xsk_wakeup to keep it
> > > >   consitent with Intel's drivers (Maxim)
> > > > - include Jesper's Acks
> > > > 
> > > > Hi!
> > > > 
> > > > This is a revival of Bjorn's idea [0] to break NAPI loop when XSK
> > > > Rx
> > > > queue gets full which in turn makes it impossible to keep on
> > > > successfully producing descriptors to XSK Rx ring. By breaking out
> > > > of
> > > > the driver side immediately we will give the user space opportunity
> > > > for
> > > > consuming descriptors from XSK Rx ring and therefore provide room
> > > > in the
> > > > ring so that HW Rx -> XSK Rx redirection can be done.
> > > > 
> > > > Maxim asked and Jesper agreed on simplifying Bjorn's original API
> > > > used
> > > > for detecting the event of interest, so let's just simply check for
> > > > -ENOBUFS within Intel's ZC drivers after an attempt to redirect a
> > > > buffer
> > > > to XSK Rx. No real need for redirect API extension.
> > > > 
> > > > One might ask why it is still relevant even after having proper
> > > > busy
> > > > poll support in place - here is the justification.
> > > > 
> > > > For xdpsock that was:
> > > > - run for l2fwd scenario,
> > > > - app/driver processing took place on the same core in busy poll
> > > >   with 2048 budget,
> > > > - HW ring sizes Tx 256, Rx 2048,
> > > > 
> > > > this work improved throughput by 78% and reduced Rx queue full
> > > > statistic
> > > > bump by 99%.
> > > > 
> > > > For testing ice, make sure that you have [1] present on your side.
> > > > 
> > > > This set, besides the work described above, carries also
> > > > improvements
> > > > around return codes in various XSK paths and lastly a minor
> > > > optimization
> > > > for xskq_cons_has_entries(), a helper that might be used when XSK
> > > > Rx
> > > > batching would make it to the kernel.
> > > > 
> > > > Link to v1 and discussion around it is at [2].
> > > > 
> > > > Thanks!
> > > > MF
> > > > 
> > > > [0]:
> > > > https://lore.kernel.org/bpf/20200904135332.60259-1-bjorn.topel@gmail.com/
> > > > [1]:
> > > > https://lore.kernel.org/netdev/20220317175727.340251-1-maciej.fijalkowski@intel.com/
> > > > [2]:
> > > > https://lore.kernel.org/bpf/5864171b-1e08-1b51-026e-5f09b8945076@nvidia.com/T/
> > > > 
> > > > Björn Töpel (1):
> > > >   xsk: improve xdp_do_redirect() error codes
> > > > 
> > > > Maciej Fijalkowski (13):
> > > >   xsk: diversify return codes in xsk_rcv_check()
> > > >   ice: xsk: decorate ICE_XDP_REDIR with likely()
> > > >   ixgbe: xsk: decorate IXGBE_XDP_REDIR with likely()
> > > >   ice: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> > > >   i40e: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> > > >   ixgbe: xsk: terminate Rx side of NAPI when XSK Rx queue gets full
> > > >   ice: xsk: diversify return values from xsk_wakeup call paths
> > > >   i40e: xsk: diversify return values from xsk_wakeup call paths
> > > >   ixgbe: xsk: diversify return values from xsk_wakeup call paths
> > > >   mlx5: xsk: diversify return values from xsk_wakeup call paths
> > > >   stmmac: xsk: diversify return values from xsk_wakeup call paths
> > > >   ice: xsk: avoid refilling single Rx descriptors
> > > >   xsk: drop ternary operator from xskq_cons_has_entries
> > > > 
> > > >  .../ethernet/intel/i40e/i40e_txrx_common.h    |  1 +
> > > >  drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 38 ++++++++-----
> > > >  drivers/net/ethernet/intel/ice/ice_txrx.h     |  1 +
> > > >  drivers/net/ethernet/intel/ice/ice_xsk.c      | 53 ++++++++++++---
> > > > ----
> > > >  .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  1 +
> > > >  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 52 ++++++++++-----
> > > > ---
> > > >  .../ethernet/mellanox/mlx5/core/en/xsk/tx.c   |  2 +-
> > > >  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  4 +-
> > > >  net/xdp/xsk.c                                 |  4 +-
> > > >  net/xdp/xsk_queue.h                           |  4 +-
> > > >  10 files changed, 99 insertions(+), 61 deletions(-)
> > > > 
> > > 
> > 

Re: [PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[John Fastabend]

Maxim Mikityanskiy wrote:
> On Tue, 2022-08-23 at 13:24 +0200, Maciej Fijalkowski wrote:
> > On Tue, Aug 23, 2022 at 09:49:43AM +0000, Maxim Mikityanskiy wrote:
> > > Anyone from Intel? Maciej, Björn, Magnus?
> > 
> > Hey Maxim,
> > 
> > how about keeping it simple and going with option 1? This behavior was
> > even proposed in the v1 submission of the patch set we're talking about.
> 
> Yeah, I know it was the behavior in v1. It was me who suggested not
> dropping that packet, and I didn't realize back then that it had this
> undesired side effect - sorry for that!

Just want to reiterate what was said originally, you'll definately confuse
our XDP programs if they ever saw the same pkt twice. It would confuse
metrics and any "tap" and so on.

> 
> Option 1 sounds good to me as the first remedy, we can start with that.
> 
> However, it's not perfect: when NAPI and the application are pinned to
> the same core, if the fill ring is bigger than the RX ring (which makes
> sense in case of multiple sockets on the same UMEM), the driver will
> constantly get into this condition, drop one packet, yield to
> userspace, the application will of course clean up the RX ring, but
> then the process will repeat.

Maybe dumb question haven't followed the entire thread or at least
don't recall it. Could you yield when you hit a high water mark at
some point before pkt drop?

> 
> That means, we'll always have a small percentage of packets dropped,
> which may trigger the congestion control algorithms on the other side,
> slowing down the TX to unacceptable speeds (because packet drops won't
> disappear after slowing down just a little).
> 
> Given the above, we may need a more complex solution for the long term.
> What do you think?
> 
> Also, if the application uses poll(), this whole logic (either v1 or
> v2) seems not needed, because poll() returns to the application when
> something becomes available in the RX ring, but I guess the reason for
> adding it was that fantastic 78% performance improvement mentioned in
> the cover letter?
> 

Re: [PATCH v2 bpf-next 00/14] xsk: stop NAPI Rx processing on full XSK RQ

[Maxim Mikityanskiy]

On Tue, 2022-08-23 at 22:18 -0700, John Fastabend wrote:
> Maxim Mikityanskiy wrote:
> > On Tue, 2022-08-23 at 13:24 +0200, Maciej Fijalkowski wrote:
> > > On Tue, Aug 23, 2022 at 09:49:43AM +0000, Maxim Mikityanskiy wrote:
> > > > Anyone from Intel? Maciej, Björn, Magnus?
> > > 
> > > Hey Maxim,
> > > 
> > > how about keeping it simple and going with option 1? This behavior was
> > > even proposed in the v1 submission of the patch set we're talking about.
> > 
> > Yeah, I know it was the behavior in v1. It was me who suggested not
> > dropping that packet, and I didn't realize back then that it had this
> > undesired side effect - sorry for that!
> 
> Just want to reiterate what was said originally, you'll definately confuse
> our XDP programs if they ever saw the same pkt twice. It would confuse
> metrics and any "tap" and so on.
> 
> > 
> > Option 1 sounds good to me as the first remedy, we can start with that.
> > 
> > However, it's not perfect: when NAPI and the application are pinned to
> > the same core, if the fill ring is bigger than the RX ring (which makes
> > sense in case of multiple sockets on the same UMEM), the driver will
> > constantly get into this condition, drop one packet, yield to
> > userspace, the application will of course clean up the RX ring, but
> > then the process will repeat.
> 
> Maybe dumb question haven't followed the entire thread or at least
> don't recall it. Could you yield when you hit a high water mark at
> some point before pkt drop?

That would be an ideal solution, but it doesn't sound feasible to me.
There may be multiple XSK sockets on the same RX queue, and each socket
has its own RX ring, which can be full or have some space. We don't
know in advance which RX ring will be chosen by the XDP program (if any
at all; the XDP program can still drop, pass or retransmit the packet),
so we can't check the watermark on a specific ring before running XDP.

It may be possible to iterate over all sockets and stop the processing
if any socket's RX ring is full, but that would be a heavy thing to do
per packet. It should be possible to optimize it to run once per NAPI
cycle, checking that each RX ring has enough space to fit the whole
NAPI budget, but I'm still not sure of performance implications, and it
sounds overly protective.

> 
> > 
> > That means, we'll always have a small percentage of packets dropped,
> > which may trigger the congestion control algorithms on the other side,
> > slowing down the TX to unacceptable speeds (because packet drops won't
> > disappear after slowing down just a little).
> > 
> > Given the above, we may need a more complex solution for the long term.
> > What do you think?
> > 
> > Also, if the application uses poll(), this whole logic (either v1 or
> > v2) seems not needed, because poll() returns to the application when
> > something becomes available in the RX ring, but I guess the reason for
> > adding it was that fantastic 78% performance improvement mentioned in
> > the cover letter?