All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rcu: remove unused variable in boot_cpu_state_init
@ 2017-06-21 21:57 Arnd Bergmann
  2017-06-21 22:10 ` Paul E. McKenney
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2017-06-21 21:57 UTC (permalink / raw)
  To: Paul E . McKenney
  Cc: Arnd Bergmann, Thomas Gleixner, Ingo Molnar,
	Sebastian Andrzej Siewior, Anna-Maria Gleixner, Boris Ostrovsky,
	linux-kernel

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 <arnd@arndb.de>
---
 kernel/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index f13ad79e785d..9cd4fc35a66f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -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;
 }
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-21 21:57 [PATCH] rcu: remove unused variable in boot_cpu_state_init Arnd Bergmann
@ 2017-06-21 22:10 ` Paul E. McKenney
  2017-06-21 22:15   ` Arnd Bergmann
  2017-06-22  7:26   ` Ingo Molnar
  0 siblings, 2 replies; 14+ messages in thread
From: Paul E. McKenney @ 2017-06-21 22:10 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Thomas Gleixner, Ingo Molnar, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, linux-kernel

On Wed, Jun 21, 2017 at 11:57:28PM +0200, Arnd Bergmann wrote:
> 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 <arnd@arndb.de>

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

> ---
>  kernel/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index f13ad79e785d..9cd4fc35a66f 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -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;
>  }
> -- 
> 2.9.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-21 22:10 ` Paul E. McKenney
@ 2017-06-21 22:15   ` Arnd Bergmann
  2017-06-22 14:36     ` Paul E. McKenney
  2017-06-22  7:26   ` Ingo Molnar
  1 sibling, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2017-06-21 22:15 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Thomas Gleixner, Ingo Molnar, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List

On Thu, Jun 22, 2017 at 12:10 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Jun 21, 2017 at 11:57:28PM +0200, Arnd Bergmann wrote:
>> 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 <arnd@arndb.de>
>
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-21 22:10 ` Paul E. McKenney
  2017-06-21 22:15   ` Arnd Bergmann
@ 2017-06-22  7:26   ` Ingo Molnar
  2017-06-22  7:41     ` Arnd Bergmann
  2017-06-22 14:55     ` Paul E. McKenney
  1 sibling, 2 replies; 14+ messages in thread
From: Ingo Molnar @ 2017-06-22  7:26 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Arnd Bergmann, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, linux-kernel


* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> On Wed, Jun 21, 2017 at 11:57:28PM +0200, Arnd Bergmann wrote:
> > 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 <arnd@arndb.de>
> 
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  7:26   ` Ingo Molnar
@ 2017-06-22  7:41     ` Arnd Bergmann
  2017-06-22  7:51       ` Ingo Molnar
  2017-06-22 14:55     ` Paul E. McKenney
  1 sibling, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2017-06-22  7:41 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Paul E. McKenney, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List

On Thu, Jun 22, 2017 at 9:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
>> On Wed, Jun 21, 2017 at 11:57:28PM +0200, Arnd Bergmann wrote:
>> > 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 <arnd@arndb.de>
>>
>> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  7:41     ` Arnd Bergmann
@ 2017-06-22  7:51       ` Ingo Molnar
  2017-06-22  7:54         ` Ingo Molnar
  0 siblings, 1 reply; 14+ messages in thread
From: Ingo Molnar @ 2017-06-22  7:51 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Paul E. McKenney, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List


* Arnd Bergmann <arnd@arndb.de> wrote:

> > 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  7:51       ` Ingo Molnar
@ 2017-06-22  7:54         ` Ingo Molnar
  2017-06-22  7:59           ` Ingo Molnar
  0 siblings, 1 reply; 14+ messages in thread
From: Ingo Molnar @ 2017-06-22  7:54 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Paul E. McKenney, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List


* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > > 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;

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

So ignore my replies! :-)

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  7:54         ` Ingo Molnar
@ 2017-06-22  7:59           ` Ingo Molnar
  2017-06-22  8:58             ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Ingo Molnar @ 2017-06-22  7:59 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Paul E. McKenney, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  7:59           ` Ingo Molnar
@ 2017-06-22  8:58             ` Arnd Bergmann
  2017-06-22  9:02               ` Ingo Molnar
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2017-06-22  8:58 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Paul E. McKenney, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List

On Thu, Jun 22, 2017 at 9:59 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  8:58             ` Arnd Bergmann
@ 2017-06-22  9:02               ` Ingo Molnar
  2017-06-22  9:44                 ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Ingo Molnar @ 2017-06-22  9:02 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Paul E. McKenney, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List


* Arnd Bergmann <arnd@arndb.de> wrote:

> On Thu, Jun 22, 2017 at 9:59 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > 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.
> 
> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  9:02               ` Ingo Molnar
@ 2017-06-22  9:44                 ` Arnd Bergmann
  0 siblings, 0 replies; 14+ messages in thread
From: Arnd Bergmann @ 2017-06-22  9:44 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Paul E. McKenney, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List

On Thu, Jun 22, 2017 at 11:02 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Arnd Bergmann <arnd@arndb.de> wrote:
>
>> On Thu, Jun 22, 2017 at 9:59 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > 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.
>>
>> 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.
>
> 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.

> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-21 22:15   ` Arnd Bergmann
@ 2017-06-22 14:36     ` Paul E. McKenney
  2017-06-22 21:10       ` Paul E. McKenney
  0 siblings, 1 reply; 14+ messages in thread
From: Paul E. McKenney @ 2017-06-22 14:36 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Thomas Gleixner, Ingo Molnar, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List

On Thu, Jun 22, 2017 at 12:15:50AM +0200, Arnd Bergmann wrote:
> On Thu, Jun 22, 2017 at 12:10 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Jun 21, 2017 at 11:57:28PM +0200, Arnd Bergmann wrote:
> >> 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 <arnd@arndb.de>
> >
> > 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.

Done, thank you!

							Thanx, Paul

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22  7:26   ` Ingo Molnar
  2017-06-22  7:41     ` Arnd Bergmann
@ 2017-06-22 14:55     ` Paul E. McKenney
  1 sibling, 0 replies; 14+ messages in thread
From: Paul E. McKenney @ 2017-06-22 14:55 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Arnd Bergmann, Thomas Gleixner, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, linux-kernel

On Thu, Jun 22, 2017 at 09:26:44AM +0200, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Jun 21, 2017 at 11:57:28PM +0200, Arnd Bergmann wrote:
> > > 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 <arnd@arndb.de>
> > 
> > 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.

I will review the ones in RCU.

							Thanx, Paul

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rcu: remove unused variable in boot_cpu_state_init
  2017-06-22 14:36     ` Paul E. McKenney
@ 2017-06-22 21:10       ` Paul E. McKenney
  0 siblings, 0 replies; 14+ messages in thread
From: Paul E. McKenney @ 2017-06-22 21:10 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Thomas Gleixner, Ingo Molnar, Sebastian Andrzej Siewior,
	Anna-Maria Gleixner, Boris Ostrovsky, Linux Kernel Mailing List

