rcu: improve diagnostics and documentation of source files

PATCH In review Paul E. McKenney 2 participants 12 messages Jan 5, 2010 – Jan 13, 2010

lore

Share & tools

lore

Paul E. McKenney

author

2010-01-05 00:03 UTC #0 lore raw

cover rcu: improve diagnostics and documentation of source files

Hello!

This patch set corrects the file specifiers for the RCU sources in the MAINTAINERS file and also makes a couple of minor improvements to RCU's diagnostics.

Thanx, Paul

 MAINTAINERS             |   14 +++++++-------
 kernel/rcutree.c        |   10 ++++++++--
 kernel/rcutree_plugin.h |    4 +++-
 3 files changed, 18 insertions(+), 10 deletions(-)

Paul E. McKenney

author

2010-01-05 00:04 UTC #1 lore raw

PATCH 1/3 rcu: make MAINTAINERS file match new RCU reality

Both rcutorture and RCU are supported. Also, update the files to match the new layout.

Signed-off-by Paul E. McKenney <[email protected]>

---
 MAINTAINERS |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

MAINTAINERS +7 -6

index e1da925..21c9d3b 100644
@@ -4335,7 +4335,7 @@ F:	drivers/net/wireless/ray*
 RCUTORTURE MODULE
 M:	Josh Triplett <[email protected]>
 M:	"Paul E. McKenney" <[email protected]>
-S:	Maintained
+S:	Supported
 F:	Documentation/RCU/torture.txt
 F:	kernel/rcutorture.c

@@ -4360,11 +4360,12 @@ M:	Dipankar Sarma <[email protected]>
 M:	"Paul E. McKenney" <[email protected]>
 W:	http://www.rdrop.com/users/paulmck/rclock/
 S:	Supported
-F:	Documentation/RCU/rcu.txt
-F:	Documentation/RCU/rcuref.txt
-F:	include/linux/rcupdate.h
-F:	include/linux/srcu.h
-F:	kernel/rcupdate.c
+F:	Documentation/RCU/
+F:	include/linux/rcu*
+F:	include/linux/srcu*
+F:	kernel/rcu*
+F:	kernel/srcu*
+X:	kernel/rcutorture.c
 
 REAL TIME CLOCK DRIVER
 M:	Paul Gortmaker <[email protected]>

Signature · 1 line

1.5.2.5

tip-bot for Paul E. McKenney

author

2010-01-13 10:28 UTC #2 lore raw

[tip:core/rcu] rcu: Make MAINTAINERS file match new RCU reality

Commit-ID:  f9094d8e5587cf21091a9516628147c0b55e4264
Gitweb:     http://git.kernel.org/tip/f9094d8e5587cf21091a9516628147c0b55e4264
Author:     Paul E. McKenney <[email protected]>
AuthorDate: Mon, 4 Jan 2010 16:04:00 -0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Jan 2010 09:06:06 +0100

rcu: Make MAINTAINERS file match new RCU reality

Both rcutorture and RCU are supported. Also, update the files to match the new layout.

Signed-off-by Paul E. McKenney <[email protected]>

Cc [email protected]

LKML-Reference: <12626498422334-git-send-email->

Signed-off-by Ingo Molnar <[email protected]>

---
 MAINTAINERS |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

MAINTAINERS +7 -6

index c8f47bf..175da7c 100644
@@ -4509,7 +4509,7 @@ F:	drivers/net/wireless/ray*
 RCUTORTURE MODULE
 M:	Josh Triplett <[email protected]>
 M:	"Paul E. McKenney" <[email protected]>
-S:	Maintained
+S:	Supported
 F:	Documentation/RCU/torture.txt
 F:	kernel/rcutorture.c

@@ -4534,11 +4534,12 @@ M:	Dipankar Sarma <[email protected]>
 M:	"Paul E. McKenney" <[email protected]>
 W:	http://www.rdrop.com/users/paulmck/rclock/
 S:	Supported
