Re: [PATCH] kthread: Atomically set completion and perform dequeue in __kthread_parkme

[Vikram Mulukutla <markivx@codeaurora.org>]

On 7/4/2017 9:07 AM, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 03:18:03PM -0700, Vikram Mulukutla wrote:
>>   kernel/kthread.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/kthread.c b/kernel/kthread.c
>> index 26db528..7ad3354 100644
>> --- a/kernel/kthread.c
>> +++ b/kernel/kthread.c
>> @@ -171,9 +171,20 @@ static void __kthread_parkme(struct kthread *self)
>>   {
>>   	__set_current_state(TASK_PARKED);
>>   	while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
>> +		/*
>> +		 * Why the preempt_disable?
>> +		 * Hotplug needs to ensure that 'self' is off of the runqueue
>> +		 * as well, before scheduling the stopper thread that will
>> +		 * migrate tasks off of the runqeue that 'self' was running on.
>> +		 * This avoids unnecessary migration work and also ensures that
>> +		 * kthread_unpark in the cpu_up path doesn't race with
>> +		 * __kthread_parkme.
>> +		 */
>> +		preempt_disable();
>>   		if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
>>   			complete(&self->parked);
>> +		schedule_preempt_disabled();
> 
> This is broken. schedule_preempt_disable() doesn't guarantee no
> preemptions, just makes it less likely.

Right, the API just informs the scheduler that the calling thread
wishes to have preemption disabled when the API returns. I thought
it was going to guarantee no preemption until the thread is actually
off of the runqueue, but I see the window where an interrupt might
preempt. Doh.

Separate from this hotplug problem, would it be entirely moronic to have
the API disable and enable local interrupts across that short window? I
suppose there's no one that needs this sort of thing so.. no?

> 
>> +		preempt_enable();
>>   		__set_current_state(TASK_PARKED);
>>   	}
>>   	clear_bit(KTHREAD_IS_PARKED, &self->flags);

Thanks,
Vikram