Messages in this thread | | | Subject | Re: [PATCH v1] kthread/smpboot: Serialize kthread parking against wakeup | From | "Kohli, Gaurav" <> | Date | Tue, 5 Jun 2018 23:51:18 +0530 |
| |
Hi,
Just for info , the patch that I have shared earlier with pi_lock approach has been tested since last one month and no issue has been observed,
https://lkml.org/lkml/2018/4/25/189
Can we take this if it looks good?
Regards Gaurav
On 6/5/2018 10:05 PM, Oleg Nesterov wrote: > On 06/05, Peter Zijlstra wrote: >> >> On Tue, Jun 05, 2018 at 05:22:12PM +0200, Peter Zijlstra wrote: >> >>>> OK, but __kthread_parkme() can be preempted before it calls schedule(), so the >>>> caller still can be migrated? Plus kthread_park_complete() can be called twice. >>> >>> Argh... I forgot TASK_DEAD does the whole thing with preempt_disable(). >>> Let me stare at that a bit. >> >> This should ensure we only ever complete when we read PARKED, right? >> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index 8d59b259af4a..e513b4600796 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -2641,7 +2641,7 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev, >> * past. prev == current is still correct but we need to recalculate this_rq >> * because prev may have moved to another CPU. >> */ >> -static struct rq *finish_task_switch(struct task_struct *prev) >> +static struct rq *finish_task_switch(struct task_struct *prev, bool preempt) >> __releases(rq->lock) >> { >> struct rq *rq = this_rq(); >> @@ -2674,7 +2674,7 @@ static struct rq *finish_task_switch(struct task_struct *prev) >> * >> * We must observe prev->state before clearing prev->on_cpu (in >> * finish_task), otherwise a concurrent wakeup can get prev >> - * running on another CPU and we could rave with its RUNNING -> DEAD >> + * running on another CPU and we could race with its RUNNING -> DEAD >> * transition, resulting in a double drop. >> */ >> prev_state = prev->state; >> @@ -2720,7 +2720,8 @@ static struct rq *finish_task_switch(struct task_struct *prev) >> break; >> >> case TASK_PARKED: >> - kthread_park_complete(prev); >> + if (!preempt) >> + kthread_park_complete(prev); > > > Yes, but this won't fix the race decribed by Kohli... > > Plus this complicates the schedule() paths for the very special case, and to me > it seems that all this kthread_park/unpark logic needs some serious cleanups... > > Not that I can suggest something better right now. > > Oleg. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
-- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |