net: Fix vlan_gro_frags vs netpoll and bonding paths

After positive netpoll_rx_on() check in vlan_gro_receive() there is skipped part of the "common" GRO_NORMAL path, especially "pull:" in dev_gro_receive(), where at least eth header should be copied for entirely paged skbs. So, eth_type_trans() can read zeroed header only.

net/8021q/vlan_core.c +0 -0

index 01ddb04..58289fe 100644
@@ -139,13 +139,19 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
 	if (!skb)
 		return GRO_DROP;

Quoted 4 lines · Jarek Poplawski

After positive netpoll_rx_on() check in vlan_gro_receive() there is skipped part of the "common" GRO_NORMAL path, especially "pull:" in dev_gro_receive(), where at least eth header should be copied for entirely paged skbs. So, eth_type_trans() can read zeroed header only.

Right, thanks for catching this.

Quoted 8 lines

net/8021q/vlan_core.c +0 -0

index 01ddb04..58289fe 100644
@@ -139,13 +139,19 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
 	if (!skb)
 		return GRO_DROP;
 

net/8021q/vlan_core.c gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp, · line 148

Quoted 9 lines

net/8021q/vlan_core.c +7 -2

-	if (netpoll_rx_on(skb)) {
-		skb->protocol = eth_type_trans(skb, skb->dev);
+	/*
+	 * Complete the eth header here, mainly for skb_bond_should_drop(),
+	 * and for netpoll_rx_on() btw.
+	 */
+	skb_gro_pull_in(skb);
+	skb->protocol = eth_type_trans(skb, skb->dev);
+	skb_gro_pull(skb, -ETH_HLEN);

But this code should go into the netpoll (i.e., slow-path) case only so as not to impede performance.

Also, we need to fix this for the non-VLAN case as well.

In commit f2bde7328633269ee935d9ed96535ade15cc348f
Author: Stephen Hemminger <[email protected]>

net: allow multiple dev per napi with GRO

GRO assumes that there is a one-to-one relationship between NAPI structure and network device. Some devices like sky2 share multiple devices on a single interrupt so only have one NAPI handler. Rather than split GRO from NAPI, just have GRO assume if device changes that it is a different flow.

It was assumed a napi could be shared by several devs, but I dont really understand, since we have an unique ->dev pointer inside "napi_struct", this one is set once, and never change.

This pointer is currently used from napi_get_frags() [but that could be avoided], and from netpoll_poll_lock().

The netpoll_poll_lock() case is problematic.

static inline void *netpoll_poll_lock(struct napi_struct *napi)

{
        struct net_device *dev = napi->_dev;

        if (dev && dev->npinfo) {
                spin_lock(&napi->poll_lock);

Maybe we should remove 'dev' field from napi_struct and replace it by a npinfo pointer ?

Quoted 25 lines · Eric Dumazet

In commit f2bde7328633269ee935d9ed96535ade15cc348f
Author: Stephen Hemminger <[email protected]>

net: allow multiple dev per napi with GRO

GRO assumes that there is a one-to-one relationship between NAPI structure and network device. Some devices like sky2 share multiple devices on a single interrupt so only have one NAPI handler. Rather than split GRO from NAPI, just have GRO assume if device changes that it is a different flow.

It was assumed a napi could be shared by several devs, but I dont really understand, since we have an unique ->dev pointer inside "napi_struct", this one is set once, and never change.

This pointer is currently used from napi_get_frags() [but that could be avoided], and from netpoll_poll_lock().

The netpoll_poll_lock() case is problematic.

static inline void *netpoll_poll_lock(struct napi_struct *napi)

{
        struct net_device *dev = napi->_dev;

Quoted 5 lines

        if (dev && dev->npinfo) {
                spin_lock(&napi->poll_lock);

Maybe we should remove 'dev' field from napi_struct and replace it by a npinfo pointer ?

Sky2 seems to work like a single netdev (with an internal sub-netdev), so I can't see your concern: what is the main aim of this replacement?

Quoted 26 lines · Jarek Poplawski

Quoted 25 lines · Eric Dumazet

In commit f2bde7328633269ee935d9ed96535ade15cc348f
Author: Stephen Hemminger <[email protected]>

net: allow multiple dev per napi with GRO

GRO assumes that there is a one-to-one relationship between NAPI structure and network device. Some devices like sky2 share multiple devices on a single interrupt so only have one NAPI handler. Rather than split GRO from NAPI, just have GRO assume if device changes that it is a different flow.

It was assumed a napi could be shared by several devs, but I dont really understand, since we have an unique ->dev pointer inside "napi_struct", this one is set once, and never change.

This pointer is currently used from napi_get_frags() [but that could be avoided], and from netpoll_poll_lock().

The netpoll_poll_lock() case is problematic.

static inline void *netpoll_poll_lock(struct napi_struct *napi)

{
        struct net_device *dev = napi->_dev;

Quoted 8 lines

Quoted 5 lines

        if (dev && dev->npinfo) {
                spin_lock(&napi->poll_lock);

Maybe we should remove 'dev' field from napi_struct and replace it by a npinfo pointer ?

Sky2 seems to work like a single netdev (with an internal sub-netdev), so I can't see your concern: what is the main aim of this replacement?

I am trying to understand why this commit was needed then.

It adds an extra test in the main loop, testing skb->dev against p->dev, it must me for something...

I am trying to say that the one to one relationship between NAPI structure and a device is not only a GRO thing, but also a netpoll one.

Quoted 26 lines · Jarek Poplawski

Quoted 25 lines · Eric Dumazet

In commit f2bde7328633269ee935d9ed96535ade15cc348f
Author: Stephen Hemminger <[email protected]>

net: allow multiple dev per napi with GRO

GRO assumes that there is a one-to-one relationship between NAPI structure and network device. Some devices like sky2 share multiple devices on a single interrupt so only have one NAPI handler. Rather than split GRO from NAPI, just have GRO assume if device changes that it is a different flow.

It was assumed a napi could be shared by several devs, but I dont really understand, since we have an unique ->dev pointer inside "napi_struct", this one is set once, and never change.

This pointer is currently used from napi_get_frags() [but that could be avoided], and from netpoll_poll_lock().

The netpoll_poll_lock() case is problematic.

static inline void *netpoll_poll_lock(struct napi_struct *napi)

{
        struct net_device *dev = napi->_dev;

Quoted 8 lines

Quoted 5 lines

        if (dev && dev->npinfo) {
                spin_lock(&napi->poll_lock);

Maybe we should remove 'dev' field from napi_struct and replace it by a npinfo pointer ?

Sky2 seems to work like a single netdev (with an internal sub-netdev), so I can't see your concern: what is the main aim of this replacement?

I am trying to understand why this commit was needed then.

It adds an extra test in the main loop, testing skb->dev against p->dev, it must me for something...

I am trying to say that the one to one relationship between NAPI structure and a device is not only a GRO thing, but also a netpoll one.

So either we completely remove this one to one relationship, either Stephen commit was not needed.

Some clarification is needed.

The Marvell Yukon2 hardware supports two interfaces sharing a common interrupt. Therfore the sky2 driver has up to two net devices and a single NAPI per board. It is possible in a single invocation of the poll loop to process frames for both ports. GRO works by combining received packets from identical flows over one NAPI interval. It is possible on sky2 that one packet could be processed for the first port, and the next packet processed was for second port and the two packets were related so that GRO would combine them. The check for the same dev is required to prevent this. Yes it is an unlikely corner case, but the purpose of GRO is to do aggregation but preserve the flow characteristics of the incoming traffic.

Since netpoll does similar things, this means both NAPI and netpoll cannot function properly with SKY2's second port. It will only work right on the first port.

Quoted 3 lines · On Sat, 28 Aug 2010 14:41:30 -0700 (PDT) David Miller <[email protected]>…

Since netpoll does similar things, this means both NAPI and netpoll cannot function properly with SKY2's second port. It will only work right on the first port.

I knew netpoll was broken on second port.

From: Stephen Hemminger <[email protected]>
Date: Sat, 28 Aug 2010 10:14:24 -0700

Quoted 10 lines

The Marvell Yukon2 hardware supports two interfaces sharing a common interrupt. Therfore the sky2 driver has up to two net devices and a single NAPI per board. It is possible in a single invocation of the poll loop to process frames for both ports. GRO works by combining received packets from identical flows over one NAPI interval. It is possible on sky2 that one packet could be processed for the first port, and the next packet processed was for second port and the two packets were related so that GRO would combine them. The check for the same dev is required to prevent this. Yes it is an unlikely corner case, but the purpose of GRO is to do aggregation but preserve the flow characteristics of the incoming traffic.

If that is true then GRO is going to refuse to merge every single frame that arrives on the second port of a SKY2 device. :-)

This is because for the two ports, you allocate and register one NAPI instance which uses only the first port's netdev pointer.

Therefore, when GRO compares napi->dev to skb->dev it will always not

match for packets coming from the second port since the netdev pointer in skb->dev will be different.

Since netpoll does similar things, this means both NAPI and netpoll cannot function properly with SKY2's second port. It will only work right on the first port.

Actually, when GRO compares napi->dev to skb->dev?

From: Jarek Poplawski <[email protected]>
Date: Sun, 29 Aug 2010 11:59:51 +0200

Quoted 1 line

Actually, when GRO compares napi->dev to skb->dev?

Hmmm, I thought the code made a skb->dev comparison with the existing SKBs in the list when checking for same-flow matches.

It doesn't, probably based upon the assumption that a NAPI instance maps to a unique device, the very topic we're discussing right now :-/

Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :

Quoted 13 lines

From: Jarek Poplawski <[email protected]>
Date: Sun, 29 Aug 2010 11:59:51 +0200

Quoted 1 line

Actually, when GRO compares napi->dev to skb->dev?

Hmmm, I thought the code made a skb->dev comparison with the existing SKBs in the list when checking for same-flow matches.

It doesn't, probably based upon the assumption that a NAPI instance maps to a unique device, the very topic we're discussing right now :-/

It does the check, Stephen added it in the commit I mentioned to start this thread.

With net-next-2.6 this now reads :

Quoted 15 lines · Eric Dumazet

Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :

Quoted 13 lines

From: Jarek Poplawski <[email protected]>
Date: Sun, 29 Aug 2010 11:59:51 +0200

Quoted 1 line

Actually, when GRO compares napi->dev to skb->dev?

Hmmm, I thought the code made a skb->dev comparison with the existing SKBs in the list when checking for same-flow matches.

It doesn't, probably based upon the assumption that a NAPI instance maps to a unique device, the very topic we're discussing right now :-/

Quoted 5 lines

It does the check, Stephen added it in the commit I mentioned to start this thread.

With net-next-2.6 this now reads :

Since Stephen didn't seem to miss this too much it seems quite obvious to me this check should be removed.

Quoted 19 lines · On Mon, 30 Aug 2010 06:42:31 +0000 Jarek Poplawski <[email protected]> wrote:

Quoted 18 lines · Eric Dumazet

Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :

Quoted 13 lines

From: Jarek Poplawski <[email protected]>
Date: Sun, 29 Aug 2010 11:59:51 +0200

Quoted 1 line

Actually, when GRO compares napi->dev to skb->dev?

Hmmm, I thought the code made a skb->dev comparison with the existing SKBs in the list when checking for same-flow matches.

It doesn't, probably based upon the assumption that a NAPI instance maps to a unique device, the very topic we're discussing right now :-/

It does the check, Stephen added it in the commit I mentioned to start this thread.

Quoted 5 lines

Quoted 2 lines

With net-next-2.6 this now reads :

Since Stephen didn't seem to miss this too much it seems quite obvious to me this check should be removed.

No. I just don't use that system much, breaking code for sake of one comparison is ridiculous.

There's something very important I forgot to tell you. What?

Don't cross the GRO streams.
Why?

It would be bad.
I'm fuzzy on the whole good/bad thing. What do you mean, "bad"?

Try to imagine all the Internet as you know it stopping instantaneously
and every bit in every packet swapping at the speed of light.
Total packet reordering.
Right. That's bad. Okay. All right. Important safety tip. Thanks, Hubert

The simplest way to stop this is just avoid doing GRO on the second port. Very few Marvell boards support two ports per ring, and GRO is just an optimization.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/sky2.c +11 -8

@@ -2520,24 +2520,27 @@ static inline void sky2_tx_done(struct n
 	}
 }
 
-static inline void sky2_skb_rx(const struct sky2_port *sky2,
+static inline void sky2_skb_rx(struct napi_struct *napi,
+			       const struct sky2_port *sky2,
 			       u32 status, struct sk_buff *skb)
 {
 #ifdef SKY2_VLAN_TAG_USED
-	u16 vlan_tag = be16_to_cpu(sky2->rx_tag);
 	if (sky2->vlgrp && (status & GMR_FS_VLAN)) {
-		if (skb->ip_summed == CHECKSUM_NONE)
+		u16 vlan_tag = be16_to_cpu(sky2->rx_tag);
+
+		if (skb->ip_summed == CHECKSUM_NONE ||
+		    sky2->netdev != napi->dev)
 			vlan_hwaccel_receive_skb(skb, sky2->vlgrp, vlan_tag);
 		else
-			vlan_gro_receive(&sky2->hw->napi, sky2->vlgrp,
-					 vlan_tag, skb);
+			vlan_gro_receive(napi, sky2->vlgrp, vlan_tag, skb);
 		return;
 	}
 #endif
-	if (skb->ip_summed == CHECKSUM_NONE)
+	if (skb->ip_summed == CHECKSUM_NONE ||
+	    sky2->netdev != napi->dev)
 		netif_receive_skb(skb);
 	else
-		napi_gro_receive(&sky2->hw->napi, skb);
+		napi_gro_receive(napi, skb);
 }
 
 static inline void sky2_rx_done(struct sky2_hw *hw, unsigned port,

@@ -2638,7 +2641,7 @@ static int sky2_status_intr(struct sky2_
 
 			skb->protocol = eth_type_trans(skb, dev);
 
-			sky2_skb_rx(sky2, status, skb);
+			sky2_skb_rx(&hw->napi, sky2, status, skb);
 
 			/* Stop after net poll weight */
 			if (++work_done >= to_do)

Quoted 10 lines

There's something very important I forgot to tell you. What?

Don't cross the GRO streams.
Why?

It would be bad.
I'm fuzzy on the whole good/bad thing. What do you mean, "bad"?

Quoted 4 lines

Try to imagine all the Internet as you know it stopping instantaneously
and every bit in every packet swapping at the speed of light.
Total packet reordering.
Right. That's bad. Okay. All right. Important safety tip. Thanks, Hubert

Looks really bad to me, so... let's forget it! ;-) (At least until next next.)

From: Jarek Poplawski <[email protected]>
Date: Mon, 30 Aug 2010 21:09:00 +0200

Quoted 18 lines · Stephen Hemminger

Quoted 7 lines

There's something very important I forgot to tell you. What?

Don't cross the GRO streams.
Why?

Quoted 7 lines

It would be bad.
I'm fuzzy on the whole good/bad thing. What do you mean, "bad"?

Try to imagine all the Internet as you know it stopping instantaneously
and every bit in every packet swapping at the speed of light.
Total packet reordering.
Right. That's bad. Okay. All right. Important safety tip. Thanks, Hubert

Looks really bad to me, so... let's forget it! ;-) (At least until next next.)

Quoted 19 lines · On Wed, 01 Sep 2010 14:51:51 -0700 (PDT) David Miller <[email protected]>…

From: Jarek Poplawski <[email protected]>
Date: Mon, 30 Aug 2010 21:09:00 +0200

Quoted 16 lines · Stephen Hemminger

Quoted 14 lines

There's something very important I forgot to tell you. What?

Don't cross the GRO streams.
Why?

It would be bad.
I'm fuzzy on the whole good/bad thing. What do you mean, "bad"?

Try to imagine all the Internet as you know it stopping instantaneously
and every bit in every packet swapping at the speed of light.
Total packet reordering.
Right. That's bad. Okay. All right. Important safety tip. Thanks, Hubert

Quoted 2 lines

Quoted 2 lines

Looks really bad to me, so... let's forget it! ;-) (At least until next next.)

The patch wasn't that bad, but the movie quote probably confused you.

Quoted 22 lines · Stephen Hemminger

Quoted 19 lines · On Wed, 01 Sep 2010 14:51:51 -0700 (PDT) David Miller <[email protected]>…

From: Jarek Poplawski <[email protected]>
Date: Mon, 30 Aug 2010 21:09:00 +0200

Quoted 16 lines · Stephen Hemminger

Quoted 14 lines

There's something very important I forgot to tell you. What?

Don't cross the GRO streams.
Why?

It would be bad.
I'm fuzzy on the whole good/bad thing. What do you mean, "bad"?

Try to imagine all the Internet as you know it stopping instantaneously
and every bit in every packet swapping at the speed of light.
Total packet reordering.
Right. That's bad. Okay. All right. Important safety tip. Thanks, Hubert

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Looks really bad to me, so... let's forget it! ;-) (At least until next next.)

The patch wasn't that bad, but the movie quote probably confused you.

Yes, I was equally confused by both of them. Good to know it's only fiction... ;-)

Stephen, after Eric's explanation, I really think this patch was a bad idea, and I apologize my false opinion started this.

I hope David, will revert this patch, please.

From: Jarek Poplawski <[email protected]>
Date: Thu, 2 Sep 2010 12:53:26 +0000

Quoted 4 lines

Stephen, after Eric's explanation, I really think this patch was a bad idea, and I apologize my false opinion started this.

I hope David, will revert this patch, please.

Ok, now that I've reread everything over I agree, I'll revert the sky2 change, thanks.

The only scenerio you can guarentee that all packets for a given flow key will be treated the same is the one where the input device is the same as well.

When there is a single napi --> device mapping, it works, but without that invariant it doesn't.

Quoted 4 lines · David Miller

The only scenerio you can guarentee that all packets for a given flow key will be treated the same is the one where the input device is the same as well.

Quoted 2 lines

When there is a single napi --> device mapping, it works, but without that invariant it doesn't.

Do you mean a single napi can't be used eg. for vlan_gro and napi_gro? (napi_gro handles flows for skb->dev different from napi->dev or I miss something?)

Quoted 5 lines · Jarek Poplawski

Quoted 4 lines · David Miller

The only scenerio you can guarentee that all packets for a given flow key will be treated the same is the one where the input device is the same as well.

Quoted 7 lines

Quoted 2 lines

When there is a single napi --> device mapping, it works, but without that invariant it doesn't.

Do you mean a single napi can't be used eg. for vlan_gro and napi_gro? (napi_gro handles flows for skb->dev different from napi->dev or I miss something?)

Same napi can be used both for vlan tagged trafic and "non tagged trafic".

vlan_gro_common() does the right thing, when initializing skb->dev to the vlan device, before doing the GRO loop.

So if we receive two packets on same ethernet device, two different vlans, vlan_gro_common() automatically say they are not part of the same flow, even if upper layers would say "it's ok for these two packets to merge". Of course, we wont ask upper layers what they think :)

So we must keep the test against skb->dev, because of vlans,

diffs = (unsigned long)p->dev ^ (unsigned long)skb->dev;

(both in vlan_gro_common() and __napi_gro_receive())

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

Quoted 6 lines · Jarek Poplawski

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 <[email protected]>

---
 net/core/dev.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

net/core/dev.c +4 -2

index d8c43e7..607057a 100644
@@ -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;
 }

Quoted 3 lines · Eric Dumazet

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

Quoted 5 lines

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?

Quoted 9 lines · Jarek Poplawski

Quoted 3 lines · Eric Dumazet

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.

[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 <[email protected]>

[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 <[email protected]>

Quoted 8 lines · Eric Dumazet

[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 <[email protected]>

Why do you think napi->dev is not needed?

Quoted 6 lines · David Miller

The only scenerio you can guarentee that all packets for a given flow key will be treated the same is the one where the input device is the same as well.

When there is a single napi --> device mapping, it works, but without that invariant it doesn't.

Do you mean a single napi can't be used eg. for vlan_gro and napi_gro? (napi_gro handles flows for skb->dev different from napi->dev or I

miss something?)

From: Stephen Hemminger <[email protected]>
Date: Mon, 30 Aug 2010 08:57:21 -0700

Quoted 25 lines

Quoted 22 lines · On Mon, 30 Aug 2010 06:42:31 +0000 Jarek Poplawski <[email protected]> wrote:

Quoted 20 lines · Eric Dumazet

Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :

Quoted 13 lines

From: Jarek Poplawski <[email protected]>
Date: Sun, 29 Aug 2010 11:59:51 +0200

Quoted 1 line

Actually, when GRO compares napi->dev to skb->dev?

Hmmm, I thought the code made a skb->dev comparison with the existing SKBs in the list when checking for same-flow matches.

It doesn't, probably based upon the assumption that a NAPI instance maps to a unique device, the very topic we're discussing right now :-/

It does the check, Stephen added it in the commit I mentioned to start this thread.

With net-next-2.6 this now reads :

Quoted 5 lines

Quoted 2 lines

Since Stephen didn't seem to miss this too much it seems quite obvious to me this check should be removed.

No. I just don't use that system much, breaking code for sake of one comparison is ridiculous.

It's not working to begin with.

I agree with Jarek that the check should be removed. And GRO is one of those places that, precisely, even one memory reference removal can improve performance dramatically.

Herbert spent a lot of time doing micro-optimizations to make GRO better and better, and the smallest things can turn out to make a huge difference there.

So everybody jumped on GRO, while my concern was more the napi->dev thing.

I pointed to gro because the commit was about gro, but gro was fine IMHO.

Are we sure netpoll is ok ?

Quoted 3 lines · On Mon, 30 Aug 2010 21:59:16 +0200 Eric Dumazet <[email protected]> wrote:

So everybody jumped on GRO, while my concern was more the napi->dev thing.

Quoted 4 lines

I pointed to gro because the commit was about gro, but gro was fine IMHO.

Are we sure netpoll is ok ?

Netpoll is dependent on napi->netdev but does not cause problems. The sky2 driver doesn't expose netpoll on second port. The second port has different net_device_ops without netpoll.