-F:	Documentation/RCU/rcu.txt
-F:	Documentation/RCU/rcuref.txt
-F:	include/linux/rcupdate.h
-F:	include/linux/srcu.h
-F:	kernel/rcupdate.c
+F:	Documentation/RCU/
+F:	include/linux/rcu*
+F:	include/linux/srcu*
+F:	kernel/rcu*
+F:	kernel/srcu*
+X:	kernel/rcutorture.c
 
 REAL TIME CLOCK DRIVER
 M:	Paul Gortmaker <[email protected]>

Paul E. McKenney

author

2010-01-05 00:04 UTC #3 lore raw

PATCH 2/3 rcu: add debug check for too many rcu_read_unlock()

In reply to Paul E. McKenney

From: Paul E. McKenney <[email protected]>

TREE_PREEMPT_RCU maintains an rcu_read_lock_nesting counter in the task structure, which happens to be a signed int. So this patch adds a check for this counter being negative at the end of __rcu_read_unlock(). This check is under CONFIG_PROVE_LOCKING, so can be thought of as being part of lockdep.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcutree_plugin.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

kernel/rcutree_plugin.h +3 -0

index f11ebd4..e77cdf3 100644
@@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
 	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
 	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 		rcu_read_unlock_special(t);
+#ifdef CONFIG_PROVE_LOCKING
+	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);

Signature · 1 line

1.5.2.5

Josh Triplett

2010-01-05 02:03 UTC #4 lore raw

Re: [PATCH tip/core/rcu 2/3] rcu: add debug check for too many rcu_read_unlock()

Quoted 18 lines · Paul E. McKenney

From: Paul E. McKenney <[email protected]>
TREE_PREEMPT_RCU maintains an rcu_read_lock_nesting counter in the task structure, which happens to be a signed int. So this patch adds a check for this counter being negative at the end of __rcu_read_unlock(). This check is under CONFIG_PROVE_LOCKING, so can be thought of as being part of lockdep.
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcutree_plugin.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
kernel/rcutree_plugin.h +0 -0
index f11ebd4..e77cdf3 100644

kernel/rcutree_plugin.h void __rcu_read_unlock(void) · line 311

Quoted 8 lines

kernel/rcutree_plugin.h +3 -0

@@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
 	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
 	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 		rcu_read_unlock_special(t);
+#ifdef CONFIG_PROVE_LOCKING
+	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);

Given that you *already* need to access t->rcu_read_lock_nesting here, why not just do the test all the time? Ideally you could access t->rcu_read_lock_nesting once, decrement it, and test for both 0 and negative.

- Josh Triplett

Paul E. McKenney

author

2010-01-05 02:19 UTC #5 lore raw

Re: [PATCH tip/core/rcu 2/3] rcu: add debug check for too many rcu_read_unlock()

Quoted 24 lines · Josh Triplett

Quoted 23 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
TREE_PREEMPT_RCU maintains an rcu_read_lock_nesting counter in the task structure, which happens to be a signed int. So this patch adds a check for this counter being negative at the end of __rcu_read_unlock(). This check is under CONFIG_PROVE_LOCKING, so can be thought of as being part of lockdep.
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcutree_plugin.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
kernel/rcutree_plugin.h +2 -0
index f11ebd4..e77cdf3 100644
@@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
 	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
 	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 		rcu_read_unlock_special(t);
+#ifdef CONFIG_PROVE_LOCKING
+	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);

Quoted 8 lines

kernel/rcutree_plugin.h void __rcu_read_unlock(void) · line 311
Quoted 3 lines
kernel/rcutree_plugin.h +1 -0
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);
Given that you *already* need to access t->rcu_read_lock_nesting here, why not just do the test all the time? Ideally you could access t->rcu_read_lock_nesting once, decrement it, and test for both 0 and negative.

Because I was paranoid about the extra branch. Perhaps needlessly paranoid, but this is rcu_read_unlock() we are talking about here. ;-)

You seem to be suggesting making the first test be "<=", then sorting things out later, but given that both the equals-zero and the greater-than-zero cases are quite common, I couldn't figure out how to avoid the extra test and branch in the common case. Hence the #ifdef.

