Message ID | 1331751214.6022.47.camel@edumazet-glaptop |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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>
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } /*
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html