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

[PATCH v2] 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.

Do nothing if an unablanced hotplug 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 3e3f6e49eabb..8011b1e40523 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -245,6 +245,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 (WARN(!cpu_hotplug_disabled, "Unbalanced cpu hotplug enable\n"))
+		return;
+
+	cpu_hotplug_disabled--;
+}
+
 /*
  * Wait for currently running CPU hotplug operations to complete (if any) and
  * disable future CPU hotplug (from sysfs). The 'cpu_add_remove_lock' protects
@@ -255,7 +268,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);
@@ -263,7 +276,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);
@@ -1071,7 +1084,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;
@@ -1091,7 +1104,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 v2] cpu/hotplug: handle unbalanced hotplug enable/disable

[Thomas Gleixner]

On Mon, 6 Jun 2016, 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.
> 
> Do nothing if an unablanced hotplug 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 3e3f6e49eabb..8011b1e40523 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -245,6 +245,19 @@ void cpu_hotplug_done(void)
>  	cpuhp_lock_release();
>  }
>  
> +static void _cpu_hotplug_disable(void)

What's the purpose of this function?

> +{
> +	cpu_hotplug_disabled++;
> +}
> +
> +static void _cpu_hotplug_enable(void)

Double underscores please

> +{
> +	if (WARN(!cpu_hotplug_disabled, "Unbalanced cpu hotplug enable\n"))
> +		return;

And this want's to be a WARN_ONCE()

Thanks,

	tglx

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

[Lianwei Wang]

On Mon, Jun 6, 2016 at 11:34 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 6 Jun 2016, 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.
>>
>> Do nothing if an unablanced hotplug 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 3e3f6e49eabb..8011b1e40523 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -245,6 +245,19 @@ void cpu_hotplug_done(void)
>>       cpuhp_lock_release();
>>  }
>>
>> +static void _cpu_hotplug_disable(void)
>
> What's the purpose of this function?
>
The only purpose is to make the cpu_hotplug_disable as one API and
always disable it from the same interface. It makes the code looks
more beautiful. But yes, we can remove it since it is not necessary
for this change.

>> +{
>> +     cpu_hotplug_disabled++;
>> +}
>> +
>> +static void _cpu_hotplug_enable(void)
>
> Double underscores please
>
Ok, I will update it.
>> +{
>> +     if (WARN(!cpu_hotplug_disabled, "Unbalanced cpu hotplug enable\n"))
>> +             return;
>
> And this want's to be a WARN_ONCE()
>
WARN is convenient for debugging because kernel buffer is limited and
we might lose the first warning message. But WARN_ONCE is good to me
too.

> Thanks,
>
>         tglx