rcu: remove unused variable in boot_cpu_state_init

PATCH In review Arnd Bergmann 3 participants 14 messages Jun 21, 2017 – Jun 22, 2017

Subsystems (2)

RCU CPU hotplug

lore

Share & tools

lore

Arnd Bergmann

author

2017-06-21 21:58 UTC #0 lore raw

PATCH 1/1 rcu: remove unused variable in boot_cpu_state_init

Without CONFIG_SMP, we get a harmless warning about an unused variable:

kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]

This reworks the function to have the declaration inside of the #ifdef.

Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")

Signed-off-by Arnd Bergmann <[email protected]>

---
 kernel/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/cpu.c +2 -2

index f13ad79e785d..9cd4fc35a66f 100644
@@ -1775,11 +1775,11 @@ void __init boot_cpu_init(void)
  */
 void __init boot_cpu_state_init(void)
 {
+#ifdef CONFIG_SMP
 	int cpu;
 
-	per_cpu_ptr(&cpuhp_state, smp_processor_id())->state = CPUHP_ONLINE;
-#ifdef CONFIG_SMP
 	for_each_possible_cpu(cpu)
 		per_cpu_ptr(&cpuhp_state, cpu)->cpu = cpu;
 #endif
+	per_cpu_ptr(&cpuhp_state, smp_processor_id())->state = CPUHP_ONLINE;
 }

Signature · 1 line

2.9.0

Paul E. McKenney

2017-06-21 22:11 UTC #1 lore raw

Quoted 11 lines · Arnd Bergmann

Without CONFIG_SMP, we get a harmless warning about an unused variable:
kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]
This reworks the function to have the declaration inside of the #ifdef.
Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")
Signed-off-by Arnd Bergmann <[email protected]>

I simply added a __maybe_unused in 6441c656acde ("rcu: Migrate callbacks earlier in the CPU-offline timeline") in my -rcu tree. However, your approach does have the advantage of complaining if the code using that variable is removed.

So, would you be OK with my folding your approach into my commit with attribution?

Thanx, Paul

Quoted 25 lines

---
 kernel/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/cpu.c +2 -2

index f13ad79e785d..9cd4fc35a66f 100644
@@ -1775,11 +1775,11 @@ void __init boot_cpu_init(void)
  */
 void __init boot_cpu_state_init(void)
 {
+#ifdef CONFIG_SMP
 	int cpu;
 
-	per_cpu_ptr(&cpuhp_state, smp_processor_id())->state = CPUHP_ONLINE;
-#ifdef CONFIG_SMP
 	for_each_possible_cpu(cpu)
 		per_cpu_ptr(&cpuhp_state, cpu)->cpu = cpu;
 #endif
+	per_cpu_ptr(&cpuhp_state, smp_processor_id())->state = CPUHP_ONLINE;
 }

Signature · 1 line

2.9.0

Arnd Bergmann

author

2017-06-21 22:15 UTC #2 lore raw

Quoted 13 lines · Paul E. McKenney

Quoted 11 lines · Arnd Bergmann
Without CONFIG_SMP, we get a harmless warning about an unused variable:
kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]
This reworks the function to have the declaration inside of the #ifdef.
Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")
Signed-off-by Arnd Bergmann <[email protected]>

Quoted 7 lines

I simply added a __maybe_unused in 6441c656acde ("rcu: Migrate callbacks earlier in the CPU-offline timeline") in my -rcu tree. However, your approach does have the advantage of complaining if the code using that variable is removed.
So, would you be OK with my folding your approach into my commit with attribution?

Sure, that's always best.

Arnd

Paul E. McKenney

2017-06-22 14:36 UTC #3 lore raw

Quoted 20 lines · Arnd Bergmann

Quoted 18 lines · Paul E. McKenney
Quoted 11 lines · Arnd Bergmann
Without CONFIG_SMP, we get a harmless warning about an unused variable:
kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]
This reworks the function to have the declaration inside of the #ifdef.
Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")
Signed-off-by Arnd Bergmann <[email protected]>
I simply added a __maybe_unused in 6441c656acde ("rcu: Migrate callbacks earlier in the CPU-offline timeline") in my -rcu tree. However, your approach does have the advantage of complaining if the code using that variable is removed.

Quoted 4 lines

Quoted 2 lines
So, would you be OK with my folding your approach into my commit with attribution?
Sure, that's always best.

Done, thank you!

Thanx, Paul

Paul E. McKenney

2017-06-22 21:10 UTC #4 lore raw

