DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org C666F6084D
Subject: Re: [PATCH] kthread: Atomically set completion and perform dequeue in
 __kthread_parkme
To: Peter Zijlstra <peterz@infradead.org>
Cc: rusty@rustcorp.com.au, tj@kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <1498515483-12743-1-git-send-email-markivx@codeaurora.org>
 <20170704160721.oawhbx3efaw4d4we@hirez.programming.kicks-ass.net>
From: Vikram Mulukutla <markivx@codeaurora.org>
Message-ID: <825a79ae-aeb7-6959-9489-2007bdc50b4a@codeaurora.org>
Date: Wed, 5 Jul 2017 10:21:13 -0700
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <20170704160721.oawhbx3efaw4d4we@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1843
Lines: 48

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