[PATCH intel-net] i40e: fix broken XDP support

[PATCH intel-net] i40e: fix broken XDP support

[Magnus Karlsson]

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

Commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c") broke
XDP support in the i40e driver. That commit was fixing a sparse error
in the code by introducing a new variable xdp_res instead of
overloading this into the skb pointer. The problem is that the code
later uses the skb pointer in if statements and these where not
extended to also test for the new xdp_res variable. Fix this by adding
the correct tests for xdp_res in these places.

Fixes: 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")
Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 06b4271219b1..46355c6bdc8f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -1961,10 +1961,6 @@ static bool i40e_cleanup_headers(struct i40e_ring *rx_ring, struct sk_buff *skb,
 				 union i40e_rx_desc *rx_desc)
 
 {
-	/* XDP packets use error pointer so abort at this point */
-	if (IS_ERR(skb))
-		return true;
-
 	/* ERR_MASK will only have valid bits if EOP set, and
 	 * what we are doing here is actually checking
 	 * I40E_RX_DESC_ERROR_RXE_SHIFT, since it is the zeroth bit in
@@ -2447,7 +2443,6 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
 	unsigned int xdp_xmit = 0;
 	bool failure = false;
 	struct xdp_buff xdp;
-	int xdp_res = 0;
 
 #if (PAGE_SIZE < 8192)
 	frame_sz = i40e_rx_frame_truesize(rx_ring, 0);
@@ -2459,6 +2454,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
 		union i40e_rx_desc *rx_desc;
 		int rx_buffer_pgcnt;
 		unsigned int size;
+		int xdp_res = 0;
 		u64 qword;
 
 		/* return some buffers to hardware, one at a time is too slow */
@@ -2534,7 +2530,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
 		}
 
 		/* exit if we failed to retrieve a buffer */