Thanx, Paul

Josh Triplett

2010-01-05 02:55 UTC #6 lore raw

Re: [PATCH tip/core/rcu 2/3] rcu: add debug check for too many rcu_read_unlock()

Quoted 34 lines · Paul E. McKenney

Quoted 24 lines · Josh Triplett
Quoted 23 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
TREE_PREEMPT_RCU maintains an rcu_read_lock_nesting counter in the task structure, which happens to be a signed int. So this patch adds a check for this counter being negative at the end of __rcu_read_unlock(). This check is under CONFIG_PROVE_LOCKING, so can be thought of as being part of lockdep.
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcutree_plugin.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
kernel/rcutree_plugin.h +2 -0
index f11ebd4..e77cdf3 100644
@@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
 	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
 	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 		rcu_read_unlock_special(t);
+#ifdef CONFIG_PROVE_LOCKING
+	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
Quoted 8 lines
kernel/rcutree_plugin.h void __rcu_read_unlock(void) · line 311
Quoted 3 lines
kernel/rcutree_plugin.h +1 -0
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);
Given that you *already* need to access t->rcu_read_lock_nesting here, why not just do the test all the time? Ideally you could access t->rcu_read_lock_nesting once, decrement it, and test for both 0 and negative.

Quoted 7 lines

Because I was paranoid about the extra branch. Perhaps needlessly paranoid, but this is rcu_read_unlock() we are talking about here. ;-)
You seem to be suggesting making the first test be "<=", then sorting things out later, but given that both the equals-zero and the greater-than-zero cases are quite common, I couldn't figure out how to avoid the extra test and branch in the common case. Hence the #ifdef.

No, I think you could simply read the predecremented value into a local variable, test it once with == 0, then have the WARN_ON_ONCE, and hope that the compiler figures out it can just test the register once and then do multiple jumps on the same flags.

You could try it and see what code it generates.

- Josh Triplett

Paul E. McKenney

author

2010-01-05 16:21 UTC #7 lore raw

Re: [PATCH tip/core/rcu 2/3] rcu: add debug check for too many rcu_read_unlock()

Quoted 43 lines · Josh Triplett

Quoted 34 lines · Paul E. McKenney
Quoted 24 lines · Josh Triplett
Quoted 23 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
TREE_PREEMPT_RCU maintains an rcu_read_lock_nesting counter in the task structure, which happens to be a signed int. So this patch adds a check for this counter being negative at the end of __rcu_read_unlock(). This check is under CONFIG_PROVE_LOCKING, so can be thought of as being part of lockdep.
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcutree_plugin.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
kernel/rcutree_plugin.h +2 -0
index f11ebd4..e77cdf3 100644
@@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
 	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
 	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 		rcu_read_unlock_special(t);
+#ifdef CONFIG_PROVE_LOCKING
+	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
Quoted 8 lines
kernel/rcutree_plugin.h void __rcu_read_unlock(void) · line 311
Quoted 3 lines
kernel/rcutree_plugin.h +1 -0
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);
Given that you *already* need to access t->rcu_read_lock_nesting here, why not just do the test all the time? Ideally you could access t->rcu_read_lock_nesting once, decrement it, and test for both 0 and negative.
Quoted 7 lines
Because I was paranoid about the extra branch. Perhaps needlessly paranoid, but this is rcu_read_unlock() we are talking about here. ;-)
You seem to be suggesting making the first test be "<=", then sorting things out later, but given that both the equals-zero and the greater-than-zero cases are quite common, I couldn't figure out how to avoid the extra test and branch in the common case. Hence the #ifdef.

Quoted 6 lines

No, I think you could simply read the predecremented value into a local variable, test it once with == 0, then have the WARN_ON_ONCE, and hope that the compiler figures out it can just test the register once and then do multiple jumps on the same flags.
You could try it and see what code it generates.

