All of lore.kernel.org
 help / color / mirror / Atom feed
* [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
@ 2016-09-16 20:36 Jesper Dangaard Brouer
  2016-09-16 20:37 ` Jesper Dangaard Brouer
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-16 20:36 UTC (permalink / raw)
  To: netdev, tariqt
  Cc: Eric Dumazet, tom, bblanco, Jesper Dangaard Brouer, rana.shahot,
	David S. Miller

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 <brouer@redhat.com>

---
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:

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-16 20:36 [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full Jesper Dangaard Brouer
@ 2016-09-16 20:37 ` Jesper Dangaard Brouer
  2016-09-16 20:43 ` Brenden Blanco
  2016-09-19  5:32 ` [net PATCH] " David Miller
  2 siblings, 0 replies; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-16 20:37 UTC (permalink / raw)
  To: netdev, tariqt
  Cc: Eric Dumazet, tom, bblanco, rana.shahot, David S. Miller, brouer

On Fri, 16 Sep 2016 22:36:12 +0200
Jesper Dangaard Brouer <brouer@redhat.com> 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-16 20:36 [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full Jesper Dangaard Brouer
  2016-09-16 20:37 ` Jesper Dangaard Brouer
@ 2016-09-16 20:43 ` Brenden Blanco
  2016-09-17 14:46   ` Jesper Dangaard Brouer
  2016-09-19  5:32 ` [net PATCH] " David Miller
  2 siblings, 1 reply; 18+ messages in thread
From: Brenden Blanco @ 2016-09-16 20:43 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: netdev, tariqt, Eric Dumazet, tom, rana.shahot, David S. Miller

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 <brouer@redhat.com>

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 <bblanco@plumgrid.com>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-16 20:43 ` Brenden Blanco
@ 2016-09-17 14:46   ` Jesper Dangaard Brouer
  2016-09-17 15:48     ` [net PATCH V3] " Jesper Dangaard Brouer
  0 siblings, 1 reply; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-17 14:46 UTC (permalink / raw)
  To: Brenden Blanco
  Cc: brouer, netdev, tariqt, Eric Dumazet, tom, rana.shahot, David S. Miller

On Fri, 16 Sep 2016 13:43:50 -0700
Brenden Blanco <bblanco@plumgrid.com> 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 <brouer@redhat.com>  
> 
> 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 <bblanco@plumgrid.com>



-- 
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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [net PATCH V3] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-17 14:46   ` Jesper Dangaard Brouer
@ 2016-09-17 15:48     ` Jesper Dangaard Brouer
  2016-09-18  8:30       ` Tariq Toukan
  2016-09-19  5:39       ` David Miller
  0 siblings, 2 replies; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-17 15:48 UTC (permalink / raw)
  To: netdev, tariqt
  Cc: tom, bblanco, rana.shahot, David S. Miller, 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 <brouer@redhat.com>

---
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(-)

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;

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [net PATCH V3] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-17 15:48     ` [net PATCH V3] " Jesper Dangaard Brouer
@ 2016-09-18  8:30       ` Tariq Toukan
  2016-09-19  5:39       ` David Miller
  1 sibling, 0 replies; 18+ messages in thread
From: Tariq Toukan @ 2016-09-18  8:30 UTC (permalink / raw)
  To: Jesper Dangaard Brouer, netdev, tariqt
  Cc: tom, bblanco, rana.shahot, David S. Miller


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 <brouer@redhat.com>
>
> ---
> 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-16 20:36 [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full Jesper Dangaard Brouer
  2016-09-16 20:37 ` Jesper Dangaard Brouer
  2016-09-16 20:43 ` Brenden Blanco
@ 2016-09-19  5:32 ` David Miller
  2 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2016-09-19  5:32 UTC (permalink / raw)
  To: brouer; +Cc: netdev, tariqt, eric.dumazet, tom, bblanco, rana.shahot

From: Jesper Dangaard Brouer <brouer@redhat.com>
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 <brouer@redhat.com>

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

Applied, thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net PATCH V3] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-17 15:48     ` [net PATCH V3] " Jesper Dangaard Brouer
  2016-09-18  8:30       ` Tariq Toukan
@ 2016-09-19  5:39       ` David Miller
  2016-09-19  7:40         ` [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure Jesper Dangaard Brouer
  2016-09-19  7:42         ` [net PATCH V3] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full Jesper Dangaard Brouer
  1 sibling, 2 replies; 18+ messages in thread
From: David Miller @ 2016-09-19  5:39 UTC (permalink / raw)
  To: brouer; +Cc: netdev, tariqt, tom, bblanco, rana.shahot

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Sat, 17 Sep 2016 17:48:00 +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 <brouer@redhat.com>

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure
  2016-09-19  5:39       ` David Miller
@ 2016-09-19  7:40         ` Jesper Dangaard Brouer
  2016-09-19 10:54           ` Tariq Toukan
  2016-09-19 14:28           ` Eric Dumazet
  2016-09-19  7:42         ` [net PATCH V3] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full Jesper Dangaard Brouer
  1 sibling, 2 replies; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-19  7:40 UTC (permalink / raw)
  To: netdev, tariqt
  Cc: tom, bblanco, rana.shahot, David S. Miller, 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 <brouer@redhat.com>
---
 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 c80073e4947f..919f0604d04a 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;
-				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;

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [net PATCH V3] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-19  5:39       ` David Miller
  2016-09-19  7:40         ` [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure Jesper Dangaard Brouer
@ 2016-09-19  7:42         ` Jesper Dangaard Brouer
  1 sibling, 0 replies; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-19  7:42 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, tariqt, tom, bblanco, rana.shahot, brouer

On Mon, 19 Sep 2016 01:39:25 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Jesper Dangaard Brouer <brouer@redhat.com>
> Date: Sat, 17 Sep 2016 17:48:00 +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 <brouer@redhat.com>  
> 
> 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure
  2016-09-19  7:40         ` [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure Jesper Dangaard Brouer
@ 2016-09-19 10:54           ` Tariq Toukan
  2016-09-19 14:28           ` Eric Dumazet
  1 sibling, 0 replies; 18+ messages in thread
From: Tariq Toukan @ 2016-09-19 10:54 UTC (permalink / raw)
  To: Jesper Dangaard Brouer, netdev, tariqt
  Cc: tom, bblanco, rana.shahot, David S. Miller


On 19/09/2016 10:40 AM, Jesper Dangaard Brouer wrote:
> 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 <brouer@redhat.com>
> ---
>   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 c80073e4947f..919f0604d04a 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;
> -				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 <tariqt@mellanox.com>
Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure
  2016-09-19  7:40         ` [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure Jesper Dangaard Brouer
  2016-09-19 10:54           ` Tariq Toukan
@ 2016-09-19 14:28           ` Eric Dumazet
  2016-09-19 15:46             ` [net-next PATCH V2] " Jesper Dangaard Brouer
  2016-09-19 15:57             ` [net-next PATCH] " Jesper Dangaard Brouer
  1 sibling, 2 replies; 18+ messages in thread
From: Eric Dumazet @ 2016-09-19 14:28 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: netdev, tariqt, tom, bblanco, rana.shahot, David S. Miller

On Mon, 2016-09-19 at 09:40 +0200, Jesper Dangaard Brouer wrote:
> 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 <brouer@redhat.com>
> ---
>  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 c80073e4947f..919f0604d04a 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;
> -				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)

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [net-next PATCH V2] mlx4: add missed recycle opportunity for XDP_TX on TX failure
  2016-09-19 14:28           ` Eric Dumazet
@ 2016-09-19 15:46             ` Jesper Dangaard Brouer
  2016-09-20  8:58               ` David Miller
  2016-09-19 15:57             ` [net-next PATCH] " Jesper Dangaard Brouer
  1 sibling, 1 reply; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-19 15:46 UTC (permalink / raw)
  To: netdev, tariqt
  Cc: Eric Dumazet, tom, bblanco, Jesper Dangaard Brouer, rana.shahot,
	David S. Miller

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 <brouer@redhat.com>
Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
---
V2: adjust indention according to Eric

 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 c80073e4947f..c46355bce613 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;
-				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;

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure
  2016-09-19 14:28           ` Eric Dumazet
  2016-09-19 15:46             ` [net-next PATCH V2] " Jesper Dangaard Brouer
@ 2016-09-19 15:57             ` Jesper Dangaard Brouer
  1 sibling, 0 replies; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-19 15:57 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, tariqt, tom, bblanco, rana.shahot,
	David S. Miller" <davem@davemloft.net>,
	brouer@redhat.com

On Mon, 19 Sep 2016 07:28:35 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

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

Understood, 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net-next PATCH V2] mlx4: add missed recycle opportunity for XDP_TX on TX failure
  2016-09-19 15:46             ` [net-next PATCH V2] " Jesper Dangaard Brouer
@ 2016-09-20  8:58               ` David Miller
  0 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2016-09-20  8:58 UTC (permalink / raw)
  To: brouer; +Cc: netdev, tariqt, eric.dumazet, tom, bblanco, rana.shahot

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Mon, 19 Sep 2016 17:46:38 +0200

> 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 <brouer@redhat.com>
> Reviewed-by: Tariq Toukan <tariqt@mellanox.com>

Applied, thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-16 20:00 ` Eric Dumazet
@ 2016-09-16 20:29   ` Jesper Dangaard Brouer
  0 siblings, 0 replies; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-16 20:29 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, tariqt, tom, bblanco, rana.shahot,
	David S. Miller" <davem@davemloft.net>,
	brouer@redhat.com

On Fri, 16 Sep 2016 13:00:50 -0700
Eric Dumazet <eric.dumazet@gmail.com> 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
  2016-09-16 19:47 Jesper Dangaard Brouer
@ 2016-09-16 20:00 ` Eric Dumazet
  2016-09-16 20:29   ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2016-09-16 20:00 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: netdev, tariqt, tom, bblanco, rana.shahot, David S. Miller

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 <brouer@redhat.com>
> 
> ---
> 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);
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full
@ 2016-09-16 19:47 Jesper Dangaard Brouer
  2016-09-16 20:00 ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Jesper Dangaard Brouer @ 2016-09-16 19:47 UTC (permalink / raw)
  To: netdev, tariqt
  Cc: tom, bblanco, rana.shahot, David S. Miller, 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 <brouer@redhat.com>

---
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);

^ permalink raw reply related	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2016-09-20  8:58 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-16 20:36 [net PATCH] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full Jesper Dangaard Brouer
2016-09-16 20:37 ` Jesper Dangaard Brouer
2016-09-16 20:43 ` Brenden Blanco
2016-09-17 14:46   ` Jesper Dangaard Brouer
2016-09-17 15:48     ` [net PATCH V3] " Jesper Dangaard Brouer
2016-09-18  8:30       ` Tariq Toukan
2016-09-19  5:39       ` David Miller
2016-09-19  7:40         ` [net-next PATCH] mlx4: add missed recycle opportunity for XDP_TX on TX failure Jesper Dangaard Brouer
2016-09-19 10:54           ` Tariq Toukan
2016-09-19 14:28           ` Eric Dumazet
2016-09-19 15:46             ` [net-next PATCH V2] " Jesper Dangaard Brouer
2016-09-20  8:58               ` David Miller
2016-09-19 15:57             ` [net-next PATCH] " Jesper Dangaard Brouer
2016-09-19  7:42         ` [net PATCH V3] mlx4: fix XDP_TX is acting like XDP_PASS on TX ring full Jesper Dangaard Brouer
2016-09-19  5:32 ` [net PATCH] " David Miller
  -- strict thread matches above, loose matches on Subject: below --
2016-09-16 19:47 Jesper Dangaard Brouer
2016-09-16 20:00 ` Eric Dumazet
2016-09-16 20:29   ` Jesper Dangaard Brouer

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.