Skip to contentSkip to conversation

ppp_generic: fix lockdep warning

PATCH Applied [email protected] 5 participants 21 messages May 11, 2007 – May 16, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 4/13 ppp_generic: fix lockdep warning

From: Jarek Poplawski <[email protected]>

Quoted 117 lines

=======================================================
[ INFO: possible circular locking dependency detected ] 2.6.21-rc4 #1
-------------------------------------------------------
pppd/8926 is trying to acquire lock:
(&vlan_netdev_xmit_lock_key){-...}, at: [<c0265486>] dev_queue_xmit+0x247/0x2f1

but task is already holding lock:
(&pch->downl){-+..}, at: [<c0230c72>] ppp_channel_push+0x19/0x9a

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 (&pch->downl){-+..}:

      [<c013642b>] __lock_acquire+0xe62/0x1010
      [<c0136642>] lock_acquire+0x69/0x83
      [<c02afc13>] _spin_lock_bh+0x30/0x3d
      [<c022f715>] ppp_push+0x5a/0x9a
      [<c022fb40>] ppp_xmit_process+0x2e/0x511
      [<c0231a05>] ppp_write+0xb8/0xf2
      [<c015ec26>] vfs_write+0x7f/0xba
      [<c015f158>] sys_write+0x3d/0x64
      [<c01027de>] sysenter_past_esp+0x5f/0x99
      [<ffffffff>] 0xffffffff

-> #2 (&ppp->wlock){-+..}:

      [<c013642b>] __lock_acquire+0xe62/0x1010
      [<c0136642>] lock_acquire+0x69/0x83
      [<c02afc13>] _spin_lock_bh+0x30/0x3d
      [<c022fb2b>] ppp_xmit_process+0x19/0x511
      [<c02318d3>] ppp_start_xmit+0x18a/0x204
      [<c0263a6f>] dev_hard_start_xmit+0x1f6/0x2c4
      [<c026ded3>] __qdisc_run+0x81/0x1bc
      [<c026549e>] dev_queue_xmit+0x25f/0x2f1
      [<c027c75f>] ip_output+0x1be/0x25f
      [<c02788ce>] ip_forward+0x159/0x22b
      [<c027745c>] ip_rcv+0x297/0x4dd
      [<c0263698>] netif_receive_skb+0x164/0x1f2
      [<c022199d>] e1000_clean_rx_irq+0x12a/0x4b7
      [<c02209bc>] e1000_clean+0x3ff/0x5dd
      [<c0265084>] net_rx_action+0x7d/0x12b
      [<c011e442>] __do_softirq+0x82/0xf2
      [<c011e509>] do_softirq+0x57/0x59
      [<c011e877>] irq_exit+0x7f/0x81
      [<c0105011>] do_IRQ+0x45/0x84
      [<c0103252>] common_interrupt+0x2e/0x34
      [<c0100b66>] mwait_idle+0x12/0x14
      [<c0100c60>] cpu_idle+0x6c/0x86
      [<c01001cd>] rest_init+0x23/0x36
      [<c0377d89>] start_kernel+0x3ca/0x461
      [<00000000>] 0x0
      [<ffffffff>] 0xffffffff

-> #1 (&dev->_xmit_lock){-+..}:

      [<c013642b>] __lock_acquire+0xe62/0x1010
      [<c0136642>] lock_acquire+0x69/0x83
      [<c02afc13>] _spin_lock_bh+0x30/0x3d
      [<c0266861>] dev_mc_add+0x34/0x16a
      [<c02ab5c7>] vlan_dev_set_multicast_list+0x88/0x25c
      [<c0266592>] __dev_mc_upload+0x22/0x24
      [<c0266914>] dev_mc_add+0xe7/0x16a
      [<c029f323>] igmp_group_added+0xe6/0xeb
      [<c029f50b>] ip_mc_inc_group+0x13f/0x210
      [<c029f5fa>] ip_mc_up+0x1e/0x61
      [<c029ab81>] inetdev_event+0x154/0x2c7
      [<c0125a46>] notifier_call_chain+0x2c/0x39
      [<c0125a7c>] raw_notifier_call_chain+0x8/0xa
      [<c026477a>] dev_open+0x6d/0x71
      [<c0263028>] dev_change_flags+0x51/0x101
      [<c029b7ca>] devinet_ioctl+0x4df/0x644
      [<c029bc03>] inet_ioctl+0x5c/0x6f
      [<c02596e0>] sock_ioctl+0x4f/0x1e8
      [<c0168c32>] do_ioctl+0x22/0x71
      [<c0168cd6>] vfs_ioctl+0x55/0x27e
      [<c0168f32>] sys_ioctl+0x33/0x51
      [<c01027de>] sysenter_past_esp+0x5f/0x99
      [<ffffffff>] 0xffffffff

-> #0 (&vlan_netdev_xmit_lock_key){-...}:

      [<c0136289>] __lock_acquire+0xcc0/0x1010
      [<c0136642>] lock_acquire+0x69/0x83
      [<c02afbd6>] _spin_lock+0x2b/0x38
      [<c0265486>] dev_queue_xmit+0x247/0x2f1
      [<c02334f6>] __pppoe_xmit+0x1a9/0x215
      [<c023356c>] pppoe_xmit+0xa/0xc
      [<c0230c9a>] ppp_channel_push+0x41/0x9a
      [<c0231a13>] ppp_write+0xc6/0xf2
      [<c015ec26>] vfs_write+0x7f/0xba
      [<c015f158>] sys_write+0x3d/0x64
      [<c01027de>] sysenter_past_esp+0x5f/0x99
      [<ffffffff>] 0xffffffff

other info that might help us debug this:

1 lock held by pppd/8926:
#0: (&pch->downl){-+..}, at: [<c0230c72>] ppp_channel_push+0x19/0x9a

