[Intel-wired-lan] [PATCH intel-net 0/5] ice: xsk: Rx processing fixes

[Intel-wired-lan] [PATCH intel-net 0/5] ice: xsk: Rx processing fixes

[Maciej Fijalkowski]

Hi there,
it seems that previous [0] Rx fix was not enough and there are still
issues with AF_XDP Rx ZC support in ice driver. Elza reported that for
multiple XSK sockets configured on a single netdev, some of them were
becoming dead after a while. We have spotted more things that needed to
be addressed this time. More of information can be found in particular
commit messages.

Thanks,
Maciej

[0]: https://lore.kernel.org/bpf/20211129231746.2767739-1-anthony.l.nguyen at intel.com/

Maciej Fijalkowski (5):
  ice: xsk: return xsk buffers back to pool when cleaning the ring
  ice: xsk: allocate separate memory for XDP SW ring
  ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor
  ice: xsk: allow empty Rx descriptors on XSK ZC data path
  ice: xsk: fix cleaned_count setting

 drivers/net/ethernet/intel/ice/ice_base.c | 19 +++++++
 drivers/net/ethernet/intel/ice/ice_txrx.c | 19 ++++---
 drivers/net/ethernet/intel/ice/ice_txrx.h |  1 -
 drivers/net/ethernet/intel/ice/ice_xsk.c  | 62 +++++++++++------------
 4 files changed, 62 insertions(+), 39 deletions(-)

-- 
2.33.1

[Intel-wired-lan] [PATCH intel-net 1/5] ice: xsk: return xsk buffers back to pool when cleaning the ring

[Maciej Fijalkowski]

Currently we only NULL the xdp_buff pointer in the internal SW ring but
we never give it back to the xsk buffer pool. This means that buffers
can be leaked out of the buff pool and never be used again.

Add missing xsk_buff_free() call to the routine that is supposed to
clean the entries that are left in the ring so that these buffers in the
umem can be used by other sockets.

Also, only go through the space that is actually left to be cleaned
instead of a whole ring.

Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index ff55cb415b11..75c3e98241e0 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -810,14 +810,14 @@ bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi)
  */
 void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring)
 {
-	u16 i;
-
-	for (i = 0; i < rx_ring->count; i++) {
-		struct xdp_buff **xdp = &rx_ring->xdp_buf[i];
+	u16 count_mask = rx_ring->count - 1;
+	u16 ntc = rx_ring->next_to_clean;
+	u16 ntu = rx_ring->next_to_use;
 
-		if (!xdp)
-			continue;
+	for ( ; ntc != ntu; ntc = (ntc + 1) & count_mask) {
+		struct xdp_buff **xdp = &rx_ring->xdp_buf[ntc];
 
+		xsk_buff_free(*xdp);
 		*xdp = NULL;
 	}
 }
-- 
2.33.1

[Intel-wired-lan] [PATCH intel-net 2/5] ice: xsk: allocate separate memory for XDP SW ring

[Maciej Fijalkowski]

Currently, the zero-copy data path is reusing the memory region that was
initially allocated for an array of struct ice_rx_buf for its own
purposes. This is error prone as it is based on the ice_rx_buf struct
always being the same size or bigger than what the zero-copy path needs.
There can also be old values present in that array giving rise to errors
when the zero-copy path uses it.

Fix this by freeing the ice_rx_buf region and allocating a new array for
the zero-copy path that has the right length and is initialized to zero.

Fixes: 57f7f8b6bc0b ("ice: Use xdp_buf instead of rx_buf for xsk zero-copy")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_base.c | 19 +++++++++++++++
 drivers/net/ethernet/intel/ice/ice_txrx.c | 19 ++++++++++-----
 drivers/net/ethernet/intel/ice/ice_xsk.c  | 29 ++++++++++++-----------
 3 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index 1efc635cc0f5..56606d477f05 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -6,6 +6,18 @@
 #include "ice_lib.h"
 #include "ice_dcb_lib.h"
 
+static int ice_alloc_rx_buf_zc(struct ice_rx_ring *rx_ring)
+{
+	rx_ring->xdp_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->xdp_buf), GFP_KERNEL);
+	return rx_ring->xdp_buf ? 0 : -ENOMEM;
+}
+
+static int ice_alloc_rx_buf(struct ice_rx_ring *rx_ring)
+{
+	rx_ring->rx_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->rx_buf), GFP_KERNEL);
+	return rx_ring->rx_buf ? 0 : -ENOMEM;
+}
+
 /**
  * __ice_vsi_get_qs_contig - Assign a contiguous chunk of queues to VSI
  * @qs_cfg: gathered variables needed for PF->VSI queues assignment
@@ -492,8 +504,12 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
 			xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev,
 					 ring->q_index, ring->q_vector->napi.napi_id);
 
+		kfree(ring->rx_buf);
 		ring->xsk_pool = ice_xsk_pool(ring);
 		if (ring->xsk_pool) {
+			err = ice_alloc_rx_buf_zc(ring);
+			if (err)
+				return err;
 			xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
 
 			ring->rx_buf_len =
@@ -508,6 +524,9 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
 			dev_info(dev, "Registered XDP mem model MEM_TYPE_XSK_BUFF_POOL on Rx ring %d\n",
 				 ring->q_index);
 		} else {
+			err = ice_alloc_rx_buf(ring);
+			if (err)
+				return err;
 			if (!xdp_rxq_info_is_reg(&ring->xdp_rxq))
 				/* coverity[check_return] */
 				xdp_rxq_info_reg(&ring->xdp_rxq,
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index bc3ba19dc88f..227513b687b9 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -419,7 +419,10 @@ void ice_clean_rx_ring(struct ice_rx_ring *rx_ring)
 	}
 
 rx_skip_free:
