[PATCH net-next 0/6][pull request] 40GbE Intel Wired LAN Driver Updates 2021-02-01

[PATCH net-next 0/6][pull request] 40GbE Intel Wired LAN Driver Updates 2021-02-01

[Tony Nguyen]

This series contains updates to i40e driver only.

Cristian makes improvements to driver XDP path. Avoids writing
next-to-clean pointer on every update, removes redundant updates of
cleaned_count and buffer info, creates a helper function to consolidate
XDP actions and simplifies some of the behavior.

Arkadiusz adds a message to inform user of the need for XDP_REDIRECT
to match number of queue on both interfaces.

Eryk adds messages to inform the user when MTU is larger than supported
for an XDP program.

The following are changes since commit 14e8e0f6008865d823a8184a276702a6c3cbef3d:
  tcp: shrink inet_connection_sock icsk_mtup enabled and probe_size
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 40GbE

Arkadiusz Kubalewski (1):
  i40e: Add info trace at loading XDP program

Cristian Dumitrescu (4):
  i40e: remove unnecessary memory writes of the next to clean pointer
  i40e: remove unnecessary cleaned_count updates
  i40e: remove the redundant buffer info updates
  i40e: consolidate handling of XDP program actions

Eryk Rybak (1):
  i40e: Log error for oversized MTU on device

 drivers/net/ethernet/intel/i40e/i40e_main.c |  19 ++-
 drivers/net/ethernet/intel/i40e/i40e_xsk.c  | 149 +++++++++++---------
 2 files changed, 93 insertions(+), 75 deletions(-)

-- 
2.26.2

[PATCH net-next 1/6] i40e: remove unnecessary memory writes of the next to clean pointer

[Tony Nguyen]

From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

For performance reasons, avoid writing the ring next-to-clean pointer
value back to memory on every update, as it is not really necessary.
Instead, simply read it at initialization into a local copy, update
the local copy as necessary and write the local copy back to memory
after the last update.

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Tested-by: Kiran Bhandare <kiranx.bhandare@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 30 ++++++++--------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 492ce213208d..87d43407653c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -261,18 +261,6 @@ static struct sk_buff *i40e_construct_skb_zc(struct i40e_ring *rx_ring,
 	return skb;
 }
 
