[PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[Eric Dumazet]

A driver start_xmit() method cannot free skb and return NETDEV_TX_BUSY,
since caller is going to reuse freed skb.

This is mostly a revert of commit bf769375c (staging: hv: fix the return
status of netvsc_start_xmit())

In fact netif_tx_stop_queue() / netif_stop_queue() is needed before
returning NETDEV_TX_BUSY or you can trigger a ksoftirqd fatal loop.

In case of memory allocation error, only safe way is to drop the packet
and return NETDEV_TX_OK

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/hyperv/netvsc_drv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 0f8e834..2517d20 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -167,7 +167,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 
 		dev_kfree_skb(skb);
 		net->stats.tx_dropped++;
-		return NETDEV_TX_BUSY;
+		return NETDEV_TX_OK;
 	}
 
 	packet->vlan_tci = skb->vlan_tci;
@@ -229,7 +229,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 		dev_kfree_skb_any(skb);
 	}
 
-	return ret ? NETDEV_TX_BUSY : NETDEV_TX_OK;
+	return NETDEV_TX_OK;
 }
 
 /*

Re: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[Ben Greear]

On 03/14/2012 11:53 AM, Eric Dumazet wrote:
> A driver start_xmit() method cannot free skb and return NETDEV_TX_BUSY,
> since caller is going to reuse freed skb.
>
> This is mostly a revert of commit bf769375c (staging: hv: fix the return
> status of netvsc_start_xmit())
>
> In fact netif_tx_stop_queue() / netif_stop_queue() is needed before
> returning NETDEV_TX_BUSY or you can trigger a ksoftirqd fatal loop.
>
> In case of memory allocation error, only safe way is to drop the packet
> and return NETDEV_TX_OK

Can you not just leave the skb alone, not bump any tx-dropped
stats, and return NETDEV_TX_BUSY?

Been a while since I looked at this code however...

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[Eric Dumazet]

On Wed, 2012-03-14 at 11:56 -0700, Ben Greear wrote:
> On 03/14/2012 11:53 AM, Eric Dumazet wrote:
> > A driver start_xmit() method cannot free skb and return NETDEV_TX_BUSY,
> > since caller is going to reuse freed skb.
> >
> > This is mostly a revert of commit bf769375c (staging: hv: fix the return
> > status of netvsc_start_xmit())
> >
> > In fact netif_tx_stop_queue() / netif_stop_queue() is needed before
> > returning NETDEV_TX_BUSY or you can trigger a ksoftirqd fatal loop.
> >
> > In case of memory allocation error, only safe way is to drop the packet
> > and return NETDEV_TX_OK
> 
> Can you not just leave the skb alone, not bump any tx-dropped
> stats, and return NETDEV_TX_BUSY?
> 

Nope. Think about OOM.

If we do that, we requeue packet on qdisc, and schedule TX softirq.

-> loop

Really, NETDEV_TX_BUSY is not for this kind of situation.

RE: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[Haiyang Zhang]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, March 14, 2012 3:17 PM
> To: Ben Greear
> Cc: David Miller; netdev; KY Srinivasan; Haiyang Zhang; Greg Kroah-Hartman
> Subject: Re: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use
> 
> On Wed, 2012-03-14 at 11:56 -0700, Ben Greear wrote:
> > On 03/14/2012 11:53 AM, Eric Dumazet wrote:
> > > A driver start_xmit() method cannot free skb and return
> NETDEV_TX_BUSY,
> > > since caller is going to reuse freed skb.
> > >
> > > This is mostly a revert of commit bf769375c (staging: hv: fix the
> return
> > > status of netvsc_start_xmit())
> > >
> > > In fact netif_tx_stop_queue() / netif_stop_queue() is needed before
> > > returning NETDEV_TX_BUSY or you can trigger a ksoftirqd fatal loop.
> > >
> > > In case of memory allocation error, only safe way is to drop the
> packet
> > > and return NETDEV_TX_OK
> >
> > Can you not just leave the skb alone, not bump any tx-dropped
> > stats, and return NETDEV_TX_BUSY?
> >
> 
> Nope. Think about OOM.
> 
> If we do that, we requeue packet on qdisc, and schedule TX softirq.
> 
> -> loop
> 
> Really, NETDEV_TX_BUSY is not for this kind of situation.
> 

Actually, I'm working on this bug, and have a patch under our internal 
review now. My fix is actually not freeing the SKB, and return NETDEV_TX_BUSY.

Even if the vmbus ring buffer is busy currently, it can be available during
the next re-try, because the host is taking away data from ring buffer. The 
stop/wake queue mechanism is in the netvsc.c file.

In the out-of-memory case, dropping packet and return NETDEV_TX_OK seems fine.

Thanks,
- Haiyang

RE: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[Eric Dumazet]

On Wed, 2012-03-14 at 20:00 +0000, Haiyang Zhang wrote:

> Actually, I'm working on this bug, and have a patch under our internal 
> review now. My fix is actually not freeing the SKB, and return NETDEV_TX_BUSY.
> 
> Even if the vmbus ring buffer is busy currently, it can be available during
> the next re-try, because the host is taking away data from ring buffer. The 
> stop/wake queue mechanism is in the netvsc.c file.
> 
> In the out-of-memory case, dropping packet and return NETDEV_TX_OK seems fine.

That will be fine for net-next.

I sent a patch for current kernels, where obvious and minimal fixes
apply.

RE: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[Haiyang Zhang]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, March 14, 2012 4:15 PM
> To: Haiyang Zhang
> Cc: Ben Greear; David Miller; netdev; KY Srinivasan; Greg Kroah-Hartman
> Subject: RE: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use
> 
> On Wed, 2012-03-14 at 20:00 +0000, Haiyang Zhang wrote:
> 
> > Actually, I'm working on this bug, and have a patch under our internal
> > review now. My fix is actually not freeing the SKB, and return
> NETDEV_TX_BUSY.
> >
> > Even if the vmbus ring buffer is busy currently, it can be available
> during
> > the next re-try, because the host is taking away data from ring buffer.
> The
> > stop/wake queue mechanism is in the netvsc.c file.
> >
> > In the out-of-memory case, dropping packet and return NETDEV_TX_OK seems
> fine.
> 
> That will be fine for net-next.
> 
> I sent a patch for current kernels, where obvious and minimal fixes
> apply.

Sounds good. Thanks.

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

Re: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use

[David Miller]

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Wed, 14 Mar 2012 20:29:13 +0000

> 
> 
>> -----Original Message-----
>> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>> Sent: Wednesday, March 14, 2012 4:15 PM
>> To: Haiyang Zhang
>> Cc: Ben Greear; David Miller; netdev; KY Srinivasan; Greg Kroah-Hartman
>> Subject: RE: [PATCH] net/hyperv: fix erroneous NETDEV_TX_BUSY use
>> 
>> On Wed, 2012-03-14 at 20:00 +0000, Haiyang Zhang wrote:
>> 
>> > Actually, I'm working on this bug, and have a patch under our internal
>> > review now. My fix is actually not freeing the SKB, and return
>> NETDEV_TX_BUSY.
>> >
>> > Even if the vmbus ring buffer is busy currently, it can be available
>> during
>> > the next re-try, because the host is taking away data from ring buffer.
>> The
>> > stop/wake queue mechanism is in the netvsc.c file.
>> >
>> > In the out-of-memory case, dropping packet and return NETDEV_TX_OK seems
>> fine.
>> 
>> That will be fine for net-next.
>> 
>> I sent a patch for current kernels, where obvious and minimal fixes
>> apply.
> 
> Sounds good. Thanks.
> 
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> 

APplied.