Re: [PATCH] sched/rt: Make rt_rq->pushable_tasks updates drive rto_mask

[Sebastian Andrzej Siewior]

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_@xxxxxxxxxxxxx
> Reported-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Signed-off-by: Valentin Schneider <vschneid@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>

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.

I understand that there is a race and the CPU is cleared from rto_mask
shortly after checking. Therefore I would suggest to look at
has_pushable_tasks() before returning a CPU in rto_next_cpu() as I did
just to avoid the interruption which does nothing.

For the second case the irq_work seems to make no progress. I don't see
any trace_events in hardirq, the mask is cleared outside hardirq (idle
code). The NEED_RESCHED bit is set for current therefore it doesn't make
sense to send irq_work to reschedule if the current already has this on
its agenda.

So what about something like:

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 00e0e50741153..d963408855e25 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -2247,8 +2247,23 @@ static int rto_next_cpu(struct root_domain *rd)
 
 		rd->rto_cpu = cpu;
 
-		if (cpu < nr_cpu_ids)
+		if (cpu < nr_cpu_ids) {
+			struct task_struct *t;
+
+			if (!has_pushable_tasks(cpu_rq(cpu)))
+				continue;
+
+			rcu_read_lock();
+			t = rcu_dereference(rq->curr);
+			/* if (test_preempt_need_resched_cpu(cpu_rq(cpu))) */
+			if (test_tsk_need_resched(t)) {
+				rcu_read_unlock();
+				continue;
+			}
+			rcu_read_unlock();
+
 			return cpu;
+		}
 
 		rd->rto_cpu = -1;
 
Sebastian