[v1][PATCH] cpu_down: move migrate_enable() back

[v1][PATCH] cpu_down: move migrate_enable() back

[Tiejun Chen]

Commit 08c1ab68, "hotplug-use-migrate-disable.patch", intends to
use migrate_enable()/migrate_disable() to replace that combination
of preempt_enable() and preempt_disable(), but actually in
!CONFIG_PREEMPT_RT_FULL case, migrate_enable()/migrate_disable()
are still equal to preempt_enable()/preempt_disable(). So that
followed cpu_hotplug_begin()/cpu_unplug_begin(cpu) would go schedule()
to trigger schedule_debug() like this:

_cpu_down()
	|
	+ migrate_disable() = preempt_disable()
	|
	+ cpu_hotplug_begin() or cpu_unplug_begin()
		|
		+ schedule()
			|
			+ __schedule()
				|
				+ preempt_disable();
				|
				+ __schedule_bug() is true!

So we should move migrate_enable() as the original scheme.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 kernel/cpu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 98f2ea3..da6e128 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -594,6 +594,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
 		err = -EBUSY;
 		goto restore_cpus;
 	}
+	migrate_enable();
 
 	cpu_hotplug_begin();
 	err = cpu_unplug_begin(cpu);
@@ -647,7 +648,6 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
 out_release:
 	cpu_unplug_done(cpu);
 out_cancel:
-	migrate_enable();
 	cpu_hotplug_done();
 	if (!err)
 		cpu_notify_nofail(CPU_POST_DEAD | mod, hcpu);
-- 
1.7.9.5

Re: [v1][PATCH] cpu_down: move migrate_enable() back

[Paul Gortmaker]

On 13-11-06 09:06 PM, Tiejun Chen wrote:
> Commit 08c1ab68, "hotplug-use-migrate-disable.patch", intends to
> use migrate_enable()/migrate_disable() to replace that combination

Since you quote a commit ID, it would be nice if you mentioned that
it is in v3.8-rt from the stable-rt repo, instead of making people
look it up.

Also, since you have cc'd stable-rt@vger, there should be some mention
of what versions it might be appropriate for (3.4?  3.10?).

> of preempt_enable() and preempt_disable(), but actually in
> !CONFIG_PREEMPT_RT_FULL case, migrate_enable()/migrate_disable()
> are still equal to preempt_enable()/preempt_disable(). So that
> followed cpu_hotplug_begin()/cpu_unplug_begin(cpu) would go schedule()
> to trigger schedule_debug() like this:
> 
> _cpu_down()
> 	|
> 	+ migrate_disable() = preempt_disable()
> 	|
> 	+ cpu_hotplug_begin() or cpu_unplug_begin()
> 		|
> 		+ schedule()
> 			|
> 			+ __schedule()
> 				|
> 				+ preempt_disable();
> 				|
> 				+ __schedule_bug() is true!
> 
> So we should move migrate_enable() as the original scheme.

It is unclear to me what context you are thinking of/referencing
as the "original scheme"...

> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  kernel/cpu.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 98f2ea3..da6e128 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -594,6 +594,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
>  		err = -EBUSY;
>  		goto restore_cpus;
>  	}
> +	migrate_enable();
>  

So, what happens if we now get migrated right here, before the
hotplug_begin call below?

P.
--

>  	cpu_hotplug_begin();
>  	err = cpu_unplug_begin(cpu);
> @@ -647,7 +648,6 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
>  out_release:
>  	cpu_unplug_done(cpu);
>  out_cancel:
> -	migrate_enable();
>  	cpu_hotplug_done();
>  	if (!err)
>  		cpu_notify_nofail(CPU_POST_DEAD | mod, hcpu);
> 

Re: [v1][PATCH] cpu_down: move migrate_enable() back

[Sebastian Andrzej Siewior]

* Paul Gortmaker | 2013-11-11 10:32:55 [-0500]:

>> of preempt_enable() and preempt_disable(), but actually in
>> !CONFIG_PREEMPT_RT_FULL case, migrate_enable()/migrate_disable()
>> are still equal to preempt_enable()/preempt_disable(). So that
>> followed cpu_hotplug_begin()/cpu_unplug_begin(cpu) would go schedule()
>> to trigger schedule_debug() like this:
>> 
>> _cpu_down()
>> 	|
>> 	+ migrate_disable() = preempt_disable()
>> 	|
>> 	+ cpu_hotplug_begin() or cpu_unplug_begin()
>> 		|
>> 		+ schedule()
>> 			|
>> 			+ __schedule()
>> 				|
>> 				+ preempt_disable();
>> 				|
>> 				+ __schedule_bug() is true!
>> 
>> So we should move migrate_enable() as the original scheme.
>
>It is unclear to me what context you are thinking of/referencing
>as the "original scheme"...
I think to what happening before the patch he quoted was applied.

>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -594,6 +594,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
>>  		err = -EBUSY;
>>  		goto restore_cpus;
>>  	}
>> +	migrate_enable();
>>  
>
>So, what happens if we now get migrated right here, before the
>hotplug_begin call below?

It should not matter. Before this we call set_cpus_allowed_ptr() to
ensure that current() may run on any CPU except that one we try to bring
down. We call later smp_processor_id() to check if this is correct. The
only reason for the preempt_disable() seems to be to keep
smp_processor_id() quiet and not spill a warning.
Later on we could switch CPUs as long as we don't get on that CPU that is
going down since we don't hold / access any per CPU data. The only
reason where set_cpus_allowed_ptr() might not work as expected is when
user land changes the mask because we don't have PF_NO_SETAFFINITY set.

>P.

Sebastian

Re: [v1][PATCH] cpu_down: move migrate_enable() back

[Sebastian Andrzej Siewior]

* Tiejun Chen | 2013-11-07 10:06:07 [+0800]:

>Commit 08c1ab68, "hotplug-use-migrate-disable.patch", intends to
>use migrate_enable()/migrate_disable() to replace that combination
>of preempt_enable() and preempt_disable(), but actually in
>!CONFIG_PREEMPT_RT_FULL case, migrate_enable()/migrate_disable()
>are still equal to preempt_enable()/preempt_disable(). So that
>followed cpu_hotplug_begin()/cpu_unplug_begin(cpu) would go schedule()
>to trigger schedule_debug() like this:
>
>_cpu_down()
>	|
>	+ migrate_disable() = preempt_disable()
>	|
>	+ cpu_hotplug_begin() or cpu_unplug_begin()
>		|
>		+ schedule()
>			|
>			+ __schedule()
>				|
>				+ preempt_disable();
>				|
>				+ __schedule_bug() is true!
>
>So we should move migrate_enable() as the original scheme.
>
>Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>

applied.

Sebastian