On Thu, Jun 22, 2017 at 07:36:29AM -0700, Paul E. McKenney wrote:
> On Thu, Jun 22, 2017 at 12:15:50AM +0200, Arnd Bergmann wrote:
> > On Thu, Jun 22, 2017 at 12:10 AM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Jun 21, 2017 at 11:57:28PM +0200, Arnd Bergmann wrote:
> > >> 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 <arnd@arndb.de>
> > >
> > > 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.
> 
> 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 <paulmck@linux.vnet.ibm.com>
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 <jhugo@codeaurora.org>
    Link: http://lkml.kernel.org/r/db9c91f6-1b17-6136-84f0-03c3c2581ab4@codeaurora.org
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Thomas Gleixner <tglx@linutronix.de>
    Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
    Cc: Ingo Molnar <mingo@kernel.org>
    Cc: Anna-Maria Gleixner <anna-maria@linutronix.de>
    Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
    Cc: Richard Weinberger <richard@nod.at>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index ecca9d2e4e4a..96f1baf62ab8 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -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);
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 9ae6fbe5b5cf..f5f3db2403fa 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -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;
 }
 
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 94ec7455fc46..e6d534946c7f 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2547,85 +2547,6 @@ rcu_check_quiescent_state(struct rcu_state *rsp, struct rcu_data *rdp)
 }
 
 /*
- * Send the specified CPU's RCU callbacks to the orphanage.  The
- * specified CPU must be offline, and the caller must hold the
- * ->orphan_lock.
- */
-static void
-rcu_send_cbs_to_orphanage(int cpu, struct rcu_state *rsp,
-			  struct rcu_node *rnp, struct rcu_data *rdp)
-{
-	lockdep_assert_held(&rsp->orphan_lock);
-
-	/* No-CBs CPUs do not have orphanable callbacks. */
-	if (!IS_ENABLED(CONFIG_HOTPLUG_CPU) || rcu_is_nocb_cpu(rdp->cpu))
-		return;
-
-	/*
-	 * Orphan the callbacks.  First adjust the counts.  This is safe
-	 * because _rcu_barrier() excludes CPU-hotplug operations, so it
-	 * cannot be running now.  Thus no memory barrier is required.
-	 */
-	rdp->n_cbs_orphaned += rcu_segcblist_n_cbs(&rdp->cblist);
-	rcu_segcblist_extract_count(&rdp->cblist, &rsp->orphan_done);
-
-	/*
-	 * Next, move those callbacks still needing a grace period to
-	 * the orphanage, where some other CPU will pick them up.
-	 * Some of the callbacks might have gone partway through a grace
-	 * period, but that is too bad.  They get to start over because we
-	 * cannot assume that grace periods are synchronized across CPUs.
-	 */
-	rcu_segcblist_extract_pend_cbs(&rdp->cblist, &rsp->orphan_pend);
-
-	/*
-	 * Then move the ready-to-invoke callbacks to the orphanage,
-	 * where some other CPU will pick them up.  These will not be
-	 * required to pass though another grace period: They are done.
-	 */
-	rcu_segcblist_extract_done_cbs(&rdp->cblist, &rsp->orphan_done);
-
-	/* Finally, disallow further callbacks on this CPU.  */
-	rcu_segcblist_disable(&rdp->cblist);
-}
-
-/*
- * Adopt the RCU callbacks from the specified rcu_state structure's
- * orphanage.  The caller must hold the ->orphan_lock.
- */
-static void rcu_adopt_orphan_cbs(struct rcu_state *rsp, unsigned long flags)
-{
-	struct rcu_data *rdp = raw_cpu_ptr(rsp->rda);
-
-	lockdep_assert_held(&rsp->orphan_lock);
-
-	/* No-CBs CPUs are handled specially. */
-	if (!IS_ENABLED(CONFIG_HOTPLUG_CPU) ||
-	    rcu_nocb_adopt_orphan_cbs(rsp, rdp, flags))
-		return;
-
-	/* Do the accounting first. */
-	rdp->n_cbs_adopted += rsp->orphan_done.len;
-	if (rsp->orphan_done.len_lazy != rsp->orphan_done.len)
-		rcu_idle_count_callbacks_posted();
-	rcu_segcblist_insert_count(&rdp->cblist, &rsp->orphan_done);
-
-	/*
-	 * We do not need a memory barrier here because the only way we
-	 * can get here if there is an rcu_barrier() in flight is if
-	 * we are the task doing the rcu_barrier().
-	 */
-
-	/* First adopt the ready-to-invoke callbacks, then the done ones. */
-	rcu_segcblist_insert_done_cbs(&rdp->cblist, &rsp->orphan_done);
-	WARN_ON_ONCE(rsp->orphan_done.head);
-	rcu_segcblist_insert_pend_cbs(&rdp->cblist, &rsp->orphan_pend);
-	WARN_ON_ONCE(rsp->orphan_pend.head);
-	WARN_ON_ONCE(rcu_segcblist_empty(&rdp->cblist) !=
-		     !rcu_segcblist_n_cbs(&rdp->cblist));
-}
-
-/*
  * Trace the fact that this CPU is going offline.
  */
 static void rcu_cleanup_dying_cpu(struct rcu_state *rsp)
