Re: 2.6.17-rc5-mm3-lockdep -

Re: 2.6.17-rc5-mm3-lockdep -

[Stefan Richter]

>>Valdis.Kletnieks@vt.edu napsal(a):
...
>>>[  464.687000] [ BUG: illegal lock usage! ]
>>>[  464.687000] ----------------------------
>>>[  464.687000] illegal {in-hardirq-W} -> {hardirq-on-W} usage.
>>>[  464.687000] id/2700 [HC0[0]:SC0[1]:HE1:SE0] takes:
>>>[  464.687000]  (&list->lock){++..}, at: [<c0351a07>] unix_stream_connect+0x334/0x408
>>>[  464.687000] {in-hardirq-W} state was registered at:
>>>[  464.687000]   [<c012dd45>] lockdep_acquire+0x67/0x7f
>>>[  464.687000]   [<c0383f11>] _spin_lock_irqsave+0x30/0x3f
>>>[  464.687000]   [<c02fa93f>] skb_dequeue+0x18/0x49
>>>[  464.687000]   [<f086b7f1>] hpsb_bus_reset+0x5e/0xa2 [ieee1394]
>>>[  464.687000]   [<f0887007>] ohci_irq_handler+0x370/0x726 [ohci1394]
>>>[  464.687000]   [<c013f9a8>] handle_IRQ_event+0x1d/0x52
>>>[  464.687000]   [<c0140bc4>] handle_level_irq+0x97/0xe3
>>>[  464.687000]   [<c01045d0>] do_IRQ+0x8b/0xaf
>>>[  464.687000] irq event stamp: 2964
>>>[  464.687000] hardirqs last  enabled at (2963): [<c0384223>] _spin_unlock_irqrestore+0x3b/0x6d
>>>[  464.687000] hardirqs last disabled at (2962): [<c0383ef5>] _spin_lock_irqsave+0x14/0x3f
>>>[  464.687000] softirqs last  enabled at (2956): [<c0119da0>] __do_softirq+0x9d/0xa5
>>>[  464.687000] softirqs last disabled at (2964): [<c0383f6b>] _spin_lock_bh+0x10/0x3a
>>>[  464.687000] 
>>>[  464.687000] other info that might help us debug this:
>>>[  464.687000] 1 locks held by id/2700:
>>>[  464.687000]  #0:  (&u->lock){--..}, at: [<c03517bb>] unix_stream_connect+0xe8/0x408
>>>[  464.687000] 
>>>[  464.687000] stack backtrace:
>>>[  464.687000]  [<c01032d6>] show_trace_log_lvl+0x64/0x125
>>>[  464.687000]  [<c0103865>] show_trace+0x1b/0x20
>>>[  464.687000]  [<c010391c>] dump_stack+0x1f/0x24
>>>[  464.687000]  [<c012bfb1>] print_usage_bug+0x1a8/0x1b4
>>>[  464.687000]  [<c012c6c7>] mark_lock+0x2ba/0x4e5
>>>[  464.687000]  [<c012d2b8>] __lockdep_acquire+0x476/0xa91
>>>[  464.687000]  [<c012dd45>] lockdep_acquire+0x67/0x7f
>>>[  464.687000]  [<c0383f87>] _spin_lock_bh+0x2c/0x3a
>>>[  464.687000]  [<c0351a07>] unix_stream_connect+0x334/0x408
>>>[  464.687000]  [<c02f7236>] sys_connect+0x6e/0xa3
>>>[  464.687000]  [<c02f79da>] sys_socketcall+0x96/0x190
>>>[  464.687000]  [<c03845e2>] sysenter_past_esp+0x63/0xa1

On second look it doesn't seem to be a problem of the ieee1394 stack but 
rather of underlying skb and net infrastructure.

BTW, the locking in -mm's net/unix/af_unix.c::unix_stream_connect() 
differs a bit from stock unix_stream_connect(). I see spin_lock_bh() in 
2.6.17-rc5-mm3 where 2.6.17-rc5 has spin_lock().

(added Cc: netdev)
-- 
Stefan Richter
-=====-=-==- -==- --==-
http://arcgraph.de/sr/

