[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZRFEeGKGzE+dY6n6@gmail.com>
Date: Mon, 25 Sep 2023 10:27:36 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Valentin Schneider <vschneid@...hat.com>,
linux-kernel@...r.kernel.org,
Daniel Bristot de Oliveira <bristot@...hat.com>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Ingo Molnar <mingo@...hat.com>,
Juri Lelli <juri.lelli@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Steven Rostedt <rostedt@...dmis.org>,
Vincent Guittot <vincent.guittot@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] sched/rt: Make rt_rq->pushable_tasks updates drive
rto_mask
* Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> On 2023-08-11 12:20:44 [+0100], Valentin Schneider wrote:
> > Sebastian noted that the rto_push_work IRQ work can be queued for a CPU
> > that has an empty pushable_tasks list, which means nothing useful will be
> > done in the IPI other than queue the work for the next CPU on the rto_mask.
> >
> > rto_push_irq_work_func() only operates on tasks in the pushable_tasks list,
> > but the conditions for that irq_work to be queued (and for a CPU to be
> > added to the rto_mask) rely on rq_rt->nr_migratory instead.
> >
> > nr_migratory is increased whenever an RT task entity is enqueued and it has
> > nr_cpus_allowed > 1. Unlike the pushable_tasks list, nr_migratory includes a
> > rt_rq's current task. This means a rt_rq can have a migratible current, N
> > non-migratible queued tasks, and be flagged as overloaded / have its CPU
> > set in the rto_mask, despite having an empty pushable_tasks list.
> >
> > Make an rt_rq's overload logic be driven by {enqueue,dequeue}_pushable_task().
> > Since rt_rq->{rt_nr_migratory,rt_nr_total} become unused, remove them.
> >
> > Note that the case where the current task is pushed away to make way for a
> > migration-disabled task remains unchanged: the migration-disabled task has
> > to be in the pushable_tasks list in the first place, which means it has
> > nr_cpus_allowed > 1.
> >
> > Link: http://lore.kernel.org/r/20230801152648._y603AS_@linutronix.de
> > Reported-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> > Signed-off-by: Valentin Schneider <vschneid@...hat.com>
> > ---
> > This is lightly tested, this looks to be working OK but I don't have nor am
> > I aware of a test case for RT balancing, I suppose we want something that
> > asserts we always run the N highest prio tasks for N CPUs, with a small
> > margin for migrations?
>
> I don't see the storm of IPIs I saw before. So as far that goes:
> Tested-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
I've applied Valentin's initial fix to tip:sched/core, for an eventual
v6.7 merge, as it addresses the IPI storm bug. Let me know if merging
this is not desirable for some reason.
> What I still observe is:
> - CPU0 is idle. CPU0 gets a task assigned from CPU1. That task receives
> a wakeup. CPU0 returns from idle and schedules the task.
> pull_rt_task() on CPU1 and sometimes on other CPU observe this, too.
> CPU1 sends irq_work to CPU0 while at the time rto_next_cpu() sees that
> has_pushable_tasks() return 0. That bit was cleared earlier (as per
> tracing).
>
> - CPU0 is idle. CPU0 gets a task assigned from CPU1. The task on CPU0 is
> woken up without an IPI (yay). But then pull_rt_task() decides that
> send irq_work and has_pushable_tasks() said that is has tasks left
> so….
> Now: rto_push_irq_work_func() run once once on CPU0, does nothing,
> rto_next_cpu() return CPU0 again and enqueues itself again on CPU0.
> Usually after the second or third round the scheduler on CPU0 makes
> enough progress to remove the task/ clear the CPU from mask.
Just curious, any progress on solving this?
Thanks,
Ingo
Powered by blists - more mailing lists