@@ -2695,7 +2616,6 @@ static void rcu_cleanup_dead_rnp(struct rcu_node *rnp_leaf)
  */
 static void rcu_cleanup_dead_cpu(int cpu, struct rcu_state *rsp)
 {
-	unsigned long flags;
 	struct rcu_data *rdp = per_cpu_ptr(rsp->rda, cpu);
 	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
 
@@ -2704,18 +2624,6 @@ static void rcu_cleanup_dead_cpu(int cpu, struct rcu_state *rsp)
 
 	/* Adjust any no-longer-needed kthreads. */
 	rcu_boost_kthread_setaffinity(rnp, -1);
-
-	/* Orphan the dead CPU's callbacks, and adopt them if appropriate. */
-	raw_spin_lock_irqsave(&rsp->orphan_lock, flags);
-	rcu_send_cbs_to_orphanage(cpu, rsp, rnp, rdp);
-	rcu_adopt_orphan_cbs(rsp, flags);
-	raw_spin_unlock_irqrestore(&rsp->orphan_lock, flags);
-
-	WARN_ONCE(rcu_segcblist_n_cbs(&rdp->cblist) != 0 ||
-		  !rcu_segcblist_empty(&rdp->cblist),
-		  "rcu_cleanup_dead_cpu: Callbacks on offline CPU %d: qlen=%lu, 1stCB=%p\n",
-		  cpu, rcu_segcblist_n_cbs(&rdp->cblist),
-		  rcu_segcblist_first_cb(&rdp->cblist));
 }
 
 /*
@@ -3927,6 +3835,116 @@ void rcu_report_dead(unsigned int cpu)
 	for_each_rcu_flavor(rsp)
 		rcu_cleanup_dying_idle_cpu(cpu, rsp);
 }
+
+/*
+ * Send the specified CPU's RCU callbacks to the orphanage.  The
+ * specified CPU must be offline, and the caller must hold the
+ * ->orphan_lock.
+ */
+static void
+rcu_send_cbs_to_orphanage(int cpu, struct rcu_state *rsp,
+			  struct rcu_node *rnp, struct rcu_data *rdp)
+{
+	lockdep_assert_held(&rsp->orphan_lock);
+
+	/* No-CBs CPUs do not have orphanable callbacks. */
+	if (!IS_ENABLED(CONFIG_HOTPLUG_CPU) || rcu_is_nocb_cpu(rdp->cpu))
+		return;
+
+	/*
+	 * Orphan the callbacks.  First adjust the counts.  This is safe
+	 * because _rcu_barrier() excludes CPU-hotplug operations, so it
+	 * cannot be running now.  Thus no memory barrier is required.
+	 */
+	rdp->n_cbs_orphaned += rcu_segcblist_n_cbs(&rdp->cblist);
+	rcu_segcblist_extract_count(&rdp->cblist, &rsp->orphan_done);
+
+	/*
+	 * Next, move those callbacks still needing a grace period to
+	 * the orphanage, where some other CPU will pick them up.
+	 * Some of the callbacks might have gone partway through a grace
+	 * period, but that is too bad.  They get to start over because we
+	 * cannot assume that grace periods are synchronized across CPUs.
+	 */
+	rcu_segcblist_extract_pend_cbs(&rdp->cblist, &rsp->orphan_pend);
+
+	/*
+	 * Then move the ready-to-invoke callbacks to the orphanage,
+	 * where some other CPU will pick them up.  These will not be
+	 * required to pass though another grace period: They are done.
+	 */
+	rcu_segcblist_extract_done_cbs(&rdp->cblist, &rsp->orphan_done);
+
+	/* Finally, disallow further callbacks on this CPU.  */
+	rcu_segcblist_disable(&rdp->cblist);
+}
+
+/*
+ * Adopt the RCU callbacks from the specified rcu_state structure's
+ * orphanage.  The caller must hold the ->orphan_lock.
+ */
+static void rcu_adopt_orphan_cbs(struct rcu_state *rsp, unsigned long flags)
+{
+	struct rcu_data *rdp = raw_cpu_ptr(rsp->rda);
+
+	lockdep_assert_held(&rsp->orphan_lock);
+
+	/* No-CBs CPUs are handled specially. */
+	if (!IS_ENABLED(CONFIG_HOTPLUG_CPU) ||
+	    rcu_nocb_adopt_orphan_cbs(rsp, rdp, flags))
+		return;
+
+	/* Do the accounting first. */
+	rdp->n_cbs_adopted += rsp->orphan_done.len;
+	if (rsp->orphan_done.len_lazy != rsp->orphan_done.len)
+		rcu_idle_count_callbacks_posted();
+	rcu_segcblist_insert_count(&rdp->cblist, &rsp->orphan_done);
+
+	/*
+	 * We do not need a memory barrier here because the only way we
+	 * can get here if there is an rcu_barrier() in flight is if
+	 * we are the task doing the rcu_barrier().
+	 */
+
+	/* First adopt the ready-to-invoke callbacks, then the done ones. */
+	rcu_segcblist_insert_done_cbs(&rdp->cblist, &rsp->orphan_done);
+	WARN_ON_ONCE(rsp->orphan_done.head);
+	rcu_segcblist_insert_pend_cbs(&rdp->cblist, &rsp->orphan_pend);
+	WARN_ON_ONCE(rsp->orphan_pend.head);
+	WARN_ON_ONCE(rcu_segcblist_empty(&rdp->cblist) !=
+		     !rcu_segcblist_n_cbs(&rdp->cblist));
+}
+
+/* Orphan the dead CPU's callbacks, and then adopt them. */
+static void rcu_migrate_callbacks(int cpu, struct rcu_state *rsp)
+{
+	unsigned long flags;
+	struct rcu_data *rdp = per_cpu_ptr(rsp->rda, cpu);
+	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
+
+	raw_spin_lock_irqsave(&rsp->orphan_lock, flags);
+	rcu_send_cbs_to_orphanage(cpu, rsp, rnp, rdp);
+	rcu_adopt_orphan_cbs(rsp, flags);
+	raw_spin_unlock_irqrestore(&rsp->orphan_lock, flags);
+	WARN_ONCE(rcu_segcblist_n_cbs(&rdp->cblist) != 0 ||
+		  !rcu_segcblist_empty(&rdp->cblist),
+		  "rcu_cleanup_dead_cpu: Callbacks on offline CPU %d: qlen=%lu, 1stCB=%p\n",
+		  cpu, rcu_segcblist_n_cbs(&rdp->cblist),
+		  rcu_segcblist_first_cb(&rdp->cblist));
+}
+
+/*
+ * The outgoing CPU has just passed through the dying-idle state,
+ * and we are being invoked from the CPU that was IPIed to continue the
+ * offline operation.  We need to migrate the outgoing CPU's callbacks.
+ */
+void rcutree_migrate_callbacks(int cpu)
+{
+	struct rcu_state *rsp;
+
+	for_each_rcu_flavor(rsp)
+		rcu_migrate_callbacks(cpu, rsp);
+}
 #endif
 
 /*

^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-06-22 21:10 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-21 21:57 [PATCH] rcu: remove unused variable in boot_cpu_state_init Arnd Bergmann
2017-06-21 22:10 ` Paul E. McKenney
2017-06-21 22:15   ` Arnd Bergmann
2017-06-22 14:36     ` Paul E. McKenney
2017-06-22 21:10       ` Paul E. McKenney
2017-06-22  7:26   ` Ingo Molnar
2017-06-22  7:41     ` Arnd Bergmann
2017-06-22  7:51       ` Ingo Molnar
2017-06-22  7:54         ` Ingo Molnar
2017-06-22  7:59           ` Ingo Molnar
2017-06-22  8:58             ` Arnd Bergmann
2017-06-22  9:02               ` Ingo Molnar
2017-06-22  9:44                 ` Arnd Bergmann
2017-06-22 14:55     ` Paul E. McKenney

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.