stack backtrace:
[<c0103834>] show_trace_log_lvl+0x1a/0x30
[<c0103f16>] show_trace+0x12/0x14
[<c0103f9d>] dump_stack+0x16/0x18
[<c01343cd>] print_circular_bug_tail+0x68/0x71
[<c0136289>] __lock_acquire+0xcc0/0x1010
[<c0136642>] lock_acquire+0x69/0x83
[<c02afbd6>] _spin_lock+0x2b/0x38
[<c0265486>] dev_queue_xmit+0x247/0x2f1
[<c02334f6>] __pppoe_xmit+0x1a9/0x215
[<c023356c>] pppoe_xmit+0xa/0xc
[<c0230c9a>] ppp_channel_push+0x41/0x9a
[<c0231a13>] ppp_write+0xc6/0xf2
[<c015ec26>] vfs_write+0x7f/0xba
[<c015f158>] sys_write+0x3d/0x64
[<c01027de>] sysenter_past_esp+0x5f/0x99
=======================
Quoted 2 lines

Clocksource tsc unstable (delta = 4686844667 ns)
Time: acpi_pm clocksource has been installed.

...

lockdep has seen locks "-> #0" - "-> #3" taken in circular order, but IMHO, lock "-> #3" (&pch->downl) taken after "-> #2" (&ppp->wlock) differs from &pch->downl lock taken in "-> #0" (before &vlan_netdev_xmit_lock_key) and lockdep should be notified about this.

Reported & tested by: "Yuriy N. Shkandybin" <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>
Cc Paul Mackerras <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 drivers/net/ppp_generic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/ppp_generic.c renamed from drivers/net/ppp_generic.c~ppp_generic-fix-lockdep-warning +4 -2
diff -puN drivers/net/ppp_generic.c~ppp_generic-fix-lockdep-warning drivers/net/ppp_generic.c
@@ -1432,7 +1432,8 @@ ppp_channel_push(struct channel *pch)
 	struct sk_buff *skb;
 	struct ppp *ppp;
 
