Skip to contentSkip to conversation

fix several lockdep splats, allow multiple splats

PATCH In review Paul E. McKenney 5 participants 16 messages Apr 21, 2010 – Apr 30, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fix several lockdep splats, allow multiple splats

Hello!

This patchset contains four RCU lockdep splat fixes, courtesy of David Howells, Peter Zijlstra, and Trond Myklebust, as well as an enhancement by Lai Jiangshan that permits collecting more than one RCU lockdep splat per boot.

Thanx, Paul

------------------------------------------------------------------------

 b/fs/nfs/delegation.c      |   42 ++++++++++++++++++++++++++++--------------
 b/include/linux/rcupdate.h |   15 +++++++++++----
 b/kernel/cgroup_freezer.c  |    5 ++++-
 b/kernel/lockdep.c         |    2 --
 b/kernel/sched.c           |   10 ++++++++++
 fs/nfs/delegation.c        |   44 +++++++++++++++++++++++---------------------
 6 files changed, 76 insertions(+), 42 deletions(-)
#1 lore raw

PATCH 1/5 rcu: Fix RCU lockdep splat in set_task_cpu on fork path

From: Peter Zijlstra <[email protected]>

Add an RCU read-side critical section to suppress this false positive.

---
 kernel/sched.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
kernel/sched.c +10 -0
index 6af210a..14c44ec 100644
@@ -323,6 +323,15 @@ static inline struct task_group *task_group(struct task_struct *p)
 /* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
 static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
 {
+	/*
+	 * Strictly speaking this rcu_read_lock() is not needed since the
+	 * task_group is tied to the cgroup, which in turn can never go away
+	 * as long as there are tasks attached to it.
+	 *
+	 * However since task_group() uses task_subsys_state() which is an
+	 * rcu_dereference() user, this quiets CONFIG_PROVE_RCU.
+	 */
+	rcu_read_lock();
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	p->se.cfs_rq = task_group(p)->cfs_rq[cpu];
 	p->se.parent = task_group(p)->se[cpu];
@@ -332,6 +341,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
 	p->rt.rt_rq  = task_group(p)->rt_rq[cpu];
 	p->rt.parent = task_group(p)->rt_se[cpu];
 #endif
+	rcu_read_unlock();
 }
 
 #else
Signature · 1 line
1.7.0
#2 lore raw

[tip:core/urgent] rcu: Fix RCU lockdep splat in set_task_cpu on fork path

Commit-ID:  8b08ca52f5942c21564bbb90ccfb61053f2c26a1
Gitweb:     http://git.kernel.org/tip/8b08ca52f5942c21564bbb90ccfb61053f2c26a1
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 21 Apr 2010 13:02:07 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Apr 2010 12:03:17 +0200

rcu: Fix RCU lockdep splat in set_task_cpu on fork path

Add an RCU read-side critical section to suppress this false positive.

LKML-Reference: <[email protected]>

---
 kernel/sched.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
kernel/sched.c +10 -0
index de0bd26..3c2a54f 100644
@@ -323,6 +323,15 @@ static inline struct task_group *task_group(struct task_struct *p)
 /* Change a task's cfs_rq and parent entity if it moves across CPUs/groups */
 static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
 {
+	/*
+	 * Strictly speaking this rcu_read_lock() is not needed since the
+	 * task_group is tied to the cgroup, which in turn can never go away
+	 * as long as there are tasks attached to it.
+	 *
+	 * However since task_group() uses task_subsys_state() which is an
+	 * rcu_dereference() user, this quiets CONFIG_PROVE_RCU.
+	 */
+	rcu_read_lock();
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	p->se.cfs_rq = task_group(p)->cfs_rq[cpu];
 	p->se.parent = task_group(p)->se[cpu];
@@ -332,6 +341,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
 	p->rt.rt_rq  = task_group(p)->rt_rq[cpu];
 	p->rt.parent = task_group(p)->rt_se[cpu];
 #endif
+	rcu_read_unlock();
 }
 
 #else
#3 lore raw