-	memset(rx_ring->rx_buf, 0, sizeof(*rx_ring->rx_buf) * rx_ring->count);
+	if (rx_ring->xsk_pool)
+		memset(rx_ring->xdp_buf, 0, sizeof(*rx_ring->xdp_buf) * rx_ring->count);
+	else
+		memset(rx_ring->rx_buf, 0, sizeof(*rx_ring->rx_buf) * rx_ring->count);
 
 	/* Zero out the descriptor ring */
 	size = ALIGN(rx_ring->count * sizeof(union ice_32byte_rx_desc),
@@ -446,8 +449,13 @@ void ice_free_rx_ring(struct ice_rx_ring *rx_ring)
 		if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
 			xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
 	rx_ring->xdp_prog = NULL;
-	devm_kfree(rx_ring->dev, rx_ring->rx_buf);
-	rx_ring->rx_buf = NULL;
+	if (rx_ring->xsk_pool) {
+		kfree(rx_ring->xdp_buf);
+		rx_ring->xdp_buf = NULL;
+	} else {
+		kfree(rx_ring->rx_buf);
+		rx_ring->rx_buf = NULL;
+	}
 
 	if (rx_ring->desc) {
 		size = ALIGN(rx_ring->count * sizeof(union ice_32byte_rx_desc),
@@ -475,8 +483,7 @@ int ice_setup_rx_ring(struct ice_rx_ring *rx_ring)
 	/* warn if we are about to overwrite the pointer */
 	WARN_ON(rx_ring->rx_buf);
 	rx_ring->rx_buf =
-		devm_kcalloc(dev, sizeof(*rx_ring->rx_buf), rx_ring->count,
-			     GFP_KERNEL);
+		kcalloc(rx_ring->count, sizeof(*rx_ring->rx_buf), GFP_KERNEL);
 	if (!rx_ring->rx_buf)
 		return -ENOMEM;
 
@@ -505,7 +512,7 @@ int ice_setup_rx_ring(struct ice_rx_ring *rx_ring)
 	return 0;
 
 err:
-	devm_kfree(dev, rx_ring->rx_buf);
+	kfree(rx_ring->rx_buf);
 	rx_ring->rx_buf = NULL;
 	return -ENOMEM;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 75c3e98241e0..5cb61955c1f3 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -12,6 +12,11 @@
 #include "ice_txrx_lib.h"
 #include "ice_lib.h"
 
+static struct xdp_buff **ice_xdp_buf(struct ice_rx_ring *rx_ring, u32 idx)
+{
+	return &rx_ring->xdp_buf[idx];
+}
+
 /**
  * ice_qp_reset_stats - Resets all stats for rings of given index
  * @vsi: VSI that contains rings of interest
@@ -372,7 +377,7 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
 	dma_addr_t dma;
 
 	rx_desc = ICE_RX_DESC(rx_ring, ntu);
-	xdp = &rx_ring->xdp_buf[ntu];
+	xdp = ice_xdp_buf(rx_ring, ntu);
 
 	nb_buffs = min_t(u16, count, rx_ring->count - ntu);
 	nb_buffs = xsk_buff_alloc_batch(rx_ring->xsk_pool, xdp, nb_buffs);
@@ -425,9 +430,8 @@ static void ice_bump_ntc(struct ice_rx_ring *rx_ring)
  * Returns the skb on success, NULL on failure.
  */
 static struct sk_buff *
-ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff **xdp_arr)
+ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp)
 {
-	struct xdp_buff *xdp = *xdp_arr;
 	unsigned int metasize = xdp->data - xdp->data_meta;
 	unsigned int datasize = xdp->data_end - xdp->data;
 	unsigned int datasize_hard = xdp->data_end - xdp->data_hard_start;
@@ -444,7 +448,6 @@ ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff **xdp_arr)
 		skb_metadata_set(skb, metasize);
 
 	xsk_buff_free(xdp);
-	*xdp_arr = NULL;
 	return skb;
 }
 
@@ -521,7 +524,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 	while (likely(total_rx_packets < (unsigned int)budget)) {
 		union ice_32b_rx_flex_desc *rx_desc;
 		unsigned int size, xdp_res = 0;
-		struct xdp_buff **xdp;
+		struct xdp_buff *xdp;
 		struct sk_buff *skb;
 		u16 stat_err_bits;
 		u16 vlan_tag = 0;
@@ -544,18 +547,17 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		if (!size)
 			break;
 
-		xdp = &rx_ring->xdp_buf[rx_ring->next_to_clean];
-		xsk_buff_set_size(*xdp, size);
-		xsk_buff_dma_sync_for_cpu(*xdp, rx_ring->xsk_pool);
+		xdp = *ice_xdp_buf(rx_ring, rx_ring->next_to_clean);
+		xsk_buff_set_size(xdp, size);
+		xsk_buff_dma_sync_for_cpu(xdp, rx_ring->xsk_pool);
 
-		xdp_res = ice_run_xdp_zc(rx_ring, *xdp, xdp_prog, xdp_ring);
+		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);
+				xsk_buff_free(xdp);
 
-			*xdp = NULL;
 			total_rx_bytes += size;
 			total_rx_packets++;
 			cleaned_count++;
@@ -815,10 +817,9 @@ void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring)
 	u16 ntu = rx_ring->next_to_use;
 
 	for ( ; ntc != ntu; ntc = (ntc + 1) & count_mask) {
-		struct xdp_buff **xdp = &rx_ring->xdp_buf[ntc];
+		struct xdp_buff *xdp = *ice_xdp_buf(rx_ring, ntc);
 
-		xsk_buff_free(*xdp);
-		*xdp = NULL;
+		xsk_buff_free(xdp);
 	}
 }
 
-- 
2.33.1

[Intel-wired-lan] [PATCH intel-net 2/5] ice: xsk: allocate separate memory for XDP SW ring

[Nguyen, Anthony L]

On Fri, 2021-12-10 at 15:59 +0100, Maciej Fijalkowski wrote:
> @@ -425,9 +430,8 @@ static void ice_bump_ntc(struct ice_rx_ring
> *rx_ring)
> ? * Returns the skb on success, NULL on failure.
> ? */
> ?static struct sk_buff *
> -ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff
> **xdp_arr)
> +ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff
> *xdp)

There's a kdoc issue here.

drivers/net/ethernet/intel/ice/ice_xsk.c:436: warning: Function
parameter or member 'xdp' not described in 'ice_construct_skb_zc'
drivers/net/ethernet/intel/ice/ice_xsk.c:436: warning: Excess function
parameter 'xdp_arr' description in 'ice_construct_skb_zc'

Thanks,
Tony

[Intel-wired-lan] [PATCH intel-net 2/5] ice: xsk: allocate separate memory for XDP SW ring

[Alexander Lobakin]

From: Alexader Lobakin <alexandr.lobakin@intel.com>

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Fri, 10 Dec 2021 15:59:38 +0100

> Currently, the zero-copy data path is reusing the memory region that was
> initially allocated for an array of struct ice_rx_buf for its own
> purposes. This is error prone as it is based on the ice_rx_buf struct
> always being the same size or bigger than what the zero-copy path needs.
> There can also be old values present in that array giving rise to errors
> when the zero-copy path uses it.
> 
> Fix this by freeing the ice_rx_buf region and allocating a new array for
> the zero-copy path that has the right length and is initialized to zero.
> 
> Fixes: 57f7f8b6bc0b ("ice: Use xdp_buf instead of rx_buf for xsk zero-copy")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_base.c | 19 +++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_txrx.c | 19 ++++++++++-----
>  drivers/net/ethernet/intel/ice/ice_xsk.c  | 29 ++++++++++++-----------
>  3 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> index 1efc635cc0f5..56606d477f05 100644
> --- a/drivers/net/ethernet/intel/ice/ice_base.c
> +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> @@ -6,6 +6,18 @@
>  #include "ice_lib.h"
>  #include "ice_dcb_lib.h"
>  
> +static int ice_alloc_rx_buf_zc(struct ice_rx_ring *rx_ring)
> +{
> +	rx_ring->xdp_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->xdp_buf), GFP_KERNEL);
> +	return rx_ring->xdp_buf ? 0 : -ENOMEM;
> +}
> +
> +static int ice_alloc_rx_buf(struct ice_rx_ring *rx_ring)
> +{
> +	rx_ring->rx_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->rx_buf), GFP_KERNEL);
> +	return rx_ring->rx_buf ? 0 : -ENOMEM;
> +}
> +

