mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full

PATCH Applied Jesper Dangaard Brouer 5 participants 15 messages Sep 16, 2016 – Sep 20, 2016

Subsystems (4)

Networking Network drivers Mellanox MLX4 core VPI driver Mellanox Ethernet driver (mlx4_en)

lore

Share & tools

lore

Jesper Dangaard Brouer

author

2016-09-16 20:36 UTC #0 lore raw

PATCH 1/1 mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full

The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.

Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
Note, this fix have nothing to do with the page-refcnt bug I just reported.

 drivers/net/ethernet/mellanox/mlx4/en_rx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx4/en_rx.c +1 -1

index 2040dad8611d..adcd55c655db 100644
@@ -906,7 +906,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 							length, tx_index,
 							&doorbell_pending))
 					goto consumed;
-				break;
+				goto next; /* Drop on xmit failure */
 			default:
 				bpf_warn_invalid_xdp_action(act);
 			case XDP_ABORTED:

Jesper Dangaard Brouer

author

2016-09-16 20:39 UTC #1 lore raw

Quoted 3 lines · On Fri, 16 Sep 2016 22:36:12 +0200 Jesper Dangaard Brouer <[email protected]>…

The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.

Ups, forgot to add the V2 subject tag... Dave let me know if I should resend with V2 in subj.?

Signature · 5 lines

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

Brenden Blanco

Reviewed-by on v1 1/1

2016-09-16 20:43 UTC #2 lore raw

In reply to Jesper Dangaard Brouer

Quoted 6 lines · Jesper Dangaard Brouer

The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.
Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")
Signed-off-by Jesper Dangaard Brouer <[email protected]>

You could in theory have also tried to recycle the page instead of dropping it, but that's probably not worth optimizing when tx is backed up, as you'll only save a handful of page_put's. The code to do so wouldn't have been pretty.

Reviewed-by Brenden Blanco <[email protected]>

Jesper Dangaard Brouer

author

2016-09-17 14:46 UTC #3 lore raw

Quoted 5 lines · On Fri, 16 Sep 2016 13:43:50 -0700 Brenden Blanco <[email protected]> wrote:

Quoted 4 lines · Jesper Dangaard Brouer
The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.

Quoted 7 lines

Quoted 2 lines
Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")
Signed-off-by Jesper Dangaard Brouer <[email protected]>
You could in theory have also tried to recycle the page instead of dropping it, but that's probably not worth optimizing when tx is backed up, as you'll only save a handful of page_put's. The code to do so wouldn't have been pretty.

Yes, we could (and perhaps should) recycle the page instead. But as you also mention it would not look pretty. I'll send a V3, as XDPs primary concern is performance.

Quoted 1 line

Reviewed-by Brenden Blanco <[email protected]>

Signature · 5 lines

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

Jesper Dangaard Brouer

author

2016-09-17 15:48 UTC #4 lore raw

PATCH v3 1/1 mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full

Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
Is this goto lable inside a switch case too ugly?
Note, this fix have nothing to do with the page-refcnt bug I reported.

 drivers/net/ethernet/mellanox/mlx4/en_rx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx4/en_rx.c +2 -1

index 2040dad8611d..9eadda431965 100644
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 							length, tx_index,
 							&doorbell_pending))
 					goto consumed;
-				break;
+				goto xdp_drop; /* Drop on xmit failure */
 			default:
 				bpf_warn_invalid_xdp_action(act);
 			case XDP_ABORTED:
 			case XDP_DROP:
+			xdp_drop:
 				if (mlx4_en_rx_recycle(ring, frags))
 					goto consumed;
 				goto next;

Tariq Toukan

2016-09-18 08:30 UTC #5 lore raw

Quoted 9 lines · Jesper Dangaard Brouer

The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.
Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")
Signed-off-by Jesper Dangaard Brouer <[email protected]>
---
Is this goto lable inside a switch case too ugly?

I thought about getting "shared code" outside of the switch case, but I don't think it will look any better.

Given this label, we can change the goto position by re-ordering the cases, swapping between XDP_TX and default so that the XDP_TX is immediately followed by XDP_ABORTED and XDP_DROP, and won't need a goto operation. Instead, it will be used in default case, which should not be reached anyway. This saves a jump for actual (error) cases.

Something like this:

act = bpf_prog_run_xdp(xdp_prog, &xdp);
switch (act) {
case XDP_PASS:

        break;
default:
        bpf_warn_invalid_xdp_action(act);
        goto xdp_drop;
case XDP_TX:
        if (!mlx4_en_xmit_frame(frags, dev,
                                length, tx_index,
                                &doorbell_pending))
                goto consumed;
case XDP_ABORTED:
case XDP_DROP:
xdp_drop:
        if (mlx4_en_rx_recycle(ring, frags))
                goto consumed;
        goto next;
}