PATCH 2/5 rcu: fix RCU lockdep splat on freezer_fork path

In reply to Paul E. McKenney

Add an RCU read-side critical section to suppress this false positive.

---
 kernel/cgroup_freezer.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/cgroup_freezer.c +4 -1
index da5e139..e5c0244 100644
@@ -205,9 +205,12 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	 * No lock is needed, since the task isn't on tasklist yet,
 	 * so it can't be moved to another cgroup, which means the
 	 * freezer won't be removed and will be valid during this
-	 * function call.
+	 * function call.  Nevertheless, apply RCU read-side critical
+	 * section to suppress RCU lockdep false positives.
 	 */
+	rcu_read_lock();
 	freezer = task_freezer(task);
+	rcu_read_unlock();
 
 	/*
 	 * The root cgroup is non-freezable, so we can skip the
Signature · 1 line
1.7.0
#4 lore raw

[tip:core/urgent] rcu: Fix RCU lockdep splat on freezer_fork path

Commit-ID:  8b46f880841aac821af8efa6581bb0e46b8b9845
Gitweb:     http://git.kernel.org/tip/8b46f880841aac821af8efa6581bb0e46b8b9845
Author:     Paul E. McKenney <[email protected]>
AuthorDate: Wed, 21 Apr 2010 13:02:08 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Apr 2010 12:03:17 +0200

rcu: Fix RCU lockdep splat on freezer_fork path

Add an RCU read-side critical section to suppress this false positive.

LKML-Reference: <[email protected]>

---
 kernel/cgroup_freezer.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/cgroup_freezer.c +4 -1
index da5e139..e5c0244 100644
@@ -205,9 +205,12 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	 * No lock is needed, since the task isn't on tasklist yet,
 	 * so it can't be moved to another cgroup, which means the
 	 * freezer won't be removed and will be valid during this
-	 * function call.
+	 * function call.  Nevertheless, apply RCU read-side critical
+	 * section to suppress RCU lockdep false positives.
 	 */
+	rcu_read_lock();
 	freezer = task_freezer(task);
+	rcu_read_unlock();
 
 	/*
 	 * The root cgroup is non-freezable, so we can skip the
#5 lore raw

PATCH 3/5 rcu: leave lockdep enabled after RCU lockdep splat

In reply to Paul E. McKenney

From: Lai Jiangshan <[email protected]>

There is no need to disable lockdep after an RCU lockdep splat, so remove the debug_lockdeps_off() from lockdep_rcu_dereference(). To avoid repeated lockdep splats, use a static variable in the inlined rcu_dereference_check() and rcu_dereference_protected() macros so that a given instance splats only once, but so that multiple instances can be detected per boot.

---
 include/linux/rcupdate.h |   15 +++++++++++----
 kernel/lockdep.c         |    2 --
 2 files changed, 11 insertions(+), 6 deletions(-)
include/linux/rcupdate.h +11 -4
index 07db2fe..ec9ab49 100644
@@ -190,6 +190,15 @@ static inline int rcu_read_lock_sched_held(void)
 
 #ifdef CONFIG_PROVE_RCU
 
+#define __do_rcu_dereference_check(c)					\
+	do {								\
+		static bool __warned;					\
+		if (debug_lockdep_rcu_enabled() && !__warned && !(c)) {	\
+			__warned = true;				\
+			lockdep_rcu_dereference(__FILE__, __LINE__);	\
+		}							\
+	} while (0)
+
 /**
  * rcu_dereference_check - rcu_dereference with debug checking
  * @p: The pointer to read, prior to dereferencing
@@ -219,8 +228,7 @@ static inline int rcu_read_lock_sched_held(void)
  */
 #define rcu_dereference_check(p, c) \
 	({ \
-		if (debug_lockdep_rcu_enabled() && !(c)) \
-			lockdep_rcu_dereference(__FILE__, __LINE__); \
+		__do_rcu_dereference_check(c); \
 		rcu_dereference_raw(p); \
 	})
 