Re that those functions can only return 0 or -ENOMEM, wouldn't it be
more elegant to make them bool

	return !!rx_ring->rx_buf; /* true on success,
				     false otherwise */
}

	if (!ice_alloc_rx_buf(rx_ring))
		return -ENOMEM;

?

>  /**
>   * __ice_vsi_get_qs_contig - Assign a contiguous chunk of queues to VSI
>   * @qs_cfg: gathered variables needed for PF->VSI queues assignment
> @@ -492,8 +504,12 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
>  			xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev,
>  					 ring->q_index, ring->q_vector->napi.napi_id);
>  
> +		kfree(ring->rx_buf);
>  		ring->xsk_pool = ice_xsk_pool(ring);
>  		if (ring->xsk_pool) {
> +			err = ice_alloc_rx_buf_zc(ring);
> +			if (err)
> +				return err;
>  			xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
>  
>  			ring->rx_buf_len =
> @@ -508,6 +524,9 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
>  			dev_info(dev, "Registered XDP mem model MEM_TYPE_XSK_BUFF_POOL on Rx ring %d\n",
>  				 ring->q_index);
>  		} else {
> +			err = ice_alloc_rx_buf(ring);
> +			if (err)
> +				return err;
>  			if (!xdp_rxq_info_is_reg(&ring->xdp_rxq))
>  				/* coverity[check_return] */
>  				xdp_rxq_info_reg(&ring->xdp_rxq,
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index bc3ba19dc88f..227513b687b9 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -419,7 +419,10 @@ void ice_clean_rx_ring(struct ice_rx_ring *rx_ring)
>  	}
>  
>  rx_skip_free:
> -	memset(rx_ring->rx_buf, 0, sizeof(*rx_ring->rx_buf) * rx_ring->count);
> +	if (rx_ring->xsk_pool)
> +		memset(rx_ring->xdp_buf, 0, sizeof(*rx_ring->xdp_buf) * rx_ring->count);
> +	else
> +		memset(rx_ring->rx_buf, 0, sizeof(*rx_ring->rx_buf) * rx_ring->count);

Consider using array_size() instead of a plain multiplication in
both of these:

	memset(s, 0, array_size(n, sizeof(*s));

It has the same internals as kcalloc() for array multiplication and
overflow checking.

>  
>  	/* Zero out the descriptor ring */
>  	size = ALIGN(rx_ring->count * sizeof(union ice_32byte_rx_desc),
> @@ -446,8 +449,13 @@ void ice_free_rx_ring(struct ice_rx_ring *rx_ring)
>  		if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
>  			xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
>  	rx_ring->xdp_prog = NULL;
> -	devm_kfree(rx_ring->dev, rx_ring->rx_buf);
> -	rx_ring->rx_buf = NULL;
> +	if (rx_ring->xsk_pool) {
> +		kfree(rx_ring->xdp_buf);
> +		rx_ring->xdp_buf = NULL;
> +	} else {
> +		kfree(rx_ring->rx_buf);
> +		rx_ring->rx_buf = NULL;
> +	}
>  
>  	if (rx_ring->desc) {
>  		size = ALIGN(rx_ring->count * sizeof(union ice_32byte_rx_desc),
> @@ -475,8 +483,7 @@ int ice_setup_rx_ring(struct ice_rx_ring *rx_ring)
>  	/* warn if we are about to overwrite the pointer */
>  	WARN_ON(rx_ring->rx_buf);
>  	rx_ring->rx_buf =
> -		devm_kcalloc(dev, sizeof(*rx_ring->rx_buf), rx_ring->count,
> -			     GFP_KERNEL);
> +		kcalloc(rx_ring->count, sizeof(*rx_ring->rx_buf), GFP_KERNEL);
>  	if (!rx_ring->rx_buf)
>  		return -ENOMEM;
>  
> @@ -505,7 +512,7 @@ int ice_setup_rx_ring(struct ice_rx_ring *rx_ring)
>  	return 0;
>  
>  err:
> -	devm_kfree(dev, rx_ring->rx_buf);
> +	kfree(rx_ring->rx_buf);
>  	rx_ring->rx_buf = NULL;
>  	return -ENOMEM;
>  }
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 75c3e98241e0..5cb61955c1f3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -12,6 +12,11 @@
>  #include "ice_txrx_lib.h"
>  #include "ice_lib.h"
>  
> +static struct xdp_buff **ice_xdp_buf(struct ice_rx_ring *rx_ring, u32 idx)
> +{
> +	return &rx_ring->xdp_buf[idx];
> +}
> +
>  /**
>   * ice_qp_reset_stats - Resets all stats for rings of given index
>   * @vsi: VSI that contains rings of interest
> @@ -372,7 +377,7 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
>  	dma_addr_t dma;
>  
>  	rx_desc = ICE_RX_DESC(rx_ring, ntu);
> -	xdp = &rx_ring->xdp_buf[ntu];
> +	xdp = ice_xdp_buf(rx_ring, ntu);
>  
>  	nb_buffs = min_t(u16, count, rx_ring->count - ntu);
>  	nb_buffs = xsk_buff_alloc_batch(rx_ring->xsk_pool, xdp, nb_buffs);
> @@ -425,9 +430,8 @@ static void ice_bump_ntc(struct ice_rx_ring *rx_ring)
>   * Returns the skb on success, NULL on failure.
>   */
>  static struct sk_buff *
> -ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff **xdp_arr)
> +ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp)
>  {
> -	struct xdp_buff *xdp = *xdp_arr;

RCT army strikes back lol.

>  	unsigned int metasize = xdp->data - xdp->data_meta;
>  	unsigned int datasize = xdp->data_end - xdp->data;
>  	unsigned int datasize_hard = xdp->data_end - xdp->data_hard_start;
> @@ -444,7 +448,6 @@ ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff **xdp_arr)
>  		skb_metadata_set(skb, metasize);
>  
>  	xsk_buff_free(xdp);
> -	*xdp_arr = NULL;
>  	return skb;
>  }
>  
> @@ -521,7 +524,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
>  	while (likely(total_rx_packets < (unsigned int)budget)) {
>  		union ice_32b_rx_flex_desc *rx_desc;
>  		unsigned int size, xdp_res = 0;
> -		struct xdp_buff **xdp;
> +		struct xdp_buff *xdp;
>  		struct sk_buff *skb;
>  		u16 stat_err_bits;
>  		u16 vlan_tag = 0;
> @@ -544,18 +547,17 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
>  		if (!size)
>  			break;
>  
> -		xdp = &rx_ring->xdp_buf[rx_ring->next_to_clean];
> -		xsk_buff_set_size(*xdp, size);
> -		xsk_buff_dma_sync_for_cpu(*xdp, rx_ring->xsk_pool);
> +		xdp = *ice_xdp_buf(rx_ring, rx_ring->next_to_clean);
> +		xsk_buff_set_size(xdp, size);
> +		xsk_buff_dma_sync_for_cpu(xdp, rx_ring->xsk_pool);
>  
> -		xdp_res = ice_run_xdp_zc(rx_ring, *xdp, xdp_prog, xdp_ring);
> +		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);
> +				xsk_buff_free(xdp);
>  
> -			*xdp = NULL;
>  			total_rx_bytes += size;
>  			total_rx_packets++;
>  			cleaned_count++;
> @@ -815,10 +817,9 @@ void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring)
>  	u16 ntu = rx_ring->next_to_use;
>  
>  	for ( ; ntc != ntu; ntc = (ntc + 1) & count_mask) {
> -		struct xdp_buff **xdp = &rx_ring->xdp_buf[ntc];
> +		struct xdp_buff *xdp = *ice_xdp_buf(rx_ring, ntc);
>  
> -		xsk_buff_free(*xdp);
> -		*xdp = NULL;
> +		xsk_buff_free(xdp);
>  	}
>  }
>  
> -- 
> 2.33.1

