[STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Andrey Vagin]

From: Krishna Kumar <krkumar2@in.ibm.com>

commit 068a2de57ddf4f472e32e7af868613c574ad1d88 upstream.

Non-GSO code drops dst entry for performance reasons, but
the same is missing for GSO code. Drop dst while cache-hot
for GSO case too.

Note: Without this patch the kernel may oops if used bridged veth
devices. A bridge set skb->dst = fake_dst_ops, veth transfers this skb
to netif_receive_skb...ip_rcv_finish and it calls dst_input(skb), but
fake_dst_ops->input = NULL -> Oops

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 net/core/dev.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 915d0ae..c325ab6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1747,6 +1747,14 @@ gso:
 
 		skb->next = nskb->next;
 		nskb->next = NULL;
+
+		/*
+		 * If device doesnt need nskb->dst, release it right now while
+		 * its hot in this cpu cache
+		 */
+		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
+			skb_dst_drop(nskb);
+
 		rc = ops->ndo_start_xmit(nskb, dev);
 		if (unlikely(rc != NETDEV_TX_OK)) {
 			nskb->next = skb->next;
-- 
1.7.2.1

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Michael Tokarev]

Andrey Vagin wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
> 
> commit 068a2de57ddf4f472e32e7af868613c574ad1d88 upstream.
> 
> Non-GSO code drops dst entry for performance reasons, but
> the same is missing for GSO code. Drop dst while cache-hot
> for GSO case too.
> 
> Note: Without this patch the kernel may oops if used bridged veth
> devices. A bridge set skb->dst = fake_dst_ops, veth transfers this skb
> to netif_receive_skb...ip_rcv_finish and it calls dst_input(skb), but
> fake_dst_ops->input = NULL -> Oops

Hmm.  Isn't this the reason for my mysterious OOPSes (jump to
NULL) which I concluded are due to stack overflow?  See f.e.
http://www.spinics.net/lists/netdev/msg142104.html

This started happening when I updated virtio drivers in a windows
virtual machne to the ones which supports GSO, and my config
involves bridging veth devices, and this is where the prob
actually occurs - when doing guest => virtio => tap => bridge => veth
route....

Thanks!

/mjt

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Eric Dumazet]

Le lundi 11 octobre 2010 à 19:40 +0400, Michael Tokarev a écrit :
> Andrey Vagin wrote:
> > From: Krishna Kumar <krkumar2@in.ibm.com>
> > 
> > commit 068a2de57ddf4f472e32e7af868613c574ad1d88 upstream.
> > 
> > Non-GSO code drops dst entry for performance reasons, but
> > the same is missing for GSO code. Drop dst while cache-hot
> > for GSO case too.
> > 
> > Note: Without this patch the kernel may oops if used bridged veth
> > devices. A bridge set skb->dst = fake_dst_ops, veth transfers this skb
> > to netif_receive_skb...ip_rcv_finish and it calls dst_input(skb), but
> > fake_dst_ops->input = NULL -> Oops
> 
> Hmm.  Isn't this the reason for my mysterious OOPSes (jump to
> NULL) which I concluded are due to stack overflow?  See f.e.
> http://www.spinics.net/lists/netdev/msg142104.html
> 
> This started happening when I updated virtio drivers in a windows
> virtual machne to the ones which supports GSO, and my config
> involves bridging veth devices, and this is where the prob
> actually occurs - when doing guest => virtio => tap => bridge => veth
> route....
> 
> Thanks!

This patch was an optimization, not a bug fix.

If something gets better, then a bug is somewhere else ?

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 17:46:49 +0200

> This patch was an optimization, not a bug fix.

Right, this has no business going into 2.6.32-stable at all.

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Andrew Vagin]

  On 10/11/2010 07:59 PM, David Miller wrote:
> From: Eric Dumazet<eric.dumazet@gmail.com>
> Date: Mon, 11 Oct 2010 17:46:49 +0200
>
>> This patch was an optimization, not a bug fix.
> Right, this has no business going into 2.6.32-stable at all.
This is bug fix. Now nobody drops dst in case gso and veth, because the 
commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop 
from the veth.c. We should commit my patch or revert commit 60df914e.

