Patchwork atl1 warn_on_slowpath help

login
register
mail settings
Submitter J. K. Cliburn
Date Oct. 29, 2008, 12:51 p.m.
Message ID <3400f2f60810290551y39c61e0aj90ca1222b339798c@mail.gmail.com>
Download mbox | patch
Permalink /patch/6247/
State Superseded
Delegated to: David Miller
Headers show

Comments

J. K. Cliburn - Oct. 29, 2008, 12:51 p.m.
[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
Patrick McHardy - Oct. 29, 2008, 12:54 p.m.
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.


--
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
Ramon Casellas - Oct. 29, 2008, 2:15 p.m.
> -----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

--
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
Patrick McHardy - Oct. 29, 2008, 4:47 p.m.
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.

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