@@ -237,8 +245,7 @@ static inline int rcu_read_lock_sched_held(void)
  */
 #define rcu_dereference_protected(p, c) \
 	({ \
-		if (debug_lockdep_rcu_enabled() && !(c)) \
-			lockdep_rcu_dereference(__FILE__, __LINE__); \
+		__do_rcu_dereference_check(c); \
 		(p); \
 	})
 
kernel/lockdep.c +0 -2
index 2594e1c..03dd1fa 100644
@@ -3801,8 +3801,6 @@ void lockdep_rcu_dereference(const char *file, const int line)
 {
 	struct task_struct *curr = current;
 
-	if (!debug_locks_off())
-		return;
 	printk("\n===================================================\n");
 	printk(  "[ INFO: suspicious rcu_dereference_check() usage. ]\n");
 	printk(  "---------------------------------------------------\n");
Signature · 1 line
1.7.0
#6 lore raw

PATCH 4/5 NFSv4: Fix the locking in nfs_inode_reclaim_delegation()

In reply to Paul E. McKenney

From: Trond Myklebust <[email protected]>

Ensure that we correctly rcu-dereference the delegation itself, and that we protect against removal while we're changing the contents.

---
 fs/nfs/delegation.c |   42 ++++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 14 deletions(-)
fs/nfs/delegation.c +28 -14
index 1567124..8d9ec49 100644
@@ -129,21 +129,35 @@ again:
  */
 void nfs_inode_reclaim_delegation(struct inode *inode, struct rpc_cred *cred, struct nfs_openres *res)
 {
-	struct nfs_delegation *delegation = NFS_I(inode)->delegation;
-	struct rpc_cred *oldcred;
+	struct nfs_delegation *delegation;
+	struct rpc_cred *oldcred = NULL;
 
-	if (delegation == NULL)
-		return;
-	memcpy(delegation->stateid.data, res->delegation.data,
-			sizeof(delegation->stateid.data));
-	delegation->type = res->delegation_type;
-	delegation->maxsize = res->maxsize;
-	oldcred = delegation->cred;
-	delegation->cred = get_rpccred(cred);
-	clear_bit(NFS_DELEGATION_NEED_RECLAIM, &delegation->flags);
-	NFS_I(inode)->delegation_state = delegation->type;
-	smp_wmb();
-	put_rpccred(oldcred);
+	rcu_read_lock();
+	delegation = rcu_dereference(NFS_I(inode)->delegation);
+	if (delegation != NULL) {
+		spin_lock(&delegation->lock);
+		if (delegation->inode != NULL) {
+			memcpy(delegation->stateid.data, res->delegation.data,
+			       sizeof(delegation->stateid.data));
+			delegation->type = res->delegation_type;
+			delegation->maxsize = res->maxsize;
+			oldcred = delegation->cred;
+			delegation->cred = get_rpccred(cred);
+			clear_bit(NFS_DELEGATION_NEED_RECLAIM,
+				  &delegation->flags);
+			NFS_I(inode)->delegation_state = delegation->type;
+			spin_unlock(&delegation->lock);
+			put_rpccred(oldcred);
+			rcu_read_unlock();
+		} else {
+			/* We appear to have raced with a delegation return. */
+			spin_unlock(&delegation->lock);
+			rcu_read_unlock();
+			nfs_inode_set_delegation(inode, cred, res);
+		}
+	} else {
+		rcu_read_unlock();
+	}
 }
 
 static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation *delegation, int issync)
Signature · 1 line
1.7.0
#7 lore raw

PATCH 5/5 NFS: Fix RCU issues in the NFSv4 delegation code

In reply to Paul E. McKenney

From: David Howells <[email protected]>

Fix a number of RCU issues in the NFSv4 delegation code.

(1) delegation->cred doesn't need to be RCU protected as it's essentially an invariant refcounted structure.

By the time we get to nfs_free_delegation(), the delegation is being released, so no one else should be attempting to use the saved credentials, and they can be cleared.

