lkml.org 
[lkml]   [2021]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.12 010/110] psi: Fix psi state corruption when schedule() races with cgroup move
    Date
    From: Johannes Weiner <hannes@cmpxchg.org>

    commit d583d360a620e6229422b3455d0be082b8255f5e upstream.

    4117cebf1a9f ("psi: Optimize task switch inside shared cgroups")
    introduced a race condition that corrupts internal psi state. This
    manifests as kernel warnings, sometimes followed by bogusly high IO
    pressure:

    psi: task underflow! cpu=1 t=2 tasks=[0 0 0 0] clear=c set=0
    (schedule() decreasing RUNNING and ONCPU, both of which are 0)

    psi: incosistent task state! task=2412744:systemd cpu=17 psi_flags=e clear=3 set=0
    (cgroup_move_task() clearing MEMSTALL and IOWAIT, but task is MEMSTALL | RUNNING | ONCPU)

    What the offending commit does is batch the two psi callbacks in
    schedule() to reduce the number of cgroup tree updates. When prev is
    deactivated and removed from the runqueue, nothing is done in psi at
    first; when the task switch completes, TSK_RUNNING and TSK_IOWAIT are
    updated along with TSK_ONCPU.

    However, the deactivation and the task switch inside schedule() aren't
    atomic: pick_next_task() may drop the rq lock for load balancing. When
    this happens, cgroup_move_task() can run after the task has been
    physically dequeued, but the psi updates are still pending. Since it
    looks at the task's scheduler state, it doesn't move everything to the
    new cgroup that the task switch that follows is about to clear from
    it. cgroup_move_task() will leak the TSK_RUNNING count in the old
    cgroup, and psi_sched_switch() will underflow it in the new cgroup.

    A similar thing can happen for iowait. TSK_IOWAIT is usually set when
    a p->in_iowait task is dequeued, but again this update is deferred to
    the switch. cgroup_move_task() can see an unqueued p->in_iowait task
    and move a non-existent TSK_IOWAIT. This results in the inconsistent
    task state warning, as well as a counter underflow that will result in
    permanent IO ghost pressure being reported.

    Fix this bug by making cgroup_move_task() use task->psi_flags instead
    of looking at the potentially mismatching scheduler state.

    [ We used the scheduler state historically in order to not rely on
    task->psi_flags for anything but debugging. But that ship has sailed
    anyway, and this is simpler and more robust.

    We previously already batched TSK_ONCPU clearing with the
    TSK_RUNNING update inside the deactivation call from schedule(). But
    that ordering was safe and didn't result in TSK_ONCPU corruption:
    unlike most places in the scheduler, cgroup_move_task() only checked
    task_current() and handled TSK_ONCPU if the task was still queued. ]

    Fixes: 4117cebf1a9f ("psi: Optimize task switch inside shared cgroups")
    Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
    Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    Link: https://lkml.kernel.org/r/20210503174917.38579-1-hannes@cmpxchg.org
    Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    kernel/sched/psi.c | 36 ++++++++++++++++++++++++++----------
    1 file changed, 26 insertions(+), 10 deletions(-)

    diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
    index 651218ded981..ef37acd28e4a 100644
    --- a/kernel/sched/psi.c
    +++ b/kernel/sched/psi.c
    @@ -965,7 +965,7 @@ void psi_cgroup_free(struct cgroup *cgroup)
    */
    void cgroup_move_task(struct task_struct *task, struct css_set *to)
    {
    - unsigned int task_flags = 0;
    + unsigned int task_flags;
    struct rq_flags rf;
    struct rq *rq;

    @@ -980,15 +980,31 @@ void cgroup_move_task(struct task_struct *task, struct css_set *to)

    rq = task_rq_lock(task, &rf);

    - if (task_on_rq_queued(task)) {
    - task_flags = TSK_RUNNING;
    - if (task_current(rq, task))
    - task_flags |= TSK_ONCPU;
    - } else if (task->in_iowait)
    - task_flags = TSK_IOWAIT;
    -
    - if (task->in_memstall)
    - task_flags |= TSK_MEMSTALL;
    + /*
    + * We may race with schedule() dropping the rq lock between
    + * deactivating prev and switching to next. Because the psi
    + * updates from the deactivation are deferred to the switch
    + * callback to save cgroup tree updates, the task's scheduling
    + * state here is not coherent with its psi state:
    + *
    + * schedule() cgroup_move_task()
    + * rq_lock()
    + * deactivate_task()
    + * p->on_rq = 0
    + * psi_dequeue() // defers TSK_RUNNING & TSK_IOWAIT updates
    + * pick_next_task()
    + * rq_unlock()
    + * rq_lock()
    + * psi_task_change() // old cgroup
    + * task->cgroups = to
    + * psi_task_change() // new cgroup
    + * rq_unlock()
    + * rq_lock()
    + * psi_sched_switch() // does deferred updates in new cgroup
    + *
    + * Don't rely on the scheduling state. Use psi_flags instead.
    + */
    + task_flags = task->psi_flags;

    if (task_flags)
    psi_task_change(task, task_flags, 0);
    --
    2.30.2
    \
     
     \ /
      Last update: 2021-06-28 16:19    [W:5.616 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site