Quoted 21 lines · Paul E. McKenney

Quoted 20 lines · Arnd Bergmann
Quoted 18 lines · Paul E. McKenney
Quoted 11 lines · Arnd Bergmann
Without CONFIG_SMP, we get a harmless warning about an unused variable:
kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]
This reworks the function to have the declaration inside of the #ifdef.
Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")
Signed-off-by Arnd Bergmann <[email protected]>
I simply added a __maybe_unused in 6441c656acde ("rcu: Migrate callbacks earlier in the CPU-offline timeline") in my -rcu tree. However, your approach does have the advantage of complaining if the code using that variable is removed.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
So, would you be OK with my folding your approach into my commit with attribution?
Sure, that's always best.
Done, thank you!

And after all that, it turned out that the only reason I needed to add that loop was that I was stupidly hooking into the wrong place in the CPU-hotplug code. Please see below for an updated patch that passes very light testing. Most of this patch is code movement to bring a couple of functions under a pre-existing #ifdef.

Thanx, Paul

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

commit 4384c26f62d90e9685a50d65dcd352dbe96ae220

Author: Paul E. McKenney <[email protected]>
Date:   Tue Jun 20 12:11:34 2017 -0700

rcu: Migrate callbacks earlier in the CPU-offline timeline

RCU callbacks must be migrated away from an outgoing CPU, and this is done near the end of the CPU-hotplug operation, after the outgoing CPU is long gone. Unfortunately, this means that other CPU-hotplug callbacks can execute while the outgoing CPU's callbacks are still immobilized on the long-gone CPU's callback lists. If any of these CPU-hotplug callbacks must wait, either directly or indirectly, for the invocation of any of the immobilized RCU callbacks, the system will hang.

This commit avoids such hangs by migrating the callbacks away from the outgoing CPU immediately upon its departure, shortly after the return from __cpu_die() in takedown_cpu(). Thus, RCU is able to advance these callbacks and invoke them, which allows all the after-the-fact CPU-hotplug callbacks to wait on these RCU callbacks without risk of a hang.

While in the neighborhood, this commit also moves rcu_send_cbs_to_orphanage() and rcu_adopt_orphan_cbs() under a pre-existing #ifdef to avoid including dead code on the one hand and to avoid define-without-use warnings on the other hand.

Reported-by Jeffrey Hugo <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

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

Cc Thomas Gleixner <[email protected]>

Cc Sebastian Andrzej Siewior <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Anna-Maria Gleixner <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Richard Weinberger <[email protected]>

include/linux/rcupdate.h +1 -0

index ecca9d2e4e4a..96f1baf62ab8 100644
@@ -109,6 +109,7 @@ void rcu_bh_qs(void);
 void rcu_check_callbacks(int user);
 void rcu_report_dead(unsigned int cpu);
 void rcu_cpu_starting(unsigned int cpu);
+void rcutree_migrate_callbacks(int cpu);
 
 #ifdef CONFIG_RCU_STALL_COMMON
 void rcu_sysrq_start(void);

kernel/cpu.c +1 -0

index 9ae6fbe5b5cf..f5f3db2403fa 100644
@@ -729,6 +729,7 @@ static int takedown_cpu(unsigned int cpu)
 	__cpu_die(cpu);
 
 	tick_cleanup_dead_cpu(cpu);
+	rcutree_migrate_callbacks(cpu);
 	return 0;
 }

kernel/rcu/tree.c +110 ~~-92~~

226 lines not on this page · raw message

Ingo Molnar

2017-06-22 07:26 UTC #5 lore raw

In reply to Paul E. McKenney

Quoted 13 lines · Paul E. McKenney

Quoted 11 lines · Arnd Bergmann
Without CONFIG_SMP, we get a harmless warning about an unused variable:
kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]
This reworks the function to have the declaration inside of the #ifdef.
Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")
Signed-off-by Arnd Bergmann <[email protected]>

Quoted 7 lines

I simply added a __maybe_unused in 6441c656acde ("rcu: Migrate callbacks earlier in the CPU-offline timeline") in my -rcu tree. However, your approach does have the advantage of complaining if the code using that variable is removed.
So, would you be OK with my folding your approach into my commit with attribution?

Also, note that __maybe_unused can be dangerous: it can hide a build warning where there's a _real_ unused variable bug now or due to future changes, causing a real runtime bug.

So I think we should consider it a syntactic construct to avoid.

Thanks,

Ingo

Arnd Bergmann

author

2017-06-22 07:41 UTC #6 lore raw

