Patchwork sky2: don't do GRO on second port

login
register
mail settings
Submitter Eric Dumazet
Date Sept. 2, 2010, 10:41 a.m.
Message ID <1283424105.2454.311.camel@edumazet-laptop>
Download mbox | patch
Permalink /patch/63469/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Sept. 2, 2010, 10:41 a.m.
Le jeudi 02 septembre 2010 à 09:55 +0000, Jarek Poplawski a écrit :

> 
> Exactly, but there is no "a single napi --> device mapping". And sky2
> uses the same model. So, there is only a question of cost of this test,
> and a question of probability of gro errors on collisions without such
> a test in normal use. (And if gro can never do such errors for other
> reasons?)

Two vlans might carry packets in different domains, with a clash of IP
space and TCP flows. Even with a probability of 0.000000001%, we cannot
ever merge two packets of different domains. Really !

napi->dev is not used in GRO path, as mentioned earlier,
but in napi_get_frags(), while not needed.

To make this very clear, I suggest following patch :

[PATCH net-next-2.6] gro: remove use of napi->dev

Only use of napi->dev in GRO stack is the one found in napi_get_frags()

We can remove it and use a plain dev_alloc_skb() call.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/core/dev.c |    6 ++++--
 1 file changed, 4 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
Jarek Poplawski - Sept. 2, 2010, 11:02 a.m.
On Thu, Sep 02, 2010 at 12:41:45PM +0200, Eric Dumazet wrote:
> Le jeudi 02 septembre 2010 ?? 09:55 +0000, Jarek Poplawski a écrit :
> 
> > 
> > Exactly, but there is no "a single napi --> device mapping". And sky2
> > uses the same model. So, there is only a question of cost of this test,
> > and a question of probability of gro errors on collisions without such
> > a test in normal use. (And if gro can never do such errors for other
> > reasons?)
> 
> Two vlans might carry packets in different domains, with a clash of IP
> space and TCP flows. Even with a probability of 0.000000001%, we cannot
> ever merge two packets of different domains. Really !

Hmm... But there is only a question of sky2 and this test in
__napi_gro_receive().

> 
> napi->dev is not used in GRO path, as mentioned earlier,
> but in napi_get_frags(), while not needed.
> 
> To make this very clear, I suggest following patch :

But where this skb gets its skb->dev now?

Jarek P.

> 
> [PATCH net-next-2.6] gro: remove use of napi->dev
> 
> Only use of napi->dev in GRO stack is the one found in napi_get_frags()
> 
> We can remove it and use a plain dev_alloc_skb() call.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/core/dev.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d8c43e7..607057a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3248,9 +3248,11 @@ struct sk_buff *napi_get_frags(struct napi_struct *napi)
>  	struct sk_buff *skb = napi->skb;
>  
>  	if (!skb) {
> -		skb = netdev_alloc_skb_ip_align(napi->dev, GRO_MAX_HEAD);
> -		if (skb)
> +		skb = dev_alloc_skb(GRO_MAX_HEAD + NET_IP_ALIGN);
> +		if (skb) {
> +			skb_reserve(skb, NET_IP_ALIGN);
>  			napi->skb = skb;
> +		}
>  	}
>  	return skb;
>  }
> 
> 
--
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
Eric Dumazet - Sept. 2, 2010, 12:09 p.m.
Le jeudi 02 septembre 2010 à 11:02 +0000, Jarek Poplawski a écrit :
> On Thu, Sep 02, 2010 at 12:41:45PM +0200, Eric Dumazet wrote:

> > Two vlans might carry packets in different domains, with a clash of IP
> > space and TCP flows. Even with a probability of 0.000000001%, we cannot
> > ever merge two packets of different domains. Really !
> 
> Hmm... But there is only a question of sky2 and this test in
> __napi_gro_receive().
> 

Any driver can receive in one napi run :

1) A TCP tagged frame for vlan 345, delivered to vlan_gro_receive(),
queued in napi->gro_list.

2) An untagged frame, delived via napi_gro_receive()
    Can meet previous frame in napi->gro_list. Should not merge.