-	spin_lock_bh(&pch->downl);
+	local_bh_disable();
+	spin_lock_nested(&pch->downl, SINGLE_DEPTH_NESTING);
 	if (pch->chan != 0) {
 		while (!skb_queue_empty(&pch->file.xq)) {
 			skb = skb_dequeue(&pch->file.xq);

@@ -1446,7 +1447,8 @@ ppp_channel_push(struct channel *pch)
 		/* channel got deregistered */
 		skb_queue_purge(&pch->file.xq);
 	}
-	spin_unlock_bh(&pch->downl);
+	spin_unlock(&pch->downl);
+	local_bh_enable();
 	/* see if there is anything from the attached unit to be sent */
 	if (skb_queue_empty(&pch->file.xq)) {
 		read_lock_bh(&pch->upl);

_

maintainer
#2 lore raw

From: Jeff Garzik <[email protected]>
Date: Fri, 11 May 2007 16:57:19 -0400

Quoted 1 line

applied

I was under the impression that this patch didn't actually fix the problem yet? I might be thinking about something else...

author
#3 lore raw
Quoted 7 lines · On Fri, 11 May 2007 14:03:09 -0700 (PDT) David Miller <[email protected]>…

From: Jeff Garzik <[email protected]>
Date: Fri, 11 May 2007 16:57:19 -0400

Quoted 1 line

applied

I was under the impression that this patch didn't actually fix the problem yet? I might be thinking about something else...

yeah, sorry, it seems that the discussion is ongoing. Please drop the patch. I did.

#4 lore raw
Quoted 6 lines · Andrew Morton
Quoted 3 lines · On Fri, 11 May 2007 14:03:09 -0700 (PDT) David Miller <[email protected]>…

From: Jeff Garzik <[email protected]>
Date: Fri, 11 May 2007 16:57:19 -0400

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

applied

I was under the impression that this patch didn't actually fix the problem yet? I might be thinking about something else...

yeah, sorry, it seems that the discussion is ongoing. Please drop the patch. I did.

After sending this patch I was a little confused, when next lockdep warning report appeared, and I thought - since this is not enough, this patch could be dumped. But now I changed my
mind: there are really many possibilities of strange connections between locks taken from vlans, ppp (with pppoe), multicasts etc.
- that every one possibility less is a gain here.

As I wrote earlier, one of main reasons of such a situation is calling dev_queue_xmit() in a nested way, with xmit locks of previous layers held (as e.g. pppoe after ppp_generic). On the other hand, changing this may be not enough to, because some locks are seen by lockdep as one: e.g. eth & ppp netdevs, or vlan's lock on eth vs. vlan's lock on ppp.

So, now I think this patch should stay (it simply says the truth to lockdep - there are two, functionally independent kinds of ppp channels with independent locks), because it does no harm (AFAIK), and there is really at least one type of lockdep report less.

I also think that my two next patches (for vlan and ppp_generic), are useful and should be applied even, if they are not enough with this, quite complex configuration.

Of course, later, if somebody will find better solution, they could be removed, but now, by not using them, some real locking problems may be hidden because of the way lockdep works (or stops to work), and there is no reason to frighten users with these current, false warnings too.

Regards,
Jarek P.

maintainer
#5 lore raw

From: Jarek Poplawski <[email protected]>
Date: Mon, 14 May 2007 08:07:00 +0200

Quoted 6 lines

After sending this patch I was a little confused, when next lockdep warning report appeared, and I thought - since this is not enough, this patch could be dumped. But now I changed my
mind: there are really many possibilities of strange connections between locks taken from vlans, ppp (with pppoe), multicasts etc.
- that every one possibility less is a gain here.

...

Quoted 2 lines

Of course, later, if somebody will find better solution, they could be removed,

I already suggested a better fix, you ignored it.

For each unique netdev type, use a different locking class.

That will fix this forever, anything else is a situation specific band-aid (but then again isn't that what every lockdep annotation is :-).

#6 lore raw
Quoted 9 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Mon, 14 May 2007 08:07:00 +0200

Quoted 6 lines

After sending this patch I was a little confused, when next lockdep warning report appeared, and I thought - since this is not enough, this patch could be dumped. But now I changed my
mind: there are really many possibilities of strange connections between locks taken from vlans, ppp (with pppoe), multicasts etc.
- that every one possibility less is a gain here.

Quoted 5 lines

...

Quoted 2 lines

Of course, later, if somebody will find better solution, they could be removed,

I already suggested a better fix, you ignored it.

No, I couldn't have ignored any of your suggestions (I would've written about any doubts, anyway). I simply misunderstood! I thought you mean different classes for netdevs used in vlan, and I prepared such a patch... Sorry!

Quoted 6 lines

For each unique netdev type, use a different locking class.

That will fix this forever, anything else is a situation specific band-aid (but then again isn't that what every lockdep annotation is :-).

Yes, this is very good idea, and I wonder, why you didn't try this yourself (after my "ignore"). I thought a little about this, but was afraid of it's wide range. Some things - like in vlans - should be removed then, for this to work. I'll try to send something like this soon (but I'm not so optimistic it will cure all or forever...).

Regards,
Jarek P.

#7 lore raw
Quoted 1 line · Jarek Poplawski

On Sun, May 13, 2007 at 11:39:37PM -0700, David Miller wrote:

...

Quoted 5 lines
Quoted 5 lines

For each unique netdev type, use a different locking class.

That will fix this forever, anything else is a situation specific band-aid (but then again isn't that what every lockdep annotation is :-).

Band-aid isn't probably too fair with lockdep. I think, it's very similar as declaring types of variables for a compiler, it really can't know until we tell this. And current locks' complexity is probably beyond possibility of brain analyzing, anyway. (Probably lockdep could be wiser too - at the cost of memory and speed - if each lock were treated individually).

Quoted 7 lines

Yes, this is very good idea, and I wonder, why you didn't try this yourself (after my "ignore"). I thought a little about this, but was afraid of it's wide range. Some things - like in vlans - should be removed then, for this to work. I'll try to send something like this soon (but I'm not so optimistic it will cure all or forever...).

So, because of this next planned patch (I hope not later than tomorrow), my two last patches for vlan and ppp_generic shouldn't be applied - their functionality will be moved to register_netdevice. (But I think this current: "nesting" patch for ppp_generic does something different and IMHO could be useful too.)

Jarek P.

#8 lore raw
Quoted 8 lines · Jarek Poplawski
Quoted 1 line · Jarek Poplawski

On Sun, May 13, 2007 at 11:39:37PM -0700, David Miller wrote:

...

Quoted 5 lines
Quoted 5 lines

For each unique netdev type, use a different locking class.

That will fix this forever, anything else is a situation specific band-aid (but then again isn't that what every lockdep annotation is :-).

...

Quoted 10 lines
Quoted 6 lines

Yes, this is very good idea, and I wonder, why you didn't try this yourself (after my "ignore"). I thought a little about this, but was afraid of it's wide range. Some things - like in vlans - should be removed then, for this to work. I'll try to send something like this soon (but I'm not so optimistic it will cure all or forever...).

So, because of this next planned patch (I hope not later than tomorrow), my two last patches for vlan and ppp_generic shouldn't be applied - their functionality will be moved to register_netdevice.

...or maybe not so easy... Here is the first problem: vlan does register_netdevice(new_dev) with new_dev->type == real_dev->type, so it still couldn't be recognized as something different from the real ppp or eth. We need some trick... Probably we could also think about using dev->name (or both), but there always would be a problem with the size of a static table reserved for this.

So, now it seems something like my patch with lockdep class for ppp could be settled with this, but more complex situations like vlan still need special treatment.

Maybe we should also care not to go too far with this: I didn't read other such reports recently, but here the problems seem to be made mainly by some virtual devices without queues...

Jarek P.

maintainer
#9 lore raw
In reply to Jarek Poplawski

From: Jarek Poplawski <[email protected]>
Date: Mon, 14 May 2007 09:28:45 +0200

Quoted 2 lines

Yes, this is very good idea, and I wonder, why you didn't try this yourself (after my "ignore").

Because you are a skilled programmer and you might find some flaw in my suggestion :-)

#10 lore raw
Quoted 9 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Mon, 14 May 2007 09:28:45 +0200

Quoted 2 lines

Yes, this is very good idea, and I wonder, why you didn't try this yourself (after my "ignore").

Because you are a skilled programmer and you might find some flaw in my suggestion :-)

Yes, here are some flaws:

if (Dave_M_was_ignored)

	goto not_skilled_programmer;

if (Jarek_P_finds_some_flow)

	goto message_sent;

else
goto message_sent; /* probably the same */

Cheers,
Jarek P.

#11 lore raw

PATCH 1/1 netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

In reply to David Miller
Quoted 13 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Mon, 14 May 2007 08:07:00 +0200

Quoted 6 lines

After sending this patch I was a little confused, when next lockdep warning report appeared, and I thought - since this is not enough, this patch could be dumped. But now I changed my
mind: there are really many possibilities of strange connections between locks taken from vlans, ppp (with pppoe), multicasts etc.
- that every one possibility less is a gain here.

...

Quoted 2 lines

Of course, later, if somebody will find better solution, they could be removed,

Quoted 7 lines

I already suggested a better fix, you ignored it.

For each unique netdev type, use a different locking class.

That will fix this forever, anything else is a situation specific band-aid (but then again isn't that what every lockdep annotation is :-).

So, I guess, you thought about something like this, plus additional annotations in specific situations like vlan (but some hint is needed, how much of this should be considered).

Jarek P.

--->

After initializing dev->_xmit_lock register_netdevice() sets lockdep class according to dev->type.

Idea of this patch - by David Miller.

Signed-off-by Jarek Poplawski <[email protected]>

---

net/core/dev.c +69 -0
diff -Nurp 2.6.22-/net/core/dev.c 2.6.22/net/core/dev.c
@@ -116,6 +116,7 @@
 #include <linux/dmaengine.h>
 #include <linux/err.h>
 #include <linux/ctype.h>
+#include <linux/if_arp.h>
 
 /*
  *	The list of packet types we will receive (as opposed to discard)

@@ -217,6 +218,73 @@ extern void netdev_unregister_sysfs(stru
 #define	netdev_unregister_sysfs(dev)	do { } while(0)
 #endif
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+/*
+ * register_netdevice() inits dev->_xmit_lock and sets lockdep class
+ * according to dev->type
+ */
+static const unsigned short netdev_lock_type[] =
+	{ARPHRD_NETROM, ARPHRD_ETHER, ARPHRD_EETHER, ARPHRD_AX25,
+	 ARPHRD_PRONET, ARPHRD_CHAOS, ARPHRD_IEEE802, ARPHRD_ARCNET,
+	 ARPHRD_APPLETLK, ARPHRD_DLCI, ARPHRD_ATM, ARPHRD_METRICOM,
+	 ARPHRD_IEEE1394, ARPHRD_EUI64, ARPHRD_INFINIBAND, ARPHRD_SLIP,
+	 ARPHRD_CSLIP, ARPHRD_SLIP6, ARPHRD_CSLIP6, ARPHRD_RSRVD,
+	 ARPHRD_ADAPT, ARPHRD_ROSE, ARPHRD_X25, ARPHRD_HWX25,
+	 ARPHRD_PPP, ARPHRD_CISCO, ARPHRD_LAPB, ARPHRD_DDCMP,
+	 ARPHRD_RAWHDLC, ARPHRD_TUNNEL, ARPHRD_TUNNEL6, ARPHRD_FRAD,
+	 ARPHRD_SKIP, ARPHRD_LOOPBACK, ARPHRD_LOCALTLK, ARPHRD_FDDI,
+	 ARPHRD_BIF, ARPHRD_SIT, ARPHRD_IPDDP, ARPHRD_IPGRE,
+	 ARPHRD_PIMREG, ARPHRD_HIPPI, ARPHRD_ASH, ARPHRD_ECONET,
+	 ARPHRD_IRDA, ARPHRD_FCPP, ARPHRD_FCAL, ARPHRD_FCPL,
+	 ARPHRD_FCFABRIC, ARPHRD_IEEE802_TR, ARPHRD_IEEE80211,
+	 ARPHRD_IEEE80211_PRISM, ARPHRD_IEEE80211_RADIOTAP, ARPHRD_VOID,
+	 ARPHRD_NONE};
+
+static const char *netdev_lock_name[] =
+	{"_xmit_NETROM", "_xmit_ETHER", "_xmit_EETHER", "_xmit_AX25",
+	 "_xmit_PRONET", "_xmit_CHAOS", "_xmit_IEEE802", "_xmit_ARCNET",
+	 "_xmit_APPLETLK", "_xmit_DLCI", "_xmit_ATM", "_xmit_METRICOM",
+	 "_xmit_IEEE1394", "_xmit_EUI64", "_xmit_INFINIBAND", "_xmit_SLIP",
+	 "_xmit_CSLIP", "_xmit_SLIP6", "_xmit_CSLIP6", "_xmit_RSRVD",
+	 "_xmit_ADAPT", "_xmit_ROSE", "_xmit_X25", "_xmit_HWX25",
+	 "_xmit_PPP", "_xmit_CISCO", "_xmit_LAPB", "_xmit_DDCMP",
+	 "_xmit_RAWHDLC", "_xmit_TUNNEL", "_xmit_TUNNEL6", "_xmit_FRAD",
+	 "_xmit_SKIP", "_xmit_LOOPBACK", "_xmit_LOCALTLK", "_xmit_FDDI",
+	 "_xmit_BIF", "_xmit_SIT", "_xmit_IPDDP", "_xmit_IPGRE",
+	 "_xmit_PIMREG", "_xmit_HIPPI", "_xmit_ASH", "_xmit_ECONET",
+	 "_xmit_IRDA", "_xmit_FCPP", "_xmit_FCAL", "_xmit_FCPL",
+	 "_xmit_FCFABRIC", "_xmit_IEEE802_TR", "_xmit_IEEE80211",
+	 "_xmit_IEEE80211_PRISM", "_xmit_IEEE80211_RADIOTAP", "_xmit_VOID",
+	 "_xmit_NONE"};
+
+static struct lock_class_key netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
+
+static inline unsigned short netdev_lock_pos(unsigned short dev_type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(netdev_lock_type); i++)
+		if (netdev_lock_type[i] == dev_type)
+			return i;
+	/* the last key is used by default */
+	return --i;
+}
+
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+					    unsigned short dev_type)
+{
+	int i;
+
+	i = netdev_lock_pos(dev_type);
+	lockdep_set_class_and_name(lock, &netdev_xmit_lock_key[i],
+				   netdev_lock_name[i]);
+}
+#else
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+					    unsigned short dev_type)
+{
+}
+#endif
 
 /*******************************************************************************
 

@@ -3001,6 +3069,7 @@ int register_netdevice(struct net_device
 
 	spin_lock_init(&dev->queue_lock);
 	spin_lock_init(&dev->_xmit_lock);
+	netdev_set_lockdep_class(&dev->_xmit_lock, dev->type);
 	dev->xmit_lock_owner = -1;
 	spin_lock_init(&dev->ingress_lock);
#12 lore raw

Re: [PATCH] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

I've patched 2.6.22-rc1 and there was no warnings from lock debugger.

Jura

----- Original Message ----- From: "Jarek Poplawski" <[email protected]>
To: "David Miller" <[email protected]>
Cc: <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Tuesday, May 15, 2007 9:31 AM
Subject: [PATCH] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

Quoted 136 lines · David Miller
Quoted 13 lines

From: Jarek Poplawski <[email protected]>
Date: Mon, 14 May 2007 08:07:00 +0200

Quoted 6 lines

After sending this patch I was a little confused, when next lockdep warning report appeared, and I thought - since this is not enough, this patch could be dumped. But now I changed my
mind: there are really many possibilities of strange connections between locks taken from vlans, ppp (with pppoe), multicasts etc.
- that every one possibility less is a gain here.

...

Quoted 2 lines

Of course, later, if somebody will find better solution, they could be removed,

Quoted 7 lines

I already suggested a better fix, you ignored it.

For each unique netdev type, use a different locking class.

That will fix this forever, anything else is a situation specific band-aid (but then again isn't that what every lockdep annotation is :-).

So, I guess, you thought about something like this, plus additional annotations in specific situations like vlan (but some hint is needed, how much of this should be considered).

Jarek P.

--->

After initializing dev->_xmit_lock register_netdevice() sets lockdep class according to dev->type.

Idea of this patch - by David Miller.

Signed-off-by Jarek Poplawski <[email protected]>

---

diff -Nurp 2.6.22-/net/core/dev.c 2.6.22/net/core/dev.c

net/core/dev.c 2007-05-14 21:22:10.000000000 +0200 renamed from net/core/dev.c 2007-05-14 20:26:16.000000000 +0200 +69 -0
@@ -116,6 +116,7 @@
 #include <linux/dmaengine.h>
 #include <linux/err.h>
 #include <linux/ctype.h>
+#include <linux/if_arp.h>
 
 /*
 * The list of packet types we will receive (as opposed to discard)

@@ -217,6 +218,73 @@ extern void netdev_unregister_sysfs(stru
 #define netdev_unregister_sysfs(dev) do { } while(0)
 #endif
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+/*
+ * register_netdevice() inits dev->_xmit_lock and sets lockdep class
+ * according to dev->type
+ */
+static const unsigned short netdev_lock_type[] =
+ {ARPHRD_NETROM, ARPHRD_ETHER, ARPHRD_EETHER, ARPHRD_AX25,
+ ARPHRD_PRONET, ARPHRD_CHAOS, ARPHRD_IEEE802, ARPHRD_ARCNET,
+ ARPHRD_APPLETLK, ARPHRD_DLCI, ARPHRD_ATM, ARPHRD_METRICOM,
+ ARPHRD_IEEE1394, ARPHRD_EUI64, ARPHRD_INFINIBAND, ARPHRD_SLIP,
+ ARPHRD_CSLIP, ARPHRD_SLIP6, ARPHRD_CSLIP6, ARPHRD_RSRVD,
+ ARPHRD_ADAPT, ARPHRD_ROSE, ARPHRD_X25, ARPHRD_HWX25,
+ ARPHRD_PPP, ARPHRD_CISCO, ARPHRD_LAPB, ARPHRD_DDCMP,
+ ARPHRD_RAWHDLC, ARPHRD_TUNNEL, ARPHRD_TUNNEL6, ARPHRD_FRAD,
+ ARPHRD_SKIP, ARPHRD_LOOPBACK, ARPHRD_LOCALTLK, ARPHRD_FDDI,
+ ARPHRD_BIF, ARPHRD_SIT, ARPHRD_IPDDP, ARPHRD_IPGRE,
+ ARPHRD_PIMREG, ARPHRD_HIPPI, ARPHRD_ASH, ARPHRD_ECONET,
+ ARPHRD_IRDA, ARPHRD_FCPP, ARPHRD_FCAL, ARPHRD_FCPL,
+ ARPHRD_FCFABRIC, ARPHRD_IEEE802_TR, ARPHRD_IEEE80211,
+ ARPHRD_IEEE80211_PRISM, ARPHRD_IEEE80211_RADIOTAP, ARPHRD_VOID,
+ ARPHRD_NONE};
+
+static const char *netdev_lock_name[] =
+ {"_xmit_NETROM", "_xmit_ETHER", "_xmit_EETHER", "_xmit_AX25",
+ "_xmit_PRONET", "_xmit_CHAOS", "_xmit_IEEE802", "_xmit_ARCNET",
+ "_xmit_APPLETLK", "_xmit_DLCI", "_xmit_ATM", "_xmit_METRICOM",
+ "_xmit_IEEE1394", "_xmit_EUI64", "_xmit_INFINIBAND", "_xmit_SLIP",
+ "_xmit_CSLIP", "_xmit_SLIP6", "_xmit_CSLIP6", "_xmit_RSRVD",
+ "_xmit_ADAPT", "_xmit_ROSE", "_xmit_X25", "_xmit_HWX25",
+ "_xmit_PPP", "_xmit_CISCO", "_xmit_LAPB", "_xmit_DDCMP",
+ "_xmit_RAWHDLC", "_xmit_TUNNEL", "_xmit_TUNNEL6", "_xmit_FRAD",
+ "_xmit_SKIP", "_xmit_LOOPBACK", "_xmit_LOCALTLK", "_xmit_FDDI",
+ "_xmit_BIF", "_xmit_SIT", "_xmit_IPDDP", "_xmit_IPGRE",
+ "_xmit_PIMREG", "_xmit_HIPPI", "_xmit_ASH", "_xmit_ECONET",
+ "_xmit_IRDA", "_xmit_FCPP", "_xmit_FCAL", "_xmit_FCPL",
+ "_xmit_FCFABRIC", "_xmit_IEEE802_TR", "_xmit_IEEE80211",
+ "_xmit_IEEE80211_PRISM", "_xmit_IEEE80211_RADIOTAP", "_xmit_VOID",
+ "_xmit_NONE"};
+
+static struct lock_class_key 
 netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