Re: 2.6.17-rc5-mm3-lockdep -

[Herbert Xu]

On Tue, Jun 06, 2006 at 04:39:21PM +0000, Stefan Richter wrote:
> 
> BTW, the locking in -mm's net/unix/af_unix.c::unix_stream_connect() 
> differs a bit from stock unix_stream_connect(). I see spin_lock_bh() in 
> 2.6.17-rc5-mm3 where 2.6.17-rc5 has spin_lock().

Hi Ingo:

Looks like this change was introduced by the validator patch.  Any idea
why this was done? AF_UNIX is a user-space-driven socket so there shouldn't
be any need for BH to be disabled there.

Even if it does this patch should go through the normal channels rather
than the lock validator.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: 2.6.17-rc5-mm3-lockdep -

[Ingo Molnar]

* Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Tue, Jun 06, 2006 at 04:39:21PM +0000, Stefan Richter wrote:
> > 
> > BTW, the locking in -mm's net/unix/af_unix.c::unix_stream_connect() 
> > differs a bit from stock unix_stream_connect(). I see spin_lock_bh() in 
> > 2.6.17-rc5-mm3 where 2.6.17-rc5 has spin_lock().
> 
> Hi Ingo:
> 
> Looks like this change was introduced by the validator patch.  Any 
> idea why this was done? AF_UNIX is a user-space-driven socket so there 
> shouldn't be any need for BH to be disabled there.

yeah. I'll investigate - it's quite likely that sk_receive_queue.lock 
will have to get per-address family locking rules - right?

Maybe it's enough to introduce a separate key for AF_UNIX alone (and 
still having all other protocols share the locking rules for 
sk_receive_queue.lock) , by reinitializing its spinlock after 
sock_init_data()?

	Ingo

Re: 2.6.17-rc5-mm3-lockdep -

[Herbert Xu]

On Mon, Jun 12, 2006 at 08:38:07AM +0200, Ingo Molnar wrote:
> 
> yeah. I'll investigate - it's quite likely that sk_receive_queue.lock 
> will have to get per-address family locking rules - right?

Yes that's the issue.

> Maybe it's enough to introduce a separate key for AF_UNIX alone (and 
> still having all other protocols share the locking rules for 
> sk_receive_queue.lock) , by reinitializing its spinlock after 
> sock_init_data()?

This could work.  AF_UNIX is probably the only family that does not
interact with hardware.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[patch] undo AF_UNIX _bh locking changes and split lock-type instead

[Ingo Molnar]

* Herbert Xu <herbert@gondor.apana.org.au> wrote:

> > Maybe it's enough to introduce a separate key for AF_UNIX alone (and 
> > still having all other protocols share the locking rules for 
> > sk_receive_queue.lock) , by reinitializing its spinlock after 
> > sock_init_data()?
> 
> This could work.  AF_UNIX is probably the only family that does not 
> interact with hardware.

ok, great. The patch below does the trick on my box.

regarding your point wrt. path of integration - it is pretty much the 
only practical way to do this centrally as part of the lock validator 
patches, but to collect ACKs from subsystem maintainers in the process. 
So if you like it i'd like to have your ACK but this patch depends on 
the other lock validator patches (and only makes sense together with 
them), so they should temporarily stay in the lock validator queue. 
Hopefully this wont be a state that lasts too long and once the 
validator is upstream, all patches of course go via the subsystem 
submission rules.

(the #ifdef LOCKDEP should probably be converted to some sort of 
lockdep_split_lock_key(&sk->sk_receive_queue.lock) op - i'll do that 
later)

	Ingo

------
Subject: undo AF_UNIX _bh locking changes and split lock-type instead
From: Ingo Molnar <mingo@elte.hu>

this cleans up lock-validator-special-locking-af_unix.patch: instead
of adding _bh locking to AF_UNIX, this patch splits their
sk_receive_queue.lock type from the other networking skb-queue locks.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 net/unix/af_unix.c |   17 +++++++++++++----
 net/unix/garbage.c |    8 ++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

