Preview of RCU patches for 2.6.39

kernel/rcutiny_plugin.h

Quoted 5 lines · Paul E. McKenney

kernel/rcutiny_plugin.h +0 -0

index 015abae..3cb8e36 100644

My original patch was just for tree RCU. It is not merged.
I forgot the tiny RCU case then, this change is needed for tree & tiny RCU.

Quoted 9 lines · Lai Jiangshan

kernel/rcutiny_plugin.h

Quoted 5 lines · Paul E. McKenney

kernel/rcutiny_plugin.h +0 -0

index 015abae..3cb8e36 100644

My original patch was just for tree RCU. It is not merged.
I forgot the tiny RCU case then, this change is needed for tree & tiny RCU.

Good catch, I have applied the TREE_RCU patch. I intend to submit both for the next merge window.

Quoted 11 lines · Paul E. McKenney

Quoted 9 lines · Lai Jiangshan

kernel/rcutiny_plugin.h

Quoted 5 lines · Paul E. McKenney

kernel/rcutiny_plugin.h +0 -0

index 015abae..3cb8e36 100644

My original patch was just for tree RCU. It is not merged.
I forgot the tiny RCU case then, this change is needed for tree & tiny RCU.

Quoted 3 lines

Good catch, I have applied the TREE_RCU patch. I intend to submit both for the next merge window.

Hi, Paul

It is still not merged, and it is not in your git tree.

I'm writing some patches, to avoid conflict with your code, which branch I should based on?

Is origin/rcu/next OK?

Quoted 12 lines · Lai Jiangshan

Quoted 11 lines · Paul E. McKenney

Quoted 9 lines · Lai Jiangshan

kernel/rcutiny_plugin.h

Quoted 5 lines · Paul E. McKenney

kernel/rcutiny_plugin.h +0 -0

index 015abae..3cb8e36 100644

My original patch was just for tree RCU. It is not merged.
I forgot the tiny RCU case then, this change is needed for tree & tiny RCU.

Quoted 7 lines

Quoted 3 lines

Good catch, I have applied the TREE_RCU patch. I intend to submit both for the next merge window.

Hi, Paul

It is still not merged, and it is not in your git tree.

Indeed it is not yet in my external git tree. Here is my normal work flow:

1. Develop and test continuously.

2. Based on testing, review, etc., determin which commits are
ready for the next merge window.

3. Midway through the -rc releases, push the commits targeted
to the next merge window to rcu/next. Push any additional commits to rcu/testing.

4. Towards the last -rc, rebase, retest, wait for the -next
tree to take the changes, and, if all is well, send Ingo a pull request.

Exposing your changes to -next during the merge window would cause pointless irritation due to random conflicts with rapidly changing code.

Quoted 2 lines

I'm writing some patches, to avoid conflict with your code, which branch I should based on?

But, yes, it would be much easier for us to handle your changes if I post them via -rcu. So I have created a new branch rcu/kfree_rcu that is -not- consumed by -next. The rcu/next and rcu/testing branches include your base kfree_rcu() patch, but not the uses of it. These branches are based on 2.6.38, and I will move them ahead at -rc3 or -rc4.

These currently contain your original patches, but I will be pulling your latest set.

Quoted 1 line

Is origin/rcu/next OK?

origin/rcu/kfree_rcu for the moment to avoid messing up -next. But it will be origin/rcu/next after rebasing onto -rc3 or -rc4.

Thanx, Paul

From: Amerigo Wang <[email protected]>

DEBUG_OBJECTS_RCU_HEAD depends on PREEMPT, so #ifndef CONFIG_PREEMPT is totally useless in kernel/rcupdate.c.

Signed-off-by WANG Cong <[email protected]>

Cc Paul E. McKenney <[email protected]>

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

---
 kernel/rcupdate.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

kernel/rcupdate.c +0 -5

index a23a57a..afd21d1 100644
@@ -215,10 +215,6 @@ static int rcuhead_fixup_free(void *addr, enum debug_obj_state state)
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
 		 */
-#ifndef CONFIG_PREEMPT
-		WARN_ON(1);
-		return 0;
-#else
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -229,7 +225,6 @@ static int rcuhead_fixup_free(void *addr, enum debug_obj_state state)
 		rcu_barrier_bh();
 		debug_object_free(head, &rcuhead_debug_descr);
 		return 1;
-#endif
 	default:
 		return 0;
 	}

Signature · 1 line

1.7.3.2

The build will break if you change the Kconfig to allow DEBUG_OBJECTS_RCU_HEAD and !PREEMPT, so document the reasoning near where the breakage would occur.

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

---
 kernel/rcupdate.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

kernel/rcupdate.c +0 -0

index afd21d1..f3240e9 100644

kernel/rcupdate.c +0 -0

 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {

Quoted 18 lines · Paul E. McKenney

The build will break if you change the Kconfig to allow DEBUG_OBJECTS_RCU_HEAD and !PREEMPT, so document the reasoning near where the breakage would occur.

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

---
 kernel/rcupdate.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

kernel/rcupdate.c +1 -0

index afd21d1..f3240e9 100644
@@ -214,6 +214,11 @@ static int rcuhead_fixup_free(void *addr, enum debug_obj_state state)
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * Note that the machinery to reliably determine whether

kernel/rcupdate.c static int rcuhead_fixup_free(void *addr, enum debug_obj_state state) · line 222

Quoted 5 lines

kernel/rcupdate.c +4 -0

+		 * or not we are in an RCU read-side critical section
+		 * exists only in the preemptible RCU implementations
+		 * (TINY_PREEMPT_RCU and TREE_PREEMPT_RCU), which is why
+		 * DEBUG_OBJECTS_RCU_HEAD is disallowed if !PREEMPT.
 		 */

Shouldn't this comment also be in the kconfig where DEBUG_OBJECTS_RCU_HEAD is defined?

-- Steve

Quoted 2 lines

kernel/rcupdate.c +0 -0

 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {

Quoted 19 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 18 lines · Paul E. McKenney

The build will break if you change the Kconfig to allow DEBUG_OBJECTS_RCU_HEAD and !PREEMPT, so document the reasoning near where the breakage would occur.

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

---
 kernel/rcupdate.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

kernel/rcupdate.c +1 -0

index afd21d1..f3240e9 100644
@@ -214,6 +214,11 @@ static int rcuhead_fixup_free(void *addr, enum debug_obj_state state)
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * Note that the machinery to reliably determine whether

Quoted 8 lines

kernel/rcupdate.c static int rcuhead_fixup_free(void *addr, enum debug_obj_state state) · line 222

Quoted 5 lines

kernel/rcupdate.c +4 -0

+		 * or not we are in an RCU read-side critical section
+		 * exists only in the preemptible RCU implementations
+		 * (TINY_PREEMPT_RCU and TREE_PREEMPT_RCU), which is why
+		 * DEBUG_OBJECTS_RCU_HEAD is disallowed if !PREEMPT.
 		 */

Shouldn't this comment also be in the kconfig where DEBUG_OBJECTS_RCU_HEAD is defined?

hrm, but this is a "rcuhead_fixup_init" : it does not need to always

succeed. It's just that when it is safe to recover from an error situation, it does it. We could do:

#ifdef CONFIG_PREEMPT
                /*
                 * Ensure that queued callbacks are all executed.
                 * If we detect that we are nested in a RCU read-side
                 * critical
                 * section, we should simply fail, otherwise we would
                 * deadlock.
                 */
                if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
                    irqs_disabled()) {
                        WARN_ON(1);
                        return 0;
                }
                rcu_barrier();
                rcu_barrier_sched();
                rcu_barrier_bh();
                debug_object_init(head, &rcuhead_debug_descr);
                return 1;
#else
		return 0;

#endif

instead, no ?

Quoted 29 lines · Mathieu Desnoyers

Quoted 27 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 14 lines · Paul E. McKenney

The build will break if you change the Kconfig to allow DEBUG_OBJECTS_RCU_HEAD and !PREEMPT, so document the reasoning near where the breakage would occur.

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

---
 kernel/rcupdate.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

kernel/rcupdate.c +0 -0

index afd21d1..f3240e9 100644

kernel/rcupdate.c static int rcuhead_fixup_free(void *addr, enum debug_obj_state state) · line 222

Quoted 9 lines

kernel/rcupdate.c +5 -0

@@ -214,6 +214,11 @@ static int rcuhead_fixup_free(void *addr, enum debug_obj_state state)
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * Note that the machinery to reliably determine whether
+		 * or not we are in an RCU read-side critical section
+		 * exists only in the preemptible RCU implementations
+		 * (TINY_PREEMPT_RCU and TREE_PREEMPT_RCU), which is why
+		 * DEBUG_OBJECTS_RCU_HEAD is disallowed if !PREEMPT.
 		 */

Shouldn't this comment also be in the kconfig where DEBUG_OBJECTS_RCU_HEAD is defined?

Quoted 1 line

hrm, but this is a "rcuhead_fixup_init" : it does not need to always

Looks like it was in rcuhead_fixup_free to me.

Quoted 26 lines

succeed. It's just that when it is safe to recover from an error situation, it does it. We could do:

#ifdef CONFIG_PREEMPT
                /*
                 * Ensure that queued callbacks are all executed.
                 * If we detect that we are nested in a RCU read-side
                 * critical
                 * section, we should simply fail, otherwise we would
                 * deadlock.
                 */
                if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
                    irqs_disabled()) {
                        WARN_ON(1);
                        return 0;
                }
                rcu_barrier();
                rcu_barrier_sched();
                rcu_barrier_bh();
                debug_object_init(head, &rcuhead_debug_descr);
                return 1;
