atl1 warn_on_slowpath help

atl1 warn_on_slowpath help

[Jay Cliburn]

I'm pretty fuzzy on the mechanics of warn_on_slowpath, so I'd like to
ask for help from more experienced netdev developers.

An atl1 user reports the following warning when receiving a VLAN tagged
packet.

[   27.779463] ------------[ cut here ]------------
[   27.779509] WARNING: at kernel/softirq.c:136 local_bh_enable+0x37/0x81()
[   27.779553] Modules linked in: ppdev lp video output ac battery
acpi_cpufreq cpufreq_ondemand cpufreq_powersave cpufreq_userspace
cpufreq_conservative cpufreq_stats freq_table nfsd auth_rpcgss exportfs
nfs lockd nfs_acl sunrpc ipv6 dm_snapshot dm_mirror dm_log dm_mod 8021q
loop snd_hda_intel snd_pcm snd_seq snd_timer snd_seq_device iTCO_wdt snd
serio_raw i2c_i801 parport_pc evdev rng_core i2c_core pcspkr psmouse
soundcore parport intel_agp agpgart button snd_page_alloc ext3 jbd mbcache
ide_cd_mod cdrom sd_mod ata_generic ata_piix libata piix scsi_mod dock
floppy ide_pci_generic ide_core uhci_hcd ehci_hcd usbcore atl1 mii e1000e
thermal processor fan thermal_sys
[   27.781709] Pid: 0, comm: swapper Not tainted 2.6.27.2 #1
[   27.781753]  [<c01240f2>] warn_on_slowpath+0x40/0x63
[   27.781824]  [<c01195e3>] enqueue_task+0x52/0x5d
[   27.781894]  [<c01196e1>] activate_task+0x16/0x1b
[   27.781963]  [<c011f3ce>] try_to_wake_up+0x13b/0x144
[   27.782033]  [<c01e5f32>] __next_cpu+0x12/0x21
[   27.782102]  [<c011ceb6>] find_busiest_group+0x2fb/0x778
[   27.782172]  [<c01198b9>] __wake_up_common+0x34/0x59
[   27.782242]  [<c012885c>] local_bh_enable+0x37/0x81
[   27.782311]  [<c026fb1f>] sk_filter+0x63/0x6c
[   27.782380]  [<c025fbfb>] sock_queue_rcv_skb+0x26/0xb3
[   27.782450]  [<c02bb741>] packet_rcv_spkt+0xe1/0xf3
[   27.782520]  [<c0264755>] netif_nit_deliver+0x5b/0x75
[   27.782590]  [<c02bba83>] __vlan_hwaccel_rx+0x79/0x162
[   27.782664]  [<f8851c1d>] atl1_intr+0x9a9/0xa7c [atl1]
[   27.782738]  [<c0155b17>] handle_IRQ_event+0x23/0x51
[   27.782808]  [<c015692e>] handle_edge_irq+0xc2/0x102
[   27.782878]  [<c0105fd5>] do_IRQ+0x4d/0x64
[   27.782947]  [<c010434f>] common_interrupt+0x23/0x28
[   27.783017]  [<c0108df6>] mwait_idle+0x2f/0x3b
[   27.783085]  [<c0102a75>] cpu_idle+0xde/0x101
[   27.783154]  =======================
[   27.783195] ---[ end trace 744634f3da93b46a ]---

I've flailed around trying to find the bug, but haven't been
successful -- primarily because I don't understand the
warn_on_slowpath stuff well enough to know what to look for. Can someone
please take a quick look at drivers/net/atlx/atl1.c around line 2017
and see if there's an obvious error?  I'd really appreciate it.

Thanks,
Jay

Re: atl1 warn_on_slowpath help

[Jarek Poplawski]

On 29-10-2008 01:08, Jay Cliburn wrote:
> I'm pretty fuzzy on the mechanics of warn_on_slowpath, so I'd like to
> ask for help from more experienced netdev developers.
> 
> An atl1 user reports the following warning when receiving a VLAN tagged
> packet.
> 
> [   27.779463] ------------[ cut here ]------------
> [   27.779509] WARNING: at kernel/softirq.c:136 local_bh_enable+0x37/0x81()
...
> [   27.782520]  [<c0264755>] netif_nit_deliver+0x5b/0x75
> [   27.782590]  [<c02bba83>] __vlan_hwaccel_rx+0x79/0x162
> [   27.782664]  [<f8851c1d>] atl1_intr+0x9a9/0xa7c [atl1]
> [   27.782738]  [<c0155b17>] handle_IRQ_event+0x23/0x51
> [   27.782808]  [<c015692e>] handle_edge_irq+0xc2/0x102
> [   27.782878]  [<c0105fd5>] do_IRQ+0x4d/0x64
> [   27.782947]  [<c010434f>] common_interrupt+0x23/0x28
> [   27.783017]  [<c0108df6>] mwait_idle+0x2f/0x3b
> [   27.783085]  [<c0102a75>] cpu_idle+0xde/0x101
> [   27.783154]  =======================
> [   27.783195] ---[ end trace 744634f3da93b46a ]---
> 
> I've flailed around trying to find the bug, but haven't been
> successful -- primarily because I don't understand the
> warn_on_slowpath stuff well enough to know what to look for. Can someone
> please take a quick look at drivers/net/atlx/atl1.c around line 2017
> and see if there's an obvious error?  I'd really appreciate it.

