From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.codeaurora.org ([198.145.29.96]:52288 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751878AbdGERVP (ORCPT ); Wed, 5 Jul 2017 13:21:15 -0400 Subject: Re: [PATCH] kthread: Atomically set completion and perform dequeue in __kthread_parkme To: Peter Zijlstra Cc: rusty@rustcorp.com.au, tj@kernel.org, Thomas Gleixner , 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 Message-ID: <825a79ae-aeb7-6959-9489-2007bdc50b4a@codeaurora.org> Date: Wed, 5 Jul 2017 10:21:13 -0700 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: stable-owner@vger.kernel.org List-ID: 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