Al

[Intel-wired-lan] [PATCH intel-net 2/5] ice: xsk: allocate separate memory for XDP SW ring

[Maciej Fijalkowski]

On Fri, Dec 10, 2021 at 11:24:51PM +0100, Alexander Lobakin wrote:
> From: Alexader Lobakin <alexandr.lobakin@intel.com>
> 
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Date: Fri, 10 Dec 2021 15:59:38 +0100
> 
> > Currently, the zero-copy data path is reusing the memory region that was
> > initially allocated for an array of struct ice_rx_buf for its own
> > purposes. This is error prone as it is based on the ice_rx_buf struct
> > always being the same size or bigger than what the zero-copy path needs.
> > There can also be old values present in that array giving rise to errors
> > when the zero-copy path uses it.
> > 
> > Fix this by freeing the ice_rx_buf region and allocating a new array for
> > the zero-copy path that has the right length and is initialized to zero.
> > 
> > Fixes: 57f7f8b6bc0b ("ice: Use xdp_buf instead of rx_buf for xsk zero-copy")
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_base.c | 19 +++++++++++++++
> >  drivers/net/ethernet/intel/ice/ice_txrx.c | 19 ++++++++++-----
> >  drivers/net/ethernet/intel/ice/ice_xsk.c  | 29 ++++++++++++-----------
> >  3 files changed, 47 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> > index 1efc635cc0f5..56606d477f05 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_base.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> > @@ -6,6 +6,18 @@
> >  #include "ice_lib.h"
> >  #include "ice_dcb_lib.h"
> >  
> > +static int ice_alloc_rx_buf_zc(struct ice_rx_ring *rx_ring)
> > +{
> > +	rx_ring->xdp_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->xdp_buf), GFP_KERNEL);
> > +	return rx_ring->xdp_buf ? 0 : -ENOMEM;
> > +}
> > +
> > +static int ice_alloc_rx_buf(struct ice_rx_ring *rx_ring)
> > +{
> > +	rx_ring->rx_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->rx_buf), GFP_KERNEL);
> > +	return rx_ring->rx_buf ? 0 : -ENOMEM;
> > +}
> > +
> 
> Re that those functions can only return 0 or -ENOMEM, wouldn't it be
> more elegant to make them bool
> 
> 	return !!rx_ring->rx_buf; /* true on success,
> 				     false otherwise */

Yes, why not.