Index: linux/net/unix/af_unix.c
===================================================================
--- linux.orig/net/unix/af_unix.c
+++ linux/net/unix/af_unix.c
@@ -557,6 +557,15 @@ static struct sock * unix_create1(struct
 	atomic_inc(&unix_nr_socks);
 
 	sock_init_data(sock,sk);
+#ifdef CONFIG_LOCKDEP
+	/*
+	 * AF_UNIX sockets do not interact with hardware, hence they
+	 * dont trigger interrupts - so it's safe for them to have
+	 * bh-unsafe locking for their sk_receive_queue.lock. Split off
+	 * this special lock-type by reinitializing the spinlock.
+	 */
+	spin_lock_init(&sk->sk_receive_queue.lock);
+#endif
 
 	sk->sk_write_space	= unix_write_space;
 	sk->sk_max_ack_backlog	= sysctl_unix_max_dgram_qlen;
@@ -1073,12 +1082,12 @@ restart:
 	unix_state_wunlock(sk);
 
 	/* take ten and and send info to listening sock */
-	spin_lock_bh(&other->sk_receive_queue.lock);
+	spin_lock(&other->sk_receive_queue.lock);
 	__skb_queue_tail(&other->sk_receive_queue, skb);
 	/* Undo artificially decreased inflight after embrion
 	 * is installed to listening socket. */
 	atomic_inc(&newu->inflight);
-	spin_unlock_bh(&other->sk_receive_queue.lock);
+	spin_unlock(&other->sk_receive_queue.lock);
 	unix_state_runlock(other);
 	other->sk_data_ready(other, 0);
 	sock_put(other);
@@ -1843,7 +1852,7 @@ static int unix_ioctl(struct socket *soc
 				break;
 			}
 
-			spin_lock_bh(&sk->sk_receive_queue.lock);
+			spin_lock(&sk->sk_receive_queue.lock);
 			if (sk->sk_type == SOCK_STREAM ||
 			    sk->sk_type == SOCK_SEQPACKET) {
 				skb_queue_walk(&sk->sk_receive_queue, skb)
@@ -1853,7 +1862,7 @@ static int unix_ioctl(struct socket *soc
 				if (skb)
 					amount=skb->len;
 			}
-			spin_unlock_bh(&sk->sk_receive_queue.lock);
+			spin_unlock(&sk->sk_receive_queue.lock);
 			err = put_user(amount, (int __user *)arg);
 			break;
 		}
Index: linux/net/unix/garbage.c
===================================================================
--- linux.orig/net/unix/garbage.c
+++ linux/net/unix/garbage.c
@@ -235,7 +235,7 @@ void unix_gc(void)
 		struct sock *x = pop_stack();
 		struct sock *sk;
 
-		spin_lock_bh(&x->sk_receive_queue.lock);
+		spin_lock(&x->sk_receive_queue.lock);
 		skb = skb_peek(&x->sk_receive_queue);
 		
 		/*
@@ -270,7 +270,7 @@ void unix_gc(void)
 				maybe_unmark_and_push(skb->sk);
 			skb=skb->next;
 		}
-		spin_unlock_bh(&x->sk_receive_queue.lock);
+		spin_unlock(&x->sk_receive_queue.lock);
 		sock_put(x);
 	}
 
@@ -283,7 +283,7 @@ void unix_gc(void)
 		if (u->gc_tree == GC_ORPHAN) {
 			struct sk_buff *nextsk;
 
-			spin_lock_bh(&s->sk_receive_queue.lock);
+			spin_lock(&s->sk_receive_queue.lock);
 			skb = skb_peek(&s->sk_receive_queue);
 			while (skb &&
 			       skb != (struct sk_buff *)&s->sk_receive_queue) {
@@ -298,7 +298,7 @@ void unix_gc(void)
 				}
 				skb = nextsk;
 			}
-			spin_unlock_bh(&s->sk_receive_queue.lock);
+			spin_unlock(&s->sk_receive_queue.lock);
 		}
 		u->gc_tree = GC_ORPHAN;
 	}

Re: [patch] undo AF_UNIX _bh locking changes and split lock-type instead

[Herbert Xu]

On Mon, Jun 12, 2006 at 08:57:01AM +0200, Ingo Molnar wrote:
>
> regarding your point wrt. path of integration - it is pretty much the 
> only practical way to do this centrally as part of the lock validator 
> patches, but to collect ACKs from subsystem maintainers in the process. 
> So if you like it i'd like to have your ACK but this patch depends on 
> the other lock validator patches (and only makes sense together with 
> them), so they should temporarily stay in the lock validator queue. 
> Hopefully this wont be a state that lasts too long and once the 
> validator is upstream, all patches of course go via the subsystem 
> submission rules.

Obviously as long as Dave is happy with it then it's fine.  However,
it's probably a good idea to cc netdev for relevant patches so that
they get a wider review.  If you've already sent this one there then
I apologise for missing it :)

> (the #ifdef LOCKDEP should probably be converted to some sort of 
> lockdep_split_lock_key(&sk->sk_receive_queue.lock) op - i'll do that 
> later)

Cool.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch] undo AF_UNIX _bh locking changes and split lock-type instead

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 12 Jun 2006 17:03:56 +1000

> On Mon, Jun 12, 2006 at 08:57:01AM +0200, Ingo Molnar wrote:
> >
> > regarding your point wrt. path of integration - it is pretty much the 
> > only practical way to do this centrally as part of the lock validator 
> > patches, but to collect ACKs from subsystem maintainers in the process. 
> > So if you like it i'd like to have your ACK but this patch depends on 
> > the other lock validator patches (and only makes sense together with 
> > them), so they should temporarily stay in the lock validator queue. 
> > Hopefully this wont be a state that lasts too long and once the 
> > validator is upstream, all patches of course go via the subsystem 
> > submission rules.
> 
> Obviously as long as Dave is happy with it then it's fine.  However,
> it's probably a good idea to cc netdev for relevant patches so that
> they get a wider review.  If you've already sent this one there then
> I apologise for missing it :)

Yes, this is fine with me.

Re: [patch] undo AF_UNIX _bh locking changes and split lock-type instead

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> (the #ifdef LOCKDEP should probably be converted to some sort of 
> lockdep_split_lock_key(&sk->sk_receive_queue.lock) op - i'll do that 
> later)

i've added such an op, lockdep_reinit_lock_key() - this makes the patch 
cleaner:

----------------------
Subject: undo AF_UNIX _bh locking changes and split lock-type
From: Ingo Molnar <mingo@elte.hu>

this cleans up lock-validator-special-locking-af_unix.patch: instead
of adding _bh locking to AF_UNIX, this patch splits their
sk_receive_queue.lock type from the other networking skb-queue locks.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 net/unix/af_unix.c |   18 ++++++++++++++----
 net/unix/garbage.c |    8 ++++----
 2 files changed, 18 insertions(+), 8 deletions(-)

Index: linux/net/unix/af_unix.c
===================================================================
--- linux.orig/net/unix/af_unix.c
+++ linux/net/unix/af_unix.c
@@ -542,6 +542,14 @@ static struct proto unix_proto = {
 	.obj_size = sizeof(struct unix_sock),
 };
 
+/*
+ * AF_UNIX sockets do not interact with hardware, hence they
+ * dont trigger interrupts - so it's safe for them to have
+ * bh-unsafe locking for their sk_receive_queue.lock. Split off
+ * this special lock-type by reinitializing the spinlock key:
+ */
+static struct lockdep_type_key af_unix_sk_receive_queue_lock_key;
+
 static struct sock * unix_create1(struct socket *sock)
 {
 	struct sock *sk = NULL;
@@ -557,6 +565,8 @@ static struct sock * unix_create1(struct
 	atomic_inc(&unix_nr_socks);
 
 	sock_init_data(sock,sk);
+	lockdep_reinit_lock_key(&sk->sk_receive_queue.lock,
+				&af_unix_sk_receive_queue_lock_key);
 
 	sk->sk_write_space	= unix_write_space;
 	sk->sk_max_ack_backlog	= sysctl_unix_max_dgram_qlen;
@@ -1073,12 +1083,12 @@ restart:
 	unix_state_wunlock(sk);
 
 	/* take ten and and send info to listening sock */
-	spin_lock_bh(&other->sk_receive_queue.lock);
+	spin_lock(&other->sk_receive_queue.lock);
 	__skb_queue_tail(&other->sk_receive_queue, skb);
 	/* Undo artificially decreased inflight after embrion
 	 * is installed to listening socket. */
 	atomic_inc(&newu->inflight);
-	spin_unlock_bh(&other->sk_receive_queue.lock);
+	spin_unlock(&other->sk_receive_queue.lock);
 	unix_state_runlock(other);
 	other->sk_data_ready(other, 0);
 	sock_put(other);
@@ -1843,7 +1853,7 @@ static int unix_ioctl(struct socket *soc
 				break;
 			}
 
-			spin_lock_bh(&sk->sk_receive_queue.lock);
+			spin_lock(&sk->sk_receive_queue.lock);
 			if (sk->sk_type == SOCK_STREAM ||
 			    sk->sk_type == SOCK_SEQPACKET) {
 				skb_queue_walk(&sk->sk_receive_queue, skb)
@@ -1853,7 +1863,7 @@ static int unix_ioctl(struct socket *soc
 				if (skb)
 					amount=skb->len;
 			}
-			spin_unlock_bh(&sk->sk_receive_queue.lock);
+			spin_unlock(&sk->sk_receive_queue.lock);
 			err = put_user(amount, (int __user *)arg);
 			break;
 		}
Index: linux/net/unix/garbage.c
===================================================================
--- linux.orig/net/unix/garbage.c
+++ linux/net/unix/garbage.c
@@ -235,7 +235,7 @@ void unix_gc(void)
 		struct sock *x = pop_stack();
 		struct sock *sk;
 
-		spin_lock_bh(&x->sk_receive_queue.lock);
+		spin_lock(&x->sk_receive_queue.lock);
 		skb = skb_peek(&x->sk_receive_queue);
 		
 		/*
@@ -270,7 +270,7 @@ void unix_gc(void)
 				maybe_unmark_and_push(skb->sk);
 			skb=skb->next;
 		}
-		spin_unlock_bh(&x->sk_receive_queue.lock);
+		spin_unlock(&x->sk_receive_queue.lock);
 		sock_put(x);
 	}
 
@@ -283,7 +283,7 @@ void unix_gc(void)
 		if (u->gc_tree == GC_ORPHAN) {
 			struct sk_buff *nextsk;
 
-			spin_lock_bh(&s->sk_receive_queue.lock);
+			spin_lock(&s->sk_receive_queue.lock);
 			skb = skb_peek(&s->sk_receive_queue);
 			while (skb &&
 			       skb != (struct sk_buff *)&s->sk_receive_queue) {
@@ -298,7 +298,7 @@ void unix_gc(void)
 				}
 				skb = nextsk;
 			}
-			spin_unlock_bh(&s->sk_receive_queue.lock);
+			spin_unlock(&s->sk_receive_queue.lock);
 		}
 		u->gc_tree = GC_ORPHAN;
 	}

Re: [patch] undo AF_UNIX _bh locking changes and split lock-type instead

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Mon, 12 Jun 2006 10:49:36 +0200

> Subject: undo AF_UNIX _bh locking changes and split lock-type
> From: Ingo Molnar <mingo@elte.hu>
> 
> this cleans up lock-validator-special-locking-af_unix.patch: instead
> of adding _bh locking to AF_UNIX, this patch splits their
> sk_receive_queue.lock type from the other networking skb-queue locks.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

Signed-off-by: David S. Miller <davem@davemloft.net>

Re: 2.6.17-rc5-mm3-lockdep -

[David Miller]

From: Ingo Molnar <mingo@elte.hu>
Date: Mon, 12 Jun 2006 08:38:07 +0200

> yeah. I'll investigate - it's quite likely that sk_receive_queue.lock 
> will have to get per-address family locking rules - right?

That's right.

> Maybe it's enough to introduce a separate key for AF_UNIX alone (and 
> still having all other protocols share the locking rules for 
> sk_receive_queue.lock) , by reinitializing its spinlock after 
> sock_init_data()?

AF_NETLINK and/or AF_PACKET might be in a similar situation
as AF_UNIX.