So napi_gro_receive() must perform the same skb->dev check, sky2 or not.

> > 
> > napi->dev is not used in GRO path, as mentioned earlier,
> > but in napi_get_frags(), while not needed.
> > 
> > To make this very clear, I suggest following patch :
> 
> But where this skb gets its skb->dev now?

Oh you are right, I thought drivers were setting this later, but its not
the case.



--
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
Jarek Poplawski - Sept. 2, 2010, 12:28 p.m.
On Thu, Sep 02, 2010 at 02:09:09PM +0200, Eric Dumazet wrote:
> Le jeudi 02 septembre 2010 ?? 11:02 +0000, Jarek Poplawski a écrit :
> > On Thu, Sep 02, 2010 at 12:41:45PM +0200, Eric Dumazet wrote:
> 
> > > Two vlans might carry packets in different domains, with a clash of IP
> > > space and TCP flows. Even with a probability of 0.000000001%, we cannot
> > > ever merge two packets of different domains. Really !
> > 
> > Hmm... But there is only a question of sky2 and this test in
> > __napi_gro_receive().
> > 
> 
> Any driver can receive in one napi run :
> 
> 1) A TCP tagged frame for vlan 345, delivered to vlan_gro_receive(),
> queued in napi->gro_list.
> 
> 2) An untagged frame, delived via napi_gro_receive()
>     Can meet previous frame in napi->gro_list. Should not merge.
> 
> So napi_gro_receive() must perform the same skb->dev check, sky2 or not.

But of course!!! I was mislead by Stephen's changelog so much. So now
I really can't understand this current: "sky2: don't do GRO on second
port"...

Thanks,
Jarek P.
--
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
David Miller - Sept. 2, 2010, 5:08 p.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 02 Sep 2010 12:41:45 +0200

> [PATCH net-next-2.6] gro: remove use of napi->dev
> 
> Only use of napi->dev in GRO stack is the one found in napi_get_frags()
> 
> We can remove it and use a plain dev_alloc_skb() call.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

If you change this to use skb->dev I can apply this.

Thanks.
--
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
Herbert Xu - Sept. 2, 2010, 9:26 p.m.
On Thu, Sep 02, 2010 at 12:41:45PM +0200, Eric Dumazet wrote:
> 
> [PATCH net-next-2.6] gro: remove use of napi->dev
> 
> Only use of napi->dev in GRO stack is the one found in napi_get_frags()
> 
> We can remove it and use a plain dev_alloc_skb() call.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Why do you think napi->dev is not needed?
Eric Dumazet - Sept. 3, 2010, 5:23 a.m.
Le vendredi 03 septembre 2010 à 05:26 +0800, Herbert Xu a écrit :
> On Thu, Sep 02, 2010 at 12:41:45PM +0200, Eric Dumazet wrote:
> > 
> > [PATCH net-next-2.6] gro: remove use of napi->dev
> > 
> > Only use of napi->dev in GRO stack is the one found in napi_get_frags()
> > 
> > We can remove it and use a plain dev_alloc_skb() call.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Why do you think napi->dev is not needed?

Origin was that sky2 could use one napi and two devices on it.
No bug, only a conceptual problem using a napi->dev.

Then, there is a small second point :

netdev_alloc_skb_ip_align() is using 'device' NUMA node to allocate
memory.

In this context, this makes litle sense because this skb is not going to
be used by the device. Its a pure software one, for the sake of GRO
handling.

It would be more efficient to allocate an skb on NUMA node of the CPU
handling the softirq.

Thanks


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

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index d8c43e7..607057a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3248,9 +3248,11 @@  struct sk_buff *napi_get_frags(struct napi_struct *napi)
 	struct sk_buff *skb = napi->skb;
 
 	if (!skb) {
-		skb = netdev_alloc_skb_ip_align(napi->dev, GRO_MAX_HEAD);
-		if (skb)
+		skb = dev_alloc_skb(GRO_MAX_HEAD + NET_IP_ALIGN);
+		if (skb) {
+			skb_reserve(skb, NET_IP_ALIGN);
 			napi->skb = skb;
+		}
 	}
 	return skb;
 }