> }
> 
> 	if (!ice_alloc_rx_buf(rx_ring))
> 		return -ENOMEM;
> 
> ?
> 
> >  /**
> >   * __ice_vsi_get_qs_contig - Assign a contiguous chunk of queues to VSI
> >   * @qs_cfg: gathered variables needed for PF->VSI queues assignment
> > @@ -492,8 +504,12 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> >  			xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev,
> >  					 ring->q_index, ring->q_vector->napi.napi_id);
> >  
> > +		kfree(ring->rx_buf);
> >  		ring->xsk_pool = ice_xsk_pool(ring);
> >  		if (ring->xsk_pool) {
> > +			err = ice_alloc_rx_buf_zc(ring);
> > +			if (err)
> > +				return err;
> >  			xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
> >  
> >  			ring->rx_buf_len =
> > @@ -508,6 +524,9 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> >  			dev_info(dev, "Registered XDP mem model MEM_TYPE_XSK_BUFF_POOL on Rx ring %d\n",
> >  				 ring->q_index);
> >  		} else {
> > +			err = ice_alloc_rx_buf(ring);
> > +			if (err)
> > +				return err;
> >  			if (!xdp_rxq_info_is_reg(&ring->xdp_rxq))
> >  				/* coverity[check_return] */
> >  				xdp_rxq_info_reg(&ring->xdp_rxq,
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > index bc3ba19dc88f..227513b687b9 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > @@ -419,7 +419,10 @@ void ice_clean_rx_ring(struct ice_rx_ring *rx_ring)
> >  	}
> >  
> >  rx_skip_free:
> > -	memset(rx_ring->rx_buf, 0, sizeof(*rx_ring->rx_buf) * rx_ring->count);
> > +	if (rx_ring->xsk_pool)
> > +		memset(rx_ring->xdp_buf, 0, sizeof(*rx_ring->xdp_buf) * rx_ring->count);
> > +	else
> > +		memset(rx_ring->rx_buf, 0, sizeof(*rx_ring->rx_buf) * rx_ring->count);
> 
> Consider using array_size() instead of a plain multiplication in
> both of these:
> 
> 	memset(s, 0, array_size(n, sizeof(*s));
> 
> It has the same internals as kcalloc() for array multiplication and
> overflow checking.

Good stuff I'll incorporate that.

> 
> >  
> >  	/* Zero out the descriptor ring */
> >  	size = ALIGN(rx_ring->count * sizeof(union ice_32byte_rx_desc),
> > @@ -446,8 +449,13 @@ void ice_free_rx_ring(struct ice_rx_ring *rx_ring)
> >  		if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
> >  			xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> >  	rx_ring->xdp_prog = NULL;
> > -	devm_kfree(rx_ring->dev, rx_ring->rx_buf);
> > -	rx_ring->rx_buf = NULL;
> > +	if (rx_ring->xsk_pool) {
> > +		kfree(rx_ring->xdp_buf);
> > +		rx_ring->xdp_buf = NULL;
> > +	} else {
> > +		kfree(rx_ring->rx_buf);
> > +		rx_ring->rx_buf = NULL;
> > +	}
> >  
> >  	if (rx_ring->desc) {
> >  		size = ALIGN(rx_ring->count * sizeof(union ice_32byte_rx_desc),
> > @@ -475,8 +483,7 @@ int ice_setup_rx_ring(struct ice_rx_ring *rx_ring)
> >  	/* warn if we are about to overwrite the pointer */
> >  	WARN_ON(rx_ring->rx_buf);
> >  	rx_ring->rx_buf =
> > -		devm_kcalloc(dev, sizeof(*rx_ring->rx_buf), rx_ring->count,
> > -			     GFP_KERNEL);
> > +		kcalloc(rx_ring->count, sizeof(*rx_ring->rx_buf), GFP_KERNEL);
> >  	if (!rx_ring->rx_buf)
> >  		return -ENOMEM;
> >  
> > @@ -505,7 +512,7 @@ int ice_setup_rx_ring(struct ice_rx_ring *rx_ring)
> >  	return 0;
> >  
> >  err:
> > -	devm_kfree(dev, rx_ring->rx_buf);
> > +	kfree(rx_ring->rx_buf);
> >  	rx_ring->rx_buf = NULL;
> >  	return -ENOMEM;
> >  }
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > index 75c3e98241e0..5cb61955c1f3 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > @@ -12,6 +12,11 @@
> >  #include "ice_txrx_lib.h"
> >  #include "ice_lib.h"
> >  
> > +static struct xdp_buff **ice_xdp_buf(struct ice_rx_ring *rx_ring, u32 idx)
> > +{
> > +	return &rx_ring->xdp_buf[idx];
> > +}
> > +
> >  /**
> >   * ice_qp_reset_stats - Resets all stats for rings of given index
> >   * @vsi: VSI that contains rings of interest
> > @@ -372,7 +377,7 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> >  	dma_addr_t dma;
> >  
> >  	rx_desc = ICE_RX_DESC(rx_ring, ntu);
> > -	xdp = &rx_ring->xdp_buf[ntu];
> > +	xdp = ice_xdp_buf(rx_ring, ntu);
> >  
> >  	nb_buffs = min_t(u16, count, rx_ring->count - ntu);
> >  	nb_buffs = xsk_buff_alloc_batch(rx_ring->xsk_pool, xdp, nb_buffs);
> > @@ -425,9 +430,8 @@ static void ice_bump_ntc(struct ice_rx_ring *rx_ring)
> >   * Returns the skb on success, NULL on failure.
> >   */
> >  static struct sk_buff *
> > -ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff **xdp_arr)
> > +ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp)
> >  {
> > -	struct xdp_buff *xdp = *xdp_arr;
> 
> RCT army strikes back lol.

 :(

> 
> >  	unsigned int metasize = xdp->data - xdp->data_meta;
> >  	unsigned int datasize = xdp->data_end - xdp->data;
> >  	unsigned int datasize_hard = xdp->data_end - xdp->data_hard_start;
> > @@ -444,7 +448,6 @@ ice_construct_skb_zc(struct ice_rx_ring *rx_ring, struct xdp_buff **xdp_arr)
> >  		skb_metadata_set(skb, metasize);
> >  
> >  	xsk_buff_free(xdp);
> > -	*xdp_arr = NULL;
> >  	return skb;
> >  }
> >  
> > @@ -521,7 +524,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> >  	while (likely(total_rx_packets < (unsigned int)budget)) {
> >  		union ice_32b_rx_flex_desc *rx_desc;
> >  		unsigned int size, xdp_res = 0;
> > -		struct xdp_buff **xdp;
> > +		struct xdp_buff *xdp;
> >  		struct sk_buff *skb;
> >  		u16 stat_err_bits;
> >  		u16 vlan_tag = 0;
> > @@ -544,18 +547,17 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> >  		if (!size)
> >  			break;
> >  
> > -		xdp = &rx_ring->xdp_buf[rx_ring->next_to_clean];
> > -		xsk_buff_set_size(*xdp, size);
> > -		xsk_buff_dma_sync_for_cpu(*xdp, rx_ring->xsk_pool);
> > +		xdp = *ice_xdp_buf(rx_ring, rx_ring->next_to_clean);
> > +		xsk_buff_set_size(xdp, size);
> > +		xsk_buff_dma_sync_for_cpu(xdp, rx_ring->xsk_pool);
> >  
> > -		xdp_res = ice_run_xdp_zc(rx_ring, *xdp, xdp_prog, xdp_ring);
> > +		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);
> > +				xsk_buff_free(xdp);
> >  
> > -			*xdp = NULL;
> >  			total_rx_bytes += size;
> >  			total_rx_packets++;
> >  			cleaned_count++;
> > @@ -815,10 +817,9 @@ void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring)
> >  	u16 ntu = rx_ring->next_to_use;
> >  
> >  	for ( ; ntc != ntu; ntc = (ntc + 1) & count_mask) {
> > -		struct xdp_buff **xdp = &rx_ring->xdp_buf[ntc];
> > +		struct xdp_buff *xdp = *ice_xdp_buf(rx_ring, ntc);
> >  
> > -		xsk_buff_free(*xdp);
> > -		*xdp = NULL;
> > +		xsk_buff_free(xdp);
> >  	}
> >  }
> >  
> > -- 
> > 2.33.1
> 
> Al

[Intel-wired-lan] [PATCH intel-net 3/5] ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor

[Maciej Fijalkowski]

The descriptor that ntu is pointing at when we exit
ice_alloc_rx_bufs_zc() should not have its corresponding DD bit cleared
as descriptor is not allocated in there and it is not valid for HW
usage.

The allocation routine at the entry will fill the descriptor that ntu
points to after it was set to ntu + nb_buffs on previous call.

Even the spec says:
"The tail pointer should be set to one descriptor beyond the last empty
descriptor in host descriptor ring."

Therefore, step away from clearing the status_error0 on ntu + nb_buffs
descriptor.

Fixes: db804cfc21e9 ("ice: Use the xsk batched rx allocation interface")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 5cb61955c1f3..874fce9fa1c3 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -394,14 +394,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
 	}
 
 	ntu += nb_buffs;
-	if (ntu == rx_ring->count) {
-		rx_desc = ICE_RX_DESC(rx_ring, 0);
-		xdp = rx_ring->xdp_buf;
+	if (ntu == rx_ring->count)
 		ntu = 0;
-	}
 
-	/* clear the status bits for the next_to_use descriptor */
-	rx_desc->wb.status_error0 = 0;
 	ice_release_rx_desc(rx_ring, ntu);
 
 	return count == nb_buffs;
-- 
2.33.1

[Intel-wired-lan] [PATCH intel-net 3/5] ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor

[Alexander Lobakin]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Fri, 10 Dec 2021 15:59:39 +0100