-/**
- * i40e_inc_ntc: Advance the next_to_clean index
- * @rx_ring: Rx ring
- **/
-static void i40e_inc_ntc(struct i40e_ring *rx_ring)
-{
-	u32 ntc = rx_ring->next_to_clean + 1;
-
-	ntc = (ntc < rx_ring->count) ? ntc : 0;
-	rx_ring->next_to_clean = ntc;
-}
-
 /**
  * i40e_clean_rx_irq_zc - Consumes Rx packets from the hardware ring
  * @rx_ring: Rx ring
@@ -284,6 +272,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 {
 	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
 	u16 cleaned_count = I40E_DESC_UNUSED(rx_ring);
+	u16 next_to_clean = rx_ring->next_to_clean;
+	u16 count_mask = rx_ring->count - 1;
 	unsigned int xdp_res, xdp_xmit = 0;
 	bool failure = false;
 	struct sk_buff *skb;
@@ -294,7 +284,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		unsigned int size;
 		u64 qword;
 
-		rx_desc = I40E_RX_DESC(rx_ring, rx_ring->next_to_clean);
+		rx_desc = I40E_RX_DESC(rx_ring, next_to_clean);
 		qword = le64_to_cpu(rx_desc->wb.qword1.status_error_len);
 
 		/* This memory barrier is needed to keep us from reading
@@ -307,11 +297,11 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 			i40e_clean_programming_status(rx_ring,
 						      rx_desc->raw.qword[0],
 						      qword);
-			bi = i40e_rx_bi(rx_ring, rx_ring->next_to_clean);
+			bi = i40e_rx_bi(rx_ring, next_to_clean);
 			xsk_buff_free(*bi);
 			*bi = NULL;
 			cleaned_count++;
-			i40e_inc_ntc(rx_ring);
+			next_to_clean = (next_to_clean + 1) & count_mask;
 			continue;
 		}
 
@@ -320,7 +310,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		if (!size)
 			break;
 
-		bi = i40e_rx_bi(rx_ring, rx_ring->next_to_clean);
+		bi = i40e_rx_bi(rx_ring, next_to_clean);
 		(*bi)->data_end = (*bi)->data + size;
 		xsk_buff_dma_sync_for_cpu(*bi, rx_ring->xsk_pool);
 
@@ -336,7 +326,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 			total_rx_packets++;
 
 			cleaned_count++;
-			i40e_inc_ntc(rx_ring);
+			next_to_clean = (next_to_clean + 1) & count_mask;
 			continue;
 		}
 
@@ -355,7 +345,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 
 		*bi = NULL;
 		cleaned_count++;
-		i40e_inc_ntc(rx_ring);
+		next_to_clean = (next_to_clean + 1) & count_mask;
 
 		if (eth_skb_pad(skb))
 			continue;
@@ -367,6 +357,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		napi_gro_receive(&rx_ring->q_vector->napi, skb);
 	}
 
+	rx_ring->next_to_clean = next_to_clean;
+
 	if (cleaned_count >= I40E_RX_BUFFER_WRITE)
 		failure = !i40e_alloc_rx_buffers_zc(rx_ring, cleaned_count);
 
@@ -374,7 +366,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 	i40e_update_rx_stats(rx_ring, total_rx_bytes, total_rx_packets);
 
 	if (xsk_uses_need_wakeup(rx_ring->xsk_pool)) {
-		if (failure || rx_ring->next_to_clean == rx_ring->next_to_use)
+		if (failure || next_to_clean == rx_ring->next_to_use)
 			xsk_set_rx_need_wakeup(rx_ring->xsk_pool);
 		else
 			xsk_clear_rx_need_wakeup(rx_ring->xsk_pool);
-- 
2.26.2

[PATCH net-next 2/6] i40e: remove unnecessary cleaned_count updates

[Tony Nguyen]

From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

For performance reasons, remove the redundant updates of the cleaned_count
variable, as its value can be computed based on the ring next-to-clean
variable, which is consistently updated.

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Tested-by: Kiran Bhandare <kiranx.bhandare@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 87d43407653c..99082abd3000 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -300,7 +300,6 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 			bi = i40e_rx_bi(rx_ring, next_to_clean);
 			xsk_buff_free(*bi);
 			*bi = NULL;
-			cleaned_count++;
 			next_to_clean = (next_to_clean + 1) & count_mask;
 			continue;
 		}
@@ -325,7 +324,6 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 			total_rx_bytes += size;
 			total_rx_packets++;
 
-			cleaned_count++;
 			next_to_clean = (next_to_clean + 1) & count_mask;
 			continue;
 		}
@@ -344,7 +342,6 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		}
 
 		*bi = NULL;
-		cleaned_count++;
 		next_to_clean = (next_to_clean + 1) & count_mask;
 
 		if (eth_skb_pad(skb))
@@ -358,6 +355,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 	}
 
 	rx_ring->next_to_clean = next_to_clean;
+	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);
-- 
2.26.2

[PATCH net-next 3/6] i40e: remove the redundant buffer info updates

[Tony Nguyen]

From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

For performance reasons, remove the redundant buffer info updates
(*bi = NULL). The buffers ready to be cleaned can easily be tracked
based on the ring next-to-clean variable, which is consistently
updated.

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Tested-by: Kiran Bhandare <kiranx.bhandare@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 33 +++++++++-------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 99082abd3000..1167496a2e08 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -280,7 +280,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 
 	while (likely(total_rx_packets < (unsigned int)budget)) {
 		union i40e_rx_desc *rx_desc;
-		struct xdp_buff **bi;
+		struct xdp_buff *bi;
 		unsigned int size;
 		u64 qword;
 
@@ -297,9 +297,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 			i40e_clean_programming_status(rx_ring,
 						      rx_desc->raw.qword[0],
 						      qword);
-			bi = i40e_rx_bi(rx_ring, next_to_clean);
-			xsk_buff_free(*bi);
-			*bi = NULL;
+			bi = *i40e_rx_bi(rx_ring, next_to_clean);
+			xsk_buff_free(bi);
 			next_to_clean = (next_to_clean + 1) & count_mask;
 			continue;
 		}
@@ -309,18 +308,17 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		if (!size)
 			break;
 
-		bi = i40e_rx_bi(rx_ring, next_to_clean);
-		(*bi)->data_end = (*bi)->data + size;
-		xsk_buff_dma_sync_for_cpu(*bi, rx_ring->xsk_pool);
+		bi = *i40e_rx_bi(rx_ring, next_to_clean);
+		bi->data_end = bi->data + size;
+		xsk_buff_dma_sync_for_cpu(bi, rx_ring->xsk_pool);
 
-		xdp_res = i40e_run_xdp_zc(rx_ring, *bi);
+		xdp_res = i40e_run_xdp_zc(rx_ring, bi);
 		if (xdp_res) {
 			if (xdp_res & (I40E_XDP_TX | I40E_XDP_REDIR))
 				xdp_xmit |= xdp_res;
 			else
-				xsk_buff_free(*bi);
+				xsk_buff_free(bi);
 
-			*bi = NULL;
 			total_rx_bytes += size;
 			total_rx_packets++;
 
@@ -335,13 +333,12 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		 * BIT(I40E_RXD_QW1_ERROR_SHIFT). This is due to that
 		 * SBP is *not* set in PRT_SBPVSI (default not set).
 		 */