We have two bug reports:

http://www.spinics.net/lists/netdev/msg142104.html

http://bugzilla.openvz.org/show_bug.cgi?id=1634

Taylan verified that the patch really fix his bug.

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Eric Dumazet]

Le lundi 11 octobre 2010 à 20:19 +0400, Andrew Vagin a écrit :
> On 10/11/2010 07:59 PM, David Miller wrote:
> > From: Eric Dumazet<eric.dumazet@gmail.com>
> > Date: Mon, 11 Oct 2010 17:46:49 +0200
> >
> >> This patch was an optimization, not a bug fix.
> > Right, this has no business going into 2.6.32-stable at all.
> This is bug fix. Now nobody drops dst in case gso and veth, because the 
> commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop 
> from the veth.c. We should commit my patch or revert commit 60df914e.
> 
> We have two bug reports:
> 
> http://www.spinics.net/lists/netdev/msg142104.html
> 
> http://bugzilla.openvz.org/show_bug.cgi?id=1634
> 
> Taylan verified that the patch really fix his bug.

Now that makes sense ;)

This is always good to mention commit id of a bug origin.

Because reading your patch (changelog and content), it was not obvious
it fixed a bug.

Thanks !

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Michael Tokarev]

11.10.2010 20:30, Eric Dumazet wrote:
> Le lundi 11 octobre 2010 à 20:19 +0400, Andrew Vagin a écrit :
>> On 10/11/2010 07:59 PM, David Miller wrote:
>>> From: Eric Dumazet<eric.dumazet@gmail.com>
>>> Date: Mon, 11 Oct 2010 17:46:49 +0200
>>>
>>>> This patch was an optimization, not a bug fix.
>>> Right, this has no business going into 2.6.32-stable at all.
>> This is bug fix. Now nobody drops dst in case gso and veth, because the 
>> commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop 
>> from the veth.c. We should commit my patch or revert commit 60df914e.
>>
>> We have two bug reports:
>>
>> http://www.spinics.net/lists/netdev/msg142104.html

This is korg#17251: https://bugzilla.kernel.org/show_bug.cgi?id=17251 ,
from me.

But I really wonder how that thing does not happen anymore
when I disabled netfilter hooks...  I can't experiment till
weekend, but I'll try to get back to it and re-verify again,
with and without this fix, with and without netfilter hooks.

What I know for sure is 2 facts: I can't trigger the problem
now (with the hooks disabled), and I can't trigger it on a
subsequent kernel releases - e.g. 2.6.35 does not have the
issue, but 2.6.32 has.

>> http://bugzilla.openvz.org/show_bug.cgi?id=1634

Thanks!

/mjt

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 18:30:52 +0200

> Le lundi 11 octobre 2010 à 20:19 +0400, Andrew Vagin a écrit :
>> On 10/11/2010 07:59 PM, David Miller wrote:
>> > From: Eric Dumazet<eric.dumazet@gmail.com>
>> > Date: Mon, 11 Oct 2010 17:46:49 +0200
>> >
>> >> This patch was an optimization, not a bug fix.
>> > Right, this has no business going into 2.6.32-stable at all.
>> This is bug fix. Now nobody drops dst in case gso and veth, because the 
>> commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop 
>> from the veth.c. We should commit my patch or revert commit 60df914e.
>> 
>> We have two bug reports:
>> 
>> http://www.spinics.net/lists/netdev/msg142104.html
>> 
>> http://bugzilla.openvz.org/show_bug.cgi?id=1634
>> 
>> Taylan verified that the patch really fix his bug.
> 
> Now that makes sense ;)

In case there is any doubt about this -stable patch submission:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[avagin]

Hi Greg,

This patch is acked by David S. Miller. Greg, maybe you can commit it?