Quoted 21 lines · Ingo Molnar

Quoted 18 lines · Paul E. McKenney
Quoted 11 lines · Arnd Bergmann
Without CONFIG_SMP, we get a harmless warning about an unused variable:
kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]
This reworks the function to have the declaration inside of the #ifdef.
Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")
Signed-off-by Arnd Bergmann <[email protected]>
I simply added a __maybe_unused in 6441c656acde ("rcu: Migrate callbacks earlier in the CPU-offline timeline") in my -rcu tree. However, your approach does have the advantage of complaining if the code using that variable is removed.

Quoted 8 lines

Quoted 2 lines
So, would you be OK with my folding your approach into my commit with attribution?
Also, note that __maybe_unused can be dangerous: it can hide a build warning where there's a _real_ unused variable bug now or due to future changes, causing a real runtime bug.
So I think we should consider it a syntactic construct to avoid.

Unused variables are relatively harmless compared to used-uninitialized variables that are always bugs (though they are provably impossible to detect correctly in some cases).

For unused variables, we might want to enable "-Wunused-but-set" again. This was introduced in gcc-5 or gcc-6 and moved to "make W=1" because of too many new warnings getting introduced, but I already fixed a lot of those. I'll give that a spin on my randconfig build test to see how many of them we have remaining these days, if any.
Arnd

Ingo Molnar

2017-06-22 07:51 UTC #7 lore raw

Quoted 5 lines · Arnd Bergmann

Quoted 1 line
So I think we should consider it a syntactic construct to avoid.
Unused variables are relatively harmless compared to used-uninitialized variables that are always bugs (though they are provably impossible to detect correctly in some cases).

So the thing I was most worried about was that old GCC used to not warn about:

	long __maybe_unused error;

	...

	if (error)
		return error;

... but recent GCC does warn if it's certain that the use is uninitialized, so the scenario I outlined should not happen.

But it will supress the warning if the variable is uninitialized but GCC cannot prove it for sure, so my point remains that it's a potentially dangerous construct.

Thanks,

Ingo

Ingo Molnar

2017-06-22 07:54 UTC #8 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 5 lines · Arnd Bergmann
Quoted 1 line
So I think we should consider it a syntactic construct to avoid.
Unused variables are relatively harmless compared to used-uninitialized variables that are always bugs (though they are provably impossible to detect correctly in some cases).

Quoted 8 lines

So the thing I was most worried about was that old GCC used to not warn about:
	long __maybe_unused error;

	...

	if (error)
		return error;

Gah - I got totally confused, the dangerous construct I was thinking of was uninitialized_var(), not __maybe_unused.

So ignore my replies! :-)

Thanks,

Ingo

Ingo Molnar

2017-06-22 07:59 UTC #9 lore raw

So, to continue this side thought about uninitialized_var(), it is dangerous because the following buggy pattern does not generate a compiler warning:

        long uninitialized_var(error);

	...

        if (error)
                return error;

... and still there are over 290 uses of uninitialized_var() in the kernel - and any of them could turn into a silent but real uninitialized variable bugs due to subsequent changes.

Thanks,

Ingo

Arnd Bergmann

author

2017-06-22 08:59 UTC #10 lore raw

Quoted 12 lines · Ingo Molnar

So, to continue this side thought about uninitialized_var(), it is dangerous because the following buggy pattern does not generate a compiler warning:
        long uninitialized_var(error);

        ...

        if (error)
                return error;

Quoted 3 lines

... and still there are over 290 uses of uninitialized_var() in the kernel - and any of them could turn into a silent but real uninitialized variable bugs due to subsequent changes.

Right, absolutely agreed on that. A related problem however is blindly initializing variables to NULL to get rid of uninitialized variable warnings, such as

      struct subsystem_specific *obj = NULL;
      if (function_argument > 10)
              goto err;
     obj = create_obj();
...
err:
      clean_up(obj->member);

I've seen a couple of variations of that problem, so simply outlawing uninitialized_var() will only solve a subset of these issues, and ideally we should also make sure that initializations at declaration time are used properly, and not just to shut up compiler warnings.

Arnd

Ingo Molnar

2017-06-22 09:02 UTC #11 lore raw

Quoted 30 lines · Arnd Bergmann

Quoted 12 lines · Ingo Molnar
So, to continue this side thought about uninitialized_var(), it is dangerous because the following buggy pattern does not generate a compiler warning:
        long uninitialized_var(error);

        ...

        if (error)
                return error;