> The descriptor that ntu is pointing at when we exit
> ice_alloc_rx_bufs_zc() should not have its corresponding DD bit cleared
> as descriptor is not allocated in there and it is not valid for HW
> usage.
> 
> The allocation routine at the entry will fill the descriptor that ntu
> points to after it was set to ntu + nb_buffs on previous call.
> 
> Even the spec says:
> "The tail pointer should be set to one descriptor beyond the last empty
> descriptor in host descriptor ring."
> 
> Therefore, step away from clearing the status_error0 on ntu + nb_buffs
> descriptor.
> 
> Fixes: db804cfc21e9 ("ice: Use the xsk batched rx allocation interface")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_xsk.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 5cb61955c1f3..874fce9fa1c3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -394,14 +394,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
>  	}
>  
>  	ntu += nb_buffs;
> -	if (ntu == rx_ring->count) {
> -		rx_desc = ICE_RX_DESC(rx_ring, 0);
> -		xdp = rx_ring->xdp_buf;
> +	if (ntu == rx_ring->count)

Maybe use unlikely() here while at it? 1/512 (depending on ring
size) chance is low enough.

>  		ntu = 0;
> -	}
>  
> -	/* clear the status bits for the next_to_use descriptor */
> -	rx_desc->wb.status_error0 = 0;
>  	ice_release_rx_desc(rx_ring, ntu);

This interferes with my patch in next-queue ([0]) (well, supersedes
it to be precise).
Tony, what would be better to do with it, just drop mine or correct
this one (it would become an oneliner removing status_error0
assignment then)?

>  
>  	return count == nb_buffs;
> -- 
> 2.33.1

[0] https://lore.kernel.org/netdev/20211130183649.1166842-2-alexandr.lobakin at intel.com

Al

[Intel-wired-lan] [PATCH intel-net 3/5] ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor

[Maciej Fijalkowski]

On Fri, Dec 10, 2021 at 11:37:46PM +0100, Alexander Lobakin wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Date: Fri, 10 Dec 2021 15:59:39 +0100
> 
> > The descriptor that ntu is pointing at when we exit
> > ice_alloc_rx_bufs_zc() should not have its corresponding DD bit cleared
> > as descriptor is not allocated in there and it is not valid for HW
> > usage.
> > 
> > The allocation routine at the entry will fill the descriptor that ntu
> > points to after it was set to ntu + nb_buffs on previous call.
> > 
> > Even the spec says:
> > "The tail pointer should be set to one descriptor beyond the last empty
> > descriptor in host descriptor ring."
> > 
> > Therefore, step away from clearing the status_error0 on ntu + nb_buffs
> > descriptor.
> > 
> > Fixes: db804cfc21e9 ("ice: Use the xsk batched rx allocation interface")
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_xsk.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > index 5cb61955c1f3..874fce9fa1c3 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > @@ -394,14 +394,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> >  	}
> >  
> >  	ntu += nb_buffs;
> > -	if (ntu == rx_ring->count) {
> > -		rx_desc = ICE_RX_DESC(rx_ring, 0);
> > -		xdp = rx_ring->xdp_buf;
> > +	if (ntu == rx_ring->count)
> 
> Maybe use unlikely() here while at it? 1/512 (depending on ring
> size) chance is low enough.

This would make sense to me if we would have this check inside some loop
going over the buffers that we received from xsk pool.

I tried such approach probably on Tx side and Magnus said that unlikely
will move this code to the cold section at the end of the text section.

> 
> >  		ntu = 0;
> > -	}
> >  
> > -	/* clear the status bits for the next_to_use descriptor */
> > -	rx_desc->wb.status_error0 = 0;
> >  	ice_release_rx_desc(rx_ring, ntu);
> 
> This interferes with my patch in next-queue ([0]) (well, supersedes
> it to be precise).
> Tony, what would be better to do with it, just drop mine or correct
> this one (it would become an oneliner removing status_error0
> assignment then)?

Oops, sorry. This set should go to net though, not net-next, but I can
base it on top of your patch.

> 
> >  
> >  	return count == nb_buffs;
> > -- 
> > 2.33.1
> 
> [0] https://lore.kernel.org/netdev/20211130183649.1166842-2-alexandr.lobakin at intel.com
> 
> Al

[Intel-wired-lan] [PATCH intel-net 3/5] ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor

[Alexander Lobakin]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Mon, 13 Dec 2021 12:12:13 +0100

> On Fri, Dec 10, 2021 at 11:37:46PM +0100, Alexander Lobakin wrote:
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Date: Fri, 10 Dec 2021 15:59:39 +0100
> > 
> > > The descriptor that ntu is pointing at when we exit
> > > ice_alloc_rx_bufs_zc() should not have its corresponding DD bit cleared
> > > as descriptor is not allocated in there and it is not valid for HW
> > > usage.
> > > 
> > > The allocation routine at the entry will fill the descriptor that ntu
> > > points to after it was set to ntu + nb_buffs on previous call.
> > > 
> > > Even the spec says:
> > > "The tail pointer should be set to one descriptor beyond the last empty
> > > descriptor in host descriptor ring."
> > > 
> > > Therefore, step away from clearing the status_error0 on ntu + nb_buffs
> > > descriptor.
> > > 
> > > Fixes: db804cfc21e9 ("ice: Use the xsk batched rx allocation interface")
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/ice/ice_xsk.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > index 5cb61955c1f3..874fce9fa1c3 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > @@ -394,14 +394,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > >  	}
> > >  
> > >  	ntu += nb_buffs;
> > > -	if (ntu == rx_ring->count) {
> > > -		rx_desc = ICE_RX_DESC(rx_ring, 0);
> > > -		xdp = rx_ring->xdp_buf;
> > > +	if (ntu == rx_ring->count)
> > 
> > Maybe use unlikely() here while at it? 1/512 (depending on ring
> > size) chance is low enough.
> 
> This would make sense to me if we would have this check inside some loop
> going over the buffers that we received from xsk pool.
> 
> I tried such approach probably on Tx side and Magnus said that unlikely
> will move this code to the cold section at the end of the text section.
> 
> > 
> > >  		ntu = 0;
> > > -	}
> > >  
> > > -	/* clear the status bits for the next_to_use descriptor */
> > > -	rx_desc->wb.status_error0 = 0;
> > >  	ice_release_rx_desc(rx_ring, ntu);
> > 
> > This interferes with my patch in next-queue ([0]) (well, supersedes
> > it to be precise).
> > Tony, what would be better to do with it, just drop mine or correct
> > this one (it would become an oneliner removing status_error0
> > assignment then)?
> 
> Oops, sorry. This set should go to net though, not net-next, but I can
> base it on top of your patch.

Ah, it's for -net, my bad. Let's leave it as it is then, my series
for -next has patches for both i40e and ice, so the latter will be
just merged when netdev mains pull net to net-next.

> 
> > 
> > >  
> > >  	return count == nb_buffs;
> > > -- 
> > > 2.33.1
> > 
> > [0] https://lore.kernel.org/netdev/20211130183649.1166842-2-alexandr.lobakin at intel.com
> > 
> > Al

Al

[Intel-wired-lan] [PATCH intel-net 3/5] ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor

[Maciej Fijalkowski]