-		skb = i40e_construct_skb_zc(rx_ring, *bi);
+		skb = i40e_construct_skb_zc(rx_ring, bi);
 		if (!skb) {
 			rx_ring->rx_stats.alloc_buff_failed++;
 			break;
 		}
 
-		*bi = NULL;
 		next_to_clean = (next_to_clean + 1) & count_mask;
 
 		if (eth_skb_pad(skb))
@@ -594,16 +591,14 @@ int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
 
 void i40e_xsk_clean_rx_ring(struct i40e_ring *rx_ring)
 {
-	u16 i;
-
-	for (i = 0; i < rx_ring->count; i++) {
-		struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, i);
+	u16 count_mask = rx_ring->count - 1;
+	u16 ntc = rx_ring->next_to_clean;
+	u16 ntu = rx_ring->next_to_use;
 
-		if (!rx_bi)
-			continue;
+	for ( ; ntc != ntu; ntc = (ntc + 1)  & count_mask) {
+		struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, ntc);
 
 		xsk_buff_free(rx_bi);
-		rx_bi = NULL;
 	}
 }
 
-- 
2.26.2

[PATCH net-next 4/6] i40e: consolidate handling of XDP program actions

[Tony Nguyen]

From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Consolidate the actions performed on the packet based on the XDP
program result into a separate function that is easier to read and
maintain. Simplify the i40e_construct_skb_zc function, so that the
input xdp buffer is always freed, regardless of whether the output
skb is successfully created or not. Simplify the behavior of the
i40e_clean_rx_irq_zc function, so that the current packet descriptor
is dropped when function i40_construct_skb_zc returns an error as
opposed to re-processing the same description on the next invocation.

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Tested-by: Kiran Bhandare <kiranx.bhandare@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 98 ++++++++++++++--------
 1 file changed, 61 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 1167496a2e08..470b8600adb1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -250,17 +250,70 @@ static struct sk_buff *i40e_construct_skb_zc(struct i40e_ring *rx_ring,
 			       xdp->data_end - xdp->data_hard_start,
 			       GFP_ATOMIC | __GFP_NOWARN);
 	if (unlikely(!skb))
-		return NULL;
+		goto out;
 
 	skb_reserve(skb, xdp->data - xdp->data_hard_start);
 	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
 	if (metasize)
 		skb_metadata_set(skb, metasize);
 
+out:
 	xsk_buff_free(xdp);
 	return skb;
 }
 