#else
		return 0;

#endif

Quoted 1 line

instead, no ?

The point is that this entire block of code is wrapped in #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD, and that config depends on PREEMPT. Thus you will never have a case where #ifdef CONFIG_PREEMPT is false.

-- Steve

#ifdef CONFIG_PREEMPT
                /*
                 * Ensure that queued callbacks are all executed.
                 * If we detect that we are nested in a RCU read-side
                 * critical
                 * section, we should simply fail, otherwise we would
                 * deadlock.
                 */
                if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
                    irqs_disabled()) {
                        WARN_ON(1);
                        return 0;
                }
                rcu_barrier();
                rcu_barrier_sched();
                rcu_barrier_bh();
                debug_object_init(head, &rcuhead_debug_descr);
                return 1;
#else
		return 0;

#endif

instead, no ?

Quoted 20 lines · Mathieu Desnoyers

#ifdef CONFIG_PREEMPT
                /*
                 * Ensure that queued callbacks are all executed.
                 * If we detect that we are nested in a RCU read-side
                 * critical
                 * section, we should simply fail, otherwise we would
                 * deadlock.
                 */
                if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
                    irqs_disabled()) {
                        WARN_ON(1);
                        return 0;
                }
                rcu_barrier();
                rcu_barrier_sched();
                rcu_barrier_bh();
                debug_object_init(head, &rcuhead_debug_descr);
                return 1;
#else
		return 0;

Quoted 3 lines

#endif

instead, no ?

BTW, if you do end up doing such a thing...

#ifndef CONFIG_PREEMPT
	return 0;

#endif

before all that would be much cleaner.

Just make sure any non defined in PREEMPT macros/functions are defined as nops in !CONFIG_PREEMPT

-- Steve

Remove DEBUG_RCU_HEAD dependency on PREEMPT config. Handle the unability to detect if within a RCU read-side critical section by never performing any attempt to recover from a failure situation in the fixup handlers. Just print the warnings.

This patch is only compile-tested.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 kernel/rcupdate.c |   17 +++++++++++++++++
 lib/Kconfig.debug |    2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================

lib/Kconfig.debug +1 -1

@@ -313,7 +313,7 @@ config DEBUG_OBJECTS_WORK
 
 config DEBUG_OBJECTS_RCU_HEAD
 	bool "Debug RCU callbacks objects"
-	depends on DEBUG_OBJECTS && PREEMPT
+	depends on DEBUG_OBJECTS
 	help
 	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
 

Index: linux-2.6-lttng/kernel/rcupdate.c
===================================================================

kernel/rcupdate.c +14 -0