It looks to me like vlan_hwaccel_rx() is to blame: I doubt we can do
netif_nit_deliver() in hard irq context. (Patrick Cc-ed.)

Jarek P.

Re: atl1 warn_on_slowpath help

[Patrick McHardy]

Jarek Poplawski wrote:
> On 29-10-2008 01:08, Jay Cliburn wrote:
>> I'm pretty fuzzy on the mechanics of warn_on_slowpath, so I'd like to
>> ask for help from more experienced netdev developers.
>>
>> An atl1 user reports the following warning when receiving a VLAN tagged
>> packet.
>>
>> [   27.779463] ------------[ cut here ]------------
>> [   27.779509] WARNING: at kernel/softirq.c:136 local_bh_enable+0x37/0x81()
> ...
>> [   27.782520]  [<c0264755>] netif_nit_deliver+0x5b/0x75
>> [   27.782590]  [<c02bba83>] __vlan_hwaccel_rx+0x79/0x162
>> [   27.782664]  [<f8851c1d>] atl1_intr+0x9a9/0xa7c [atl1]
>> [   27.782738]  [<c0155b17>] handle_IRQ_event+0x23/0x51
>> [   27.782808]  [<c015692e>] handle_edge_irq+0xc2/0x102
>> [   27.782878]  [<c0105fd5>] do_IRQ+0x4d/0x64
>> [   27.782947]  [<c010434f>] common_interrupt+0x23/0x28
>> [   27.783017]  [<c0108df6>] mwait_idle+0x2f/0x3b
>> [   27.783085]  [<c0102a75>] cpu_idle+0xde/0x101
>> [   27.783154]  =======================
>> [   27.783195] ---[ end trace 744634f3da93b46a ]---
>>
>> I've flailed around trying to find the bug, but haven't been
>> successful -- primarily because I don't understand the
>> warn_on_slowpath stuff well enough to know what to look for. Can someone
>> please take a quick look at drivers/net/atlx/atl1.c around line 2017
>> and see if there's an obvious error?  I'd really appreciate it.
> 
> It looks to me like vlan_hwaccel_rx() is to blame: I doubt we can do
> netif_nit_deliver() in hard irq context. (Patrick Cc-ed.)

Crap, I didn't think of that, all drivers I tested with support
NAPI. I can't think of a clean way to fix it right now, but I'll
look into it.

Re: atl1 warn_on_slowpath help

[Patrick McHardy]

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

Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> On 29-10-2008 01:08, Jay Cliburn wrote:
>>> [   27.779463] ------------[ cut here ]------------
>>> [   27.779509] WARNING: at kernel/softirq.c:136 
>>> local_bh_enable+0x37/0x81()
>> ...
>>> [   27.782520]  [<c0264755>] netif_nit_deliver+0x5b/0x75
>>> [   27.782590]  [<c02bba83>] __vlan_hwaccel_rx+0x79/0x162
>>> [   27.782664]  [<f8851c1d>] atl1_intr+0x9a9/0xa7c [atl1]
>>>>
>>> warn_on_slowpath stuff well enough to know what to look for. Can someone
>>> please take a quick look at drivers/net/atlx/atl1.c around line 2017
>>> and see if there's an obvious error?  I'd really appreciate it.
>>
>> It looks to me like vlan_hwaccel_rx() is to blame: I doubt we can do
>> netif_nit_deliver() in hard irq context. (Patrick Cc-ed.)
> 
> Crap, I didn't think of that, all drivers I tested with support
> NAPI. I can't think of a clean way to fix it right now, but I'll
> look into it.

This is the best I could come up with, short of simply restoring
the old behaviour for non-polling drivers.

The __vlan_hwaccel_rx function only does the device lookup and
stores it in the cb. The remaining processing is done in a new
function that is invoked by netif_receive_skb(), in the proper
context. Unfortunatly this needs vlan-specific handling in
netif_receive_skb().


[-- Attachment #2: 01.diff --]
[-- Type: text/x-patch, Size: 3602 bytes --]

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 9e7b49b..a5cb0c3 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -114,6 +114,8 @@ extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
 extern int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 			     u16 vlan_tci, int polling);
