Messages in this thread | | | Date | Fri, 8 Nov 2019 14:28:43 +0000 | From | Quentin Perret <> | Subject | Re: [PATCH 1/7] sched: Fix pick_next_task() vs change pattern race |
| |
On Friday 08 Nov 2019 at 14:15:54 (+0100), Peter Zijlstra wrote: > Commit 67692435c411 ("sched: Rework pick_next_task() slow-path") > inadvertly introduced a race because it changed a previously > unexplored dependency between dropping the rq->lock and > sched_class::put_prev_task(). > > The comments about dropping rq->lock, in for example > newidle_balance(), only mentions the task being current and ->on_cpu > being set. But when we look at the 'change' pattern (in for example > sched_setnuma()): > > queued = task_on_rq_queued(p); /* p->on_rq == TASK_ON_RQ_QUEUED */ > running = task_current(rq, p); /* rq->curr == p */ > > if (queued) > dequeue_task(...); > if (running) > put_prev_task(...); > > /* change task properties */ > > if (queued) > enqueue_task(...); > if (running) > set_next_task(...); > > It becomes obvious that if we do this after put_prev_task() has > already been called on @p, things go sideways. This is exactly what > the commit in question allows to happen when it does: > > prev->sched_class->put_prev_task(rq, prev, rf); > if (!rq->nr_running) > newidle_balance(rq, rf); > > The newidle_balance() call will drop rq->lock after we've called > put_prev_task() and that allows the above 'change' pattern to > interleave and mess up the state. > > Furthermore, it turns out we lost the RT-pull when we put the last DL > task. > > Fix both problems by extracting the balancing from put_prev_task() and > doing a multi-class balance() pass before put_prev_task(). > > Fixes: 67692435c411 ("sched: Rework pick_next_task() slow-path") > Reported-by: Quentin Perret <qperret@google.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
The reproducer that triggered in 30sec or so has now been running for 3 hours:
Tested-by: Quentin Perret <qperret@google.com>
Thanks for fix, Quentin
| |