+static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
+				      struct xdp_buff *xdp_buff,
+				      union i40e_rx_desc *rx_desc,
+				      unsigned int *rx_packets,
+				      unsigned int *rx_bytes,
+				      unsigned int size,
+				      unsigned int xdp_res)
+{
+	struct sk_buff *skb;
+
+	*rx_packets = 1;
+	*rx_bytes = size;
+
+	if (likely(xdp_res == I40E_XDP_REDIR) || xdp_res == I40E_XDP_TX)
+		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
+		 * BIT(I40E_RXD_QW1_ERROR_SHIFT). This is due to that
+		 * SBP is *not* set in PRT_SBPVSI (default not set).
+		 */
+		skb = i40e_construct_skb_zc(rx_ring, xdp_buff);
+		if (!skb) {
+			rx_ring->rx_stats.alloc_buff_failed++;
+			*rx_packets = 0;
+			*rx_bytes = 0;
+			return;
+		}
+
+		if (eth_skb_pad(skb)) {
+			*rx_packets = 0;
+			*rx_bytes = 0;
+			return;
+		}
+
+		*rx_bytes = skb->len;
+		i40e_process_skb_fields(rx_ring, rx_desc, skb);
+		napi_gro_receive(&rx_ring->q_vector->napi, skb);
+		return;
+	}
+
+	/* Should never get here, as all valid cases have been handled already.
+	 */
+	WARN_ON_ONCE(1);
+}
+
 /**
  * i40e_clean_rx_irq_zc - Consumes Rx packets from the hardware ring
  * @rx_ring: Rx ring
@@ -276,10 +329,11 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 	u16 count_mask = rx_ring->count - 1;
 	unsigned int xdp_res, xdp_xmit = 0;
 	bool failure = false;
-	struct sk_buff *skb;
 
 	while (likely(total_rx_packets < (unsigned int)budget)) {
 		union i40e_rx_desc *rx_desc;
+		unsigned int rx_packets;
+		unsigned int rx_bytes;
 		struct xdp_buff *bi;
 		unsigned int size;
 		u64 qword;
@@ -313,42 +367,12 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		xsk_buff_dma_sync_for_cpu(bi, rx_ring->xsk_pool);
 
 		xdp_res = i40e_run_xdp_zc(rx_ring, bi);
-		if (xdp_res) {
-			if (xdp_res & (I40E_XDP_TX | I40E_XDP_REDIR))
-				xdp_xmit |= xdp_res;
-			else
-				xsk_buff_free(bi);
-
-			total_rx_bytes += size;
-			total_rx_packets++;
-
-			next_to_clean = (next_to_clean + 1) & count_mask;
-			continue;
-		}
-
-		/* XDP_PASS path */
-
-		/* NB! We are not checking for errors using
-		 * i40e_test_staterr with
-		 * BIT(I40E_RXD_QW1_ERROR_SHIFT). This is due to that
-		 * SBP is *not* set in PRT_SBPVSI (default not set).
-		 */
-		skb = i40e_construct_skb_zc(rx_ring, bi);
-		if (!skb) {
-			rx_ring->rx_stats.alloc_buff_failed++;
-			break;
-		}
-
+		i40e_handle_xdp_result_zc(rx_ring, bi, rx_desc, &rx_packets,
+					  &rx_bytes, size, xdp_res);
+		total_rx_packets += rx_packets;
+		total_rx_bytes += rx_bytes;
+		xdp_xmit |= xdp_res & (I40E_XDP_TX | I40E_XDP_REDIR);
 		next_to_clean = (next_to_clean + 1) & count_mask;
-
-		if (eth_skb_pad(skb))
-			continue;
-
-		total_rx_bytes += skb->len;
-		total_rx_packets++;
-
-		i40e_process_skb_fields(rx_ring, rx_desc, skb);
-		napi_gro_receive(&rx_ring->q_vector->napi, skb);
 	}
 
 	rx_ring->next_to_clean = next_to_clean;
-- 
2.26.2

[PATCH net-next 5/6] i40e: Add info trace at loading XDP program

[Tony Nguyen]

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

New trace indicates that the XDP program was loaded.
The trace has a note that in case of using XDP_REDIRECT,
number of queues on both interfaces shall be the same.
This is required for optimal performance of XDP_REDIRECT,
if interface used for TX has lower number of queues than
a RX interface, the packets may be dropped (depending on
RSS queue assignment).

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 521ea9df38d5..f35bd9164106 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
 	/* Kick start the NAPI context if there is an AF_XDP socket open
 	 * on that queue id. This so that receiving will start.
 	 */
-	if (need_reset && prog)
+	if (need_reset && prog) {
+		dev_info(&pf->pdev->dev,
+			 "Loading XDP program, please note: XDP_REDIRECT action requires the same number of queues on both interfaces\n");
 		for (i = 0; i < vsi->num_queue_pairs; i++)
 			if (vsi->xdp_rings[i]->xsk_pool)
 				(void)i40e_xsk_wakeup(vsi->netdev, i,
 						      XDP_WAKEUP_RX);
+	}
 
 	return 0;
 }
-- 
2.26.2

Re: [PATCH net-next 5/6] i40e: Add info trace at loading XDP program

[Jakub Kicinski]

On Mon,  1 Feb 2021 18:24:19 -0800 Tony Nguyen wrote:
> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> 
> New trace indicates that the XDP program was loaded.
> The trace has a note that in case of using XDP_REDIRECT,
> number of queues on both interfaces shall be the same.
> This is required for optimal performance of XDP_REDIRECT,
> if interface used for TX has lower number of queues than
> a RX interface, the packets may be dropped (depending on
> RSS queue assignment).