+extern int vlan_hwaccel_do_receive(struct sk_buff *skb);
+
 #else
 static inline struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
@@ -133,6 +135,11 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 	BUG();
 	return NET_XMIT_SUCCESS;
 }
+
+static inline int vlan_hwaccel_do_receive(struct sk_buff *skb)
+{
+	return 0;
+}
 #endif
 
 /**
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 916061f..68ced4b 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -3,11 +3,20 @@
 #include <linux/if_vlan.h>
 #include "vlan.h"
 
+struct vlan_hwaccel_cb {
+	struct net_device	*dev;
+};
+
+static inline struct vlan_hwaccel_cb *vlan_hwaccel_cb(struct sk_buff *skb)
+{
+	return (struct vlan_hwaccel_cb *)skb->cb;
+}
+
 /* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
 int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 		      u16 vlan_tci, int polling)
 {
-	struct net_device_stats *stats;
+	struct vlan_hwaccel_cb *cb = vlan_hwaccel_cb(skb);
 
 	if (skb_bond_should_drop(skb)) {
 		dev_kfree_skb_any(skb);
@@ -15,23 +24,35 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 	}
 
 	skb->vlan_tci = vlan_tci;
+	cb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
+
+	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+}
+EXPORT_SYMBOL(__vlan_hwaccel_rx);
+
+int vlan_hwaccel_do_receive(struct sk_buff *skb)
+{
+	struct vlan_hwaccel_cb *cb = vlan_hwaccel_cb(skb);
+	struct net_device *dev = cb->dev;
+	struct net_device_stats *stats;
+
 	netif_nit_deliver(skb);
 
-	skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
-	if (skb->dev == NULL) {
-		dev_kfree_skb_any(skb);
-		/* Not NET_RX_DROP, this is not being dropped
-		 * due to congestion. */
-		return NET_RX_SUCCESS;
+	if (dev == NULL) {
+		kfree_skb(skb);
+		return -1;
 	}
-	skb->dev->last_rx = jiffies;
+
+	skb->dev = dev;
+	skb->priority = vlan_get_ingress_priority(dev, skb->vlan_tci);
 	skb->vlan_tci = 0;
 
-	stats = &skb->dev->stats;
+	dev->last_rx = jiffies;
+
+	stats = &dev->stats;
 	stats->rx_packets++;
 	stats->rx_bytes += skb->len;
 
-	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tci);
 	switch (skb->pkt_type) {
 	case PACKET_BROADCAST:
 		break;
@@ -43,13 +64,12 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 		 * This allows the VLAN to have a different MAC than the
 		 * underlying device, and still route correctly. */
 		if (!compare_ether_addr(eth_hdr(skb)->h_dest,
-					skb->dev->dev_addr))
+					dev->dev_addr))
 			skb->pkt_type = PACKET_HOST;
 		break;
 	};
-	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+	return 0;
 }
-EXPORT_SYMBOL(__vlan_hwaccel_rx);
 
 struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
diff --git a/net/core/dev.c b/net/core/dev.c
index d9038e3..9174c77 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2218,6 +2218,9 @@ int netif_receive_skb(struct sk_buff *skb)
 	int ret = NET_RX_DROP;
 	__be16 type;
 
+	if (skb->vlan_tci && vlan_hwaccel_do_receive(skb))
+		return NET_RX_SUCCESS;
+
 	/* if we've gotten here through NAPI, check netpoll */
 	if (netpoll_receive_skb(skb))
 		return NET_RX_DROP;

Re: atl1 warn_on_slowpath help

[J. K. Cliburn]

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

[adding bug reporter to cc list]


On Wed, Oct 29, 2008 at 5:17 AM, Patrick McHardy <kaber@trash.net> wrote:
> Patrick McHardy wrote:
>>
>> Jarek Poplawski wrote:
>>>
>>> On 29-10-2008 01:08, Jay Cliburn wrote:
>>>>
>>>> [   27.779463] ------------[ cut here ]------------
>>>> [   27.779509] WARNING: at kernel/softirq.c:136
>>>> local_bh_enable+0x37/0x81()
>>>
>>> ...
>>>>
>>>> [   27.782520]  [<c0264755>] netif_nit_deliver+0x5b/0x75
>>>> [   27.782590]  [<c02bba83>] __vlan_hwaccel_rx+0x79/0x162
>>>> [   27.782664]  [<f8851c1d>] atl1_intr+0x9a9/0xa7c [atl1]
>>>>>
>>>> warn_on_slowpath stuff well enough to know what to look for. Can someone
>>>> please take a quick look at drivers/net/atlx/atl1.c around line 2017
>>>> and see if there's an obvious error?  I'd really appreciate it.
>>>
>>> It looks to me like vlan_hwaccel_rx() is to blame: I doubt we can do
>>> netif_nit_deliver() in hard irq context. (Patrick Cc-ed.)
>>
>> Crap, I didn't think of that, all drivers I tested with support
>> NAPI. I can't think of a clean way to fix it right now, but I'll
>> look into it.
>
> This is the best I could come up with, short of simply restoring
> the old behaviour for non-polling drivers.
>
> The __vlan_hwaccel_rx function only does the device lookup and
> stores it in the cb. The remaining processing is done in a new
> function that is invoked by netif_receive_skb(), in the proper
> context. Unfortunatly this needs vlan-specific handling in
> netif_receive_skb().
>
>