Quoted 14 lines

Note, this fix have nothing to do with the page-refcnt bug I reported.

  drivers/net/ethernet/mellanox/mlx4/en_rx.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx4/en_rx.c +0 -0

index 2040dad8611d..9eadda431965 100644
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
  							length, tx_index,
  							&doorbell_pending))
  					goto consumed;

drivers/net/ethernet/mellanox/mlx4/en_rx.c int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud · line 917

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx4/en_rx.c +2 -1

-				break;
+				goto xdp_drop; /* Drop on xmit failure */
  			default:
  				bpf_warn_invalid_xdp_action(act);
  			case XDP_ABORTED:
  			case XDP_DROP:
+			xdp_drop:
  				if (mlx4_en_rx_recycle(ring, frags))
  					goto consumed;
  				goto next;

But also this way is fine by me.

Regards,
Tariq

David Miller

maintainer

2016-09-19 05:39 UTC #6 lore raw

In reply to Jesper Dangaard Brouer

From: Jesper Dangaard Brouer <[email protected]>
Date: Sat, 17 Sep 2016 17:48:00 +0200

Quoted 6 lines

The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.
Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")
Signed-off-by Jesper Dangaard Brouer <[email protected]>

Oops, I applied v2 of this patch. Please send me any necessary relative fixups, thanks.

Jesper Dangaard Brouer

author

2016-09-19 07:40 UTC #7 lore raw

PATCH 1/1 mlx4: add missed recycle opportunity for XDP_TX on TX failure

Correct drop handling for XDP_TX on TX failure, were recently added in commit 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full").

The change missed an opportunity for recycling the RX page, instead of going through the page allocator, like the regular XDP_DROP action does. This patch cease the opportunity, by going through the XDP_DROP case.

Fixes 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full")

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx4/en_rx.c +2 -1

index c80073e4947f..919f0604d04a 100644
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 							length, tx_index,
 							&doorbell_pending))
 					goto consumed;
-				goto next; /* Drop on xmit failure */
+				goto xdp_drop; /* Drop on xmit failure */
 			default:
 				bpf_warn_invalid_xdp_action(act);
 			case XDP_ABORTED:
 			case XDP_DROP:
+			xdp_drop:
 				if (mlx4_en_rx_recycle(ring, frags))
 					goto consumed;
 				goto next;

Tariq Toukan

Reviewed-by on v1 1/1

2016-09-19 10:54 UTC #8 lore raw

Re: [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure

Quoted 23 lines · Jesper Dangaard Brouer

Correct drop handling for XDP_TX on TX failure, were recently added in commit 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full").
The change missed an opportunity for recycling the RX page, instead of going through the page allocator, like the regular XDP_DROP action does. This patch cease the opportunity, by going through the XDP_DROP case.
Fixes 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full")
Signed-off-by Jesper Dangaard Brouer <[email protected]>
---
  drivers/net/ethernet/mellanox/mlx4/en_rx.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx4/en_rx.c +0 -0
index c80073e4947f..919f0604d04a 100644
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
  							length, tx_index,
  							&doorbell_pending))
  					goto consumed;

drivers/net/ethernet/mellanox/mlx4/en_rx.c int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud · line 917

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx4/en_rx.c +2 -1

-				goto next; /* Drop on xmit failure */
+				goto xdp_drop; /* Drop on xmit failure */
  			default:
  				bpf_warn_invalid_xdp_action(act);
  			case XDP_ABORTED:
  			case XDP_DROP:
+			xdp_drop:
  				if (mlx4_en_rx_recycle(ring, frags))
  					goto consumed;
  				goto next;

Reviewed-by Tariq Toukan <[email protected]>

Thanks.

Eric Dumazet

2016-09-19 14:45 UTC #9 lore raw

Re: [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure

In reply to Jesper Dangaard Brouer

Quoted 19 lines · Jesper Dangaard Brouer

Correct drop handling for XDP_TX on TX failure, were recently added in commit 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full").
The change missed an opportunity for recycling the RX page, instead of going through the page allocator, like the regular XDP_DROP action does. This patch cease the opportunity, by going through the XDP_DROP case.
Fixes 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full")
Signed-off-by Jesper Dangaard Brouer <[email protected]>
---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx4/en_rx.c +0 -0
index c80073e4947f..919f0604d04a 100644

drivers/net/ethernet/mellanox/mlx4/en_rx.c int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud · line 914

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx4/en_rx.c +2 -1

@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 							length, tx_index,
 							&doorbell_pending))
 					goto consumed;