On 10/26/2010 10:54 PM, David Miller wrote:
> From: Eric Dumazet<eric.dumazet@gmail.com>
> Date: Mon, 11 Oct 2010 18:30:52 +0200
>
>> Le lundi 11 octobre 2010 à 20:19 +0400, Andrew Vagin a écrit :
>>> On 10/11/2010 07:59 PM, David Miller wrote:
>>>> From: Eric Dumazet<eric.dumazet@gmail.com>
>>>> Date: Mon, 11 Oct 2010 17:46:49 +0200
>>>>
>>>>> This patch was an optimization, not a bug fix.
>>>> Right, this has no business going into 2.6.32-stable at all.
>>> This is bug fix. Now nobody drops dst in case gso and veth, because the
>>> commit 60df914e295a21a223e43a7ee01e0c73c64dd111 deletes skb_dst_drop
>>> from the veth.c. We should commit my patch or revert commit 60df914e.
>>>
>>> We have two bug reports:
>>>
>>> http://www.spinics.net/lists/netdev/msg142104.html
>>>
>>> http://bugzilla.openvz.org/show_bug.cgi?id=1634
>>>
>>> Taylan verified that the patch really fix his bug.
>>
>> Now that makes sense ;)
>
> In case there is any doubt about this -stable patch submission:
>
> Acked-by: David S. Miller<davem@davemloft.net>

Re: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Greg KH]

On Thu, Dec 09, 2010 at 01:47:41AM +0300, avagin@gmail.com wrote:
> Hi Greg,
> 
> This patch is acked by David S. Miller. Greg, maybe you can commit it?

What specific patch please?

confused,

greg k-h

Re: [stable] [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[avagin]

[-- Attachment #1: Type: text/plain, Size: 288 bytes --]

I add the patch in attachments

On 12/09/2010 02:05 AM, Greg KH wrote:
> On Thu, Dec 09, 2010 at 01:47:41AM +0300, avagin@gmail.com wrote:
>> Hi Greg,
>>
>> This patch is acked by David S. Miller. Greg, maybe you can commit it?
>
> What specific patch please?
>
> confused,
>
> greg k-h


[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 3214 bytes --]

From: Andrey Vagin <avagin@openvz.org>
To: stable@kernel.org
Cc: netdev@vger.kernel.org, Krishna Kumar <krkumar2@in.ibm.com>, "David S. Miller" <davem@davemloft.net>, Andrey Vagin <avagin@openvz.org>
Subject: [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too
Date: Mon, 11 Oct 2010 19:20:13 +0400
Message-ID: <1286810413-30238-1-git-send-email-avagin@openvz.org>

From: Krishna Kumar <krkumar2@in.ibm.com>

commit 068a2de57ddf4f472e32e7af868613c574ad1d88 upstream.

Non-GSO code drops dst entry for performance reasons, but
the same is missing for GSO code. Drop dst while cache-hot
for GSO case too.

Note: Without this patch the kernel may oops if used bridged veth
devices. A bridge set skb->dst = fake_dst_ops, veth transfers this skb
to netif_receive_skb...ip_rcv_finish and it calls dst_input(skb), but
fake_dst_ops->input = NULL -> Oops

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 net/core/dev.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 915d0ae..c325ab6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1747,6 +1747,14 @@ gso:
 
 		skb->next = nskb->next;
 		nskb->next = NULL;
+
+		/*
+		 * If device doesnt need nskb->dst, release it right now while
+		 * its hot in this cpu cache
+		 */
+		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
+			skb_dst_drop(nskb);
+
 		rc = ops->ndo_start_xmit(nskb, dev);
 		if (unlikely(rc != NETDEV_TX_OK)) {
 			nskb->next = skb->next;
-- 
1.7.2.1

Re: [stable] [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Greg KH]

On Thu, Dec 09, 2010 at 09:43:03AM +0300, avagin@gmail.com wrote:
> I add the patch in attachments

Ok, thanks, I'll queue it up for the next .32 release after this one.

greg k-h

Re: [stable] [STABLE 2.6.32 PATCH] net: release dst entry while cache-hot for GSO case too

[Greg KH]

On Thu, Dec 09, 2010 at 10:19:37AM -0800, Greg KH wrote:
> On Thu, Dec 09, 2010 at 09:43:03AM +0300, avagin@gmail.com wrote:
> > I add the patch in attachments
> 
> Ok, thanks, I'll queue it up for the next .32 release after this one.

Now applied.

greg k-h