Thanks Jarek and Patrick.

Ramon,

Can you please try the attached patch from Patrick and see if it fixes
your kernel warning?

Thanks,
Jay

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 01.diff --]
[-- Type: text/x-patch; name=01.diff, Size: 3602 bytes --]

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 9e7b49b..a5cb0c3 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -114,6 +114,8 @@ extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
 extern int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 			     u16 vlan_tci, int polling);
+extern int vlan_hwaccel_do_receive(struct sk_buff *skb);
+
 #else
 static inline struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
@@ -133,6 +135,11 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 	BUG();
 	return NET_XMIT_SUCCESS;
 }
+
+static inline int vlan_hwaccel_do_receive(struct sk_buff *skb)
+{
+	return 0;
+}
 #endif
 
 /**
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 916061f..68ced4b 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -3,11 +3,20 @@
 #include <linux/if_vlan.h>
 #include "vlan.h"
 
+struct vlan_hwaccel_cb {
+	struct net_device	*dev;
+};
+
+static inline struct vlan_hwaccel_cb *vlan_hwaccel_cb(struct sk_buff *skb)
+{
+	return (struct vlan_hwaccel_cb *)skb->cb;
+}
+
 /* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
 int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 		      u16 vlan_tci, int polling)
 {
-	struct net_device_stats *stats;
+	struct vlan_hwaccel_cb *cb = vlan_hwaccel_cb(skb);
 
 	if (skb_bond_should_drop(skb)) {
 		dev_kfree_skb_any(skb);
@@ -15,23 +24,35 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 	}
 
 	skb->vlan_tci = vlan_tci;
+	cb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
+
+	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+}
+EXPORT_SYMBOL(__vlan_hwaccel_rx);
+
+int vlan_hwaccel_do_receive(struct sk_buff *skb)
+{
+	struct vlan_hwaccel_cb *cb = vlan_hwaccel_cb(skb);
+	struct net_device *dev = cb->dev;
+	struct net_device_stats *stats;
+
 	netif_nit_deliver(skb);
 
-	skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
-	if (skb->dev == NULL) {
-		dev_kfree_skb_any(skb);
-		/* Not NET_RX_DROP, this is not being dropped
-		 * due to congestion. */
-		return NET_RX_SUCCESS;
+	if (dev == NULL) {
+		kfree_skb(skb);
+		return -1;
 	}
-	skb->dev->last_rx = jiffies;
+
+	skb->dev = dev;
+	skb->priority = vlan_get_ingress_priority(dev, skb->vlan_tci);
 	skb->vlan_tci = 0;
 
-	stats = &skb->dev->stats;
+	dev->last_rx = jiffies;
+
+	stats = &dev->stats;
 	stats->rx_packets++;
 	stats->rx_bytes += skb->len;
 
-	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tci);
 	switch (skb->pkt_type) {
 	case PACKET_BROADCAST:
 		break;
@@ -43,13 +64,12 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 		 * This allows the VLAN to have a different MAC than the
 		 * underlying device, and still route correctly. */
 		if (!compare_ether_addr(eth_hdr(skb)->h_dest,
-					skb->dev->dev_addr))
+					dev->dev_addr))
 			skb->pkt_type = PACKET_HOST;
 		break;
 	};
-	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+	return 0;
 }
-EXPORT_SYMBOL(__vlan_hwaccel_rx);
 
 struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
diff --git a/net/core/dev.c b/net/core/dev.c
index d9038e3..9174c77 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2218,6 +2218,9 @@ int netif_receive_skb(struct sk_buff *skb)
 	int ret = NET_RX_DROP;
 	__be16 type;
 
+	if (skb->vlan_tci && vlan_hwaccel_do_receive(skb))
+		return NET_RX_SUCCESS;
+
 	/* if we've gotten here through NAPI, check netpoll */
 	if (netpoll_receive_skb(skb))
 		return NET_RX_DROP;

Re: atl1 warn_on_slowpath help

[Patrick McHardy]

