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
---
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;
}
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(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 2040dad8611d..9eadda431965 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -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;
But also this way is fine by me.
Regards,
Tariq