+
+static inline unsigned short netdev_lock_pos(unsigned short dev_type)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(netdev_lock_type); i++)
+ if (netdev_lock_type[i] == dev_type)
+ return i;
+ /* the last key is used by default */
+ return --i;
+}
+
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+     unsigned short dev_type)
+{
+ int i;
+
+ i = netdev_lock_pos(dev_type);
+ lockdep_set_class_and_name(lock, &netdev_xmit_lock_key[i],
+    netdev_lock_name[i]);
+}
+#else
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+     unsigned short dev_type)
+{
+}
+#endif
 
 /*******************************************************************************

@@ -3001,6 +3069,7 @@ int register_netdevice(struct net_device
 
 spin_lock_init(&dev->queue_lock);
 spin_lock_init(&dev->_xmit_lock);
+ netdev_set_lockdep_class(&dev->_xmit_lock, dev->type);
 dev->xmit_lock_owner = -1;
 spin_lock_init(&dev->ingress_lock);
#13 lore raw

Re: [PATCH] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

Quoted 3 lines · Yuriy N. Shkandybin

I've patched 2.6.22-rc1 and there was no warnings from lock debugger.

Jura

Many thanks, Jura!

It seems reality is sometimes merciful...

On the other hand I wonder, how all this could stay so long:
a configuration similar to yours should be quite common. It looks, some people probably don't care, and here your help is even more precious!

Best regards,
Jarek P.

#14 lore raw

Re: [PATCH] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

In reply to Yuriy N. Shkandybin
Quoted 2 lines · Yuriy N. Shkandybin

I've patched 2.6.22-rc1 and there was no warnings from lock debugger.

So, you mean only this one patch - without previous vlan patch? Very interesting...

Thanks once more,
Jarek P.

#15 lore raw

Re: [PATCH] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

In reply to Jarek Poplawski

I've patched 2.6.22-rc1 and there was no warnings from lock debugger.

Jura

----- Original Message ----- From: "Jarek Poplawski" <[email protected]>
To: "David Miller" <[email protected]>
Cc: <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Tuesday, May 15, 2007 9:31 AM
Subject: [PATCH] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

Quoted 136 lines · David Miller
Quoted 13 lines

From: Jarek Poplawski <[email protected]>
Date: Mon, 14 May 2007 08:07:00 +0200

Quoted 6 lines

After sending this patch I was a little confused, when next lockdep warning report appeared, and I thought - since this is not enough, this patch could be dumped. But now I changed my
mind: there are really many possibilities of strange connections between locks taken from vlans, ppp (with pppoe), multicasts etc.
- that every one possibility less is a gain here.

...

Quoted 2 lines

Of course, later, if somebody will find better solution, they could be removed,

Quoted 7 lines

I already suggested a better fix, you ignored it.

For each unique netdev type, use a different locking class.

That will fix this forever, anything else is a situation specific band-aid (but then again isn't that what every lockdep annotation is :-).

So, I guess, you thought about something like this, plus additional annotations in specific situations like vlan (but some hint is needed, how much of this should be considered).

Jarek P.

--->

After initializing dev->_xmit_lock register_netdevice() sets lockdep class according to dev->type.

Idea of this patch - by David Miller.

Signed-off-by Jarek Poplawski <[email protected]>

---

diff -Nurp 2.6.22-/net/core/dev.c 2.6.22/net/core/dev.c

net/core/dev.c 2007-05-14 21:22:10.000000000 +0200 renamed from net/core/dev.c 2007-05-14 20:26:16.000000000 +0200 +69 -0
@@ -116,6 +116,7 @@
 #include <linux/dmaengine.h>
 #include <linux/err.h>
 #include <linux/ctype.h>
+#include <linux/if_arp.h>
 
 /*
 * The list of packet types we will receive (as opposed to discard)

@@ -217,6 +218,73 @@ extern void netdev_unregister_sysfs(stru
 #define netdev_unregister_sysfs(dev) do { } while(0)
 #endif
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+/*
+ * register_netdevice() inits dev->_xmit_lock and sets lockdep class
+ * according to dev->type
+ */
+static const unsigned short netdev_lock_type[] =
+ {ARPHRD_NETROM, ARPHRD_ETHER, ARPHRD_EETHER, ARPHRD_AX25,
+ ARPHRD_PRONET, ARPHRD_CHAOS, ARPHRD_IEEE802, ARPHRD_ARCNET,
+ ARPHRD_APPLETLK, ARPHRD_DLCI, ARPHRD_ATM, ARPHRD_METRICOM,
+ ARPHRD_IEEE1394, ARPHRD_EUI64, ARPHRD_INFINIBAND, ARPHRD_SLIP,
+ ARPHRD_CSLIP, ARPHRD_SLIP6, ARPHRD_CSLIP6, ARPHRD_RSRVD,
+ ARPHRD_ADAPT, ARPHRD_ROSE, ARPHRD_X25, ARPHRD_HWX25,
+ ARPHRD_PPP, ARPHRD_CISCO, ARPHRD_LAPB, ARPHRD_DDCMP,
+ ARPHRD_RAWHDLC, ARPHRD_TUNNEL, ARPHRD_TUNNEL6, ARPHRD_FRAD,
+ ARPHRD_SKIP, ARPHRD_LOOPBACK, ARPHRD_LOCALTLK, ARPHRD_FDDI,
+ ARPHRD_BIF, ARPHRD_SIT, ARPHRD_IPDDP, ARPHRD_IPGRE,
+ ARPHRD_PIMREG, ARPHRD_HIPPI, ARPHRD_ASH, ARPHRD_ECONET,
+ ARPHRD_IRDA, ARPHRD_FCPP, ARPHRD_FCAL, ARPHRD_FCPL,
+ ARPHRD_FCFABRIC, ARPHRD_IEEE802_TR, ARPHRD_IEEE80211,
+ ARPHRD_IEEE80211_PRISM, ARPHRD_IEEE80211_RADIOTAP, ARPHRD_VOID,
+ ARPHRD_NONE};
+
+static const char *netdev_lock_name[] =
+ {"_xmit_NETROM", "_xmit_ETHER", "_xmit_EETHER", "_xmit_AX25",
+ "_xmit_PRONET", "_xmit_CHAOS", "_xmit_IEEE802", "_xmit_ARCNET",
+ "_xmit_APPLETLK", "_xmit_DLCI", "_xmit_ATM", "_xmit_METRICOM",
+ "_xmit_IEEE1394", "_xmit_EUI64", "_xmit_INFINIBAND", "_xmit_SLIP",
+ "_xmit_CSLIP", "_xmit_SLIP6", "_xmit_CSLIP6", "_xmit_RSRVD",
+ "_xmit_ADAPT", "_xmit_ROSE", "_xmit_X25", "_xmit_HWX25",
+ "_xmit_PPP", "_xmit_CISCO", "_xmit_LAPB", "_xmit_DDCMP",
+ "_xmit_RAWHDLC", "_xmit_TUNNEL", "_xmit_TUNNEL6", "_xmit_FRAD",
+ "_xmit_SKIP", "_xmit_LOOPBACK", "_xmit_LOCALTLK", "_xmit_FDDI",
+ "_xmit_BIF", "_xmit_SIT", "_xmit_IPDDP", "_xmit_IPGRE",
+ "_xmit_PIMREG", "_xmit_HIPPI", "_xmit_ASH", "_xmit_ECONET",
+ "_xmit_IRDA", "_xmit_FCPP", "_xmit_FCAL", "_xmit_FCPL",
+ "_xmit_FCFABRIC", "_xmit_IEEE802_TR", "_xmit_IEEE80211",
+ "_xmit_IEEE80211_PRISM", "_xmit_IEEE80211_RADIOTAP", "_xmit_VOID",
+ "_xmit_NONE"};
+
+static struct lock_class_key 
 netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