On Mon, Dec 13, 2021 at 12:26:34PM +0100, Alexander Lobakin wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Date: Mon, 13 Dec 2021 12:12:13 +0100
> 
> > On Fri, Dec 10, 2021 at 11:37:46PM +0100, Alexander Lobakin wrote:
> > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > Date: Fri, 10 Dec 2021 15:59:39 +0100
> > > 
> > > > The descriptor that ntu is pointing at when we exit
> > > > ice_alloc_rx_bufs_zc() should not have its corresponding DD bit cleared
> > > > as descriptor is not allocated in there and it is not valid for HW
> > > > usage.
> > > > 
> > > > The allocation routine at the entry will fill the descriptor that ntu
> > > > points to after it was set to ntu + nb_buffs on previous call.
> > > > 
> > > > Even the spec says:
> > > > "The tail pointer should be set to one descriptor beyond the last empty
> > > > descriptor in host descriptor ring."
> > > > 
> > > > Therefore, step away from clearing the status_error0 on ntu + nb_buffs
> > > > descriptor.
> > > > 
> > > > Fixes: db804cfc21e9 ("ice: Use the xsk batched rx allocation interface")
> > > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > > ---
> > > >  drivers/net/ethernet/intel/ice/ice_xsk.c | 7 +------
> > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > > index 5cb61955c1f3..874fce9fa1c3 100644
> > > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > > @@ -394,14 +394,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > >  	}
> > > >  
> > > >  	ntu += nb_buffs;
> > > > -	if (ntu == rx_ring->count) {
> > > > -		rx_desc = ICE_RX_DESC(rx_ring, 0);
> > > > -		xdp = rx_ring->xdp_buf;
> > > > +	if (ntu == rx_ring->count)
> > > 
> > > Maybe use unlikely() here while at it? 1/512 (depending on ring
> > > size) chance is low enough.
> > 
> > This would make sense to me if we would have this check inside some loop
> > going over the buffers that we received from xsk pool.
> > 
> > I tried such approach probably on Tx side and Magnus said that unlikely
> > will move this code to the cold section at the end of the text section.
> > 
> > > 
> > > >  		ntu = 0;
> > > > -	}
> > > >  
> > > > -	/* clear the status bits for the next_to_use descriptor */
> > > > -	rx_desc->wb.status_error0 = 0;
> > > >  	ice_release_rx_desc(rx_ring, ntu);
> > > 
> > > This interferes with my patch in next-queue ([0]) (well, supersedes
> > > it to be precise).
> > > Tony, what would be better to do with it, just drop mine or correct
> > > this one (it would become an oneliner removing status_error0
> > > assignment then)?
> > 
> > Oops, sorry. This set should go to net though, not net-next, but I can
> > base it on top of your patch.
> 
> Ah, it's for -net, my bad. Let's leave it as it is then, my series
> for -next has patches for both i40e and ice, so the latter will be
> just merged when netdev mains pull net to net-next.

But we remove the same line in these patches. Your patch has a fixes tag,
so what was the reason for directing it to -next? Maybe I can include your
patch onto my set?

> 
> > 
> > > 
> > > >  
> > > >  	return count == nb_buffs;
> > > > -- 
> > > > 2.33.1
> > > 
> > > [0] https://lore.kernel.org/netdev/20211130183649.1166842-2-alexandr.lobakin at intel.com
> > > 
> > > Al
> 
> Al

[Intel-wired-lan] [PATCH intel-net 3/5] ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor

[Alexander Lobakin]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Mon, 13 Dec 2021 12:33:02 +0100

> On Mon, Dec 13, 2021 at 12:26:34PM +0100, Alexander Lobakin wrote:
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Date: Mon, 13 Dec 2021 12:12:13 +0100
> > 
> > > On Fri, Dec 10, 2021 at 11:37:46PM +0100, Alexander Lobakin wrote:
> > > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > > Date: Fri, 10 Dec 2021 15:59:39 +0100
> > > > 
> > > > > The descriptor that ntu is pointing at when we exit
> > > > > ice_alloc_rx_bufs_zc() should not have its corresponding DD bit cleared
> > > > > as descriptor is not allocated in there and it is not valid for HW
> > > > > usage.
> > > > > 
> > > > > The allocation routine at the entry will fill the descriptor that ntu
> > > > > points to after it was set to ntu + nb_buffs on previous call.
> > > > > 
> > > > > Even the spec says:
> > > > > "The tail pointer should be set to one descriptor beyond the last empty
> > > > > descriptor in host descriptor ring."
> > > > > 
> > > > > Therefore, step away from clearing the status_error0 on ntu + nb_buffs
> > > > > descriptor.
> > > > > 
> > > > > Fixes: db804cfc21e9 ("ice: Use the xsk batched rx allocation interface")
> > > > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > > > ---
> > > > >  drivers/net/ethernet/intel/ice/ice_xsk.c | 7 +------
> > > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > > > index 5cb61955c1f3..874fce9fa1c3 100644
> > > > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > > > @@ -394,14 +394,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > > >  	}
> > > > >  
> > > > >  	ntu += nb_buffs;
> > > > > -	if (ntu == rx_ring->count) {
> > > > > -		rx_desc = ICE_RX_DESC(rx_ring, 0);
> > > > > -		xdp = rx_ring->xdp_buf;
> > > > > +	if (ntu == rx_ring->count)
> > > > 
> > > > Maybe use unlikely() here while at it? 1/512 (depending on ring
> > > > size) chance is low enough.
> > > 
> > > This would make sense to me if we would have this check inside some loop
> > > going over the buffers that we received from xsk pool.
> > > 
> > > I tried such approach probably on Tx side and Magnus said that unlikely
> > > will move this code to the cold section at the end of the text section.
> > > 
> > > > 
> > > > >  		ntu = 0;
> > > > > -	}
> > > > >  
> > > > > -	/* clear the status bits for the next_to_use descriptor */
> > > > > -	rx_desc->wb.status_error0 = 0;
> > > > >  	ice_release_rx_desc(rx_ring, ntu);
> > > > 
> > > > This interferes with my patch in next-queue ([0]) (well, supersedes
> > > > it to be precise).
> > > > Tony, what would be better to do with it, just drop mine or correct
> > > > this one (it would become an oneliner removing status_error0
> > > > assignment then)?
> > > 
> > > Oops, sorry. This set should go to net though, not net-next, but I can
> > > base it on top of your patch.
> > 
> > Ah, it's for -net, my bad. Let's leave it as it is then, my series
> > for -next has patches for both i40e and ice, so the latter will be
> > just merged when netdev mains pull net to net-next.
> 
> But we remove the same line in these patches. Your patch has a fixes tag,
> so what was the reason for directing it to -next? Maybe I can include your
> patch onto my set?

My reasoning was that those dead stores don't hurt, so they're
fixes, but mostly cosmetic -> such usually goes to next.
But I think it should be fine to include mines into your series,
thanks!

> 
> > 
> > > 
> > > > 
> > > > >  
> > > > >  	return count == nb_buffs;
> > > > > -- 
> > > > > 2.33.1
> > > > 
> > > > [0] https://lore.kernel.org/netdev/20211130183649.1166842-2-alexandr.lobakin at intel.com
> > > > 
> > > > Al
> > 
> > Al

