Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383871 > unrolled thread

[PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable

Back to article view | Back to linux.kernel

Contents

  [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Lianwei Wang <lianwei.wang@gmail.com> - 2016-04-21 07:00 +0200
    Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Peter Zijlstra <peterz@infradead.org> - 2016-04-21 13:00 +0200
      Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Lianwei Wang <lianwei.wang@gmail.com> - 2016-04-22 18:40 +0200
        Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Thomas Gleixner <tglx@linutronix.de> - 2016-04-22 18:40 +0200
          Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Lianwei Wang <lianwei.wang@gmail.com> - 2016-04-23 00:00 +0200
            Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Thomas Gleixner <tglx@linutronix.de> - 2016-04-25 10:30 +0200
              Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Lianwei Wang <lianwei.wang@gmail.com> - 2016-04-26 09:00 +0200
                Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Thomas Gleixner <tglx@linutronix.de> - 2016-04-27 12:20 +0200
                  Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Thomas Gleixner <tglx@linutronix.de> - 2016-04-28 08:20 +0200
                  Re: [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable Lianwei Wang <lianwei.wang@gmail.com> - 2016-04-28 08:20 +0200

#1383871 — [PATCH] cpu/hotplug: handle unbalanced hotplug enable/disable

Currently it just print a warning message but did not
reset cpu_hotplug_disabled when the enable/disable is
unbalanced. The unbalanced enable/disable will lead
the cpu hotplug work abnormally.

Reset it to 0 when an unablanced enable detected.

Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 kernel/cpu.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 6ea42e8da861..fef6caed77a3 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -243,6 +243,19 @@ void cpu_hotplug_done(void)
 	cpuhp_lock_release();
 }
 
+static void _cpu_hotplug_disable(void)
+{
+	cpu_hotplug_disabled++;
+}
+
+static void _cpu_hotplug_enable(void)
+{
+	if (--cpu_hotplug_disabled < 0) {
+		WARN(1, "unbalanced hotplug enable %d\n", cpu_hotplug_disabled);
+		cpu_hotplug_disabled = 0;
+	}
+}
+
 /*
  * Wait for currently running CPU hotplug operations to complete (if any) and
  * disable future CPU hotplug (from sysfs). The 'cpu_add_remove_lock' protects
@@ -253,7 +266,7 @@ void cpu_hotplug_done(void)
 void cpu_hotplug_disable(void)
 {
 	cpu_maps_update_begin();
-	cpu_hotplug_disabled++;
+	_cpu_hotplug_disable();
 	cpu_maps_update_done();
 }
 EXPORT_SYMBOL_GPL(cpu_hotplug_disable);
@@ -261,7 +274,7 @@ EXPORT_SYMBOL_GPL(cpu_hotplug_disable);
 void cpu_hotplug_enable(void)
 {
 	cpu_maps_update_begin();
-	WARN_ON(--cpu_hotplug_disabled < 0);
+	_cpu_hotplug_enable();
 	cpu_maps_update_done();
 }
 EXPORT_SYMBOL_GPL(cpu_hotplug_enable);
@@ -1053,7 +1066,7 @@ int disable_nonboot_cpus(void)
 	 * this even in case of failure as all disable_nonboot_cpus() users are
 	 * supposed to do enable_nonboot_cpus() on the failure path.
 	 */
-	cpu_hotplug_disabled++;
+	_cpu_hotplug_disable();
 
 	cpu_maps_update_done();
 	return error;
@@ -1073,7 +1086,7 @@ void enable_nonboot_cpus(void)
 
 	/* Allow everyone to use the CPU hotplug again */
 	cpu_maps_update_begin();
-	WARN_ON(--cpu_hotplug_disabled < 0);
+	_cpu_hotplug_enable();
 	if (cpumask_empty(frozen_cpus))
 		goto out;
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1384057

On Wed, Apr 20, 2016 at 09:56:07PM -0700, Lianwei Wang wrote:
> Currently it just print a warning message but did not
> reset cpu_hotplug_disabled when the enable/disable is
> unbalanced. The unbalanced enable/disable will lead
> the cpu hotplug work abnormally.
> 
> Reset it to 0 when an unablanced enable detected.

How can this happen in the first place?

[toc] | [prev] | [next] | [standalone]

#1385336

On Thu, Apr 21, 2016 at 3:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Apr 20, 2016 at 09:56:07PM -0700, Lianwei Wang wrote:
>> Currently it just print a warning message but did not
>> reset cpu_hotplug_disabled when the enable/disable is
>> unbalanced. The unbalanced enable/disable will lead
>> the cpu hotplug work abnormally.
>>
>> Reset it to 0 when an unablanced enable detected.
>
> How can this happen in the first place?

That's is my question too, and why we check it with WARN_ON here?
Obviously it is possible to happened because the
cpu_hotplug_disable/enable are both kernel API and any driver can call
it. A unbalanced check is a good way to handle it.

The actually problem here is that what we do in case it happened? Just
give a warning or do some error handling and recover it back? This's
my focus..

[toc] | [prev] | [next] | [standalone]

#1385339

On Fri, 22 Apr 2016, Lianwei Wang wrote:

> On Thu, Apr 21, 2016 at 3:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, Apr 20, 2016 at 09:56:07PM -0700, Lianwei Wang wrote:
> >> Currently it just print a warning message but did not
> >> reset cpu_hotplug_disabled when the enable/disable is
> >> unbalanced. The unbalanced enable/disable will lead
> >> the cpu hotplug work abnormally.
> >>
> >> Reset it to 0 when an unablanced enable detected.
> >
> > How can this happen in the first place?
> 
> That's is my question too, and why we check it with WARN_ON here?
> Obviously it is possible to happened because the
> cpu_hotplug_disable/enable are both kernel API and any driver can call
> it. A unbalanced check is a good way to handle it.
> 
> The actually problem here is that what we do in case it happened? Just
> give a warning or do some error handling and recover it back? This's
> my focus..

Actually we do nothing if it happens. We just emit a warning and that's good
enough because the machine is still accessible and therefor debugable. It just
renders cpu hotplug useless, but that's not a fundamental problem. If you look
at the checks we do with preempt count, where we actually restore the counter
that's a different issue. If we would not do that we simply would break the
machine completely and end up in an endless storm of warnings. Different
story, but that hotplug thing is just not in that class of problems.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1385485

On Fri, Apr 22, 2016 at 9:37 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 22 Apr 2016, Lianwei Wang wrote:
>
>> On Thu, Apr 21, 2016 at 3:50 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Wed, Apr 20, 2016 at 09:56:07PM -0700, Lianwei Wang wrote:
>> >> Currently it just print a warning message but did not
>> >> reset cpu_hotplug_disabled when the enable/disable is
>> >> unbalanced. The unbalanced enable/disable will lead
>> >> the cpu hotplug work abnormally.
>> >>
>> >> Reset it to 0 when an unablanced enable detected.
>> >
>> > How can this happen in the first place?
>>
>> That's is my question too, and why we check it with WARN_ON here?
>> Obviously it is possible to happened because the
>> cpu_hotplug_disable/enable are both kernel API and any driver can call
>> it. A unbalanced check is a good way to handle it.
>>
>> The actually problem here is that what we do in case it happened? Just
>> give a warning or do some error handling and recover it back? This's
>> my focus..
>
> Actually we do nothing if it happens. We just emit a warning and that's good
> enough because the machine is still accessible and therefor debugable. It just
> renders cpu hotplug useless, but that's not a fundamental problem. If you look
> at the checks we do with preempt count, where we actually restore the counter
> that's a different issue. If we would not do that we simply would break the
> machine completely and end up in an endless storm of warnings. Different
> story, but that hotplug thing is just not in that class of problems.
>
> Thanks,
>
>         tglx

Any way is Ok for debugging purpose. But think the kernel run on a
customer machine, such as PC, Mobile phone or other devices. How we
let the customer debug it but not recover it smartly?

If it happened then the cpu hotplug is not useless but may work in a
wrong way. E.g. the cpu_hotplug_disabled now is -1 after the last call
of cpu_hotplug_enable, then it is actually DISABLED but not enabled,
and the following  call of cpu_hotplug_disable is actually enable but
not disable. There is no way for the customer or end user to recover
it except do a power cycle and reboot.

Anyway, from a product perspective way, if we don't want to restore
the unbalanced counter to 0, then maybe a BUG_ON is more reasonable
than WARN_ON.

[toc] | [prev] | [next] | [standalone]

#1386097

On Fri, 22 Apr 2016, Lianwei Wang wrote:
> Any way is Ok for debugging purpose. But think the kernel run on a
> customer machine, such as PC, Mobile phone or other devices. How we
> let the customer debug it but not recover it smartly?

There is nothing smart here. Restoring the count is a bandaid and has nothing
to do with recovery. If that WARN_ON triggers then other stuff is going to be
more fundamentally wrong so restoring the count is the least of our worries.

> Anyway, from a product perspective way, if we don't want to restore
> the unbalanced counter to 0, then maybe a BUG_ON is more reasonable
> than WARN_ON.

Not at all. BUG_ON is the last resort if we have no other way to handle an
issue.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1387178

On Mon, Apr 25, 2016 at 1:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 22 Apr 2016, Lianwei Wang wrote:
>> Any way is Ok for debugging purpose. But think the kernel run on a
>> customer machine, such as PC, Mobile phone or other devices. How we
>> let the customer debug it but not recover it smartly?
>
> There is nothing smart here. Restoring the count is a bandaid and has nothing
> to do with recovery. If that WARN_ON triggers then other stuff is going to be
> more fundamentally wrong so restoring the count is the least of our worries.
>
You are still think it from a developer view. You can not let the
customer/consumer to fix such issue, right? You even can not let the
customer/consumer to wait for the fix, right?

Take the suspend for example, the suspend_prepare will call
pm_notifier_call_chain to send PM_SUSPEND_PREPARE notification. If one
of the function on the chain return NOTIFY_BAD or NOTIFY_STOP before
calling cpu_hotplug_pm_callback, then either way will cause the
cpu_hotplug_disable() not called in
cpu_hotplug_pm_callback(PM_SUSPEND_PREPARE). When the suspend is going
to call pm_notifier_call_chain(PM_POST_SUSPEND) ->
cpu_hotplug_pm_callback -> cpu_hotplug_enable() , then it is
Unbalanced...

There are other paths to cause the counter unbalanced too. But no
matter how it is unbalanced, we can detect it and recover it to normal
state.

>> Anyway, from a product perspective way, if we don't want to restore
>> the unbalanced counter to 0, then maybe a BUG_ON is more reasonable
>> than WARN_ON.
>
> Not at all. BUG_ON is the last resort if we have no other way to handle an
> issue.
Actually to the customer, you do nothing currently at all, and once it
happened then there is no way for the customer to recover it except do
a power cycle. A BUG_ON can trigger a power cycle and recover it.
>
> Thanks,
>
>         tglx

[toc] | [prev] | [next] | [standalone]

#1388875

On Mon, 25 Apr 2016, Lianwei Wang wrote:
> On Mon, Apr 25, 2016 at 1:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> Anyway, from a product perspective way, if we don't want to restore
> >> the unbalanced counter to 0, then maybe a BUG_ON is more reasonable
> >> than WARN_ON.
> >
> > Not at all. BUG_ON is the last resort if we have no other way to handle an
> > issue.
> Actually to the customer, you do nothing currently at all, and once it
> happened then there is no way for the customer to recover it except do
> a power cycle. A BUG_ON can trigger a power cycle and recover it.

Do you have a single incident where this happened?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1389797

On Wed, 27 Apr 2016, Lianwei Wang wrote:
> Yes. In our project, there is a kernel driver which register a pm
> notifier. On some conditions this pm notifier will return an error and
> abort the suspend process. The counter will be unbalanced in case it
> happened.

So what? You wreckaged your driver, so you fix it and be done with it.

[toc] | [prev] | [next] | [standalone]

#1389798

On Wed, Apr 27, 2016 at 3:17 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 25 Apr 2016, Lianwei Wang wrote:
>> On Mon, Apr 25, 2016 at 1:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> >> Anyway, from a product perspective way, if we don't want to restore
>> >> the unbalanced counter to 0, then maybe a BUG_ON is more reasonable
>> >> than WARN_ON.
>> >
>> > Not at all. BUG_ON is the last resort if we have no other way to handle an
>> > issue.
>> Actually to the customer, you do nothing currently at all, and once it
>> happened then there is no way for the customer to recover it except do
>> a power cycle. A BUG_ON can trigger a power cycle and recover it.
>
> Do you have a single incident where this happened?
>
> Thanks,
>
>         tglx

Yes. In our project, there is a kernel driver which register a pm
notifier. On some conditions this pm notifier will return an error and
abort the suspend process. The counter will be unbalanced in case it
happened.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web