[PATCH bpf 0/4] xsk: another round of fixes

[PATCH bpf 0/4] xsk: another round of fixes

[Maciej Fijalkowski]

Hello,

yet another fixes for XSK from Magnus and me.

Magnus addresses the fact that xp_alloc() can return NULL, so this needs
to be handled to avoid clearing entries in the SW ring on driver side.
Then he addresses the off-by-one problem in Tx desc cleaning routine for
ice ZC driver.

From my side, I am adding protection to ZC Rx processing loop so that
cleaning of descriptors wouldn't go over already processed entries.
Then I also fix an issue with assigning XSK pool to Tx queues.

This is directed to bpf tree.

Thanks!

Maciej Fijalkowski (2):
  ice: xsk: stop Rx processing when ntc catches ntu
  ice: xsk: fix indexing in ice_tx_xsk_pool()

Magnus Karlsson (2):
  xsk: do not write NULL in SW ring at allocation failure
  ice: xsk: eliminate unnecessary loop iteration

 drivers/net/ethernet/intel/ice/ice.h     | 2 +-
 drivers/net/ethernet/intel/ice/ice_xsk.c | 5 ++++-
 net/xdp/xsk_buff_pool.c                  | 8 ++++++--
 3 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH bpf 1/4] xsk: do not write NULL in SW ring at allocation failure

[Maciej Fijalkowski]

From: Magnus Karlsson <magnus.karlsson@intel.com>

For the case when xp_alloc_batch() is used but the batched allocation
cannot be used, there is a slow path that uses the non-batched
xp_alloc(). When it fails to allocate an entry, it returns NULL. The
current code wrote this NULL into the entry of the provided results
array (pointer to the driver SW ring usually) and returned. This might
not be what the driver expects and to make things simpler, just write
successfully allocated xdp_buffs into the SW ring,. The driver might
have information in there that is still important after an allocation
failure.

Note that at this point in time, there are no drivers using
xp_alloc_batch() that could trigger this slow path. But one might get
added.

Fixes: 47e4075df300 ("xsk: Batched buffer allocation for the pool")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 net/xdp/xsk_buff_pool.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index b34fca6ada86..af040ffa14ff 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -591,9 +591,13 @@ u32 xp_alloc_batch(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u32 max)
 	u32 nb_entries1 = 0, nb_entries2;
 
 	if (unlikely(pool->dma_need_sync)) {
+		struct xdp_buff *buff;
+
 		/* Slow path */
-		*xdp = xp_alloc(pool);
-		return !!*xdp;
+		buff = xp_alloc(pool);
+		if (buff)
+			*xdp = buff;
+		return !!buff;
 	}
 
 	if (unlikely(pool->free_list_cnt)) {
-- 
2.27.0

[PATCH bpf 2/4] ice: xsk: eliminate unnecessary loop iteration

[Maciej Fijalkowski]

From: Magnus Karlsson <magnus.karlsson@intel.com>

The NIC Tx ring completion routine cleans entries from the ring in
batches. However, it processes one more batch than it is supposed
to. Note that this does not matter from a functionality point of view
since it will not find a set DD bit for the next batch and just exit
the loop. But from a performance perspective, it is faster to
terminate the loop before and not issue an expensive read over PCIe to
get the DD bit.

Fixes: 126cdfe1007a ("ice: xsk: Improve AF_XDP ZC Tx and use batching API")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 88853a6ed931..51427cb4971a 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -754,7 +754,7 @@ static u16 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring, int napi_budget)
 		next_dd = next_dd + tx_thresh;
 		if (next_dd >= desc_cnt)
 			next_dd = tx_thresh - 1;
-	} while (budget--);
+	} while (--budget);
 
 	xdp_ring->next_dd = next_dd;
 
-- 
2.27.0

[PATCH bpf 3/4] ice: xsk: stop Rx processing when ntc catches ntu

[Maciej Fijalkowski]

This can happen with big budget values and some breakage of re-filling
descriptors as we do not clear the entry that ntu is pointing at the end
of ice_alloc_rx_bufs_zc. So if ntc is at ntu then it might be the case
that status_error0 has an old, uncleared value and ntc would go over
with processing which would result in false results.

Break Rx loop when ntc == ntu to avoid broken behavior.

Fixes: 3876ff525de7 ("ice: xsk: Handle SW XDP ring wrap and bump tail more often")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 51427cb4971a..dfbcaf08520e 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -608,6 +608,9 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		 */
 		dma_rmb();
 
+		if (unlikely(rx_ring->next_to_clean == rx_ring->next_to_use))
+			break;
+
 		xdp = *ice_xdp_buf(rx_ring, rx_ring->next_to_clean);
 
 		size = le16_to_cpu(rx_desc->wb.pkt_len) &
