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

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

[Lianwei Wang]

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

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

[Peter Zijlstra]

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?

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

[Lianwei Wang]

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

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

[Thomas Gleixner]

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

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

[Lianwei Wang]

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.

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

[Thomas Gleixner]

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

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

[Lianwei Wang]

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

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

[Thomas Gleixner]

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

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

[Lianwei Wang]

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.

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

[Thomas Gleixner]

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.

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

[Lianwei Wang]

On Wed, Apr 27, 2016 at 11:15 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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.

Do you mean no pm_notifier callback can return an error or NOTIFY_BAD
to abort the suspend process?

It's not the driver issue. The driver return an error to abort the
suspend process on purpose. Why do you think it is not allowed to
return an error to abort suspend?

The issue is very clear as described below.
1. How the issue happened?
One of the pm notifier return error to abort suspend before
cpu_hotplug_disable() is called on PM_SUSPEND_PREPARE.

2. What's the result?
CPU hotplug work in a wrong way, or it doesn't work anymore. No way to
recover it.

3. The root cause is that there is no any handling for the unbalanced
cpu_hotplug_disable/enable calling. This patch add a protection for
such issue.

Anything not clear?

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

[Thomas Gleixner]

On Thu, 28 Apr 2016, Lianwei Wang wrote:
> On Wed, Apr 27, 2016 at 11:15 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 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.
> 
> Do you mean no pm_notifier callback can return an error or NOTIFY_BAD
> to abort the suspend process?
> 
> It's not the driver issue. The driver return an error to abort the
> suspend process on purpose. Why do you think it is not allowed to
> return an error to abort suspend?
> 
> The issue is very clear as described below.
> 1. How the issue happened?
> One of the pm notifier return error to abort suspend before
> cpu_hotplug_disable() is called on PM_SUSPEND_PREPARE.
> 
> 2. What's the result?
> CPU hotplug work in a wrong way, or it doesn't work anymore. No way to
> recover it.
> 
> 3. The root cause is that there is no any handling for the unbalanced
> cpu_hotplug_disable/enable calling. This patch add a protection for
> such issue.

Wrong. This is the symptom. The root cause is in #1. Therefor you are trying
to fix the symptom and not the root cause
 
> Anything not clear?

No. 

I completely understand that you are tyring to put the cart before the horse.

Thanks,

	tglx

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

[Lianwei Wang]

On Thu, Apr 28, 2016 at 5:44 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 28 Apr 2016, Lianwei Wang wrote:
>> On Wed, Apr 27, 2016 at 11:15 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > 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.
>>
>> Do you mean no pm_notifier callback can return an error or NOTIFY_BAD
>> to abort the suspend process?
>>
>> It's not the driver issue. The driver return an error to abort the
>> suspend process on purpose. Why do you think it is not allowed to
>> return an error to abort suspend?
>>
>> The issue is very clear as described below.
>> 1. How the issue happened?
>> One of the pm notifier return error to abort suspend before
>> cpu_hotplug_disable() is called on PM_SUSPEND_PREPARE.
>>
>> 2. What's the result?
>> CPU hotplug work in a wrong way, or it doesn't work anymore. No way to
>> recover it.
>>
>> 3. The root cause is that there is no any handling for the unbalanced
>> cpu_hotplug_disable/enable calling. This patch add a protection for
>> such issue.
>
> Wrong. This is the symptom. The root cause is in #1. Therefor you are trying
> to fix the symptom and not the root cause
>
I don't understand why you keep saying that the issue is in the pm
notifier callback. As I told you, the pm notifier return an error(or
NOTIFY_BAD) on purpose to abort the suspend process. This is work as
design. Any driver can abort the suspend process if it is not ready to
suspend.

I know your maintainers are busy but it is not hard for you to
understand it. If you did not look at the suspend code for a long time
then you can look at it now.

Below are some examples to return error to abort the suspend on
PM_SUSPEND_PREPARE pm notifier.
http://lxr.free-electrons.com/source/arch/x86/power/cpu.c#L290
http://lxr.free-electrons.com/source/arch/s390/kernel/suspend.c#L164
http://lxr.free-electrons.com/source/drivers/s390/cio/css.c#L840
http://lxr.free-electrons.com/source/drivers/devfreq/exynos/exynos5_bus.c#L202