-				goto next; /* Drop on xmit failure */
+				goto xdp_drop; /* Drop on xmit failure */
 			default:
 				bpf_warn_invalid_xdp_action(act);
 			case XDP_ABORTED:
 			case XDP_DROP:
+			xdp_drop:

A label should start at the beginning of the line (ie : not indented)

Quoted 4 lines

drivers/net/ethernet/mellanox/mlx4/en_rx.c +0 -0

 				if (mlx4_en_rx_recycle(ring, frags))
 					goto consumed;
 				goto next;

Jesper Dangaard Brouer

author

2016-09-19 15:46 UTC #10 lore raw

PATCH v2 1/1 mlx4: add missed recycle opportunity for XDP_TX on TX failure

Correct drop handling for XDP_TX on TX failure, were recently added in commit 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full").

Fixes 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full")

Signed-off-by Jesper Dangaard Brouer <[email protected]>

Reviewed-by Tariq Toukan <[email protected]>

---
V2: adjust indention according to Eric

 drivers/net/ethernet/mellanox/mlx4/en_rx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/ethernet/mellanox/mlx4/en_rx.c +2 -1

index c80073e4947f..c46355bce613 100644
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 							length, tx_index,
 							&doorbell_pending))
 					goto consumed;
-				goto next; /* Drop on xmit failure */
+				goto xdp_drop; /* Drop on xmit failure */
 			default:
 				bpf_warn_invalid_xdp_action(act);
 			case XDP_ABORTED:
 			case XDP_DROP:
+xdp_drop:
 				if (mlx4_en_rx_recycle(ring, frags))
 					goto consumed;
 				goto next;

David Miller

maintainer Applied

2016-09-20 08:58 UTC #11 lore raw

Re: [net-next PATCH V2] mlx4: add missed recycle opportunity for XDP_TX on TX failure

From: Jesper Dangaard Brouer <[email protected]>
Date: Mon, 19 Sep 2016 17:46:38 +0200

Quoted 11 lines

Correct drop handling for XDP_TX on TX failure, were recently added in commit 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full").
The change missed an opportunity for recycling the RX page, instead of going through the page allocator, like the regular XDP_DROP action does. This patch cease the opportunity, by going through the XDP_DROP case.
Fixes 95357907ae73 ("mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full")
Signed-off-by Jesper Dangaard Brouer <[email protected]>
Reviewed-by Tariq Toukan <[email protected]>

Applied, thanks.

Jesper Dangaard Brouer

author

2016-09-19 15:57 UTC #12 lore raw

Re: [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure

In reply to Eric Dumazet

Quoted 3 lines · On Mon, 19 Sep 2016 07:28:35 -0700 Eric Dumazet <[email protected]> wrote:

drivers/net/ethernet/mellanox/mlx4/en_rx.c int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud · line 914
Quoted 1 line
drivers/net/ethernet/mellanox/mlx4/en_rx.c +1 -0
+			xdp_drop:  
A label should start at the beginning of the line (ie : not indented)

Understood, send a V2

Signature · 5 lines

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

Jesper Dangaard Brouer

author

2016-09-19 07:42 UTC #13 lore raw

In reply to David Miller

Quoted 7 lines · On Mon, 19 Sep 2016 01:39:25 -0400 (EDT) David Miller <[email protected]>…

From: Jesper Dangaard Brouer <[email protected]>
Date: Sat, 17 Sep 2016 17:48:00 +0200
Quoted 4 lines
The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.

Quoted 5 lines

Quoted 2 lines
Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")
Signed-off-by Jesper Dangaard Brouer <[email protected]>
Oops, I applied v2 of this patch. Please send me any necessary relative fixups, thanks.

Okay, I just send the needed fix up in/with:

Subj: [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure

Signature · 5 lines

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

David Miller

maintainer Applied

2016-09-19 05:33 UTC #14 lore raw

In reply to Jesper Dangaard Brouer

From: Jesper Dangaard Brouer <[email protected]>
Date: Fri, 16 Sep 2016 22:36:12 +0200

Quoted 6 lines

The XDP_TX action can fail transmitting the frame in case the TX ring is full or port is down. In case of TX failure it should drop the frame, and not as now call 'break' which is the same as XDP_PASS.
Fixes 9ecc2d86171a ("net/mlx4_en: add xdp forwarding and data write support")
Signed-off-by Jesper Dangaard Brouer <[email protected]>

This patch is for net-next, please be explicit about that in the future.

Applied, thanks.