However, since the list of delegations could still be under traversal at this point by such as nfs_client_return_marked_delegations(), the cred should be released in nfs_do_free_delegation() rather than in nfs_free_delegation(). Simply using rcu_assign_pointer() to clear it is insufficient as that doesn't stop the cred from being destroyed, and nor does calling put_rpccred() after call_rcu(), given that the latter is asynchronous.

(2) nfs_detach_delegation_locked() and nfs_inode_set_delegation() should use rcu_derefence_protected() because they can only be called if nfs_client::cl_lock is held, and that guards against anyone changing nfsi->delegation under it. Furthermore, the barrier imposed by rcu_dereference() is superfluous, given that the spin_lock() is also a barrier.

(3) nfs_detach_delegation_locked() is now passed a pointer to the nfs_client

     struct so that it can issue lockdep advice based on clp->cl_lock for (2).

(4) nfs_inode_return_delegation_noreclaim() and nfs_inode_return_delegation() should use rcu_access_pointer() outside the spinlocked region as they merely examine the pointer and don't follow it, thus rendering unnecessary the need to impose a partial ordering over the one item of interest.

These result in an RCU warning like the following:

[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
fs/nfs/delegation.c:332 invoked rcu_dereference_check() without protection!

other info that might help us debug this:

rcu_scheduler_active = 1, debug_locks = 0
2 locks held by mount.nfs4/2281:
#0: (&type->s_umount_key#34){+.+...}, at: [<ffffffff810b25b4>] deactivate_super+0x60/0x80 #1: (iprune_sem){+.+...}, at: [<ffffffff810c332a>] invalidate_inodes+0x39/0x13a

stack backtrace:
Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110
Call Trace:
 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b4591>] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs]
 [<ffffffffa0095d63>] nfs4_clear_inode+0x11/0x1e [nfs]
 [<ffffffff810c2d92>] clear_inode+0x9e/0xf8
 [<ffffffff810c3028>] dispose_list+0x67/0x10e
 [<ffffffff810c340d>] invalidate_inodes+0x11c/0x13a
 [<ffffffff810b1dc1>] generic_shutdown_super+0x42/0xf4
 [<ffffffff810b1ebe>] kill_anon_super+0x11/0x4f
 [<ffffffffa009893c>] nfs4_kill_super+0x3f/0x72 [nfs]
 [<ffffffff810b25bc>] deactivate_super+0x68/0x80
 [<ffffffff810c6744>] mntput_no_expire+0xbb/0xf8
 [<ffffffff810c681b>] release_mounts+0x9a/0xb0
 [<ffffffff810c689b>] put_mnt_ns+0x6a/0x79
 [<ffffffffa00983a1>] nfs_follow_remote_path+0x5a/0x146 [nfs]
 [<ffffffffa0098334>] ? nfs_do_root_mount+0x82/0x95 [nfs]
 [<ffffffffa00985a9>] nfs4_try_mount+0x75/0xaf [nfs]
 [<ffffffffa0098874>] nfs4_get_sb+0x291/0x31a [nfs]
 [<ffffffff810b2059>] vfs_kern_mount+0xb8/0x177
 [<ffffffff810b2176>] do_kern_mount+0x48/0xe8
 [<ffffffff810c810b>] do_mount+0x782/0x7f9
 [<ffffffff810c8205>] sys_mount+0x83/0xbe
 [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b

Also on:

fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection!

 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b4223>] nfs_inode_set_delegation+0xfe/0x219 [nfs]
 [<ffffffffa00a9c6f>] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs]
 [<ffffffffa00aa15d>] nfs4_do_open+0x2a6/0x3a6 [nfs]
 ...

And:

fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection!

 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b3bef>] nfs_free_delegation+0x3d/0x6e [nfs]
 [<ffffffffa00b3e71>] nfs_do_return_delegation+0x26/0x30 [nfs]
 [<ffffffffa00b406a>] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs]
 [<ffffffffa00b448a>] nfs_client_return_marked_delegations+0xc9/0x124 [nfs]
 ...
