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 <redacted>
---
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(-)
@@ -906,7 +906,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budlength,tx_index,&doorbell_pending))gotoconsumed;-break;+gotonext;/* Drop on xmit failure */default:bpf_warn_invalid_xdp_action(act);caseXDP_ABORTED:
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.?
--
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
On Fri, Sep 16, 2016 at 10:36:12PM +0200, Jesper Dangaard Brouer wrote:
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 <redacted>
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 <redacted>
On Fri, Sep 16, 2016 at 10:36:12PM +0200, Jesper Dangaard Brouer wrote:
quoted
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 <redacted>
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.
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 <redacted>
---
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(-)
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budlength,tx_index,&doorbell_pending))gotoconsumed;-break;+gotoxdp_drop;/* Drop on xmit failure */default:bpf_warn_invalid_xdp_action(act);caseXDP_ABORTED:caseXDP_DROP:+xdp_drop:if(mlx4_en_rx_recycle(ring,frags))gotoconsumed;gotonext;
On 17/09/2016 6:48 PM, Jesper Dangaard Brouer wrote:
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 <redacted>
---
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 hunk
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(-)
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budlength,tx_index,&doorbell_pending))gotoconsumed;-break;+gotoxdp_drop;/* Drop on xmit failure */default:bpf_warn_invalid_xdp_action(act);caseXDP_ABORTED:caseXDP_DROP:+xdp_drop:if(mlx4_en_rx_recycle(ring,frags))gotoconsumed;gotonext;
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 <redacted>
This patch is for net-next, please be explicit about that in the future.
Applied, thanks.
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 <redacted>
Oops, I applied v2 of this patch. Please send me any necessary
relative fixups, thanks.
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 <redacted>
---
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budlength,tx_index,&doorbell_pending))gotoconsumed;-gotonext;/* Drop on xmit failure */+gotoxdp_drop;/* Drop on xmit failure */default:bpf_warn_invalid_xdp_action(act);caseXDP_ABORTED:caseXDP_DROP:+xdp_drop:if(mlx4_en_rx_recycle(ring,frags))gotoconsumed;gotonext;
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 <redacted>
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
--
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
On 19/09/2016 10:40 AM, Jesper Dangaard Brouer wrote:
quoted hunk
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 <redacted>
---
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budlength,tx_index,&doorbell_pending))gotoconsumed;-gotonext;/* Drop on xmit failure */+gotoxdp_drop;/* Drop on xmit failure */default:bpf_warn_invalid_xdp_action(act);caseXDP_ABORTED:caseXDP_DROP:+xdp_drop:if(mlx4_en_rx_recycle(ring,frags))gotoconsumed;gotonext;
From: Eric Dumazet <hidden> Date: 2016-09-19 14:45:42
On Mon, 2016-09-19 at 09:40 +0200, Jesper Dangaard Brouer wrote:
quoted hunk
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 <redacted>
---
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budlength,tx_index,&doorbell_pending))gotoconsumed;-gotonext;/* Drop on xmit failure */+gotoxdp_drop;/* Drop on xmit failure */default:bpf_warn_invalid_xdp_action(act);caseXDP_ABORTED:caseXDP_DROP:+xdp_drop:
A label should start at the beginning of the line (ie : not indented)
if (mlx4_en_rx_recycle(ring, frags))
goto consumed;
goto next;
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 <redacted>
Reviewed-by: Tariq Toukan <redacted>
---
V2: adjust indention according to Eric
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -906,11 +906,12 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budlength,tx_index,&doorbell_pending))gotoconsumed;-gotonext;/* Drop on xmit failure */+gotoxdp_drop;/* Drop on xmit failure */default:bpf_warn_invalid_xdp_action(act);caseXDP_ABORTED:caseXDP_DROP:+xdp_drop:if(mlx4_en_rx_recycle(ring,frags))gotoconsumed;gotonext;
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 <redacted>
Reviewed-by: Tariq Toukan <redacted>