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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

* 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