-		if (!skb) {
+		if (!xdp_res && !skb) {
 			rx_ring->rx_stats.alloc_buff_failed++;
 			rx_buffer->pagecnt_bias++;
 			break;
@@ -2547,7 +2543,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
 		if (i40e_is_non_eop(rx_ring, rx_desc))
 			continue;
 
-		if (i40e_cleanup_headers(rx_ring, skb, rx_desc)) {
+		if (xdp_res || i40e_cleanup_headers(rx_ring, skb, rx_desc)) {
 			skb = NULL;
 			continue;
 		}

base-commit: bb556de79f0a9e647e8febe15786ee68483fa67b
-- 
2.29.0

Re: [PATCH intel-net] i40e: fix broken XDP support

[Jesper Dangaard Brouer]

On Fri, 23 Apr 2021 11:59:55 +0200
Magnus Karlsson <magnus.karlsson@gmail.com> wrote:

> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c") broke
> XDP support in the i40e driver. That commit was fixing a sparse error
> in the code by introducing a new variable xdp_res instead of
> overloading this into the skb pointer. The problem is that the code
> later uses the skb pointer in if statements and these where not
> extended to also test for the new xdp_res variable. Fix this by adding
> the correct tests for xdp_res in these places.
> 
> Fixes: 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")
> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>

I also tested that i40e works on my system again.
 Intel(R) Xeon(R) CPU E5-1650 v4 @ 3.60GHz

Running XDP on dev:i40e2 (ifindex:8) action:XDP_DROP options:no_touch
XDP stats       CPU     pps         issue-pps  
XDP-RX CPU      4       33,670,895  0          
XDP-RX CPU      total   33,670,895 

RXQ stats       RXQ:CPU pps         issue-pps  
rx_queue_index    4:4   33,670,900  0          
rx_queue_index    4:sum 33,670,900 


Running XDP on dev:i40e2 (ifindex:8) action:XDP_DROP options:read
XDP stats       CPU     pps         issue-pps  
XDP-RX CPU      4       31,424,994  0          
XDP-RX CPU      total   31,424,994 

RXQ stats       RXQ:CPU pps         issue-pps  
rx_queue_index    4:4   31,424,997  0          
rx_queue_index    4:sum 31,424,997 


Running XDP on dev:i40e2 (ifindex:8) action:XDP_TX options:swapmac
XDP stats       CPU     pps         issue-pps  
XDP-RX CPU      4       14,777,900  0          
XDP-RX CPU      total   14,777,900 

RXQ stats       RXQ:CPU pps         issue-pps  
rx_queue_index    4:4   14,777,893  0          
rx_queue_index    4:sum 14,777,893 


$ sudo ./xdp_redirect_map i40e2 i40e2
input: 8 output: 8
libbpf: elf: skipping unrecognized data section(24) .eh_frame
libbpf: elf: skipping relo section(25) .rel.eh_frame for section(24) .eh_frame
libbpf: Kernel error message: XDP program already attached
WARN: link set xdp fd failed on 8
ifindex 8:    8212980 pkt/s
ifindex 8:   11725145 pkt/s
ifindex 8:   11727939 pkt/s
ifindex 8:   11727640 pkt/s
ifindex 8:   11729593 pkt/s

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH intel-net] i40e: fix broken XDP support

[Maciej Fijalkowski]

On Fri, Apr 23, 2021 at 11:59:55AM +0200, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c") broke
> XDP support in the i40e driver. That commit was fixing a sparse error
> in the code by introducing a new variable xdp_res instead of
> overloading this into the skb pointer. The problem is that the code

'this' means the result of xdp program, right?

> later uses the skb pointer in if statements and these where not
> extended to also test for the new xdp_res variable. Fix this by adding
> the correct tests for xdp_res in these places.

Let's be more specific what was happening. Would be good to mention what
these if statements were actually about.

i40e_cleanup_headers() had a check that based on the skb value that was
adequate to what we stored there (ERR_PTR(-result)) on exit from
i40e_run_xdp() made a whole napi processing loop not to advance with the
logic which would in turn pass the skb to the stack, but rather start to
process the next descriptor. IOW that point was the end of the XDP data
path for result != XDP_PASS.

Given that we mask the XDP_PASS internally to I40E_XDP_PASS which is 0, we
simply introduce the test against xdp_res and drop the IS_ERR(skb) from
i40e_cleanup_headers() since it's not legit anymore.

Without your change, we probably were terminating the whole loop over
here:
		/* exit if we failed to retrieve a buffer */
		if (!skb) {
			rx_ring->rx_stats.alloc_buff_failed++;
			rx_buffer->pagecnt_bias++;
			break;
		}

For XDP actions as the skb wasn't set anymore. So check you're adding
would make us skip the above for correct cases but then we need also the
next changes around i40e_cleanup_headers() as otherwise we would be
passing the NULL skb to the stack AFAICT :/

Would be also good to hear about the rationale behind initialization of
xdp_res per each loop iteration.

Can you send a v2 with improved commit message? So that in future we would
be aware what was fixed. Probably not the best write up from my side, but
I wanted to make it more clear.

> 
> Fixes: 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")
> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 06b4271219b1..46355c6bdc8f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -1961,10 +1961,6 @@ static bool i40e_cleanup_headers(struct i40e_ring *rx_ring, struct sk_buff *skb,
>  				 union i40e_rx_desc *rx_desc)
>  
>  {
> -	/* XDP packets use error pointer so abort at this point */
> -	if (IS_ERR(skb))
> -		return true;
> -
>  	/* ERR_MASK will only have valid bits if EOP set, and
>  	 * what we are doing here is actually checking
>  	 * I40E_RX_DESC_ERROR_RXE_SHIFT, since it is the zeroth bit in
> @@ -2447,7 +2443,6 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
>  	unsigned int xdp_xmit = 0;
>  	bool failure = false;
>  	struct xdp_buff xdp;
> -	int xdp_res = 0;
>  
>  #if (PAGE_SIZE < 8192)
>  	frame_sz = i40e_rx_frame_truesize(rx_ring, 0);
> @@ -2459,6 +2454,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
>  		union i40e_rx_desc *rx_desc;
>  		int rx_buffer_pgcnt;
>  		unsigned int size;
> +		int xdp_res = 0;
>  		u64 qword;
>  
>  		/* return some buffers to hardware, one at a time is too slow */
> @@ -2534,7 +2530,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
>  		}
>  
>  		/* exit if we failed to retrieve a buffer */
> -		if (!skb) {
> +		if (!xdp_res && !skb) {
>  			rx_ring->rx_stats.alloc_buff_failed++;
>  			rx_buffer->pagecnt_bias++;
>  			break;
> @@ -2547,7 +2543,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
>  		if (i40e_is_non_eop(rx_ring, rx_desc))
>  			continue;
>  
> -		if (i40e_cleanup_headers(rx_ring, skb, rx_desc)) {
> +		if (xdp_res || i40e_cleanup_headers(rx_ring, skb, rx_desc)) {
>  			skb = NULL;
>  			continue;
>  		}
> 
> base-commit: bb556de79f0a9e647e8febe15786ee68483fa67b
> -- 
> 2.29.0
> 

[PATCH intel-net] i40e: fix broken XDP support

[Magnus Karlsson]

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

Commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c") broke
XDP support in the i40e driver. That commit was fixing a sparse error
in the code by introducing a new variable xdp_res instead of
overloading this into the skb pointer. The problem is that the code
later uses the skb pointer in if statements and these where not
extended to also test for the new xdp_res variable. Fix this by adding
the correct tests for xdp_res in these places.

The skb pointer was used to store the result of the XDP program by
overloading the results in the errror pointer
ERR_PTR(-result). Therefore, the allocation failure test that used to
only test for !skb now need to be extended to also consider !xdp_res.

i40e_cleanup_headers() had a check that based on the skb value being
an error pointer, i.e. a result from the XDP program != XDP_PASS, and
if so start to process a new packet immediately, instead of populating
skb fields and sending the skb to the stack. This check is not needed
anymore, since we have added an explicit test for xdp_res being set
and if so just do continue to pick the next packet from the NIC.

v1 -> v2:

* Improved commit message.

* Restored the xdp_res = 0 initialization to its original place
  outside the per-packet loop. The original reason to move it inside
  the loop was that it was only initialized inside the loop code if
  skb was not set. But as skb can only be non-null if we have packets
  consisting of multiple frames (skb is set for all frames except the
  last one in a packet) and when this is true XDP cannot be active, so
  this does not matter. xdp_res == 0 is the same as I40E_XDP_PASS
  which is the default action if XDP is not active and it is then true
  for every single packet in this case.

Fixes: 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 06b4271219b1..70b515049540 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -1961,10 +1961,6 @@ static bool i40e_cleanup_headers(struct i40e_ring *rx_ring, struct sk_buff *skb,
 				 union i40e_rx_desc *rx_desc)
 
 {
-	/* XDP packets use error pointer so abort at this point */
-	if (IS_ERR(skb))
-		return true;
-
 	/* ERR_MASK will only have valid bits if EOP set, and
 	 * what we are doing here is actually checking
 	 * I40E_RX_DESC_ERROR_RXE_SHIFT, since it is the zeroth bit in
@@ -2534,7 +2530,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
 		}
 
 		/* exit if we failed to retrieve a buffer */
-		if (!skb) {
+		if (!xdp_res && !skb) {
 			rx_ring->rx_stats.alloc_buff_failed++;
 			rx_buffer->pagecnt_bias++;
 			break;
@@ -2547,7 +2543,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
 		if (i40e_is_non_eop(rx_ring, rx_desc))
 			continue;
 
-		if (i40e_cleanup_headers(rx_ring, skb, rx_desc)) {
+		if (xdp_res || i40e_cleanup_headers(rx_ring, skb, rx_desc)) {
 			skb = NULL;
 			continue;
 		}

base-commit: bbd6f0a948139970f4a615dff189d9a503681a39
-- 
2.29.0

Re: [PATCH intel-net] i40e: fix broken XDP support

[Jesper Dangaard Brouer]

Hi Tony, (+ Kuba and DaveM),

What is the status on this patch[2] that fixes a crash[1] for i40e driver?

I'm getting offlist and internal IRC questions to why i40e doesn't
work, and I noticed that it seems this have not been applied.

I don't see it in net-next or net tree... would it make sense to route
this via DaveM, or does it depend on the other fixes for i40e.

[1] https://lore.kernel.org/netdev/20210422170508.22c58226@carbon/
[2] https://patchwork.kernel.org/project/netdevbpf/patch/20210426111401.28369-1-magnus.karlsson@gmail.com/

(top-post)

On Mon, 26 Apr 2021 13:14:01 +0200
Magnus Karlsson <magnus.karlsson@gmail.com> wrote:

> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c") broke
> XDP support in the i40e driver. That commit was fixing a sparse error
> in the code by introducing a new variable xdp_res instead of
> overloading this into the skb pointer. The problem is that the code
> later uses the skb pointer in if statements and these where not
> extended to also test for the new xdp_res variable. Fix this by adding
> the correct tests for xdp_res in these places.
> 
> The skb pointer was used to store the result of the XDP program by
> overloading the results in the errror pointer
> ERR_PTR(-result). Therefore, the allocation failure test that used to
> only test for !skb now need to be extended to also consider !xdp_res.
> 
> i40e_cleanup_headers() had a check that based on the skb value being
> an error pointer, i.e. a result from the XDP program != XDP_PASS, and
> if so start to process a new packet immediately, instead of populating
> skb fields and sending the skb to the stack. This check is not needed
> anymore, since we have added an explicit test for xdp_res being set
> and if so just do continue to pick the next packet from the NIC.
> 
> v1 -> v2:
> 
> * Improved commit message.
> 
> * Restored the xdp_res = 0 initialization to its original place
>   outside the per-packet loop. The original reason to move it inside
>   the loop was that it was only initialized inside the loop code if
>   skb was not set. But as skb can only be non-null if we have packets
>   consisting of multiple frames (skb is set for all frames except the
>   last one in a packet) and when this is true XDP cannot be active, so
>   this does not matter. xdp_res == 0 is the same as I40E_XDP_PASS
>   which is the default action if XDP is not active and it is then true
>   for every single packet in this case.
> 
> Fixes: 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH intel-net] i40e: fix broken XDP support

[Nguyen, Anthony L]

On Thu, 2021-04-29 at 11:10 +0200, Jesper Dangaard Brouer wrote:
> Hi Tony, (+ Kuba and DaveM),
> 
> What is the status on this patch[2] that fixes a crash[1] for i40e
> driver?

They are currently applied to the Intel-wired-lan tree[1] awaiting
validation.

> I'm getting offlist and internal IRC questions to why i40e doesn't
> work, and I noticed that it seems this have not been applied.
> 
> I don't see it in net-next or net tree... would it make sense to
> route
> this via DaveM, or does it depend on the other fixes for i40e.

There are no other dependent changes I'm aware of. As this resolves the
issue for you, I'll go ahead and send this patch to DaveM.

Thanks,
Tony


[1] https://patchwork.ozlabs.org/project/intel-wired-
lan/patch/20210426111401.28369-1-magnus.karlsson@gmail.com/

> [1] https://lore.kernel.org/netdev/20210422170508.22c58226@carbon/
> [2] 
> https://patchwork.kernel.org/project/netdevbpf/patch/20210426111401.28369-1-magnus.karlsson@gmail.com/
> 
> (top-post)
> 
> On Mon, 26 Apr 2021 13:14:01 +0200
> Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
> 
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> > 
> > Commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")
> > broke
> > XDP support in the i40e driver. That commit was fixing a sparse
> > error
> > in the code by introducing a new variable xdp_res instead of
> > overloading this into the skb pointer. The problem is that the code
> > later uses the skb pointer in if statements and these where not
> > extended to also test for the new xdp_res variable. Fix this by
> > adding
> > the correct tests for xdp_res in these places.
> > 
> > The skb pointer was used to store the result of the XDP program by
> > overloading the results in the errror pointer
> > ERR_PTR(-result). Therefore, the allocation failure test that used
> > to
> > only test for !skb now need to be extended to also consider
> > !xdp_res.
> > 
> > i40e_cleanup_headers() had a check that based on the skb value
> > being
> > an error pointer, i.e. a result from the XDP program != XDP_PASS,
> > and
> > if so start to process a new packet immediately, instead of
> > populating
> > skb fields and sending the skb to the stack. This check is not
> > needed
> > anymore, since we have added an explicit test for xdp_res being set
> > and if so just do continue to pick the next packet from the NIC.
> > 
> > v1 -> v2:
> > 
> > * Improved commit message.
> > 
> > * Restored the xdp_res = 0 initialization to its original place
> >   outside the per-packet loop. The original reason to move it
> > inside
> >   the loop was that it was only initialized inside the loop code if
> >   skb was not set. But as skb can only be non-null if we have
> > packets
> >   consisting of multiple frames (skb is set for all frames except
> > the
> >   last one in a packet) and when this is true XDP cannot be active,
> > so
> >   this does not matter. xdp_res == 0 is the same as I40E_XDP_PASS
> >   which is the default action if XDP is not active and it is then
> > true
> >   for every single packet in this case.
> > 
> > Fixes: 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")
> > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> 
>