I agree that a smart compiler could share condition-code state, but there still will be the extra branch. (Keep in mind that this is a .h file, so #ifdef is permitted -- though I might nevertheless make a one-line function/macro.)

Thanx, Paul

Josh Triplett

2010-01-05 17:08 UTC #8 lore raw

Re: [PATCH tip/core/rcu 2/3] rcu: add debug check for too many rcu_read_unlock()

Quoted 49 lines · Paul E. McKenney

Quoted 48 lines · Josh Triplett
Quoted 34 lines · Paul E. McKenney
Quoted 24 lines · Josh Triplett
Quoted 23 lines · Paul E. McKenney
From: Paul E. McKenney <[email protected]>
TREE_PREEMPT_RCU maintains an rcu_read_lock_nesting counter in the task structure, which happens to be a signed int. So this patch adds a check for this counter being negative at the end of __rcu_read_unlock(). This check is under CONFIG_PROVE_LOCKING, so can be thought of as being part of lockdep.
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcutree_plugin.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
kernel/rcutree_plugin.h +2 -0
index f11ebd4..e77cdf3 100644
@@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
 	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
 	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 		rcu_read_unlock_special(t);
+#ifdef CONFIG_PROVE_LOCKING
+	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
Quoted 8 lines
kernel/rcutree_plugin.h void __rcu_read_unlock(void) · line 311
Quoted 3 lines
kernel/rcutree_plugin.h +1 -0
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);
Given that you *already* need to access t->rcu_read_lock_nesting here, why not just do the test all the time? Ideally you could access t->rcu_read_lock_nesting once, decrement it, and test for both 0 and negative.
Quoted 7 lines
Because I was paranoid about the extra branch. Perhaps needlessly paranoid, but this is rcu_read_unlock() we are talking about here. ;-)
You seem to be suggesting making the first test be "<=", then sorting things out later, but given that both the equals-zero and the greater-than-zero cases are quite common, I couldn't figure out how to avoid the extra test and branch in the common case. Hence the #ifdef.
No, I think you could simply read the predecremented value into a local variable, test it once with == 0, then have the WARN_ON_ONCE, and hope that the compiler figures out it can just test the register once and then do multiple jumps on the same flags.

Quoted 6 lines

Quoted 1 line
You could try it and see what code it generates.
I agree that a smart compiler could share condition-code state, but there still will be the extra branch. (Keep in mind that this is a .h file, so #ifdef is permitted -- though I might nevertheless make a one-line function/macro.)

Hmmm. Seems like one untaken branch, with unlikely() even, should prove sufficiently cheap. But, as you said, this is rcu_read_unlock() we are talking about here. ;)

Fair enough; might as well hide it under CONFIG_PROVE_LOCKING, or perhaps some RCU-specific debugging-related CONFIG symbol.

- Josh Triplett

tip-bot for Paul E. McKenney

author

2010-01-13 10:28 UTC #9 lore raw

[tip:core/rcu] rcu: Add debug check for too many rcu_read_unlock()

In reply to Paul E. McKenney

Commit-ID:  cba8244a0f1c277b6b1e48ed6504fa434119e24d
Gitweb:     http://git.kernel.org/tip/cba8244a0f1c277b6b1e48ed6504fa434119e24d
Author:     Paul E. McKenney <[email protected]>
AuthorDate: Mon, 4 Jan 2010 16:04:01 -0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Jan 2010 09:06:06 +0100

rcu: Add debug check for too many rcu_read_unlock()

Signed-off-by Paul E. McKenney <[email protected]>

Cc [email protected]

LKML-Reference: <12626498423064-git-send-email->

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/rcutree_plugin.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

kernel/rcutree_plugin.h +3 -0

index f11ebd4..e77cdf3 100644
@@ -304,6 +304,9 @@ void __rcu_read_unlock(void)
 	if (--ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 &&
 	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 		rcu_read_unlock_special(t);
+#ifdef CONFIG_PROVE_LOCKING
+	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);

Paul E. McKenney

author

2010-01-05 00:04 UTC #10 lore raw

PATCH 3/3 rcu: give different levels of the rcu_node hierarchy distinct lockdep names

In reply to Paul E. McKenney

From: Paul E. McKenney <[email protected]>

