Messages in this thread | | | Date | Fri, 1 Apr 2022 13:41:20 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH] sched/core: Fix forceidle balancing |
| |
On Wed, Mar 30, 2022 at 06:05:35PM +0200, Peter Zijlstra wrote: > > Steve reported that ChromeOS encounters the forceidle balancer being > ran from rt_mutex_setprio()'s balance_callback() invocation and > explodes. > > Now, the forceidle balancer gets queued every time the idle task gets > selected, set_next_task(), which is strictly too often. > rt_mutex_setprio() also uses set_next_task() in the 'change' pattern: > > 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(...); > > However, rt_mutex_setprio() will explicitly not run this pattern on > the idle task (since priority boosting the idle task is quite insane). > Most other 'change' pattern users are pidhash based and would also not > apply to idle. > > Also, the change pattern doesn't contain a __balance_callback() > invocation and hence we could have an out-of-band balance-callback, > which *should* trigger the WARN in rq_pin_lock() (which guards against > this exact anti-pattern). > > So while none of that explains how this happens, it does indicate that > having it in set_next_task() might not be the most robust option. > > Instead, explicitly queue the forceidle balancer from pick_next_task() > when it does indeed result in forceidle selection. Having it here, > ensures it can only be triggered under the __schedule() rq->lock > instance, and hence must be ran from that context. > > This also happens to clean up the code a little, so win-win.
So I couldn't figure out how this could happen without triggering other warnings, because as I mentioned elsewhere, commit 565790d28b1e ("sched: Fix balance_callback()") should've caused a different splat.
But then Dietmar reminded me that ChromeOS is probably running some ancient crud with backports on :/ and will most likely not have that commit.
| |