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

[Ingo Molnar]

* Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxx>

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@xxxxxxx>
---
 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;
 	}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/