Re: [PATCH] allow preemption in recursive migrate_disable call

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

* Peter Zijlstra | 2013-11-21 11:19:13 [+0100]:

>On Wed, Nov 20, 2013 at 01:33:33AM +0100, Nicholas Mc Guire wrote:
>> From a7259c360b6c8b873f5fcf6d5eed0ae78534a6c5 Mon Sep 17 00:00:00 2001
>> From: Nicholas Mc Guire <der.herr@hofr.at>
>> Date: Wed, 20 Nov 2013 07:22:09 +0800
>> Subject: [PATCH] allow preemption in recursive migrate_disable call
>> 
>>  Minor cleanup in migrate_disable/migrate_enable. The recursive case
>>  does not need to disable preemption as it is "pinned" to the current
>>  cpu any way so it is safe to preempt it.
>> 
>>  No functional change to migrate_disable/enable
>> 
>> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
>> ---
>>  kernel/sched/core.c |    6 ++----
>>  1 files changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index a7fafc28..22fa2e2 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2418,13 +2418,12 @@ void migrate_disable(void)
>>  	}
>>  #endif
>>  
>> -	preempt_disable();
>>  	if (p->migrate_disable) {
>>  		p->migrate_disable++;
>> -		preempt_enable();
>>  		return;
>>  	}
>>  
>> +	preempt_disable();
>>  	preempt_lazy_disable();
>>  	pin_current_cpu();
>>  	p->migrate_disable = 1;
>> @@ -2454,13 +2453,12 @@ void migrate_enable(void)
>>  #endif
>>  	WARN_ON_ONCE(p->migrate_disable <= 0);
>>  
>> -	preempt_disable();
>>  	if (migrate_disable_count(p) > 1) {
>>  		p->migrate_disable--;
>> -		preempt_enable();
>>  		return;
>>  	}
>>  
>> +	preempt_disable();
>>  	if (unlikely(migrate_disabled_updated(p))) {
>>  		/*
>>  		 * Undo whatever update_migrate_disable() did, also see there
>
>Is there a reason one uses p->migrate_disable and the other uses
>migrate_disable_count(p) ? 

After staring at code I would say migrate_disable() needs to ignore
MIGRATE_DISABLE_SET_AFFIN in order to complete. In the migrate_disable()
case it simply doesn't matter if it is set and it can't be set for the 0
case.
I was a little worried because the preempt_disable() also protects
counter during add/substract operations but since there can only be one
task at a time doing this, there should be no problem.

>Other than that,
>
>Acked-by: Peter Zijlstra <peterz@infradead.org>

Sebastian