>> Anything not clear?
>
> No.
>
> I completely understand that you are tyring to put the cart before the horse.
No. Your understanding is wrong.
>
> Thanks,
>
>         tglx

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

[Thomas Gleixner]

On Fri, 29 Apr 2016, Lianwei Wang wrote:
> On Thu, Apr 28, 2016 at 5:44 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > Wrong. This is the symptom. The root cause is in #1. Therefor you are trying
> > to fix the symptom and not the root cause
> >
> I don't understand why you keep saying that the issue is in the pm
> notifier callback. As I told you, the pm notifier return an error(or
> NOTIFY_BAD) on purpose to abort the suspend process. This is work as
> design. Any driver can abort the suspend process if it is not ready to
> suspend.

Right. That's not the issue. The issue is that as a consequence we end up with
an unbalanced count. So how do we end up with an unbalanced count? That's what
needs to be fixed and not worked around.

> > I completely understand that you are tyring to put the cart before the horse.
> No. Your understanding is wrong.

My understanding is very correct. We have a situation which leads to an
unbalanced count. Instead of fixing that, you fix up the unbalanced count.

Thanks,

	tglx

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

[Lianwei Wang]

On Mon, May 2, 2016 at 1:11 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 29 Apr 2016, Lianwei Wang wrote:
>> On Thu, Apr 28, 2016 at 5:44 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > Wrong. This is the symptom. The root cause is in #1. Therefor you are trying
>> > to fix the symptom and not the root cause
>> >
>> I don't understand why you keep saying that the issue is in the pm
>> notifier callback. As I told you, the pm notifier return an error(or
>> NOTIFY_BAD) on purpose to abort the suspend process. This is work as
>> design. Any driver can abort the suspend process if it is not ready to
>> suspend.
>
> Right. That's not the issue. The issue is that as a consequence we end up with
> an unbalanced count. So how do we end up with an unbalanced count? That's what
> needs to be fixed and not worked around.
>
In this example, the unbalanced count is caused by the
cpu_hotplug_pm_callback pm notifier callback function. We can add a
variable to avoid the unbalanced call of cpu_hotplug_enable ,e.g.
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 3e3f6e49eabb..aa6694f0e9d3 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1140,16 +1140,21 @@ static int
 cpu_hotplug_pm_callback(struct notifier_block *nb,
                        unsigned long action, void *ptr)
 {
+       static int disabled;
+
        switch (action) {

        case PM_SUSPEND_PREPARE:
        case PM_HIBERNATION_PREPARE:
                cpu_hotplug_disable();
+               disabled = 1;
                break;

        case PM_POST_SUSPEND:
        case PM_POST_HIBERNATION:
-               cpu_hotplug_enable();
+               if (disabled)
+                       cpu_hotplug_enable();
+               disabled = 0;
                break;

        default:

Please let me know if you like to fix it in this way.

But actually I think we don't need to add a new variable to check if
the cpu_hotplug_disable() is called or not. We already have a disable
counter which can be used to check if the cpu_hotpug_disable is called
or not, as my original patch do in cpu_hotplug_enable() function.
Maybe the reset comments and reset cpu_hotplug_disabled to 0 operation
confuse you. I should check it firstly and do nothing if it is already
0.
e.g.
+static void _cpu_hotplug_enable(void)
+{
+       if (WARN(!cpu_hotplug_disabled, "Unbalanced cpu hotplug enable\n"))
+               return;
+
+       cpu_hotplug_disabled--;
+}

I like to fix it in the cpu_hotplug_enable because it is a public
kernel API and fix in it can prevent any other unbalanced calling. I
will update the patch.

>> > I completely understand that you are tyring to put the cart before the horse.
>> No. Your understanding is wrong.
>
> My understanding is very correct. We have a situation which leads to an
> unbalanced count. Instead of fixing that, you fix up the unbalanced count.
>
Yes, that's right. We are on the same page. The only difference is
that where/how to fix it. See my two solutions above and let me know
which one you like?
> Thanks,
>
>         tglx

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

[Thomas Gleixner]

On Wed, 4 May 2016, Lianwei Wang wrote:
> In this example, the unbalanced count is caused by the
> cpu_hotplug_pm_callback pm notifier callback function.

I doubt that.

> We can add a variable to avoid the unbalanced call of cpu_hotplug_enable
> ,e.g.

> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 3e3f6e49eabb..aa6694f0e9d3 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1140,16 +1140,21 @@ static int
>  cpu_hotplug_pm_callback(struct notifier_block *nb,
>                         unsigned long action, void *ptr)
>  {
> +       static int disabled;
> +
>         switch (action) {
> 
>         case PM_SUSPEND_PREPARE:
>         case PM_HIBERNATION_PREPARE:
>                 cpu_hotplug_disable();
> +               disabled = 1;
>                 break;
> 
>         case PM_POST_SUSPEND:
>         case PM_POST_HIBERNATION:
> -               cpu_hotplug_enable();
> +               if (disabled)
> +                       cpu_hotplug_enable();
> +               disabled = 0;
>                 break;
> 
>         default:
> 
> Please let me know if you like to fix it in this way.

So you are moving the work around one step down w/o providing any reasonable
explanation how this asymetric call of that callback can happen.

Can you eventually come up with a coherent explanation of the problem down to
the root cause or are we going to play this "move the workaround one step
down" game for another 10 rounds?
 
> +static void _cpu_hotplug_enable(void)
> +{
> +       if (WARN(!cpu_hotplug_disabled, "Unbalanced cpu hotplug enable\n"))
> +               return;
> +
> +       cpu_hotplug_disabled--;
> +}
> 
> I like to fix it in the cpu_hotplug_enable because it is a public

You CANNOT fix it there. The problem is the call site and NOT
cpu_hotplug_enable(). Can you finally accept this?

> kernel API and fix in it can prevent any other unbalanced calling. I

It cannot prevent any unbalanced calls. It mitigates the issue, but that's a
different problem.

We can discuss that seperately after fixing the offending call site.

Thanks,

	tglx

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

[Lianwei Wang]

On Thu, May 5, 2016 at 5:13 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 4 May 2016, Lianwei Wang wrote:
>> In this example, the unbalanced count is caused by the
>> cpu_hotplug_pm_callback pm notifier callback function.
>
> I doubt that.
>
>> We can add a variable to avoid the unbalanced call of cpu_hotplug_enable
>> ,e.g.
>
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 3e3f6e49eabb..aa6694f0e9d3 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -1140,16 +1140,21 @@ static int
>>  cpu_hotplug_pm_callback(struct notifier_block *nb,
>>                         unsigned long action, void *ptr)
>>  {
>> +       static int disabled;
>> +
>>         switch (action) {
>>
>>         case PM_SUSPEND_PREPARE:
>>         case PM_HIBERNATION_PREPARE:
>>                 cpu_hotplug_disable();
>> +               disabled = 1;
>>                 break;
>>
>>         case PM_POST_SUSPEND:
>>         case PM_POST_HIBERNATION:
>> -               cpu_hotplug_enable();
>> +               if (disabled)
>> +                       cpu_hotplug_enable();
>> +               disabled = 0;
>>                 break;
>>
>>         default:
>>
>> Please let me know if you like to fix it in this way.
>
> So you are moving the work around one step down w/o providing any reasonable
> explanation how this asymetric call of that callback can happen.
>
> Can you eventually come up with a coherent explanation of the problem down to
> the root cause or are we going to play this "move the workaround one step
> down" game for another 10 rounds?
>
Do you agree that any driver can abort the suspend process by
returning an error or NOTIFY_BAD if it is not ready to suspend?
I have explain it and I also copied the example code that abort
suspend by returning an error or NOTIFY_BAD in the pm notifier
callback function.
The cpu_hotplug_disable and cpu_hotplug_enable are called in one of
the PM notifier callback. And they are called from two difference
place.
Below is how it happened:
  pm_suspend
    |--enter_state
        |--suspend_prepare
            |--pm_notifier_call_chain(PM_SUSPEND_PREPARE)
            |    |--call_back_1
            |    |--call_back_..
            |    |--call_back_n ===> return NOTIFY_BAD to abort call chain and
            |    |                                suspend process here
            |    |--cpu_hotplug_pm_callback()
            |    |   |--cpu_hotplug_disable  =====> remember it is not
called yet
            |    |--call_back_..
            |
            |--pm_notifier_call_chain(PM_POST_SUSPEND)
            |    |--call_back_1
            |    |--call_back_..
            |    |--call_back_n
            |    |--cpu_hotplug_pm_callback()
            |    |   |--cpu_hotplug_enable  =====> Here it is unbalanced called
            |    |--call_back_..
            |
So, keep in mind that for pm notifier call chain, the
PM_SUSPEND_PREPARE notifier and PM_POST_SUSPEND notifier is not always
paired called. Sometimes for a driver's pm notifier callback, the
PM_POST_SUSPEND is called without PM_SUSPEND_PREPARE.

>> +static void _cpu_hotplug_enable(void)
>> +{
>> +       if (WARN(!cpu_hotplug_disabled, "Unbalanced cpu hotplug enable\n"))
>> +               return;
>> +
>> +       cpu_hotplug_disabled--;
>> +}
>>
>> I like to fix it in the cpu_hotplug_enable because it is a public
>
> You CANNOT fix it there. The problem is the call site and NOT
> cpu_hotplug_enable(). Can you finally accept this?
I know what you mean. But why let the driver unconditionally do
"--cpu_hotplug_disabled" without any checking. It should do nothing if
it detect a unbalanced enable. A good example for unbalanced checking
from enable_irq is here:
http://lxr.free-electrons.com/source/kernel/irq/manage.c#L512

It's the cpu hotplug driver's responsibility to guarantee that the cpu
hotplug always working well even others failed do something to it. And
the driver can check and handle it itself, why not let the driver to
handle it and make the cpu hotplug driver more strong?
>
>> kernel API and fix in it can prevent any other unbalanced calling.
>
> It cannot prevent any unbalanced calls. It mitigates the issue, but that's a
> different problem.
It did not migrate the issue. It give a warning message to log the
unbalanced issue and it also make sure the cpu hotplug continue to
work well even someone do an unbalanced call. It is a good checking as
the enable_irq/disable_irq do. There are some other unbalanced
checking in kernel too. All make sure the kernel has a better
stability.
>
> We can discuss that seperately after fixing the offending call site.
>
> Thanks,
>
>         tglx

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

[Thomas Gleixner]

On Fri, 6 May 2016, Lianwei Wang wrote:
> On Thu, May 5, 2016 at 5:13 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > Can you eventually come up with a coherent explanation of the problem down to
> > the root cause or are we going to play this "move the workaround one step
> > down" game for another 10 rounds?
> >
> Do you agree that any driver can abort the suspend process by
> returning an error or NOTIFY_BAD if it is not ready to suspend?
> I have explain it and I also copied the example code that abort
> suspend by returning an error or NOTIFY_BAD in the pm notifier
> callback function.

I don't need copied example code which does not tell me what the real problem
is.

> The cpu_hotplug_disable and cpu_hotplug_enable are called in one of
> the PM notifier callback. And they are called from two difference
> place.
> Below is how it happened:
>   pm_suspend
>     |--enter_state
>         |--suspend_prepare
>             |--pm_notifier_call_chain(PM_SUSPEND_PREPARE)
>             |    |--call_back_1
>             |    |--call_back_..
>             |    |--call_back_n ===> return NOTIFY_BAD to abort call chain and
>             |    |                                suspend process here
>             |    |--cpu_hotplug_pm_callback()
>             |    |   |--cpu_hotplug_disable  =====> remember it is not
> called yet
>             |    |--call_back_..
>             |
>             |--pm_notifier_call_chain(PM_POST_SUSPEND)
>             |    |--call_back_1
>             |    |--call_back_..
>             |    |--call_back_n
>             |    |--cpu_hotplug_pm_callback()
>             |    |   |--cpu_hotplug_enable  =====> Here it is unbalanced called
>             |    |--call_back_..
>             |
> So, keep in mind that for pm notifier call chain, the
> PM_SUSPEND_PREPARE notifier and PM_POST_SUSPEND notifier is not always
> paired called. Sometimes for a driver's pm notifier callback, the
> PM_POST_SUSPEND is called without PM_SUSPEND_PREPARE.

So that is the real problem: cpu_hotplug_pm_callback(PM_POST_SUSPEND) can be
called w/o a previous call to cpu_hotplug_pm_callback(PM_SUSPEND_PREPARE).
 
> > It cannot prevent any unbalanced calls. It mitigates the issue, but that's a
> > different problem.
> It did not migrate the issue. It give a warning message to log the
> unbalanced issue and it also make sure the cpu hotplug continue to
> work well even someone do an unbalanced call. It is a good checking as
> the enable_irq/disable_irq do. There are some other unbalanced
> checking in kernel too. All make sure the kernel has a better
> stability.

I'm not opposed to do that and I said so several times. But I said as well,
that we do not add this without fixing the problem which made you write that
patch in the first place.

So we have a proper explanation for the real problem now, but we have no
fix. 

And again: Your patch is NOT a fix. Simply because it will emit a warning
everytime the above happens. And that's wrong because the abort is a
legitimate scenario.

So please come up with a sensible fix for the suspend abort issue and then we
can add the balance check/fixup to the hotplug_disable/enable() code.

Thanks,

	tglx

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

[Lianwei Wang]

I have come up a patch to make the pm notifier called symmetrically
and currently being tested. I will send it out after pass the test.

On Fri, May 6, 2016 at 12:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 6 May 2016, Lianwei Wang wrote:
>> On Thu, May 5, 2016 at 5:13 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > Can you eventually come up with a coherent explanation of the problem down to
>> > the root cause or are we going to play this "move the workaround one step
>> > down" game for another 10 rounds?
>> >
>> Do you agree that any driver can abort the suspend process by
>> returning an error or NOTIFY_BAD if it is not ready to suspend?
>> I have explain it and I also copied the example code that abort
>> suspend by returning an error or NOTIFY_BAD in the pm notifier
>> callback function.
>
> I don't need copied example code which does not tell me what the real problem
> is.
>
>> The cpu_hotplug_disable and cpu_hotplug_enable are called in one of
>> the PM notifier callback. And they are called from two difference
>> place.
>> Below is how it happened:
>>   pm_suspend
>>     |--enter_state
>>         |--suspend_prepare
>>             |--pm_notifier_call_chain(PM_SUSPEND_PREPARE)
>>             |    |--call_back_1
>>             |    |--call_back_..
>>             |    |--call_back_n ===> return NOTIFY_BAD to abort call chain and
>>             |    |                                suspend process here
>>             |    |--cpu_hotplug_pm_callback()
>>             |    |   |--cpu_hotplug_disable  =====> remember it is not
>> called yet
>>             |    |--call_back_..
>>             |
>>             |--pm_notifier_call_chain(PM_POST_SUSPEND)
>>             |    |--call_back_1
>>             |    |--call_back_..
>>             |    |--call_back_n
>>             |    |--cpu_hotplug_pm_callback()
>>             |    |   |--cpu_hotplug_enable  =====> Here it is unbalanced called
>>             |    |--call_back_..
>>             |
>> So, keep in mind that for pm notifier call chain, the
>> PM_SUSPEND_PREPARE notifier and PM_POST_SUSPEND notifier is not always
>> paired called. Sometimes for a driver's pm notifier callback, the
>> PM_POST_SUSPEND is called without PM_SUSPEND_PREPARE.
>
> So that is the real problem: cpu_hotplug_pm_callback(PM_POST_SUSPEND) can be
> called w/o a previous call to cpu_hotplug_pm_callback(PM_SUSPEND_PREPARE).
>
>> > It cannot prevent any unbalanced calls. It mitigates the issue, but that's a
>> > different problem.
>> It did not migrate the issue. It give a warning message to log the
>> unbalanced issue and it also make sure the cpu hotplug continue to
>> work well even someone do an unbalanced call. It is a good checking as
>> the enable_irq/disable_irq do. There are some other unbalanced
>> checking in kernel too. All make sure the kernel has a better
>> stability.
>
> I'm not opposed to do that and I said so several times. But I said as well,
> that we do not add this without fixing the problem which made you write that
> patch in the first place.
>
> So we have a proper explanation for the real problem now, but we have no
> fix.
>
> And again: Your patch is NOT a fix. Simply because it will emit a warning
> everytime the above happens. And that's wrong because the abort is a
> legitimate scenario.
>
> So please come up with a sensible fix for the suspend abort issue and then we
> can add the balance check/fixup to the hotplug_disable/enable() code.
>
> Thanks,
>
>         tglx
>
>

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

[Lianwei Wang]

On Thu, May 12, 2016 at 1:06 AM, Lianwei Wang <lianwei.wang@gmail.com> wrote:
> I have come up a patch to make the pm notifier called symmetrically
> and currently being tested. I will send it out after pass the test.
>
> On Fri, May 6, 2016 at 12:18 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Fri, 6 May 2016, Lianwei Wang wrote:
>>> On Thu, May 5, 2016 at 5:13 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>> > Can you eventually come up with a coherent explanation of the problem down to
>>> > the root cause or are we going to play this "move the workaround one step
>>> > down" game for another 10 rounds?
>>> >
>>> Do you agree that any driver can abort the suspend process by
>>> returning an error or NOTIFY_BAD if it is not ready to suspend?
>>> I have explain it and I also copied the example code that abort
>>> suspend by returning an error or NOTIFY_BAD in the pm notifier
>>> callback function.
>>
>> I don't need copied example code which does not tell me what the real problem
>> is.
>>
>>> The cpu_hotplug_disable and cpu_hotplug_enable are called in one of
>>> the PM notifier callback. And they are called from two difference
>>> place.
>>> Below is how it happened:
>>>   pm_suspend
>>>     |--enter_state
>>>         |--suspend_prepare
>>>             |--pm_notifier_call_chain(PM_SUSPEND_PREPARE)
>>>             |    |--call_back_1
>>>             |    |--call_back_..
>>>             |    |--call_back_n ===> return NOTIFY_BAD to abort call chain and
>>>             |    |                                suspend process here
>>>             |    |--cpu_hotplug_pm_callback()
>>>             |    |   |--cpu_hotplug_disable  =====> remember it is not
>>> called yet
>>>             |    |--call_back_..
>>>             |
>>>             |--pm_notifier_call_chain(PM_POST_SUSPEND)
>>>             |    |--call_back_1
>>>             |    |--call_back_..
>>>             |    |--call_back_n
>>>             |    |--cpu_hotplug_pm_callback()
>>>             |    |   |--cpu_hotplug_enable  =====> Here it is unbalanced called
>>>             |    |--call_back_..
>>>             |
>>> So, keep in mind that for pm notifier call chain, the
>>> PM_SUSPEND_PREPARE notifier and PM_POST_SUSPEND notifier is not always
>>> paired called. Sometimes for a driver's pm notifier callback, the
>>> PM_POST_SUSPEND is called without PM_SUSPEND_PREPARE.
>>
>> So that is the real problem: cpu_hotplug_pm_callback(PM_POST_SUSPEND) can be
>> called w/o a previous call to cpu_hotplug_pm_callback(PM_SUSPEND_PREPARE).
>>
>>> > It cannot prevent any unbalanced calls. It mitigates the issue, but that's a
>>> > different problem.
>>> It did not migrate the issue. It give a warning message to log the
>>> unbalanced issue and it also make sure the cpu hotplug continue to
>>> work well even someone do an unbalanced call. It is a good checking as
>>> the enable_irq/disable_irq do. There are some other unbalanced
>>> checking in kernel too. All make sure the kernel has a better
>>> stability.
>>
>> I'm not opposed to do that and I said so several times. But I said as well,
>> that we do not add this without fixing the problem which made you write that
>> patch in the first place.
>>
>> So we have a proper explanation for the real problem now, but we have no
>> fix.
>>
>> And again: Your patch is NOT a fix. Simply because it will emit a warning
>> everytime the above happens. And that's wrong because the abort is a
>> legitimate scenario.
>>
>> So please come up with a sensible fix for the suspend abort issue and then we
>> can add the balance check/fixup to the hotplug_disable/enable() code.
>>
>> Thanks,
>>
>>         tglx
>>
>>

I think this is the best solution to fix this unbalanced issue for now.