---
 fs/nfs/delegation.c |   44 +++++++++++++++++++++++---------------------
 1 files changed, 23 insertions(+), 21 deletions(-)
fs/nfs/delegation.c +23 -21
index 8d9ec49..ea61d26 100644
@@ -24,6 +24,8 @@
 
 static void nfs_do_free_delegation(struct nfs_delegation *delegation)
 {
+	if (delegation->cred)
+		put_rpccred(delegation->cred);
 	kfree(delegation);
 }
 
@@ -36,13 +38,7 @@ static void nfs_free_delegation_callback(struct rcu_head *head)
 
 static void nfs_free_delegation(struct nfs_delegation *delegation)
 {
-	struct rpc_cred *cred;
-
-	cred = rcu_dereference(delegation->cred);
-	rcu_assign_pointer(delegation->cred, NULL);
 	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
-	if (cred)
-		put_rpccred(cred);
 }
 
 void nfs_mark_delegation_referenced(struct nfs_delegation *delegation)
@@ -180,9 +176,13 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation
 	return inode;
 }
 
-static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid)
+static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi,
+							   const nfs4_stateid *stateid,
+							   struct nfs_client *clp)
 {
-	struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation);
+	struct nfs_delegation *delegation =
+		rcu_dereference_protected(nfsi->delegation,
+					  lockdep_is_held(&clp->cl_lock));
 
 	if (delegation == NULL)
 		goto nomatch;