Quoted 3 lines
... and still there are over 290 uses of uninitialized_var() in the kernel - and any of them could turn into a silent but real uninitialized variable bugs due to subsequent changes.
Right, absolutely agreed on that. A related problem however is blindly initializing variables to NULL to get rid of uninitialized variable warnings, such as
      struct subsystem_specific *obj = NULL;
      if (function_argument > 10)
              goto err;
     obj = create_obj();
...
err:
      clean_up(obj->member);

Quoted 4 lines

I've seen a couple of variations of that problem, so simply outlawing uninitialized_var() will only solve a subset of these issues, and ideally we should also make sure that initializations at declaration time are used properly, and not just to shut up compiler warnings.

Well, a deterministic crash on a NULL dereference is still (much) better than a non-deterministic 'use random value from stack and corrupt memory or crash' bug pattern, right?

Also, static analysis tools ought to be pretty good about finding control flows where a NULL gets dereferenced.

Thanks,

Ingo

Arnd Bergmann

author

2017-06-22 09:44 UTC #12 lore raw

Quoted 33 lines · Ingo Molnar

Quoted 30 lines · Arnd Bergmann
Quoted 12 lines · Ingo Molnar
So, to continue this side thought about uninitialized_var(), it is dangerous because the following buggy pattern does not generate a compiler warning:
        long uninitialized_var(error);

        ...

        if (error)
                return error;
Quoted 3 lines
... and still there are over 290 uses of uninitialized_var() in the kernel - and any of them could turn into a silent but real uninitialized variable bugs due to subsequent changes.
Right, absolutely agreed on that. A related problem however is blindly initializing variables to NULL to get rid of uninitialized variable warnings, such as
      struct subsystem_specific *obj = NULL;
      if (function_argument > 10)
              goto err;
     obj = create_obj();
...
err:
      clean_up(obj->member);

Quoted 8 lines

Quoted 4 lines
I've seen a couple of variations of that problem, so simply outlawing uninitialized_var() will only solve a subset of these issues, and ideally we should also make sure that initializations at declaration time are used properly, and not just to shut up compiler warnings.
Well, a deterministic crash on a NULL dereference is still (much) better than a non-deterministic 'use random value from stack and corrupt memory or crash' bug pattern, right?

The NULL initialization is more reproducible, but has also led to easier exploits in the past, when user space could more easily map a writable memory to virtual address zero and make the kernel jump there for privilege escalation.

Quoted 2 lines

Also, static analysis tools ought to be pretty good about finding control flows where a NULL gets dereferenced.

I think the tooling we have for uninitialized data (assuming uninitialized_var() is not used) is better than that for NULL dereferences. While gcc can detect cases where a NULL pointer is dereferenced, it won't warn about that and instead invoke its undefined-behavior optimization: it will assume that this code path is never hit and may run off the end of a function or worse instead of actually doing the NULL pointer dereference. I think with CONFIG_UBSAN, it will insert a trapping instruction instead for a known NULL dereference.

A similar thing happens for integer divide-by-zero, which can also go unnoticed because of extraneous initializations. gcc-7 can detect more often now than it used to, but often simply assumes that any code path leading up to div0 exception
cannot happen, and will instead trap when it gets there, and silently discard any code following it.

Arnd

Paul E. McKenney

2017-06-22 14:57 UTC #13 lore raw

In reply to Ingo Molnar

Quoted 21 lines · Ingo Molnar

Quoted 18 lines · Paul E. McKenney
Quoted 11 lines · Arnd Bergmann
Without CONFIG_SMP, we get a harmless warning about an unused variable:
kernel/cpu.c: In function 'boot_cpu_state_init':
kernel/cpu.c:1778:6: error: unused variable 'cpu' [-Werror=unused-variable]
This reworks the function to have the declaration inside of the #ifdef.
Fixes faeb334286b7 ("rcu: Migrate callbacks earlier in the CPU-offline timeline")
Signed-off-by Arnd Bergmann <[email protected]>
I simply added a __maybe_unused in 6441c656acde ("rcu: Migrate callbacks earlier in the CPU-offline timeline") in my -rcu tree. However, your approach does have the advantage of complaining if the code using that variable is removed.

Quoted 8 lines

Quoted 2 lines
So, would you be OK with my folding your approach into my commit with attribution?
Also, note that __maybe_unused can be dangerous: it can hide a build warning where there's a _real_ unused variable bug now or due to future changes, causing a real runtime bug.
So I think we should consider it a syntactic construct to avoid.

I will review the ones in RCU.

Thanx, Paul