Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
From: Jesper Dangaard Brouer Date: Fri, 16 Sep 2016 22:36:12 +0200 > 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 This patch is for net-next, please be explicit about that in the future. Applied, thanks.
Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
On Fri, 16 Sep 2016 13:43:50 -0700 Brenden Blanco wrote: > 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 > > 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. > Reviewed-by: Brenden Blanco -- 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
Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
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 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
Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
On Fri, 16 Sep 2016 22:36:12 +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. 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
[net PATCH] 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 --- 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(-) diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c index 2040dad8611d..adcd55c655db 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c @@ -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:
Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
On Fri, 16 Sep 2016 13:00:50 -0700 Eric Dumazet wrote: > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > > b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > > index 2040dad8611d..d414c67dfd12 100644 > > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > > @@ -906,6 +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; > > + goto next; > > break; > > Why keeping this break; then ? ;) I'll send a V2 -- 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
Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
On Fri, 2016-09-16 at 21:47 +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 > > --- > Note, this fix have nothing to do with the page-refcnt bug I just reported. Yeah, the e1000 driver proposal patch had the same issue. > > drivers/net/ethernet/mellanox/mlx4/en_rx.c |1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > index 2040dad8611d..d414c67dfd12 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > @@ -906,6 +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; > + goto next; > break; Why keeping this break; then ? ;) > default: > bpf_warn_invalid_xdp_action(act); >
[net PATCH] 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 --- Note, this fix have nothing to do with the page-refcnt bug I just reported. drivers/net/ethernet/mellanox/mlx4/en_rx.c |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c index 2040dad8611d..d414c67dfd12 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c @@ -906,6 +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; + goto next; break; default: bpf_warn_invalid_xdp_action(act);