By RSS queue assignment you mean interrupt mapping?

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 521ea9df38d5..f35bd9164106 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
>  	/* Kick start the NAPI context if there is an AF_XDP socket open
>  	 * on that queue id. This so that receiving will start.
>  	 */
> -	if (need_reset && prog)
> +	if (need_reset && prog) {
> +		dev_info(&pf->pdev->dev,
> +			 "Loading XDP program, please note: XDP_REDIRECT action requires the same number of queues on both interfaces\n");

We try to avoid spamming logs. This message will be helpful to users
only the first time, if at all.

RE: [PATCH net-next 5/6] i40e: Add info trace at loading XDP program

[Kubalewski, Arkadiusz]

Hi Kuba, thank you for the comments!

>On Mon,  1 Feb 2021 18:24:19 -0800 Tony Nguyen wrote:
>> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> 
>> New trace indicates that the XDP program was loaded.
>> The trace has a note that in case of using XDP_REDIRECT,
>> number of queues on both interfaces shall be the same.
>> This is required for optimal performance of XDP_REDIRECT,
>> if interface used for TX has lower number of queues than
>> a RX interface, the packets may be dropped (depending on
>> RSS queue assignment).
>
>By RSS queue assignment you mean interrupt mapping?

Yes, interrupt mapping seems more accurate, will fix it.

>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index 521ea9df38d5..f35bd9164106 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
>>  	/* Kick start the NAPI context if there is an AF_XDP socket open
>>  	 * on that queue id. This so that receiving will start.
>>  	 */
>> -	if (need_reset && prog)
>> +	if (need_reset && prog) {
>> +		dev_info(&pf->pdev->dev,
>> +			 "Loading XDP program, please note: XDP_REDIRECT action requires the same number of queues on both interfaces\n");
>
>We try to avoid spamming logs. This message will be helpful to users
>only the first time, if at all.

You are probably right, it would look like a spam to the one who is
continuously loading and unloading the XDP programs.
But still, want to remain as much user friendly as possible.
Will use dev_info_once(...) instead.
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
 

Re: [PATCH net-next 5/6] i40e: Add info trace at loading XDP program

[Jakub Kicinski]

On Wed, 3 Feb 2021 10:00:07 +0000 Kubalewski, Arkadiusz wrote:
> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> >> index 521ea9df38d5..f35bd9164106 100644
> >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> >> @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
> >>  	/* Kick start the NAPI context if there is an AF_XDP socket open
> >>  	 * on that queue id. This so that receiving will start.
> >>  	 */
> >> -	if (need_reset && prog)
> >> +	if (need_reset && prog) {
> >> +		dev_info(&pf->pdev->dev,
> >> +			 "Loading XDP program, please note: XDP_REDIRECT action requires the same number of queues on both interfaces\n");  
> >
> >We try to avoid spamming logs. This message will be helpful to users
> >only the first time, if at all.  
> 
> You are probably right, it would look like a spam to the one who is
> continuously loading and unloading the XDP programs.
> But still, want to remain as much user friendly as possible.
> Will use dev_info_once(...) instead.

Not exactly what I meant, I meant that it's only marginally useful the
first time the user sees it. Not first time since boot.

The two options that I think could be better are:
 - work on improving the interfaces in terms of IRQ/queue config and
   capabilities so the user is not confused in the first place;
 - detect that the configuration is in fact problematic 
   (IOW #Qs < #CPUs) and setting extack. If you set the extact and
   return 0 / success the extact will show as "Warning: " in iproute2
   output.

RE: [PATCH net-next 5/6] i40e: Add info trace at loading XDP program

[Kubalewski, Arkadiusz]

>On Wed, 3 Feb 2021 10:00:07 +0000 Kubalewski, Arkadiusz wrote:
>> >> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
>> >> b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> >> index 521ea9df38d5..f35bd9164106 100644
>> >> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> >> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> >> @@ -12489,11 +12489,14 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
>> >>  	/* Kick start the NAPI context if there is an AF_XDP socket open
>> >>  	 * on that queue id. This so that receiving will start.
>> >>  	 */
>> >> -	if (need_reset && prog)
>> >> +	if (need_reset && prog) {
>> >> +		dev_info(&pf->pdev->dev,
>> >> +			 "Loading XDP program, please note: XDP_REDIRECT action 
>> >> +requires the same number of queues on both interfaces\n");
>> >
>> >We try to avoid spamming logs. This message will be helpful to users 
>> >only the first time, if at all.
>> 
>> You are probably right, it would look like a spam to the one who is 
>> continuously loading and unloading the XDP programs.
>> But still, want to remain as much user friendly as possible.
>> Will use dev_info_once(...) instead.
>
>Not exactly what I meant, I meant that it's only marginally useful the first time the user sees it. Not first time since boot.
>
>The two options that I think could be better are:

Well, I know that this is far from being perfect.
If I understand your comments correctly:

> - work on improving the interfaces in terms of IRQ/queue config and
>   capabilities so the user is not confused in the first place;

Improved interface would allow the driver which is being loaded with 
the xdp program to receive configuration of the other NICs 
on the system. (its number of queues/IRQs), and in such case we could
warn the user about possible drops.


> - detect that the configuration is in fact problematic 
>   (IOW #Qs < #CPUs) and setting extack. If you set the extact and
>   return 0 / success the extact will show as "Warning: " in iproute2
>   output.
>

It seems like this is the same idea? Detect number of queues of other NIC.
Then warn the user.


In general I agree and that was my first idea... but after all, we decided
to just try hint the user, that proper configuration is required.


(Hopefully) the proper solution.. 
With my current knowledge and understanding of how XDP_REDIRECT works:
It cannot be loaded with iproute2, it uses bpf maps thus it also
requires an loader application to create ones
(i.e. the one from samples/bpf/)
The sample uses /tools/lib/bpf library calls, which uses netlink to 
eventually do the .ndo_bpf call on two ports. The one responsible
for the RX and the other TX one. Although XDP can redirect to more then
one TX. Thus proper solution has to work for both cases.
In case of two or more devies used for redirecting TX (i.e. properly
implemented xdp_redirect_map). The interface which is used for RX shall
receive the lowest number of queues/IRQs of all the possible TX interfaces.
Then it can properly warn the user.

I think this is doable, but it requires changes on all the way from
bpf program loader, through: libbpf, netlink, net/core..
Probably finally extending netdev_bpf with a field that stores
the lowest number of queues of the interfaces which are used for TX.


Real proper solution..
Please, let me know if this is good approach, especially all the XDP experts.
Maybe there are similar problems that I am not aware of?


This patch..
So we end up with the user which has to properly implement its
bpf xdp redirect loader to pass the proper number of queues to the
RX interface. Even with all the above changes in the kernel and its
interfaces he still might not know that something is wrong with his
configuration/code.
Thus, even then information added in this patch might be useful.
At least that is what I think.

[PATCH net-next 6/6] i40e: Log error for oversized MTU on device

[Tony Nguyen]

From: Eryk Rybak <eryk.roch.rybak@intel.com>

When attempting to link XDP prog with MTU larger than supported,
user is not informed why XDP linking fails. Adding proper
error message: "MTU too large to enable XDP".
Due to the lack of support for non-static variables in netlinks
extended ACK feature, additional information has been added to dmesg
to better inform about invalid MTU setting.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Eryk Rybak <eryk.roch.rybak@intel.com>
Tested-by: Kiran Bhandare <kiranx.bhandare@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index f35bd9164106..b52a324a9f28 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12448,9 +12448,10 @@ static netdev_features_t i40e_features_check(struct sk_buff *skb,
  * i40e_xdp_setup - add/remove an XDP program
  * @vsi: VSI to changed
  * @prog: XDP program
+ * @extack: netlink extended ack
  **/
-static int i40e_xdp_setup(struct i40e_vsi *vsi,
-			  struct bpf_prog *prog)
+static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
+			  struct netlink_ext_ack *extack)
 {
 	int frame_size = vsi->netdev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
 	struct i40e_pf *pf = vsi->back;
@@ -12459,8 +12460,13 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
 	int i;
 
 	/* Don't allow frames that span over multiple buffers */
-	if (frame_size > vsi->rx_buf_len)
+	if (frame_size > vsi->rx_buf_len) {
+		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
+		dev_info(&pf->pdev->dev,
+			 "MTU of %u bytes is too large to enable XDP (maximum: %u bytes)\n",
+			 vsi->netdev->mtu, vsi->rx_buf_len);
 		return -EINVAL;
+	}
 
 	if (!i40e_enabled_xdp_vsi(vsi) && !prog)
 		return 0;
@@ -12772,7 +12778,7 @@ static int i40e_xdp(struct net_device *dev,
 
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		return i40e_xdp_setup(vsi, xdp->prog);
+		return i40e_xdp_setup(vsi, xdp->prog, xdp->extack);
 	case XDP_SETUP_XSK_POOL:
 		return i40e_xsk_pool_setup(vsi, xdp->xsk.pool,
 					   xdp->xsk.queue_id);
-- 
2.26.2

Re: [PATCH net-next 6/6] i40e: Log error for oversized MTU on device

[Jakub Kicinski]

On Mon,  1 Feb 2021 18:24:20 -0800 Tony Nguyen wrote:
> From: Eryk Rybak <eryk.roch.rybak@intel.com>
> 
> When attempting to link XDP prog with MTU larger than supported,
> user is not informed why XDP linking fails. Adding proper
> error message: "MTU too large to enable XDP".
> Due to the lack of support for non-static variables in netlinks
> extended ACK feature, additional information has been added to dmesg
> to better inform about invalid MTU setting.
> 
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Eryk Rybak <eryk.roch.rybak@intel.com>
> Tested-by: Kiran Bhandare <kiranx.bhandare@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

> @@ -12459,8 +12460,13 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
>  	int i;
>  
>  	/* Don't allow frames that span over multiple buffers */
> -	if (frame_size > vsi->rx_buf_len)
> +	if (frame_size > vsi->rx_buf_len) {
> +		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
> +		dev_info(&pf->pdev->dev,
> +			 "MTU of %u bytes is too large to enable XDP (maximum: %u bytes)\n",
> +			 vsi->netdev->mtu, vsi->rx_buf_len);

Extack should be enough.

RE: [PATCH net-next 6/6] i40e: Log error for oversized MTU on device

[Loktionov, Aleksandr]

Good day Jakub

We want to be user friendly to help users troubleshoot faster.
Only dmesg message can have template parameters so we can provide exact acceptable maximum bytes.
Can you could you take this into account?

Thank you


-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org> 
Sent: Wednesday, February 3, 2021 3:35 AM
To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
Cc: davem@davemloft.net; Rybak, Eryk Roch <eryk.roch.rybak@intel.com>; netdev@vger.kernel.org; sassmann@redhat.com; Topel, Bjorn <bjorn.topel@intel.com>; Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; Karlsson, Magnus <magnus.karlsson@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Bhandare, KiranX <kiranx.bhandare@intel.com>
Subject: Re: [PATCH net-next 6/6] i40e: Log error for oversized MTU on device

On Mon,  1 Feb 2021 18:24:20 -0800 Tony Nguyen wrote:
> From: Eryk Rybak <eryk.roch.rybak@intel.com>
> 
> When attempting to link XDP prog with MTU larger than supported, user 
> is not informed why XDP linking fails. Adding proper error message: 
> "MTU too large to enable XDP".
> Due to the lack of support for non-static variables in netlinks 
> extended ACK feature, additional information has been added to dmesg 
> to better inform about invalid MTU setting.
> 
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Eryk Rybak <eryk.roch.rybak@intel.com>
> Tested-by: Kiran Bhandare <kiranx.bhandare@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

> @@ -12459,8 +12460,13 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
>  	int i;
>  
>  	/* Don't allow frames that span over multiple buffers */
> -	if (frame_size > vsi->rx_buf_len)
> +	if (frame_size > vsi->rx_buf_len) {
> +		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
> +		dev_info(&pf->pdev->dev,
> +			 "MTU of %u bytes is too large to enable XDP (maximum: %u bytes)\n",
> +			 vsi->netdev->mtu, vsi->rx_buf_len);

Extack should be enough.
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
 

Re: [PATCH net-next 6/6] i40e: Log error for oversized MTU on device

[Jakub Kicinski]

On Wed, 3 Feb 2021 09:14:24 +0000 Loktionov, Aleksandr wrote:
> Good day Jakub

Please don't top post.

> We want to be user friendly to help users troubleshoot faster.
> Only dmesg message can have template parameters so we can provide
> exact acceptable maximum bytes. Can you could you take this into
> account?

I was making the same exact point when adding the message for NFP
years ago and it was shot down :)

Today upstream is getting close to removing the page-per-packet
requirement, so this will hopefully become irrelevant soon. Maciej
should have the details on that, he seems to be keeping up the most
with upstream in ITP.