J. K. Cliburn wrote:
> [adding bug reporter to cc list]
> 
> On Wed, Oct 29, 2008 at 5:17 AM, Patrick McHardy <kaber@trash.net> wrote:
>> This is the best I could come up with, short of simply restoring
>> the old behaviour for non-polling drivers.
>>
>> The __vlan_hwaccel_rx function only does the device lookup and
>> stores it in the cb. The remaining processing is done in a new
>> function that is invoked by netif_receive_skb(), in the proper
>> context. Unfortunatly this needs vlan-specific handling in
>> netif_receive_skb().
>>
> 
> Thanks Jarek and Patrick.
> 
> Ramon,
> 
> Can you please try the attached patch from Patrick and see if it fixes
> your kernel warning?

Just to make sure we don't run into testing mistakes -
the ethernet device needs to have tcpdump or something
similar running to trigger this warning.

Re: atl1 warn_on_slowpath help

[Jarek Poplawski]

On Wed, Oct 29, 2008 at 11:17:09AM +0100, Patrick McHardy wrote:
> Patrick McHardy wrote:
>> Jarek Poplawski wrote:
>>> On 29-10-2008 01:08, Jay Cliburn wrote:
>>>> [   27.779463] ------------[ cut here ]------------
>>>> [   27.779509] WARNING: at kernel/softirq.c:136  
>>>> local_bh_enable+0x37/0x81()
>>> ...
>>>> [   27.782520]  [<c0264755>] netif_nit_deliver+0x5b/0x75
>>>> [   27.782590]  [<c02bba83>] __vlan_hwaccel_rx+0x79/0x162
>>>> [   27.782664]  [<f8851c1d>] atl1_intr+0x9a9/0xa7c [atl1]
>>>>>
>>>> warn_on_slowpath stuff well enough to know what to look for. Can someone
>>>> please take a quick look at drivers/net/atlx/atl1.c around line 2017
>>>> and see if there's an obvious error?  I'd really appreciate it.
>>>
>>> It looks to me like vlan_hwaccel_rx() is to blame: I doubt we can do
>>> netif_nit_deliver() in hard irq context. (Patrick Cc-ed.)
>>
>> Crap, I didn't think of that, all drivers I tested with support
>> NAPI. I can't think of a clean way to fix it right now, but I'll
>> look into it.
>
> This is the best I could come up with, short of simply restoring
> the old behaviour for non-polling drivers.
>
> The __vlan_hwaccel_rx function only does the device lookup and
> stores it in the cb. The remaining processing is done in a new
> function that is invoked by netif_receive_skb(), in the proper
> context. Unfortunatly this needs vlan-specific handling in
> netif_receive_skb().
>

Unfortunately I'm not up-to-date with vlans and especially this
nit_deliver problem, but here is a little doubt: since this is
probably for stable, and skb->cb is rather tricky, I wonder why
vlan_group_get_device() couldn't be used directly in
vlan_hwaccel_do_receive()?

BTW: if we call netif_nit_deliver() from vlan_hwaccel_do_receive()
from netif_receive_skb() this comment about bypassing looks a bit
confusing to me:

/*
 *     netif_nit_deliver - deliver received packets to network taps
 *     @skb: buffer
 *
 *     This function is used to deliver incoming packets to network
 *     taps. It should be used when the normal netif_receive_skb path
 *     is bypassed, for example because of VLAN acceleration.
 */

As a matter of fact without this patch it's not so apparent why
netif_receive_skb() can't happen after netif_nit_deliver() in
__vlan_hwaccel_rx() too.

Jarek P.

> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 9e7b49b..a5cb0c3 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -114,6 +114,8 @@ extern u16 vlan_dev_vlan_id(const struct net_device *dev);
>  
>  extern int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>  			     u16 vlan_tci, int polling);
> +extern int vlan_hwaccel_do_receive(struct sk_buff *skb);
> +
>  #else
>  static inline struct net_device *vlan_dev_real_dev(const struct net_device *dev)
>  {
> @@ -133,6 +135,11 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>  	BUG();
>  	return NET_XMIT_SUCCESS;
>  }
> +
> +static inline int vlan_hwaccel_do_receive(struct sk_buff *skb)
> +{
> +	return 0;
> +}
>  #endif
>  
>  /**
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 916061f..68ced4b 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -3,11 +3,20 @@
>  #include <linux/if_vlan.h>
>  #include "vlan.h"
>  
> +struct vlan_hwaccel_cb {
> +	struct net_device	*dev;
> +};
> +
> +static inline struct vlan_hwaccel_cb *vlan_hwaccel_cb(struct sk_buff *skb)
> +{
> +	return (struct vlan_hwaccel_cb *)skb->cb;
> +}
> +
>  /* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
>  int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>  		      u16 vlan_tci, int polling)
>  {
> -	struct net_device_stats *stats;
> +	struct vlan_hwaccel_cb *cb = vlan_hwaccel_cb(skb);
>  
>  	if (skb_bond_should_drop(skb)) {
>  		dev_kfree_skb_any(skb);
> @@ -15,23 +24,35 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>  	}
>  
>  	skb->vlan_tci = vlan_tci;
> +	cb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> +
> +	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> +}
> +EXPORT_SYMBOL(__vlan_hwaccel_rx);
> +
> +int vlan_hwaccel_do_receive(struct sk_buff *skb)
> +{
> +	struct vlan_hwaccel_cb *cb = vlan_hwaccel_cb(skb);
> +	struct net_device *dev = cb->dev;
> +	struct net_device_stats *stats;
> +
>  	netif_nit_deliver(skb);
>  
> -	skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> -	if (skb->dev == NULL) {
> -		dev_kfree_skb_any(skb);
> -		/* Not NET_RX_DROP, this is not being dropped
> -		 * due to congestion. */
> -		return NET_RX_SUCCESS;
> +	if (dev == NULL) {
> +		kfree_skb(skb);
> +		return -1;
>  	}
> -	skb->dev->last_rx = jiffies;
> +
> +	skb->dev = dev;
> +	skb->priority = vlan_get_ingress_priority(dev, skb->vlan_tci);
>  	skb->vlan_tci = 0;
>  
> -	stats = &skb->dev->stats;
> +	dev->last_rx = jiffies;
> +
> +	stats = &dev->stats;
>  	stats->rx_packets++;
>  	stats->rx_bytes += skb->len;
>  
> -	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tci);
>  	switch (skb->pkt_type) {
>  	case PACKET_BROADCAST:
>  		break;
> @@ -43,13 +64,12 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>  		 * This allows the VLAN to have a different MAC than the
>  		 * underlying device, and still route correctly. */
>  		if (!compare_ether_addr(eth_hdr(skb)->h_dest,
> -					skb->dev->dev_addr))
> +					dev->dev_addr))
>  			skb->pkt_type = PACKET_HOST;
>  		break;
>  	};
> -	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> +	return 0;
>  }
> -EXPORT_SYMBOL(__vlan_hwaccel_rx);
>  
>  struct net_device *vlan_dev_real_dev(const struct net_device *dev)
>  {
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d9038e3..9174c77 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2218,6 +2218,9 @@ int netif_receive_skb(struct sk_buff *skb)
>  	int ret = NET_RX_DROP;
>  	__be16 type;
>  
> +	if (skb->vlan_tci && vlan_hwaccel_do_receive(skb))
> +		return NET_RX_SUCCESS;
> +
>  	/* if we've gotten here through NAPI, check netpoll */
>  	if (netpoll_receive_skb(skb))
>  		return NET_RX_DROP;

Re: atl1 warn_on_slowpath help

[Patrick McHardy]

Jarek Poplawski wrote:
> On Wed, Oct 29, 2008 at 11:17:09AM +0100, Patrick McHardy wrote:
>> The __vlan_hwaccel_rx function only does the device lookup and
>> stores it in the cb. The remaining processing is done in a new
>> function that is invoked by netif_receive_skb(), in the proper
>> context. Unfortunatly this needs vlan-specific handling in
>> netif_receive_skb().
>>
> 
> Unfortunately I'm not up-to-date with vlans and especially this
> nit_deliver problem, but here is a little doubt: since this is
> probably for stable, and skb->cb is rather tricky, I wonder why
> vlan_group_get_device() couldn't be used directly in
> vlan_hwaccel_do_receive()?

Because it needs the VLAN group, and that has to be passed
around somehow (=> skb->cb). So we might as well look up the
device immediately.

Its trivial to verify that the use of skb->cb is fine, the
only codepath besides the one leading to vlan_hwaccel_do_receive
immediately is through netif_rx.

> BTW: if we call netif_nit_deliver() from vlan_hwaccel_do_receive()
> from netif_receive_skb() this comment about bypassing looks a bit
> confusing to me:
> 
> /*
>  *     netif_nit_deliver - deliver received packets to network taps
>  *     @skb: buffer
>  *
>  *     This function is used to deliver incoming packets to network
>  *     taps. It should be used when the normal netif_receive_skb path
>  *     is bypassed, for example because of VLAN acceleration.
>  */

Agreed, this could be improved.

> As a matter of fact without this patch it's not so apparent why
> netif_receive_skb() can't happen after netif_nit_deliver() in
> __vlan_hwaccel_rx() too.

I don't understand what you're saying.

Re: atl1 warn_on_slowpath help

[Jarek Poplawski]

On Wed, Oct 29, 2008 at 02:09:08PM +0100, Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> On Wed, Oct 29, 2008 at 11:17:09AM +0100, Patrick McHardy wrote:
>>> The __vlan_hwaccel_rx function only does the device lookup and
>>> stores it in the cb. The remaining processing is done in a new
>>> function that is invoked by netif_receive_skb(), in the proper
>>> context. Unfortunatly this needs vlan-specific handling in
>>> netif_receive_skb().
>>>
>>
>> Unfortunately I'm not up-to-date with vlans and especially this
>> nit_deliver problem, but here is a little doubt: since this is
>> probably for stable, and skb->cb is rather tricky, I wonder why
>> vlan_group_get_device() couldn't be used directly in
>> vlan_hwaccel_do_receive()?
>
> Because it needs the VLAN group, and that has to be passed
> around somehow (=> skb->cb). So we might as well look up the
> device immediately.

Sure! I've missed this little detail...

>
> Its trivial to verify that the use of skb->cb is fine, the
> only codepath besides the one leading to vlan_hwaccel_do_receive
> immediately is through netif_rx.
>
>> BTW: if we call netif_nit_deliver() from vlan_hwaccel_do_receive()
>> from netif_receive_skb() this comment about bypassing looks a bit
>> confusing to me:
>>
>> /*
>>  *     netif_nit_deliver - deliver received packets to network taps
>>  *     @skb: buffer
>>  *
>>  *     This function is used to deliver incoming packets to network
>>  *     taps. It should be used when the normal netif_receive_skb path
>>  *     is bypassed, for example because of VLAN acceleration.
>>  */
>
> Agreed, this could be improved.
>
>> As a matter of fact without this patch it's not so apparent why
>> netif_receive_skb() can't happen after netif_nit_deliver() in
>> __vlan_hwaccel_rx() too.
>
> I don't understand what you're saying.

It's still about this bypassing: netif_receive_skb() can be called
after netif_nit_deliver().

Thanks,
Jarek P.

Re: atl1 warn_on_slowpath help

[Patrick McHardy]

Jarek Poplawski wrote:
> On Wed, Oct 29, 2008 at 02:09:08PM +0100, Patrick McHardy wrote:
>   
>>> As a matter of fact without this patch it's not so apparent why
>>> netif_receive_skb() can't happen after netif_nit_deliver() in
>>> __vlan_hwaccel_rx() too.
>>>       
>> I don't understand what you're saying.
>>     
>
> It's still about this bypassing: netif_receive_skb() can be called
> after netif_nit_deliver().
>   

I still don't follow - are you talking about the code with out
without this patch? In the later case, why should we call it
recursively without the need to do so?

Re: atl1 warn_on_slowpath help

[Jarek Poplawski]

On Wed, Oct 29, 2008 at 02:26:50PM +0100, Patrick McHardy wrote:
...
> I still don't follow - are you talking about the code with out
> without this patch? In the later case, why should we call it
> recursively without the need to do so?

I mean the current version (e.g. net-2.6). This comment reads that
netif_nit_deliver() is needed when we bypass netif_receive_skb().
But we call netif_receive_skb() from __vlan_hwaccel_rx(), and it's
not clear if some skbs are not tapped 2x.

Jarek P.

RE: atl1 warn_on_slowpath help

[Ramon Casellas]

> -----Mensaje original-----
> De: J. K. Cliburn [mailto:jcliburn@gmail.com]

> >> Crap, I didn't think of that, all drivers I tested with support
> >> NAPI. I can't think of a clean way to fix it right now, but I'll
> >> look into it.
> >
> > This is the best I could come up with, short of simply restoring
> > the old behaviour for non-polling drivers.
> >
> > The __vlan_hwaccel_rx function only does the device lookup and
> > stores it in the cb. The remaining processing is done in a new
> > function that is invoked by netif_receive_skb(), in the proper
> > context. Unfortunatly this needs vlan-specific handling in
> > netif_receive_skb().
> 
> Can you please try the attached patch from Patrick and see if it fixes
> your kernel warning?

All, 

I applied the patch to linux.2.6.27.2 (already patched for VLAN support on
ATL1 devices as per Jay fix). Patch applied cleanly and there was no warn on
slow path on dmesg after the reboot, with fully functional VLAN (broken in
atl cards since 2.6.26)

Linux failamp 2.6.27.2 #1 SMP Wed Oct 29 14:22:49 CET 2008 i686 GNU/Linux

3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state
UP qlen 1000
    link/ether 00:1f:c6:bb:75:fa brd ff:ff:ff:ff:ff:ff
    inet 1.1.1.91/24 brd 1.1.1.255 scope global eth0
    inet6 fe80::21f:c6ff:febb:75fa/64 scope link 
       valid_lft forever preferred_lft forever

4: eth0.200@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
state UP 
    link/ether 00:1f:c6:bb:75:fa brd ff:ff:ff:ff:ff:ff
    inet 10.1.1.91/24 brd 10.1.1.255 scope global eth0.200
    inet6 fe80::21f:c6ff:febb:75fa/64 scope link 
       valid_lft forever preferred_lft forever
5: eth0.300@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
state UP 
    link/ether 00:1f:c6:bb:75:fa brd ff:ff:ff:ff:ff:ff
    inet 192.168.100.91/24 brd 192.168.100.255 scope global eth0.300
    inet6 fe80::21f:c6ff:febb:75fa/64 scope link 
       valid_lft forever preferred_lft forever

I played a bit with tshark: 

failamp:/mnt# tshark -i eth0.300
Running as user "root" and group "root". This could be dangerous.
Capturing on eth0.300
  0.000000 Cisco_35:9a:11 -> PVST+        STP Conf. Root = 
33068/00:0e:84:50:ff:80  Cost = 12  Port = 0x8011

Thanks for your efforts. Let me know if you need further testing.

Ramon

(reboot)

[    1.916334] atl1 0000:02:00.0: PCI INT A -> GSI 17 (level, low) -> IRQ 17
[    1.916380] atl1 0000:02:00.0: setting latency timer to 64
[    1.916390] atl1 0000:02:00.0: version 2.1.3
[   10.602350] atl1 0000:02:00.0: eth0 link is up 100 Mbps full duplex
[   10.602389] atl1 0000:02:00.0: eth0 link is up 1000 Mbps full duplex
[   20.696004] eth0: no IPv6 routers present
[   21.680003] eth0.200: no IPv6 routers present
[   22.072004] eth0.300: no IPv6 routers present
[  859.560556] device eth0 left promiscuous mode
[  862.100013] device eth0.300 entered promiscuous mode
[  862.100061] device eth0 entered promiscuous mode
[  869.311133] device eth0.300 left promiscuous mode
[  869.311180] device eth0 left promiscuous mode

Re: atl1 warn_on_slowpath help

[Patrick McHardy]

Jarek Poplawski wrote:
> On Wed, Oct 29, 2008 at 02:26:50PM +0100, Patrick McHardy wrote:
> ...
>> I still don't follow - are you talking about the code with out
>> without this patch? In the later case, why should we call it
>> recursively without the need to do so?
> 
> I mean the current version (e.g. net-2.6). This comment reads that
> netif_nit_deliver() is needed when we bypass netif_receive_skb().
> But we call netif_receive_skb() from __vlan_hwaccel_rx(), and it's
> not clear if some skbs are not tapped 2x.

They could be, but in different states (device, vlan_tci, priority).
Bypassing refers to the first state of the skb.

Re: atl1 warn_on_slowpath help

[Patrick McHardy]

Ramon Casellas wrote:
>> -----Mensaje original-----
>> De: J. K. Cliburn [mailto:jcliburn@gmail.com]
> 
>> Can you please try the attached patch from Patrick and see if it fixes
>> your kernel warning?
> 
> I applied the patch to linux.2.6.27.2 (already patched for VLAN support on
> ATL1 devices as per Jay fix). Patch applied cleanly and there was no warn on
> slow path on dmesg after the reboot, with fully functional VLAN (broken in
> atl cards since 2.6.26)
> 
> Linux failamp 2.6.27.2 #1 SMP Wed Oct 29 14:22:49 CET 2008 i686 GNU/Linux
> 
> 3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state
> UP qlen 1000
>     link/ether 00:1f:c6:bb:75:fa brd ff:ff:ff:ff:ff:ff
>     inet 1.1.1.91/24 brd 1.1.1.255 scope global eth0
>     inet6 fe80::21f:c6ff:febb:75fa/64 scope link 
>        valid_lft forever preferred_lft forever
> 
> 4: eth0.200@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
> state UP 
>     link/ether 00:1f:c6:bb:75:fa brd ff:ff:ff:ff:ff:ff
>     inet 10.1.1.91/24 brd 10.1.1.255 scope global eth0.200
>     inet6 fe80::21f:c6ff:febb:75fa/64 scope link 
>        valid_lft forever preferred_lft forever
> 5: eth0.300@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
> state UP 
>     link/ether 00:1f:c6:bb:75:fa brd ff:ff:ff:ff:ff:ff
>     inet 192.168.100.91/24 brd 192.168.100.255 scope global eth0.300
>     inet6 fe80::21f:c6ff:febb:75fa/64 scope link 
>        valid_lft forever preferred_lft forever
> 
> I played a bit with tshark: 
> 
> failamp:/mnt# tshark -i eth0.300
> Running as user "root" and group "root". This could be dangerous.
> Capturing on eth0.300
>   0.000000 Cisco_35:9a:11 -> PVST+        STP Conf. Root = 
> 33068/00:0e:84:50:ff:80  Cost = 12  Port = 0x8011
> 
> Thanks for your efforts. Let me know if you need further testing.

This seems to be enough, thanks a lot.