+
+static inline unsigned short netdev_lock_pos(unsigned short dev_type)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(netdev_lock_type); i++)
+ if (netdev_lock_type[i] == dev_type)
+ return i;
+ /* the last key is used by default */
+ return --i;
+}
+
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+     unsigned short dev_type)
+{
+ int i;
+
+ i = netdev_lock_pos(dev_type);
+ lockdep_set_class_and_name(lock, &netdev_xmit_lock_key[i],
+    netdev_lock_name[i]);
+}
+#else
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+     unsigned short dev_type)
+{
+}
+#endif
 
 /*******************************************************************************

@@ -3001,6 +3069,7 @@ int register_netdevice(struct net_device
 
 spin_lock_init(&dev->queue_lock);
 spin_lock_init(&dev->_xmit_lock);
+ netdev_set_lockdep_class(&dev->_xmit_lock, dev->type);
 dev->xmit_lock_owner = -1;
 spin_lock_init(&dev->ingress_lock);
#16 lore raw

PATCH 1/1 netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

In reply to David Miller

Sorry - I've fogotten about something very important! (Plus a small change in the diff.)

Jarek P.

---> (take 2)

After initializing dev->_xmit_lock register_netdevice() sets lockdep class according to dev->type.

Idea of this patch - by David Miller.

Reported & tested by: "Yuriy N. Shkandybin" <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---

net/core/dev.c +69 -0
diff -Nurp 2.6.22-/net/core/dev.c 2.6.22/net/core/dev.c
@@ -116,6 +116,7 @@
 #include <linux/dmaengine.h>
 #include <linux/err.h>
 #include <linux/ctype.h>
+#include <linux/if_arp.h>
 
 /*
  *	The list of packet types we will receive (as opposed to discard)

@@ -217,6 +218,73 @@ extern void netdev_unregister_sysfs(stru
 #define	netdev_unregister_sysfs(dev)	do { } while(0)
 #endif
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+/*
+ * register_netdevice() inits dev->_xmit_lock and sets lockdep class
+ * according to dev->type
+ */
+static const unsigned short netdev_lock_type[] =
+	{ARPHRD_NETROM, ARPHRD_ETHER, ARPHRD_EETHER, ARPHRD_AX25,
+	 ARPHRD_PRONET, ARPHRD_CHAOS, ARPHRD_IEEE802, ARPHRD_ARCNET,
+	 ARPHRD_APPLETLK, ARPHRD_DLCI, ARPHRD_ATM, ARPHRD_METRICOM,
+	 ARPHRD_IEEE1394, ARPHRD_EUI64, ARPHRD_INFINIBAND, ARPHRD_SLIP,
+	 ARPHRD_CSLIP, ARPHRD_SLIP6, ARPHRD_CSLIP6, ARPHRD_RSRVD,
+	 ARPHRD_ADAPT, ARPHRD_ROSE, ARPHRD_X25, ARPHRD_HWX25,
+	 ARPHRD_PPP, ARPHRD_CISCO, ARPHRD_LAPB, ARPHRD_DDCMP,
+	 ARPHRD_RAWHDLC, ARPHRD_TUNNEL, ARPHRD_TUNNEL6, ARPHRD_FRAD,
+	 ARPHRD_SKIP, ARPHRD_LOOPBACK, ARPHRD_LOCALTLK, ARPHRD_FDDI,
+	 ARPHRD_BIF, ARPHRD_SIT, ARPHRD_IPDDP, ARPHRD_IPGRE,
+	 ARPHRD_PIMREG, ARPHRD_HIPPI, ARPHRD_ASH, ARPHRD_ECONET,
+	 ARPHRD_IRDA, ARPHRD_FCPP, ARPHRD_FCAL, ARPHRD_FCPL,
+	 ARPHRD_FCFABRIC, ARPHRD_IEEE802_TR, ARPHRD_IEEE80211,
+	 ARPHRD_IEEE80211_PRISM, ARPHRD_IEEE80211_RADIOTAP, ARPHRD_VOID,
+	 ARPHRD_NONE};
+
+static const char *netdev_lock_name[] =
+	{"_xmit_NETROM", "_xmit_ETHER", "_xmit_EETHER", "_xmit_AX25",
+	 "_xmit_PRONET", "_xmit_CHAOS", "_xmit_IEEE802", "_xmit_ARCNET",
+	 "_xmit_APPLETLK", "_xmit_DLCI", "_xmit_ATM", "_xmit_METRICOM",
+	 "_xmit_IEEE1394", "_xmit_EUI64", "_xmit_INFINIBAND", "_xmit_SLIP",
+	 "_xmit_CSLIP", "_xmit_SLIP6", "_xmit_CSLIP6", "_xmit_RSRVD",
+	 "_xmit_ADAPT", "_xmit_ROSE", "_xmit_X25", "_xmit_HWX25",
+	 "_xmit_PPP", "_xmit_CISCO", "_xmit_LAPB", "_xmit_DDCMP",
+	 "_xmit_RAWHDLC", "_xmit_TUNNEL", "_xmit_TUNNEL6", "_xmit_FRAD",
+	 "_xmit_SKIP", "_xmit_LOOPBACK", "_xmit_LOCALTLK", "_xmit_FDDI",
+	 "_xmit_BIF", "_xmit_SIT", "_xmit_IPDDP", "_xmit_IPGRE",
+	 "_xmit_PIMREG", "_xmit_HIPPI", "_xmit_ASH", "_xmit_ECONET",
+	 "_xmit_IRDA", "_xmit_FCPP", "_xmit_FCAL", "_xmit_FCPL",
+	 "_xmit_FCFABRIC", "_xmit_IEEE802_TR", "_xmit_IEEE80211",
+	 "_xmit_IEEE80211_PRISM", "_xmit_IEEE80211_RADIOTAP", "_xmit_VOID",
+	 "_xmit_NONE"};
+
+static struct lock_class_key netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
+
+static inline unsigned short netdev_lock_pos(unsigned short dev_type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(netdev_lock_type); i++)
+		if (netdev_lock_type[i] == dev_type)
+			return i;
+	/* the last key is used by default */
+	return ARRAY_SIZE(netdev_lock_type) - 1;
+}
+
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+					    unsigned short dev_type)
+{
+	int i;
+
+	i = netdev_lock_pos(dev_type);
+	lockdep_set_class_and_name(lock, &netdev_xmit_lock_key[i],
+				   netdev_lock_name[i]);
+}
+#else
+static inline void netdev_set_lockdep_class(spinlock_t *lock,
+					    unsigned short dev_type)
+{
+}
+#endif
 
 /*******************************************************************************
 

@@ -3001,6 +3069,7 @@ int register_netdevice(struct net_device
 
 	spin_lock_init(&dev->queue_lock);
 	spin_lock_init(&dev->_xmit_lock);
+	netdev_set_lockdep_class(&dev->_xmit_lock, dev->type);
 	dev->xmit_lock_owner = -1;
 	spin_lock_init(&dev->ingress_lock);
maintainer
#17 lore raw

Re: [PATCH (take 2)] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

From: Jarek Poplawski <[email protected]>
Date: Wed, 16 May 2007 07:40:00 +0200

Quoted 7 lines

After initializing dev->_xmit_lock register_netdevice() sets lockdep class according to dev->type.

Idea of this patch - by David Miller.

Reported & tested by: "Yuriy N. Shkandybin" <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

Patch applied, dziekuje bardzo.

#18 lore raw

Re: [PATCH (take 2)] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

Quoted 6 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Wed, 16 May 2007 07:40:00 +0200

Quoted 3 lines

After initializing dev->_xmit_lock register_netdevice() sets lockdep class according to dev->type.

Quoted 7 lines
Quoted 4 lines

Idea of this patch - by David Miller.

Reported & tested by: "Yuriy N. Shkandybin" <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

Patch applied, dziekuje bardzo.

I cannot believe this! I'm even more surprised then, when I heard it from Borat. So, you know the ... Kazakh too!)

BTW - I think some patch on vlan cannot do any harm (at least like this previous of mine - with only ppp considered), and maybe this all could be forgotten.

Pozdrawiam,
Jarek P.

maintainer
#19 lore raw

Re: [PATCH (take 2)] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

From: Jarek Poplawski <[email protected]>
Date: Wed, 16 May 2007 08:17:32 +0200

Quoted 3 lines

BTW - I think some patch on vlan cannot do any harm (at least like this previous of mine - with only ppp considered), and maybe this all could be forgotten.

Let's wait to see if any new messages show up.

I think how we've seperated PPP from ethernet locking has annotated things sufficiently for lockdep to see what's going on even in that VLAN case.

#20 lore raw

Re: [PATCH (take 2)] netdev: lockdep classes in register_netdevice Re: [patch 04/13] ppp_generic: fix lockdep warning

Quoted 7 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Wed, 16 May 2007 08:17:32 +0200

Quoted 3 lines

BTW - I think some patch on vlan cannot do any harm (at least like this previous of mine - with only ppp considered), and maybe this all could be forgotten.

Quoted 6 lines

Let's wait to see if any new messages show up.

I think how we've seperated PPP from ethernet locking has annotated things sufficiently for lockdep to see what's going on even in that VLAN case.

I'm 99,9% sure lockdep's warning like this will be back:

#0 vlan_netdev_xmit_lock_key

	|
	V
#1 ppp->wlock
	|
	V

(pppoe)

#0 ppp->wlock

	|
	V
#1 pch->downl
	|
	V
#2 vlan_netdev_xmit_lock_key
	|
	V

(eth)

This 0,1% is for Yuriy - when he changes config or turns off lockdep, like others.

But I have no problem with waiting too.

Jarek P.