Al

[Intel-wired-lan] [PATCH intel-net 3/5] ice: xsk: do not clear status_error0 for ntu + nb_buffs descriptor

[Magnus Karlsson]

On Mon, Dec 13, 2021 at 12:12 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Fri, Dec 10, 2021 at 11:37:46PM +0100, Alexander Lobakin wrote:
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Date: Fri, 10 Dec 2021 15:59:39 +0100
> >
> > > The descriptor that ntu is pointing at when we exit
> > > ice_alloc_rx_bufs_zc() should not have its corresponding DD bit cleared
> > > as descriptor is not allocated in there and it is not valid for HW
> > > usage.
> > >
> > > The allocation routine at the entry will fill the descriptor that ntu
> > > points to after it was set to ntu + nb_buffs on previous call.
> > >
> > > Even the spec says:
> > > "The tail pointer should be set to one descriptor beyond the last empty
> > > descriptor in host descriptor ring."
> > >
> > > Therefore, step away from clearing the status_error0 on ntu + nb_buffs
> > > descriptor.
> > >
> > > Fixes: db804cfc21e9 ("ice: Use the xsk batched rx allocation interface")
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/ice/ice_xsk.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > index 5cb61955c1f3..874fce9fa1c3 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > @@ -394,14 +394,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > >     }
> > >
> > >     ntu += nb_buffs;
> > > -   if (ntu == rx_ring->count) {
> > > -           rx_desc = ICE_RX_DESC(rx_ring, 0);
> > > -           xdp = rx_ring->xdp_buf;
> > > +   if (ntu == rx_ring->count)
> >
> > Maybe use unlikely() here while at it? 1/512 (depending on ring
> > size) chance is low enough.
>
> This would make sense to me if we would have this check inside some loop
> going over the buffers that we received from xsk pool.
>
> I tried such approach probably on Tx side and Magnus said that unlikely
> will move this code to the cold section at the end of the text section.

Not guaranteed but can be put in a cold section which will induce a
cache miss most of the time. Not having an unlikely statement might
mean no cache miss and better performance. I had an unlikely statement
in the SW ring code for a simple wrap case like this and it did not
pay off. It performed better without it as it was hit often enough so
I eventually removed it in a follow up patch. For something small like
this, it is probably not worth the unlikely. I see unlikely as
something you use when it is an error, fault, initialization or
something else that happens once or less.

> >
> > >             ntu = 0;
> > > -   }
> > >
> > > -   /* clear the status bits for the next_to_use descriptor */
> > > -   rx_desc->wb.status_error0 = 0;
> > >     ice_release_rx_desc(rx_ring, ntu);
> >
> > This interferes with my patch in next-queue ([0]) (well, supersedes
> > it to be precise).
> > Tony, what would be better to do with it, just drop mine or correct
> > this one (it would become an oneliner removing status_error0
> > assignment then)?
>
> Oops, sorry. This set should go to net though, not net-next, but I can
> base it on top of your patch.
>
> >
> > >
> > >     return count == nb_buffs;
> > > --
> > > 2.33.1
> >
> > [0] https://lore.kernel.org/netdev/20211130183649.1166842-2-alexandr.lobakin at intel.com
> >
> > Al
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH intel-net 4/5] ice: xsk: allow empty Rx descriptors on XSK ZC data path

[Maciej Fijalkowski]

Commit ac6f733a7bd5 ("ice: allow empty Rx descriptors") stated that ice
HW can produce empty descriptors that are valid and they should be
processed.

Add this support to xsk ZC path to avoid potential processing problems.

Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 874fce9fa1c3..80f5a6194a49 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -537,12 +537,18 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		 */
 		dma_rmb();
 
+		xdp = *ice_xdp_buf(rx_ring, rx_ring->next_to_clean);
+
 		size = le16_to_cpu(rx_desc->wb.pkt_len) &
 				   ICE_RX_FLX_DESC_PKT_LEN_M;
-		if (!size)
-			break;
+		if (!size) {
+			xdp->data = NULL;
+			xdp->data_end = NULL;
+			xdp->data_hard_start = NULL;
+			xdp->data_meta = NULL;
+			goto construct_skb;
+		}
 
-		xdp = *ice_xdp_buf(rx_ring, rx_ring->next_to_clean);
 		xsk_buff_set_size(xdp, size);
 		xsk_buff_dma_sync_for_cpu(xdp, rx_ring->xsk_pool);
 
@@ -560,7 +566,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 			ice_bump_ntc(rx_ring);
 			continue;
 		}
-
+construct_skb:
 		/* XDP_PASS path */
 		skb = ice_construct_skb_zc(rx_ring, xdp);
 		if (!skb) {
-- 
2.33.1

[Intel-wired-lan] [PATCH intel-net 5/5] ice: xsk: fix cleaned_count setting

[Maciej Fijalkowski]

Currently cleaned_count is initialized to ICE_DESC_UNUSED(rx_ring) and
later on during the Rx processing it is incremented per each frame that
driver consumed. This can result in excessive buffers requested from xsk
pool based on that value.

To address this, just drop cleaned_count and pass
ICE_DESC_UNUSED(rx_ring) directly as a function argument to
ice_alloc_rx_bufs_zc(). Idea is to ask for buffers as many as consumed.

Let us also call ice_alloc_rx_bufs_zc unconditionally at the end of
ice_clean_rx_irq_zc. This has been changed in that way for corresponding
ice_clean_rx_irq, but not here.

Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
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  | 6 +-----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index c56dd1749903..b7b3bd4816f0 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -24,7 +24,6 @@
 #define ICE_MAX_DATA_PER_TXD_ALIGNED \
 	(~(ICE_MAX_READ_REQ_SIZE - 1) & ICE_MAX_DATA_PER_TXD)
 
-#define ICE_RX_BUF_WRITE	16	/* Must be power of 2 */
 #define ICE_MAX_TXQ_PER_TXQG	128
 
 /* Attempt to maximize the headroom available for incoming frames. We use a 2K
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 80f5a6194a49..925326c70701 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -504,7 +504,6 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
 int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 {
 	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
-	u16 cleaned_count = ICE_DESC_UNUSED(rx_ring);
 	struct ice_tx_ring *xdp_ring;
 	unsigned int xdp_xmit = 0;
 	struct bpf_prog *xdp_prog;
@@ -561,7 +560,6 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 
 			total_rx_bytes += size;
 			total_rx_packets++;
-			cleaned_count++;
 
 			ice_bump_ntc(rx_ring);
 			continue;
@@ -574,7 +572,6 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 			break;
 		}
 
-		cleaned_count++;
 		ice_bump_ntc(rx_ring);
 
 		if (eth_skb_pad(skb)) {
@@ -596,8 +593,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
 		ice_receive_skb(rx_ring, skb, vlan_tag);
 	}
 
-	if (cleaned_count >= ICE_RX_BUF_WRITE)
-		failure = !ice_alloc_rx_bufs_zc(rx_ring, cleaned_count);
+	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