Previously, each level of the rcu_node hierarchy had the same rather unimaginative name: "&rcu_node_class[i]". This makes lockdep diagnostics involving these lockdep classes less helpful than would be nice. This patch fixes this by giving each level of the rcu_node hierarchy a distinct name: "rcu_node_level_0", "rcu_node_level_1", and so on. This version of the patch includes improved diagnostics suggested by Josh Triplett and Peter Zijlstra.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcutree.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

kernel/rcutree.c +8 -1

index 0a4c328..3b13d64 100644
@@ -1811,11 +1811,17 @@ static void __init rcu_init_levelspread(struct rcu_state *rsp)
  */
 static void __init rcu_init_one(struct rcu_state *rsp)
 {
+	static char *buf[] = { "rcu_node_level_0",
+			       "rcu_node_level_1",
+			       "rcu_node_level_2",
+			       "rcu_node_level_3" };  /* Match MAX_RCU_LVLS */
 	int cpustride = 1;
 	int i;
 	int j;
 	struct rcu_node *rnp;
 
+	BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf));  /* Fix buf[] init! */
+
 	/* Initialize the level-tracking arrays. */
 
 	for (i = 1; i < NUM_RCU_LVLS; i++)

@@ -1829,7 +1835,8 @@ static void __init rcu_init_one(struct rcu_state *rsp)
 		rnp = rsp->level[i];
 		for (j = 0; j < rsp->levelcnt[i]; j++, rnp++) {
 			spin_lock_init(&rnp->lock);
-			lockdep_set_class(&rnp->lock, &rcu_node_class[i]);
+			lockdep_set_class_and_name(&rnp->lock,
+						   &rcu_node_class[i], buf[i]);
 			rnp->gpnum = 0;
 			rnp->qsmask = 0;
 			rnp->qsmaskinit = 0;

Signature · 1 line

1.5.2.5

tip-bot for Paul E. McKenney

author

2010-01-13 10:28 UTC #11 lore raw

[tip:core/rcu] rcu: Give different levels of the rcu_node hierarchy distinct lockdep names

Commit-ID:  b6407e863934965cdc66cbc244d811ceeb6f4d77
Gitweb:     http://git.kernel.org/tip/b6407e863934965cdc66cbc244d811ceeb6f4d77
Author:     Paul E. McKenney <[email protected]>
AuthorDate: Mon, 4 Jan 2010 16:04:02 -0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Jan 2010 09:06:07 +0100

rcu: Give different levels of the rcu_node hierarchy distinct lockdep names

Signed-off-by Paul E. McKenney <[email protected]>

Cc [email protected]

LKML-Reference: <12626498421830-git-send-email->

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/rcutree.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

kernel/rcutree.c +8 -1

index 0a4c328..3b13d64 100644
@@ -1811,11 +1811,17 @@ static void __init rcu_init_levelspread(struct rcu_state *rsp)
  */
 static void __init rcu_init_one(struct rcu_state *rsp)
 {
+	static char *buf[] = { "rcu_node_level_0",
+			       "rcu_node_level_1",
+			       "rcu_node_level_2",
+			       "rcu_node_level_3" };  /* Match MAX_RCU_LVLS */
 	int cpustride = 1;
 	int i;
 	int j;
 	struct rcu_node *rnp;
 
+	BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf));  /* Fix buf[] init! */
+
 	/* Initialize the level-tracking arrays. */
 
 	for (i = 1; i < NUM_RCU_LVLS; i++)

@@ -1829,7 +1835,8 @@ static void __init rcu_init_one(struct rcu_state *rsp)
 		rnp = rsp->level[i];
 		for (j = 0; j < rsp->levelcnt[i]; j++, rnp++) {
 			spin_lock_init(&rnp->lock);
-			lockdep_set_class(&rnp->lock, &rcu_node_class[i]);
+			lockdep_set_class_and_name(&rnp->lock,
+						   &rcu_node_class[i], buf[i]);
 			rnp->gpnum = 0;
 			rnp->qsmask = 0;
 			rnp->qsmaskinit = 0;