@@ -209,7 +209,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 {
 	struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
 	struct nfs_inode *nfsi = NFS_I(inode);
-	struct nfs_delegation *delegation;
+	struct nfs_delegation *delegation, *old_delegation;
 	struct nfs_delegation *freeme = NULL;
 	int status = 0;
 
@@ -227,10 +227,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	spin_lock_init(&delegation->lock);
 
 	spin_lock(&clp->cl_lock);
-	if (rcu_dereference(nfsi->delegation) != NULL) {
-		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
-					sizeof(delegation->stateid)) == 0 &&
-				delegation->type == nfsi->delegation->type) {
+	old_delegation = rcu_dereference_protected(nfsi->delegation,
+						   lockdep_is_held(&clp->cl_lock));
+	if (old_delegation != NULL) {
+		if (memcmp(&delegation->stateid, &old_delegation->stateid,
+					sizeof(old_delegation->stateid)) == 0 &&
+				delegation->type == old_delegation->type) {
 			goto out;
 		}
 		/*
@@ -240,12 +242,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 		dfprintk(FILE, "%s: server %s handed out "
 				"a duplicate delegation!\n",
 				__func__, clp->cl_hostname);
-		if (delegation->type <= nfsi->delegation->type) {
+		if (delegation->type <= old_delegation->type) {
 			freeme = delegation;
 			delegation = NULL;
 			goto out;
 		}
-		freeme = nfs_detach_delegation_locked(nfsi, NULL);
+		freeme = nfs_detach_delegation_locked(nfsi, NULL, clp);
 	}
 	list_add_rcu(&delegation->super_list, &clp->cl_delegations);
 	nfsi->delegation_state = delegation->type;
@@ -315,7 +317,7 @@ restart:
 		if (inode == NULL)
 			continue;
 		spin_lock(&clp->cl_lock);
-		delegation = nfs_detach_delegation_locked(NFS_I(inode), NULL);
+		delegation = nfs_detach_delegation_locked(NFS_I(inode), NULL, clp);
 		spin_unlock(&clp->cl_lock);
 		rcu_read_unlock();
 		if (delegation != NULL) {
@@ -344,9 +346,9 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode)
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_delegation *delegation;
 
-	if (rcu_dereference(nfsi->delegation) != NULL) {
+	if (rcu_access_pointer(nfsi->delegation) != NULL) {
 		spin_lock(&clp->cl_lock);
-		delegation = nfs_detach_delegation_locked(nfsi, NULL);
+		delegation = nfs_detach_delegation_locked(nfsi, NULL, clp);
 		spin_unlock(&clp->cl_lock);
 		if (delegation != NULL)
 			nfs_do_return_delegation(inode, delegation, 0);
@@ -360,9 +362,9 @@ int nfs_inode_return_delegation(struct inode *inode)
 	struct nfs_delegation *delegation;
 	int err = 0;
 
-	if (rcu_dereference(nfsi->delegation) != NULL) {
+	if (rcu_access_pointer(nfsi->delegation) != NULL) {
 		spin_lock(&clp->cl_lock);
-		delegation = nfs_detach_delegation_locked(nfsi, NULL);
+		delegation = nfs_detach_delegation_locked(nfsi, NULL, clp);
 		spin_unlock(&clp->cl_lock);
 		if (delegation != NULL) {
 			nfs_msync_inode(inode);
@@ -540,7 +542,7 @@ restart:
 		if (inode == NULL)
 			continue;
 		spin_lock(&clp->cl_lock);
-		delegation = nfs_detach_delegation_locked(NFS_I(inode), NULL);
+		delegation = nfs_detach_delegation_locked(NFS_I(inode), NULL, clp);
 		spin_unlock(&clp->cl_lock);
 		rcu_read_unlock();
 		if (delegation != NULL)
Signature · 1 line
1.7.0
#8 lore raw
In reply to Paul E. McKenney
Quoted 4 lines · Paul E. McKenney

Hello!

This patchset contains four RCU lockdep splat fixes, courtesy of David Howells, Peter Zijlstra, and Trond Myklebust, [...]

I've applied #1 and #2 - but shouldnt #4 and #5 go via the NFS tree?

Quoted 2 lines

[...] as well as an enhancement by Lai Jiangshan that permits collecting more than one RCU lockdep splat per boot.

Hm, this #3 patch i disagree with quite fundamentally: one of the big virtues of lockdep is that it complains only once and then shuts up and lets the system work. It allows distro debug kernels to have lockdep enabled, etc.

One bugreport per bootup per user is the most we can expect really. Not disabling it risks getting a stream of repeat messages, annoyed testers and gives us _less_ bugreports in the end.

Also, often the _first_ warning is the most reliable one - sometimes there's interactions, and the first bug causing a second warning as well, etc. So reporting just the highest-quality (i.e. first) issue we detect is the best approach.

Thanks,

Ingo

#9 lore raw
Quoted 17 lines · * Ingo Molnar ([email protected]) wrote:
Quoted 4 lines · Paul E. McKenney

Hello!

This patchset contains four RCU lockdep splat fixes, courtesy of David Howells, Peter Zijlstra, and Trond Myklebust, [...]

I've applied #1 and #2 - but shouldnt #4 and #5 go via the NFS tree?

Quoted 2 lines

[...] as well as an enhancement by Lai Jiangshan that permits collecting more than one RCU lockdep splat per boot.

Hm, this #3 patch i disagree with quite fundamentally: one of the big virtues of lockdep is that it complains only once and then shuts up and lets the system work. It allows distro debug kernels to have lockdep enabled, etc.

Quoted 8 lines

One bugreport per bootup per user is the most we can expect really. Not disabling it risks getting a stream of repeat messages, annoyed testers and gives us _less_ bugreports in the end.

Also, often the _first_ warning is the most reliable one - sometimes there's interactions, and the first bug causing a second warning as well, etc. So reporting just the highest-quality (i.e. first) issue we detect is the best approach.

I recommend creating a kernel command line parameter that would tweak the number of messages printed by lockdep. The default would indeed by 1 message, but people in a debugging marathon can specify a larger value so they won't have to reboot between each individual lockdep error.

Thanks,

Mathieu

Quoted 4 lines

Thanks,

Ingo

Signature · 4 lines
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
#10 lore raw
Quoted 4 lines · Mathieu Desnoyers

I recommend creating a kernel command line parameter that would tweak the number of messages printed by lockdep. The default would indeed by 1 message, but people in a debugging marathon can specify a larger value so they won't have to reboot between each individual lockdep error.

Yeah, that would rock for development kernels - playing whack-a-mole with a half-dozen new lockdep whinges can easily stretch out for quite some time.

#11 lore raw
Quoted 9 lines · On Fri, Apr 30, 2010 at 12:16:45PM -0400, [email protected] wrote:
Quoted 4 lines · Mathieu Desnoyers

I recommend creating a kernel command line parameter that would tweak the number of messages printed by lockdep. The default would indeed by 1 message, but people in a debugging marathon can specify a larger value so they won't have to reboot between each individual lockdep error.

Yeah, that would rock for development kernels - playing whack-a-mole with a half-dozen new lockdep whinges can easily stretch out for quite some time.

The RCU-lockdep splats are a bit different in nature than the deadlock-related splats that lockdep normally prints. The RCU-lockdep splats are transient in nature, and it is easy to apply WARN_ON_ONCE(). In contrast, if you permit multiple deadlock-related lockdep splats, you tend to get lots of warnings about the same deadlock cycle.

So how about an additional kernel configuration variable, default disabled, perhaps named CONFIG_PROVE_RCU_MULTIPLE, that allows a single boot to see multiple messages? Unlike the dyntick-idle WARN_ON()s that generated multi-gigabyte console logs in a great hurry, I haven't yet seen excessive quantities of RCU-lockdep splats, so I don't see the need for an integer limit.

Thoughts?

Thanx, Paul

#12 lore raw
Quoted 17 lines · * Paul E. McKenney ([email protected]) wrote:
Quoted 9 lines · On Fri, Apr 30, 2010 at 12:16:45PM -0400, [email protected] wrote:
Quoted 4 lines · Mathieu Desnoyers

I recommend creating a kernel command line parameter that would tweak the number of messages printed by lockdep. The default would indeed by 1 message, but people in a debugging marathon can specify a larger value so they won't have to reboot between each individual lockdep error.

Yeah, that would rock for development kernels - playing whack-a-mole with a half-dozen new lockdep whinges can easily stretch out for quite some time.

The RCU-lockdep splats are a bit different in nature than the deadlock-related splats that lockdep normally prints. The RCU-lockdep splats are transient in nature, and it is easy to apply WARN_ON_ONCE(). In contrast, if you permit multiple deadlock-related lockdep splats, you tend to get lots of warnings about the same deadlock cycle.

Quoted 8 lines

So how about an additional kernel configuration variable, default disabled, perhaps named CONFIG_PROVE_RCU_MULTIPLE, that allows a single boot to see multiple messages? Unlike the dyntick-idle WARN_ON()s that generated multi-gigabyte console logs in a great hurry, I haven't yet seen excessive quantities of RCU-lockdep splats, so I don't see the need for an integer limit.

Thoughts?

Ideally we don't want to flood the console with thousands of instances of the same RCU-lockdep splat (think of a missing read lock on a common code path). Therefore I think keeping an integer limit is relevant here. I agree that this integer limit could be selected by a CONFIG_ option rather than by a kernel parameter, as it will typically only be used on development kernels with "kernel hacking" enabled anyway. There is not much point in bloating the kernel code with an extra debug-only kernel parameter parsing.

Thanks,

Mathieu

Quoted 2 lines

Thanx, Paul

Signature · 4 lines
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
#13 lore raw
Quoted 27 lines · Mathieu Desnoyers
Quoted 17 lines · * Paul E. McKenney ([email protected]) wrote:
Quoted 9 lines · On Fri, Apr 30, 2010 at 12:16:45PM -0400, [email protected] wrote:
Quoted 4 lines · Mathieu Desnoyers

I recommend creating a kernel command line parameter that would tweak the number of messages printed by lockdep. The default would indeed by 1 message, but people in a debugging marathon can specify a larger value so they won't have to reboot between each individual lockdep error.

Yeah, that would rock for development kernels - playing whack-a-mole with a half-dozen new lockdep whinges can easily stretch out for quite some time.

The RCU-lockdep splats are a bit different in nature than the deadlock-related splats that lockdep normally prints. The RCU-lockdep splats are transient in nature, and it is easy to apply WARN_ON_ONCE(). In contrast, if you permit multiple deadlock-related lockdep splats, you tend to get lots of warnings about the same deadlock cycle.

Quoted 8 lines

So how about an additional kernel configuration variable, default disabled, perhaps named CONFIG_PROVE_RCU_MULTIPLE, that allows a single boot to see multiple messages? Unlike the dyntick-idle WARN_ON()s that generated multi-gigabyte console logs in a great hurry, I haven't yet seen excessive quantities of RCU-lockdep splats, so I don't see the need for an integer limit.

Thoughts?

Quoted 8 lines

Ideally we don't want to flood the console with thousands of instances of the same RCU-lockdep splat (think of a missing read lock on a common code path). Therefore I think keeping an integer limit is relevant here. I agree that this integer limit could be selected by a CONFIG_ option rather than by a kernel parameter, as it will typically only be used on development kernels with "kernel hacking" enabled anyway. There is not much point in bloating the kernel code with an extra debug-only kernel parameter parsing.

We already limit via WARN_ON_ONCE(), and there are fewer than 500 lines of code in the kernel that can give RCU lockdep splats, so I really believe that we are OK without an overall limit for the foreseeable future.

Thanx, Paul

#14 lore raw
Quoted 37 lines · * Paul E. McKenney ([email protected]) wrote:
Quoted 27 lines · Mathieu Desnoyers
Quoted 17 lines · * Paul E. McKenney ([email protected]) wrote:
Quoted 9 lines · On Fri, Apr 30, 2010 at 12:16:45PM -0400, [email protected] wrote:
Quoted 4 lines · Mathieu Desnoyers

I recommend creating a kernel command line parameter that would tweak the number of messages printed by lockdep. The default would indeed by 1 message, but people in a debugging marathon can specify a larger value so they won't have to reboot between each individual lockdep error.

Yeah, that would rock for development kernels - playing whack-a-mole with a half-dozen new lockdep whinges can easily stretch out for quite some time.

The RCU-lockdep splats are a bit different in nature than the deadlock-related splats that lockdep normally prints. The RCU-lockdep splats are transient in nature, and it is easy to apply WARN_ON_ONCE(). In contrast, if you permit multiple deadlock-related lockdep splats, you tend to get lots of warnings about the same deadlock cycle.

Quoted 8 lines

So how about an additional kernel configuration variable, default disabled, perhaps named CONFIG_PROVE_RCU_MULTIPLE, that allows a single boot to see multiple messages? Unlike the dyntick-idle WARN_ON()s that generated multi-gigabyte console logs in a great hurry, I haven't yet seen excessive quantities of RCU-lockdep splats, so I don't see the need for an integer limit.

Thoughts?

Quoted 8 lines

Ideally we don't want to flood the console with thousands of instances of the same RCU-lockdep splat (think of a missing read lock on a common code path). Therefore I think keeping an integer limit is relevant here. I agree that this integer limit could be selected by a CONFIG_ option rather than by a kernel parameter, as it will typically only be used on development kernels with "kernel hacking" enabled anyway. There is not much point in bloating the kernel code with an extra debug-only kernel parameter parsing.

Quoted 3 lines

We already limit via WARN_ON_ONCE(), and there are fewer than 500 lines of code in the kernel that can give RCU lockdep splats, so I really believe that we are OK without an overall limit for the foreseeable future.

Your idea makes sense then.

Thanks,

Mathieu

Quoted 2 lines

Thanx, Paul

Signature · 4 lines
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
#15 lore raw
In reply to Ingo Molnar
Quoted 9 lines · Ingo Molnar
Quoted 4 lines · Paul E. McKenney

Hello!

This patchset contains four RCU lockdep splat fixes, courtesy of David Howells, Peter Zijlstra, and Trond Myklebust, [...]

I've applied #1 and #2 - but shouldnt #4 and #5 go via the NFS tree?

Good point -- I will forward them on to Trond.

Thanx, Paul