-- 
2.27.0

[PATCH bpf 4/4] ice: xsk: fix indexing in ice_tx_xsk_pool()

[Maciej Fijalkowski]

Ice driver tries to always create XDP rings array to be
num_possible_cpus() sized, regardless of user's queue count setting that
can be changed via ethtool -L for example.

Currently, ice_tx_xsk_pool() calculates the qid by decrementing the
ring->q_index by the count of XDP queues, but ring->q_index is set to 'i
+ vsi->alloc_txq'.

When user did ethtool -L $IFACE combined 1, alloc_txq is 1, but
vsi->num_xdp_txq is still num_possible_cpus(). Then, ice_tx_xsk_pool()
will do OOB access and in the final result ring would not get xsk_pool
pointer assigned. Then, each ice_xsk_wakeup() call will fail with error
and it will not be possible to get into NAPI and do the processing from
driver side.

Fix this by decrementing vsi->alloc_txq instead of vsi->num_xdp_txq from
ring-q_index in ice_tx_xsk_pool() so the calculation is reflected to the
setting of ring->q_index.

CC: Ciara Loftus <ciara.loftus@intel.com>
Fixes: 22bf877e528f ("ice: introduce XDP_TX fallback path")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index b0b27bfcd7a2..d4f1874df7d0 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -710,7 +710,7 @@ static inline struct xsk_buff_pool *ice_tx_xsk_pool(struct ice_tx_ring *ring)
 	struct ice_vsi *vsi = ring->vsi;
 	u16 qid;
 
-	qid = ring->q_index - vsi->num_xdp_txq;
+	qid = ring->q_index - vsi->alloc_txq;
 
 	if (!ice_is_xdp_ena_vsi(vsi) || !test_bit(qid, vsi->af_xdp_zc_qps))
 		return NULL;
-- 
2.27.0

Re: [PATCH bpf 0/4] xsk: another round of fixes

[Alexander Lobakin]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Mon, 28 Mar 2022 16:21:19 +0200

> Hello,
> 
> yet another fixes for XSK from Magnus and me.
> 
> Magnus addresses the fact that xp_alloc() can return NULL, so this needs
> to be handled to avoid clearing entries in the SW ring on driver side.
> Then he addresses the off-by-one problem in Tx desc cleaning routine for
> ice ZC driver.
> 
> From my side, I am adding protection to ZC Rx processing loop so that
> cleaning of descriptors wouldn't go over already processed entries.
> Then I also fix an issue with assigning XSK pool to Tx queues.

Nice, thanks!
For the series:

Acked-by: Alexander Lobakin <alexandr.lobakin@intel.com>

> 
> This is directed to bpf tree.
> 
> Thanks!
> 
> Maciej Fijalkowski (2):
>   ice: xsk: stop Rx processing when ntc catches ntu
>   ice: xsk: fix indexing in ice_tx_xsk_pool()
> 
> Magnus Karlsson (2):
>   xsk: do not write NULL in SW ring at allocation failure
>   ice: xsk: eliminate unnecessary loop iteration
> 
>  drivers/net/ethernet/intel/ice/ice.h     | 2 +-
>  drivers/net/ethernet/intel/ice/ice_xsk.c | 5 ++++-
>  net/xdp/xsk_buff_pool.c                  | 8 ++++++--
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> -- 
> 2.27.0

Thanks,
Al

Re: [PATCH bpf 0/4] xsk: another round of fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 28 Mar 2022 16:21:19 +0200 you wrote:
> Hello,
> 
> yet another fixes for XSK from Magnus and me.
> 
> Magnus addresses the fact that xp_alloc() can return NULL, so this needs
> to be handled to avoid clearing entries in the SW ring on driver side.
> Then he addresses the off-by-one problem in Tx desc cleaning routine for
> ice ZC driver.
> 
> [...]

Here is the summary with links:
  - [bpf,1/4] xsk: do not write NULL in SW ring at allocation failure
    https://git.kernel.org/bpf/bpf/c/a95a4d9b39b0
  - [bpf,2/4] ice: xsk: eliminate unnecessary loop iteration
    https://git.kernel.org/bpf/bpf/c/30d19d57d513
  - [bpf,3/4] ice: xsk: stop Rx processing when ntc catches ntu
    https://git.kernel.org/bpf/bpf/c/0ec1713009c5
  - [bpf,4/4] ice: xsk: fix indexing in ice_tx_xsk_pool()
    https://git.kernel.org/bpf/bpf/c/1ac2524de7b3

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