@@ -142,7 +142,14 @@ static int rcuhead_fixup_init(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -184,7 +191,14 @@ static int rcuhead_fixup_activate(void *
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

Quoted 65 lines · Mathieu Desnoyers

Remove DEBUG_RCU_HEAD dependency on PREEMPT config. Handle the unability to detect if within a RCU read-side critical section by never performing any attempt to recover from a failure situation in the fixup handlers. Just print the warnings.

This patch is only compile-tested.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 kernel/rcupdate.c |   17 +++++++++++++++++
 lib/Kconfig.debug |    2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================

lib/Kconfig.debug +1 -1

@@ -313,7 +313,7 @@ config DEBUG_OBJECTS_WORK
 
 config DEBUG_OBJECTS_RCU_HEAD
 	bool "Debug RCU callbacks objects"
-	depends on DEBUG_OBJECTS && PREEMPT
+	depends on DEBUG_OBJECTS
 	help
 	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
 

Index: linux-2.6-lttng/kernel/rcupdate.c
===================================================================

kernel/rcupdate.c +15 -0

@@ -142,7 +142,14 @@ static int rcuhead_fixup_init(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -184,7 +191,14 @@ static int rcuhead_fixup_activate(void *
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -214,6 +228,9 @@ static int rcuhead_fixup_free(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are

kernel/rcupdate.c static int rcuhead_fixup_free(void *addr · line 236

Quoted 5 lines

kernel/rcupdate.c +2 -0

+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
 #ifndef CONFIG_PREEMPT
 		WARN_ON(1);

Hmm, I wonder if s/WARN_ON/WARN_ON_ONCE/g is in order. Why spam the console if it happens to trigger all the time?

-- Steve

Quoted 66 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 65 lines · Mathieu Desnoyers

Remove DEBUG_RCU_HEAD dependency on PREEMPT config. Handle the unability to detect if within a RCU read-side critical section by never performing any attempt to recover from a failure situation in the fixup handlers. Just print the warnings.

This patch is only compile-tested.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 kernel/rcupdate.c |   17 +++++++++++++++++
 lib/Kconfig.debug |    2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================

lib/Kconfig.debug +1 -1

@@ -313,7 +313,7 @@ config DEBUG_OBJECTS_WORK
 
 config DEBUG_OBJECTS_RCU_HEAD
 	bool "Debug RCU callbacks objects"
-	depends on DEBUG_OBJECTS && PREEMPT
+	depends on DEBUG_OBJECTS
 	help
 	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
 

Index: linux-2.6-lttng/kernel/rcupdate.c
===================================================================

kernel/rcupdate.c +15 -0

@@ -142,7 +142,14 @@ static int rcuhead_fixup_init(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -184,7 +191,14 @@ static int rcuhead_fixup_activate(void *
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -214,6 +228,9 @@ static int rcuhead_fixup_free(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are

Quoted 8 lines

kernel/rcupdate.c static int rcuhead_fixup_free(void *addr · line 236

Quoted 5 lines

kernel/rcupdate.c +2 -0

+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
 #ifndef CONFIG_PREEMPT
 		WARN_ON(1);

Hmm, I wonder if s/WARN_ON/WARN_ON_ONCE/g is in order. Why spam the console if it happens to trigger all the time?

The system should die pretty soon anyway due to list corruption, so I don't think it's a problem in practice.

Quoted 76 lines · Mathieu Desnoyers

Quoted 74 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 61 lines · Mathieu Desnoyers

Remove DEBUG_RCU_HEAD dependency on PREEMPT config. Handle the unability to detect if within a RCU read-side critical section by never performing any attempt to recover from a failure situation in the fixup handlers. Just print the warnings.

This patch is only compile-tested.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 kernel/rcupdate.c |   17 +++++++++++++++++
 lib/Kconfig.debug |    2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================

lib/Kconfig.debug +1 -1

@@ -313,7 +313,7 @@ config DEBUG_OBJECTS_WORK
 
 config DEBUG_OBJECTS_RCU_HEAD
 	bool "Debug RCU callbacks objects"
-	depends on DEBUG_OBJECTS && PREEMPT
+	depends on DEBUG_OBJECTS
 	help
 	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
 

Index: linux-2.6-lttng/kernel/rcupdate.c
===================================================================

kernel/rcupdate.c +14 -0

@@ -142,7 +142,14 @@ static int rcuhead_fixup_init(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -184,7 +191,14 @@ static int rcuhead_fixup_activate(void *
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

kernel/rcupdate.c static int rcuhead_fixup_free(void *addr · line 236

Quoted 9 lines

kernel/rcupdate.c +3 -0

@@ -214,6 +228,9 @@ static int rcuhead_fixup_free(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
 #ifndef CONFIG_PREEMPT
 		WARN_ON(1);

Hmm, I wonder if s/WARN_ON/WARN_ON_ONCE/g is in order. Why spam the console if it happens to trigger all the time?

Quoted 2 lines

The system should die pretty soon anyway due to list corruption, so I don't think it's a problem in practice.

Well, it would add noise, so I added a patch converting the WARN_ON's to WARN_ON_ONCE's.

Remove DEBUG_RCU_HEAD dependency on PREEMPT config. Handle the unability to detect if within a RCU read-side critical section by never performing any attempt to recover from a failure situation in the fixup handlers. Just print the warnings.

This patch is only compile-tested.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 kernel/rcupdate.c |   17 +++++++++++++++++
 lib/Kconfig.debug |    2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================

lib/Kconfig.debug +1 -1

@@ -313,7 +313,7 @@ config DEBUG_OBJECTS_WORK
 
 config DEBUG_OBJECTS_RCU_HEAD
 	bool "Debug RCU callbacks objects"
-	depends on DEBUG_OBJECTS && PREEMPT
+	depends on DEBUG_OBJECTS
 	help
 	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
 

Index: linux-2.6-lttng/kernel/rcupdate.c
===================================================================

kernel/rcupdate.c +17 -0

@@ -142,7 +142,14 @@ static int rcuhead_fixup_init(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -184,7 +191,14 @@ static int rcuhead_fixup_activate(void *
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
+#ifndef CONFIG_PREEMPT
+		WARN_ON(1);
+		return 0;
+#endif
 		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
 		    irqs_disabled()) {
 			WARN_ON(1);

@@ -214,6 +228,9 @@ static int rcuhead_fixup_free(void *addr
 		 * Ensure that queued callbacks are all executed.
 		 * If we detect that we are nested in a RCU read-side critical
 		 * section, we should simply fail, otherwise we would deadlock.
+		 * In !PREEMPT configurations, there is no way to tell if we are
+		 * in a RCU read-side critical section or not, so we never
+		 * attempt any fixup and just print a warning.
 		 */
 #ifndef CONFIG_PREEMPT
 		WARN_ON(1);

Signature · 4 lines

Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

kernel/rcutree_trace.c +9 -5

 	retval = debugfs_create_file("rcudata", 0444, rcudir,
 						NULL, &rcudata_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 
 	retval = debugfs_create_file("rcudata.csv", 0444, rcudir,
 						NULL, &rcudata_csv_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
+		goto free_out;
+
+	retval = rcu_boost_trace_create_file(rcudir);
+	if (retval)
 		goto free_out;
 
 	retval = debugfs_create_file("rcugp", 0444, rcudir, NULL, &rcugp_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 
 	retval = debugfs_create_file("rcuhier", 0444, rcudir,
 						NULL, &rcuhier_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 
 	retval = debugfs_create_file("rcu_pending", 0444, rcudir,
 						NULL, &rcu_pending_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 	return 0;
 free_out:

kernel/rcutree_trace.c static int __init rcutree_trace_init(void) · line 372

Quoted 4 lines · Paul E. McKenney

kernel/rcutree_trace.c +1 -1

 	rcudir = debugfs_create_dir("rcu", NULL);
-	if (!rcudir)
+	if (IS_ERR_VALUE(rcudir))
 		goto free_out;

if !defined(CONFIG_DEBUG_FS)

	debugfs_create_xxx() returns ERR_PTR(-ENODEV);

else
debugfs_create_xxx() returns NULL when failed.

Since TREE_RCU_TRACE selects DEBUG_FS, "if (!rcudir)" is correct, we don't need to change it.

Quoted 24 lines

kernel/rcutree_trace.c +7 -4

 	retval = debugfs_create_file("rcudata", 0444, rcudir,
 						NULL, &rcudata_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 
 	retval = debugfs_create_file("rcudata.csv", 0444, rcudir,
 						NULL, &rcudata_csv_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
+		goto free_out;
+
+	retval = rcu_boost_trace_create_file(rcudir);
+	if (retval)
 		goto free_out;
 
 	retval = debugfs_create_file("rcugp", 0444, rcudir, NULL, &rcugp_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 
 	retval = debugfs_create_file("rcuhier", 0444, rcudir,
 						NULL, &rcuhier_fops);
-	if (!retval)

kernel/rcutree_trace.c static int __init rcutree_trace_init(void) · line 402

Quoted 10 lines

kernel/rcutree_trace.c +2 -1

+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 
 	retval = debugfs_create_file("rcu_pending", 0444, rcudir,
 						NULL, &rcu_pending_fops);
-	if (!retval)
+	if (IS_ERR_VALUE(retval))
 		goto free_out;
 	return 0;
 free_out:

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Transitivity is guaranteed only for full memory barriers (smp_mb()).

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

---
 Documentation/memory-barriers.txt |   58 +++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

Documentation/memory-barriers.txt +43 -0

index 631ad2f..f0d3a80 100644
@@ -21,6 +21,7 @@ Contents:
      - SMP barrier pairing.
      - Examples of memory barrier sequences.
      - Read memory barriers vs load speculation.
+     - Transitivity
 
  (*) Explicit kernel barriers.
 

@@ -959,6 +960,63 @@ the speculation will be cancelled and the value reloaded:
 	retrieved                               :       :       +-------+
 
 
+TRANSITIVITY
+------------
+
+Transitivity is a deeply intuitive notion about ordering that is not
+always provided by real computer systems.  The following example
+demonstrates transitivity (also called "cumulativity"):
+
+	CPU 1			CPU 2			CPU 3
+	=======================	=======================	=======================
+		{ X = 0, Y = 0 }
+	STORE X=1		LOAD X			STORE Y=1
+				<general barrier>	<general barrier>
+				LOAD Y			LOAD X
+
+Suppose that CPU 2's load from X returns 1 and its load from Y returns 0.
+This indicates that CPU 2's load from X in some sense follows CPU 1's
+store to X and that CPU 2's load from Y in some sense preceded CPU 3's
+store to Y.  The question is then "Can CPU 3's load from X return 0?"
+
+Because CPU 2's load from X in some sense came after CPU 1's store, it
+is natural to expect that CPU 3's load from X must therefore return 1.
+This expectation is an example of transitivity: if a load executing on
+CPU A follows a load from the same variable executing on CPU B, then
+CPU A's load must either return the same value that CPU B's load did,
+or must return some later value.
+
+In the Linux kernel, use of general memory barriers guarantees
+transitivity.  Therefore, in the above example, if CPU 2's load from X
+returns 1 and its load from Y returns 0, then CPU 3's load from X must
+also return 1.
+
+However, transitivity is -not- guaranteed for read or write barriers.
+For example, suppose that CPU 2's general barrier in the above example
+is changed to a read barrier as shown below:
+
+	CPU 1			CPU 2			CPU 3
+	=======================	=======================	=======================
+		{ X = 0, Y = 0 }
+	STORE X=1		LOAD X			STORE Y=1
+				<read barrier>		<general barrier>
+				LOAD Y			LOAD X
+

Documentation/memory-barriers.txt +2 -0

+
+
 ========================
 EXPLICIT KERNEL BARRIERS
 ========================

Quoted 65 lines · Paul E. McKenney

Transitivity is guaranteed only for full memory barriers (smp_mb()).

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

---
 Documentation/memory-barriers.txt |   58 +++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

Documentation/memory-barriers.txt +43 -0

index 631ad2f..f0d3a80 100644
@@ -21,6 +21,7 @@ Contents:
      - SMP barrier pairing.
      - Examples of memory barrier sequences.
      - Read memory barriers vs load speculation.
+     - Transitivity
 
  (*) Explicit kernel barriers.
 

@@ -959,6 +960,63 @@ the speculation will be cancelled and the value reloaded:
 	retrieved                               :       :       +-------+
 
 
+TRANSITIVITY
+------------
+
+Transitivity is a deeply intuitive notion about ordering that is not
+always provided by real computer systems.  The following example
+demonstrates transitivity (also called "cumulativity"):
+
+	CPU 1			CPU 2			CPU 3
+	=======================	=======================	=======================
+		{ X = 0, Y = 0 }
+	STORE X=1		LOAD X			STORE Y=1
+				<general barrier>	<general barrier>
+				LOAD Y			LOAD X
+
+Suppose that CPU 2's load from X returns 1 and its load from Y returns 0.
+This indicates that CPU 2's load from X in some sense follows CPU 1's
+store to X and that CPU 2's load from Y in some sense preceded CPU 3's
+store to Y.  The question is then "Can CPU 3's load from X return 0?"
+
+Because CPU 2's load from X in some sense came after CPU 1's store, it
+is natural to expect that CPU 3's load from X must therefore return 1.
+This expectation is an example of transitivity: if a load executing on
+CPU A follows a load from the same variable executing on CPU B, then
+CPU A's load must either return the same value that CPU B's load did,
+or must return some later value.
+
+In the Linux kernel, use of general memory barriers guarantees
+transitivity.  Therefore, in the above example, if CPU 2's load from X
+returns 1 and its load from Y returns 0, then CPU 3's load from X must
+also return 1.
+
+However, transitivity is -not- guaranteed for read or write barriers.
+For example, suppose that CPU 2's general barrier in the above example
+is changed to a read barrier as shown below:
+
+	CPU 1			CPU 2			CPU 3
+	=======================	=======================	=======================
+		{ X = 0, Y = 0 }
+	STORE X=1		LOAD X			STORE Y=1
+				<read barrier>		<general barrier>
+				LOAD Y			LOAD X
+

Documentation/memory-barriers.txt the speculation will be cancelled and the value reloaded: · line 1014

Quoted 10 lines

Documentation/memory-barriers.txt +10 -0

+This substitution destroys transitivity: in this example, it is perfectly
+legal for CPU 2's load from X to return 1, its load from Y to return 0,
+and CPU 3's load from X to return 0.
+
+The key point is that although CPU 2's read barrier orders its pair
+of loads, it does not guarantee to order CPU 1's store.  Therefore, if
+this example runs on a system where CPUs 1 and 2 share a store buffer
+or a level of cache, CPU 2 might have early access to CPU 1's writes.
+General barriers are therefore required to ensure that all CPUs agree
+on the combined order of CPU 1's and CPU 2's accesses.

Sounds like someone had a fun time debugging their code.

Documentation/memory-barriers.txt the speculation will be cancelled and the value reloaded: · line 1017

Quoted 3 lines

Documentation/memory-barriers.txt +3 -0

+
+To reiterate, if your code requires transitivity, use general barriers
+throughout.

I expect that your code is the only code in the kernel that actually requires transitivity ;-)

Quoted 79 lines · Steven Rostedt

Quoted 65 lines · Paul E. McKenney

Transitivity is guaranteed only for full memory barriers (smp_mb()).

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

---
 Documentation/memory-barriers.txt |   58 +++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

Documentation/memory-barriers.txt +43 -0

index 631ad2f..f0d3a80 100644
@@ -21,6 +21,7 @@ Contents:
      - SMP barrier pairing.
      - Examples of memory barrier sequences.
      - Read memory barriers vs load speculation.
+     - Transitivity
 
  (*) Explicit kernel barriers.
 

@@ -959,6 +960,63 @@ the speculation will be cancelled and the value reloaded:
 	retrieved                               :       :       +-------+
 
 
+TRANSITIVITY
+------------
+
+Transitivity is a deeply intuitive notion about ordering that is not
+always provided by real computer systems.  The following example
+demonstrates transitivity (also called "cumulativity"):
+
+	CPU 1			CPU 2			CPU 3
+	=======================	=======================	=======================
+		{ X = 0, Y = 0 }
+	STORE X=1		LOAD X			STORE Y=1
+				<general barrier>	<general barrier>
+				LOAD Y			LOAD X
+
+Suppose that CPU 2's load from X returns 1 and its load from Y returns 0.
+This indicates that CPU 2's load from X in some sense follows CPU 1's
+store to X and that CPU 2's load from Y in some sense preceded CPU 3's
+store to Y.  The question is then "Can CPU 3's load from X return 0?"
+
+Because CPU 2's load from X in some sense came after CPU 1's store, it
+is natural to expect that CPU 3's load from X must therefore return 1.
+This expectation is an example of transitivity: if a load executing on
+CPU A follows a load from the same variable executing on CPU B, then
+CPU A's load must either return the same value that CPU B's load did,
+or must return some later value.
+
+In the Linux kernel, use of general memory barriers guarantees
+transitivity.  Therefore, in the above example, if CPU 2's load from X
+returns 1 and its load from Y returns 0, then CPU 3's load from X must
+also return 1.
+
+However, transitivity is -not- guaranteed for read or write barriers.
+For example, suppose that CPU 2's general barrier in the above example
+is changed to a read barrier as shown below:
+
+	CPU 1			CPU 2			CPU 3
+	=======================	=======================	=======================
+		{ X = 0, Y = 0 }
+	STORE X=1		LOAD X			STORE Y=1
+				<read barrier>		<general barrier>
+				LOAD Y			LOAD X
+

Documentation/memory-barriers.txt the speculation will be cancelled and the value reloaded: · line 1014

Quoted 10 lines

Documentation/memory-barriers.txt +10 -0

+This substitution destroys transitivity: in this example, it is perfectly
+legal for CPU 2's load from X to return 1, its load from Y to return 0,
+and CPU 3's load from X to return 0.
+
+The key point is that although CPU 2's read barrier orders its pair
+of loads, it does not guarantee to order CPU 1's store.  Therefore, if
+this example runs on a system where CPUs 1 and 2 share a store buffer
+or a level of cache, CPU 2 might have early access to CPU 1's writes.
+General barriers are therefore required to ensure that all CPUs agree
+on the combined order of CPU 1's and CPU 2's accesses.

Sounds like someone had a fun time debugging their code.

Quoted 6 lines

Documentation/memory-barriers.txt the speculation will be cancelled and the value reloaded: · line 1017

Quoted 3 lines

Documentation/memory-barriers.txt +3 -0

+
+To reiterate, if your code requires transitivity, use general barriers
+throughout.

I expect that your code is the only code in the kernel that actually requires transitivity ;-)

Maybe, but my RCURING also requires transitivity, I had asked Paul for advice one years ago when I was writing the patch. Good document for it!

kernel/rcutree.c +9 -0

+static int rcu_node_kthread(void *arg)
+{
+	int cpu;
+	unsigned long flags;
+	unsigned long mask;
+	struct rcu_node *rnp = (struct rcu_node *)arg;
+	struct sched_param sp;
+	struct task_struct *t;
+

kernel/rcutree.c +17 -0

+			if ((mask & 0x1) == 0)
+				continue;
+			preempt_disable();
+			per_cpu(rcu_cpu_has_work, cpu) = 1;
+			t = per_cpu(rcu_cpu_kthread_task, cpu);
+			if (t == NULL) {
+				preempt_enable();
+				continue;
+			}
+			sp.sched_priority = RCU_KTHREAD_PRIO;
+			sched_setscheduler_nocheck(t, cpu, &sp);
+			wake_up_process(t);
+			preempt_enable();
+		}
+	}
+	return 0;
+}

Quoted 13 lines · Paul E. McKenney

kernel/rcutree.c +13 -0

+static int rcu_node_kthread(void *arg)
+{
+	int cpu;
+	unsigned long flags;
+	unsigned long mask;
+	struct rcu_node *rnp = (struct rcu_node *)arg;
+	struct sched_param sp;
+	struct task_struct *t;
+
+	for (;;) {
+		wait_event_interruptible(rnp->node_wq, rnp->wakemask != 0 ||
+						       kthread_should_stop());
+		if (kthread_should_stop())

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1584

Quoted 6 lines

kernel/rcutree.c +6 -0

+			break;
+		raw_spin_lock_irqsave(&rnp->lock, flags);
+		mask = rnp->wakemask;
+		rnp->wakemask = 0;
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
+		for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++, mask <<= 1) {

I may be confused, but shouldn't it be mask >>= 1 instead?

rnp->wakemask is the unioned rdp->grpmask of the cpu(s) for which we woke that node thread up. Those mask start from 0, so what you want with the below check is to watch if the next CPU in group range is in the wakeup mask by shifting to the right.

No?

Quoted 17 lines

kernel/rcutree.c +17 -0

+			if ((mask & 0x1) == 0)
+				continue;
+			preempt_disable();
+			per_cpu(rcu_cpu_has_work, cpu) = 1;
+			t = per_cpu(rcu_cpu_kthread_task, cpu);
+			if (t == NULL) {
+				preempt_enable();
+				continue;
+			}
+			sp.sched_priority = RCU_KTHREAD_PRIO;
+			sched_setscheduler_nocheck(t, cpu, &sp);
+			wake_up_process(t);
+			preempt_enable();
+		}
+	}
+	return 0;
+}

kernel/rcutree.c +7 -0

+/*
+ * Set the per-rcu_node kthread's affinity to cover all CPUs that are
+ * served by the rcu_node in question.
+ */
+static void rcu_node_kthread_setaffinity(struct rcu_node *rnp)
+{
+	cpumask_var_t cm;

kernel/rcutree.c +5 -0

+		if (mask & 01)
+			cpumask_set_cpu(cpu, cm);
+	set_cpus_allowed_ptr(rnp->node_kthread_task, cm);
+	free_cpumask_var(cm);
+}

Quoted 11 lines · Paul E. McKenney

kernel/rcutree.c +11 -0

+/*
+ * Set the per-rcu_node kthread's affinity to cover all CPUs that are
+ * served by the rcu_node in question.
+ */
+static void rcu_node_kthread_setaffinity(struct rcu_node *rnp)
+{
+	cpumask_var_t cm;
+	int cpu;
+	unsigned long mask = rnp->qsmaskinit;
+
+	if (rnp->node_kthread_task == NULL ||

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1619

Quoted 6 lines

kernel/rcutree.c +6 -0

+	    rnp->qsmaskinit == 0)
+		return;
+	if (!alloc_cpumask_var(&cm, GFP_KERNEL))
+		return;
+	cpumask_clear(cm);
+	for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++, mask <<= 1)

There seem to be the same shift problem here.

Quoted 5 lines

kernel/rcutree.c +5 -0

+		if (mask & 01)
+			cpumask_set_cpu(cpu, cm);
+	set_cpus_allowed_ptr(rnp->node_kthread_task, cm);
+	free_cpumask_var(cm);
+}

* Paul E. McKenney ([email protected]) wrote:
[...]

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1620

Quoted 2 lines

kernel/rcutree.c +2 -0

+	for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++, mask <<= 1)
+		if (mask & 01)

Wow, octal notation! ;)

Maybe we could consider 0x1 here.

kernel/rcutree.c +8 -0

+/*
+ * Wake up the current CPU's kthread.  This replaces raise_softirq()
+ * in earlier versions of RCU.
+ */
+static void invoke_rcu_kthread(void)
+{
+	unsigned long flags;
+	wait_queue_head_t *q;

kernel/rcutree.c +3 -0

+	wake_up(q);
+	local_irq_restore(flags);
+}

Quoted 8 lines · Paul E. McKenney

kernel/rcutree.c +8 -0

+/*
+ * Wake up the current CPU's kthread.  This replaces raise_softirq()
+ * in earlier versions of RCU.
+ */
+static void invoke_rcu_kthread(void)
+{
+	unsigned long flags;
+	wait_queue_head_t *q;

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1424

Quoted 10 lines

kernel/rcutree.c +10 -0

+	int cpu;
+
+	local_irq_save(flags);
+	cpu = smp_processor_id();
+	if (per_cpu(rcu_cpu_kthread_task, cpu) == NULL) {
+		local_irq_restore(flags);
+		return;
+	}
+	per_cpu(rcu_cpu_has_work, cpu) = 1;
+	q = &per_cpu(rcu_cpu_wq, cpu);

I see you make extensive use of per_cpu() accessors even for local variables.

I tend to think it's better to use __get_cpu_var() for local accesses and keep per_cpu() for remote accesses.

There are several reasons for that:

* __get_cpu_var() checks we are in a non-preemptible section, per_cpu() doesn't. That may sound of a limited interest for code like the above, but by the time code can move, and then we might lose track of some things, etc...

* local accesses can be optimized by architectures. per_cpu() implies finding the local cpu number, and dereference an array of cpu offsets with that number to find the local cpu offset.
__get_cpu_var() does a direct access to __my_cpu_offset which is a nice shortcut if the arch implements it.

* It makes code easier to review: we know that __get_cpu_var() is for local accesses and per_cpu() for remote.

Quoted 4 lines

kernel/rcutree.c +3 -0

+	wake_up(q);
+	local_irq_restore(flags);
+}

Quoted 26 lines · Frederic Weisbecker

Quoted 8 lines · Paul E. McKenney

kernel/rcutree.c +8 -0

+/*
+ * Wake up the current CPU's kthread.  This replaces raise_softirq()
+ * in earlier versions of RCU.
+ */
+static void invoke_rcu_kthread(void)
+{
+	unsigned long flags;
+	wait_queue_head_t *q;

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1424

Quoted 10 lines

kernel/rcutree.c +10 -0

+	int cpu;
+
+	local_irq_save(flags);
+	cpu = smp_processor_id();
+	if (per_cpu(rcu_cpu_kthread_task, cpu) == NULL) {
+		local_irq_restore(flags);
+		return;
+	}
+	per_cpu(rcu_cpu_has_work, cpu) = 1;
+	q = &per_cpu(rcu_cpu_wq, cpu);

I see you make extensive use of per_cpu() accessors even for local variables.

I tend to think it's better to use __get_cpu_var() for local accesses and keep per_cpu() for remote accesses.

Quoted 6 lines

There are several reasons for that:

* __get_cpu_var() checks we are in a non-preemptible section, per_cpu() doesn't. That may sound of a limited interest for code like the above, but by the time code can move, and then we might lose track of some things, etc...

Ah, but so does smp_processor_id() ;-)

Quoted 6 lines

* local accesses can be optimized by architectures. per_cpu() implies finding the local cpu number, and dereference an array of cpu offsets with that number to find the local cpu offset.
__get_cpu_var() does a direct access to __my_cpu_offset which is a nice shortcut if the arch implements it.

True, but we could also argue that the multiple checks for being preempt can also be an issue.

Quoted 4 lines

* It makes code easier to review: we know that __get_cpu_var() is for local accesses and per_cpu() for remote.

This I'll agree with you.

In the past, I've thought about which one is better (per_cpu() vs __get_cpu_var()).

But, that last point is a good one. Just knowing that this is for the local CPU helps with the amount of info your mind needs to process when looking at this code. And the mind needs all the help it can get when reviewing this code ;-)

-- Steve

Quoted 4 lines

Quoted 4 lines

kernel/rcutree.c +3 -0

+	wake_up(q);
+	local_irq_restore(flags);
+}

Quoted 6 lines · Frederic Weisbecker

I see you make extensive use of per_cpu() accessors even for local variables.

I tend to think it's better to use __get_cpu_var() for local accesses and keep per_cpu() for remote accesses.

Quoted 6 lines

There are several reasons for that:

* __get_cpu_var() checks we are in a non-preemptible section, per_cpu() doesn't. That may sound of a limited interest for code like the above, but by the time code can move, and then we might lose track of some things, etc...

Ah, but so does smp_processor_id() ;-)

Quoted 6 lines

* local accesses can be optimized by architectures. per_cpu() implies finding the local cpu number, and dereference an array of cpu offsets with that number to find the local cpu offset.
__get_cpu_var() does a direct access to __my_cpu_offset which is a nice shortcut if the arch implements it.

True, but we could also argue that the multiple checks for being preempt can also be an issue.

Quoted 3 lines

* It makes code easier to review: we know that __get_cpu_var() is for local accesses and per_cpu() for remote.

This I'll agree with you.

In the past, I've thought about which one is better (per_cpu() vs __get_cpu_var()).

But, that last point is a good one. Just knowing that this is for the local CPU helps with the amount of info your mind needs to process when looking at this code. And the mind needs all the help it can get when reviewing this code ;-)

Quoted 27 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 26 lines · Frederic Weisbecker

Quoted 8 lines · Paul E. McKenney

kernel/rcutree.c +8 -0

+/*
+ * Wake up the current CPU's kthread.  This replaces raise_softirq()
+ * in earlier versions of RCU.
+ */
+static void invoke_rcu_kthread(void)
+{
+	unsigned long flags;
+	wait_queue_head_t *q;

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1424

Quoted 10 lines

kernel/rcutree.c +10 -0

+	int cpu;
+
+	local_irq_save(flags);
+	cpu = smp_processor_id();
+	if (per_cpu(rcu_cpu_kthread_task, cpu) == NULL) {
+		local_irq_restore(flags);
+		return;
+	}
+	per_cpu(rcu_cpu_has_work, cpu) = 1;
+	q = &per_cpu(rcu_cpu_wq, cpu);

I see you make extensive use of per_cpu() accessors even for local variables.

I tend to think it's better to use __get_cpu_var() for local accesses and keep per_cpu() for remote accesses.

Quoted 15 lines

Quoted 6 lines

There are several reasons for that:

* __get_cpu_var() checks we are in a non-preemptible section, per_cpu() doesn't. That may sound of a limited interest for code like the above, but by the time code can move, and then we might lose track of some things, etc...

Ah, but so does smp_processor_id() ;-)

Quoted 6 lines

* local accesses can be optimized by architectures. per_cpu() implies finding the local cpu number, and dereference an array of cpu offsets with that number to find the local cpu offset.
__get_cpu_var() does a direct access to __my_cpu_offset which is a nice shortcut if the arch implements it.

[Adding Christoph Lameter to CC list]

This is not quite true on x86_64 and s390 anymore. __get_cpu_var() now uses a segment selector override to get the local CPU variable on x86. See x86's percpu.h for details.

So even performance-wise, using __get_cpu_var() over per_cpu() should be a win on widely used architectures nowadays,

Quoted 4 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Quoted 3 lines

Quoted 3 lines

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1418

Quoted 3 lines

kernel/rcutree.c +3 -0

+
+	local_irq_save(flags);
+	cpu = smp_processor_id();

Quoted 1 line

Quoted 1 line

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1419

Quoted 1 line

kernel/rcutree.c +1 -0

+	if (per_cpu(rcu_cpu_kthread_task, cpu) == NULL) {

Quoted 4 lines

Quoted 4 lines

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1423

Quoted 4 lines

kernel/rcutree.c +4 -0

+		local_irq_restore(flags);
+		return;
+	}
+	per_cpu(rcu_cpu_has_work, cpu) = 1;

This is not quite true on x86_64 and s390 anymore. __get_cpu_var() now uses a segment selector override to get the local CPU variable on x86. See x86's percpu.h for details.

Quoted 2 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Quoted 6 lines

Quoted 2 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Right.

Quoted 7 lines · Christoph Lameter

Quoted 6 lines

Quoted 2 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Quoted 1 line

Right.

But a test still needs to be made. Because three access of this_cpu_*() that gets preempted and scheduled on another CPU can access a different CPU var for each access. This does not matter how atomic the this_cpu_*() code is.

Right if the kthread context can be rescheduled then either preemption needs to be disabled to guarantee that all three access the same per cpu area data or the code needs to be changed in such a way that a this_cpu RMW instructions can do the mods in one go.

Quoted 4 lines · Christoph Lameter

Right if the kthread context can be rescheduled then either preemption needs to be disabled to guarantee that all three access the same per cpu area data or the code needs to be changed in such a way that a this_cpu RMW instructions can do the mods in one go.

Are you suggesting a this_cpu_atomic_inc()?

this_cpu_inc is already percpu atomic. On x86 it is an instruction that cannot be interrupted nor preempted while in progress.

Quoted 2 lines · Christoph Lameter

this_cpu_inc is already percpu atomic. On x86 it is an instruction that cannot be interrupted nor preempted while in progress.

On x86, yes, but is this true for all architectures? I guess the fallback is implemented with local_irq_disable() which might be good enough for some but not for NMI usage.

Quoted 2 lines · Christoph Lameter

this_cpu_inc is already percpu atomic. On x86 it is an instruction that cannot be interrupted nor preempted while in progress.

On x86, yes, but is this true for all architectures? I guess the fallback is implemented with local_irq_disable() which might be good enough for some but not for NMI usage.

Quoted 2 lines · Christoph Lameter

this_cpu_inc is already percpu atomic. On x86 it is an instruction that cannot be interrupted nor preempted while in progress.

On x86, yes, but is this true for all architectures? I guess the fallback is implemented with local_irq_disable() which might be good enough for some but not for NMI usage.

Quoted 9 lines · Steven Rostedt

Quoted 7 lines · Christoph Lameter

Quoted 6 lines

Quoted 2 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Quoted 6 lines

Quoted 1 line

Right.

But a test still needs to be made. Because three access of this_cpu_*() that gets preempted and scheduled on another CPU can access a different CPU var for each access. This does not matter how atomic the this_cpu_*() code is.

Right if the kthread context can be rescheduled then either preemption needs to be disabled to guarantee that all three access the same per cpu area data or the code needs to be changed in such a way that a this_cpu RMW instructions can do the mods in one go.

Reschedules can happen in this case due to CPU hotplug events. :-(

Quoted 1 line · Paul E. McKenney

Reschedules can happen in this case due to CPU hotplug events. :-(

Lockout cpu hotplug instead of disabling interrupts? Systems that support cpu hotplug in hardware are rare and so the cpu hotplug locking may be become noop.

Quoted 8 lines · Christoph Lameter

Quoted 6 lines

Quoted 2 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Right.

But a test still needs to be made. Because three access of this_cpu_*() that gets preempted and scheduled on another CPU can access a different CPU var for each access. This does not matter how atomic the this_cpu_*() code is.

IOW, the use of this_cpu_*() without preemption disabled is 99% of the time a bug.

Quoted 3 lines · Mathieu Desnoyers

Quoted 3 lines

Quoted 3 lines

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1418

Quoted 3 lines

kernel/rcutree.c +3 -0

+
+	local_irq_save(flags);
+	cpu = smp_processor_id();

Drop this line.

Quoted 1 line

Quoted 1 line

Quoted 1 line

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1419

Quoted 1 line

kernel/rcutree.c +1 -0

+	if (per_cpu(rcu_cpu_kthread_task, cpu) == NULL) {

use this_cpu_read(rcu_cpu_kthread_task)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1423

Quoted 4 lines

kernel/rcutree.c +4 -0

+		local_irq_restore(flags);
+		return;
+	}
+	per_cpu(rcu_cpu_has_work, cpu) = 1;

this_cpu_write(rcu_cpu_has_work, 1);

Quoted 3 lines

This is not quite true on x86_64 and s390 anymore. __get_cpu_var() now uses a segment selector override to get the local CPU variable on x86. See x86's percpu.h for details.

__get_cpu_var cannot use a segment override since there are places where the address of the variable is taken. One needs to use this_cpu_ops for that.

Quoted 8 lines

Quoted 4 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Right.

Quoted 3 lines · Mathieu Desnoyers

Quoted 3 lines

Quoted 3 lines

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1418

Quoted 3 lines

kernel/rcutree.c +3 -0

+
+	local_irq_save(flags);
+	cpu = smp_processor_id();

Drop this line.

Quoted 1 line

Quoted 1 line

Quoted 1 line

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1419

Quoted 1 line

kernel/rcutree.c +1 -0

+	if (per_cpu(rcu_cpu_kthread_task, cpu) == NULL) {

use this_cpu_read(rcu_cpu_kthread_task)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1423

Quoted 4 lines

kernel/rcutree.c +4 -0

+		local_irq_restore(flags);
+		return;
+	}
+	per_cpu(rcu_cpu_has_work, cpu) = 1;

this_cpu_write(rcu_cpu_has_work, 1);

Quoted 3 lines

This is not quite true on x86_64 and s390 anymore. __get_cpu_var() now uses a segment selector override to get the local CPU variable on x86. See x86's percpu.h for details.

__get_cpu_var cannot use a segment override since there are places where the address of the variable is taken. One needs to use this_cpu_ops for that.

Quoted 6 lines

Quoted 2 lines

True, but we could also argue that the multiple checks for being preempt can also be an issue.

At least on x86 preemption don't actually need to be disabled: selection of the right per-cpu memory location is done atomically with the rest of the instruction by the segment selector.

Right.

These do introduce redundant preempt_disable()/preempt_enable() calls, but this is not on a fastpath, so should be OK, and the improved readability is certainly nice. The read and the write do need to happen on the same CPU, FWIW.

Quoted 4 lines · Paul E. McKenney

These do introduce redundant preempt_disable()/preempt_enable() calls, but this is not on a fastpath, so should be OK, and the improved readability is certainly nice. The read and the write do need to happen on the same CPU, FWIW.

this_cpu_xxx only use preempt_enable/disable() on platforms that do not support per cpu atomic instructions. On x86 no preempt enable/disable will be inserted.

You can also use the __this_cpu_xxx operations which never add preempt disable/enable because they expect the caller to deal with preemption.

kernel/rcutree.c +33 -0

+}
+
+/*
+ * Drop to non-real-time priority and yield, but only after posting a
+ * timer that will cause us to regain our real-time priority if we
+ * remain preempted.  Either way, we restore our real-time priority
+ * before returning.
+ */
+static void rcu_yield(int cpu)
+{
+	struct rcu_data *rdp = per_cpu_ptr(rcu_sched_state.rda, cpu);
+	struct sched_param sp;
+	struct timer_list yield_timer;
+
+	setup_timer(&yield_timer, rcu_cpu_kthread_timer, (unsigned long)rdp);
+	mod_timer(&yield_timer, jiffies + 2);
+	sp.sched_priority = 0;
+	sched_setscheduler_nocheck(current, SCHED_NORMAL, &sp);
+	schedule();
+	sp.sched_priority = RCU_KTHREAD_PRIO;
+	sched_setscheduler_nocheck(current, SCHED_FIFO, &sp);
+	del_timer(&yield_timer);
+}
+
+/*
+ * Handle cases where the rcu_cpu_kthread() ends up on the wrong CPU.
+ * This can happen while the corresponding CPU is either coming online
+ * or going offline.  We cannot wait until the CPU is fully online
+ * before starting the kthread, because the various notifier functions
+ * can wait for RCU grace periods.  So we park rcu_cpu_kthread() until
+ * the corresponding CPU is online.
+ *
+ * Return 1 if the kthread needs to stop, 0 otherwise.

kernel/rcutree.c +72 -0

+		if (smp_processor_id() != cpu)
+			set_cpus_allowed_ptr(current, cpumask_of(cpu));
+		local_bh_disable();
+	}
+	return 0;
+}
+
+/*
+ * Per-CPU kernel thread that invokes RCU callbacks.  This replaces the
+ * earlier RCU softirq.
+ */
+static int rcu_cpu_kthread(void *arg)
+{
+	int cpu = (int)(long)arg;
+	unsigned long flags;
+	int spincnt = 0;
+	wait_queue_head_t *wqp = &per_cpu(rcu_cpu_wq, cpu);
+	char work;
+	char *workp = &per_cpu(rcu_cpu_has_work, cpu);
+
+	for (;;) {
+		wait_event_interruptible(*wqp,
+					 *workp != 0 || kthread_should_stop());
+		local_bh_disable();
+		if (rcu_cpu_kthread_should_stop(cpu)) {
+			local_bh_enable();
+			break;
+		}
+		local_irq_save(flags);
+		work = *workp;
+		*workp = 0;
+		local_irq_restore(flags);
+		if (work)
+			rcu_process_callbacks();
+		local_bh_enable();
+		if (*workp != 0)
+			spincnt++;
+		else
+			spincnt = 0;
+		if (spincnt > 10) {
+			rcu_yield(cpu);
+			spincnt = 0;
+		}
+	}
+	return 0;
+}
+
+/*
+ * Per-rcu_node kthread, which is in charge of waking up the per-CPU
+ * kthreads when needed.
+ */
+static int rcu_node_kthread(void *arg)
+{
+	int cpu;
+	unsigned long flags;
+	unsigned long mask;
+	struct rcu_node *rnp = (struct rcu_node *)arg;
+	struct sched_param sp;
+	struct task_struct *t;
+
+	for (;;) {
+		wait_event_interruptible(rnp->node_wq, rnp->wakemask != 0 ||
+						       kthread_should_stop());
+		if (kthread_should_stop())
+			break;
+		raw_spin_lock_irqsave(&rnp->lock, flags);
+		mask = rnp->wakemask;
+		rnp->wakemask = 0;
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
+		for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++, mask <<= 1) {
+			if ((mask & 0x1) == 0)
+				continue;

kernel/rcutree.c +5 -0

+			preempt_enable();
+		}
+	}
+	return 0;
+}

Quoted 39 lines · Paul E. McKenney

kernel/rcutree.c +39 -0

+}
+
+/*
+ * Drop to non-real-time priority and yield, but only after posting a
+ * timer that will cause us to regain our real-time priority if we
+ * remain preempted.  Either way, we restore our real-time priority
+ * before returning.
+ */
+static void rcu_yield(int cpu)
+{
+	struct rcu_data *rdp = per_cpu_ptr(rcu_sched_state.rda, cpu);
+	struct sched_param sp;
+	struct timer_list yield_timer;
+
+	setup_timer(&yield_timer, rcu_cpu_kthread_timer, (unsigned long)rdp);
+	mod_timer(&yield_timer, jiffies + 2);
+	sp.sched_priority = 0;
+	sched_setscheduler_nocheck(current, SCHED_NORMAL, &sp);
+	schedule();
+	sp.sched_priority = RCU_KTHREAD_PRIO;
+	sched_setscheduler_nocheck(current, SCHED_FIFO, &sp);
+	del_timer(&yield_timer);
+}
+
+/*
+ * Handle cases where the rcu_cpu_kthread() ends up on the wrong CPU.
+ * This can happen while the corresponding CPU is either coming online
+ * or going offline.  We cannot wait until the CPU is fully online
+ * before starting the kthread, because the various notifier functions
+ * can wait for RCU grace periods.  So we park rcu_cpu_kthread() until
+ * the corresponding CPU is online.
+ *
+ * Return 1 if the kthread needs to stop, 0 otherwise.
+ *
+ * Caller must disable bh.  This function can momentarily enable it.
+ */
+static int rcu_cpu_kthread_should_stop(int cpu)
+{
+	while (cpu_is_offline(cpu) || smp_processor_id() != cpu) {

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1491

Quoted 4 lines

kernel/rcutree.c +4 -0

+		if (kthread_should_stop())
+			return 1;
+		local_bh_enable();
+		schedule_timeout_uninterruptible(1);

Why is it uninterruptible? Well that doesn't change much anyway. It can be a problem for long time sleeping kernel threads because of the hung task detector, but certainly not for 1 jiffy.

Quoted 72 lines

kernel/rcutree.c +72 -0

+		if (smp_processor_id() != cpu)
+			set_cpus_allowed_ptr(current, cpumask_of(cpu));
+		local_bh_disable();
+	}
+	return 0;
+}
+
+/*
+ * Per-CPU kernel thread that invokes RCU callbacks.  This replaces the
+ * earlier RCU softirq.
+ */
+static int rcu_cpu_kthread(void *arg)
+{
+	int cpu = (int)(long)arg;
+	unsigned long flags;
+	int spincnt = 0;
+	wait_queue_head_t *wqp = &per_cpu(rcu_cpu_wq, cpu);
+	char work;
+	char *workp = &per_cpu(rcu_cpu_has_work, cpu);
+
+	for (;;) {
+		wait_event_interruptible(*wqp,
+					 *workp != 0 || kthread_should_stop());
+		local_bh_disable();
+		if (rcu_cpu_kthread_should_stop(cpu)) {
+			local_bh_enable();
+			break;
+		}
+		local_irq_save(flags);
+		work = *workp;
+		*workp = 0;
+		local_irq_restore(flags);
+		if (work)
+			rcu_process_callbacks();
+		local_bh_enable();
+		if (*workp != 0)
+			spincnt++;
+		else
+			spincnt = 0;
+		if (spincnt > 10) {
+			rcu_yield(cpu);
+			spincnt = 0;
+		}
+	}
+	return 0;
+}
+
+/*
+ * Per-rcu_node kthread, which is in charge of waking up the per-CPU
+ * kthreads when needed.
+ */
+static int rcu_node_kthread(void *arg)
+{
+	int cpu;
+	unsigned long flags;
+	unsigned long mask;
+	struct rcu_node *rnp = (struct rcu_node *)arg;
+	struct sched_param sp;
+	struct task_struct *t;
+
+	for (;;) {
+		wait_event_interruptible(rnp->node_wq, rnp->wakemask != 0 ||
+						       kthread_should_stop());
+		if (kthread_should_stop())
+			break;
+		raw_spin_lock_irqsave(&rnp->lock, flags);
+		mask = rnp->wakemask;
+		rnp->wakemask = 0;
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
+		for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++, mask <<= 1) {
+			if ((mask & 0x1) == 0)
+				continue;

kernel/rcutree.c static void rcu_process_callbacks(struct softirq_action *unused) · line 1596

Quoted 10 lines

kernel/rcutree.c +10 -0

+			preempt_disable();
+			per_cpu(rcu_cpu_has_work, cpu) = 1;
+			t = per_cpu(rcu_cpu_kthread_task, cpu);
+			if (t == NULL) {
+				preempt_enable();
+				continue;
+			}
+			sp.sched_priority = RCU_KTHREAD_PRIO;
+			sched_setscheduler_nocheck(t, cpu, &sp);
+			wake_up_process(t);

My (mis?)understanding of the picture is this node kthread is there to wake up cpu threads that called rcu_yield(). But actually rcu_yield() doesn't make the cpu thread sleeping, instead it switches to SCHED_NORMAL, to avoid starving the system with callbacks.

So I wonder if this wake_up_process() is actually relevant. sched_setscheduler_nocheck() already handles the per sched policy rq migration and the process is not sleeping.

That said, by the time the process may have gone to sleep, because if no other SCHED_NORMAL task was there, it has just continued and may have flushed every callbacks. So this wake_up_process() may actually wake up the task but it will sleep again right away due to the condition in wait_event_interruptible() of the cpu thread.

Right?

Quoted 5 lines

kernel/rcutree.c +5 -0

+			preempt_enable();
+		}
+	}
+	return 0;
+}