[PATCH 0/3] Reduce spurious RT_PUSH_IPI storms

[PATCH 0/3] Reduce spurious RT_PUSH_IPI storms

[Mel Gorman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2894 bytes --]

RT_PUSH_IPI is intended to minimise latency and rq lock contention when
migrating tasks to balance RT task distribution.  For example, if a high
priority task changes to a lower priority then CPUs with off-CPU RT tasks
may be sent an IPI to "push" tasks away to an available CPU. The problem
is that if a CPU is rt_overloaded but has no tasks that can be pushed then
it continues to receive IPIs -- specifically tasks that are migrate_disabled.

An issue was identified where a CPU received a large number of IPIs in a
short space of time but no tasks could move, it was still marked rt
overloaded and continued to receive IPIs.

This problem was examined in commit dae5c0292080 ("sched/rt,dl: Skip
migrate-disabled tasks when picking a push candidate") but the fix is
incomplete as noted in the changelog.

The original bug against an old kernel was due to pgrep running with RT
priority on an isolated CPU with an active RT task. It was acknowledged
that the "proper" fix was to move pgrep, drop its priority or both but
the underlying reason for the storm was pgrep being spinlock-intensive
and excessive IPIs being sent which is still sub-optimal behaviour. A test
case creates a "victim" task with short work/sleep cycles allowed to run
on CPU1 and CPU2 that optionally disables migration (md=1) while running.
A second task mostly does work pinned on CPU1 with short sleeps, a third
task mostly sleeps on CPU2 so CPU2 pulls frequently.  The results are

| Kernel (7.4-rc2)       |     CPU1-md=1 |   CPU2-md=1 |    CPU1-md=0 |    CPU2-md=0 |
|------------------------|---------------|-------------|--------------|--------------|
| vanilla-rt-push-ipi    | 41474.7 ±0.1% |           0 | 4784.4 ±0.2% |   85.0 ±9.9% |
| series-rt-push-ipi     |             0 | 0.3 ±380.6% | 4792.1 ±0.3% |  87.1 ±10.8% |
| series-no-rt-push-ipi  |             0 |           0 |            0 |            0 |
(RT push IPIs per CPU per 5 s interval, mean ±CoV over 30 intervals,
 md=1 means the "victim" with cpumask for CPUs 1-2 disables migration)

The vanilla 7.4-rc2 kernel fired 40k+ IPIs every 5 seconds with low variance
over 30 samples with a migrate-disabled task sitting on CPU1. Separate
testing showed that the IPIs were not resulting in tasks moving and therefore
spurious. The series eliminates the spurious IPIs (second row). Disabling
IPIs also showed no IPIs but it was confirmed that tasks were still moved.

Yousaf (cc'd) is the original author of the first patch, I only decorated it
and provided transport, hence why his Signed-off-by is first on that patch.

Mel Gorman (3):
  sched/rt: Do not account migrate-disabled tasks as pushable
  sched/rt: Check eligibility of pushable task in one place
  sched/rt: Treat migrate-disabled tasks as pushable if RT_PUSH_IPI is
    disabled

 kernel/sched/rt.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.55.0

[PATCH 1/3] sched/rt: Do not account migrate-disabled tasks as pushable

[Mel Gorman]

Commit dae5c0292080 ("sched/rt,dl: Skip migrate-disabled tasks when
picking a push candidate") stops pick_next_pushable_task() returning a
migrate-disabled task which fixes the balancer trying to push pinned tasks
off the CPU. It notes

    The self-IPI path is closed by the rto_next_cpu() fix above, and that
    part works. But the runqueue is still marked overloaded, because the
    pinned task is still advertised as pushable. Other CPUs now send the
    push-IPIs during their own RT balancing, and the same loop runs again.
    Closing the self-IPI path did not stop a pinned task from triggering
    push balancing.

As above, the runqueue is still marked overloaded by enqueue_task_rt()
and put_prev_task_rt() even if the task is migrate-disabled. A
migrate-disabled task reaches the pushable list through enqueue_task_rt()
or put_prev_task_rt() when it is preempted, or when it wakes while
still migrate-disabled. Either path calls enqueue_pushable_task() to set
rq->rt.overloaded and rto_mask if unset. Once set, every pull from another
CPU that fires an IPI may only find migrate-disabled tasks rendering the IPI
useless for a condition that was detectable when placed on the pushable list.

Unfortunately, IPIs can be continually fire at the same task that cannot
move.  The problem motivating this patch was a misplaced pgrep running
at RT priority and while the pgrep can be bound to another CPU by the
application, the underlying problem was that pgrep is spinlock-intensive.

Stop adding migrate-disabled tasks to the pushable list. This is not a
heuristic as migrate-disabled tasks cannot move. Even when enabled, the
rq still has no pushable candidates until the task schedules.

Note that altering the RT class only is deliberate and deadline
is left alone. The justification for RT is eliminating ineffective
RT_PUSH_IPIs. Deadline is trickier -- it tries to push the task running
on the rq that holds an off-CPU, migrate-disabled task, so that the
migrate-disabled task can run there instead of waiting while another CPU
runs later-deadline work. The cost of that push is bounded, unlike arbitrary
IPIs, and justifiable because deadline ordering is otherwise violated.

For deadline the same two decision sites exist, and any change must update
both together.

In the specific context of put_prev_task_dl(), the task is going off-CPU
with migration disabled, so pushing the running task may take longer
than letting the running task use its budget. The problem is that the
time to migrate the running task can only be estimated but ignoring EDF
requirements is a root-domain-wide violation of deadline principles.
That is out of scope for this patch.

In the specific context of enqueue_task_dl(), it could be justified on
the basis that the task is only enqueued, not yet running. This is the
same question as put_prev_task_dl() -- honour EDF ordering or ignore it.
That is also out of scope for this patch.

Updating deadline in a similar fashion is not justified by the RT IPI
problem. It has different requirements that merit its own patch and
separate justification.

-stable warning: This patch is part of a series and applying just this
	patch on its own breaks the !RT_PUSH_IPI case.

[mgorman@techsingularity.net: deadline decision, changelog]
Signed-off-by: Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 kernel/sched/rt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 1535046a23ff..2587ef92efc3 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1448,7 +1448,7 @@ enqueue_task_rt(struct rq *rq, struct task_struct *p, int flags)
 	if (task_is_blocked(p))
 		return;
 
-	if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
+	if (!task_current(rq, p) && p->nr_cpus_allowed > 1 && !is_migration_disabled(p))
 		enqueue_pushable_task(rq, p);
 }
 
@@ -1746,7 +1746,7 @@ static void put_prev_task_rt(struct rq *rq, struct task_struct *p, struct task_s
 	 * The previous task needs to be made eligible for pushing
 	 * if it is still active
 	 */
-	if (on_rt_rq(&p->rt) && p->nr_cpus_allowed > 1)
+	if (on_rt_rq(&p->rt) && p->nr_cpus_allowed > 1 && !is_migration_disabled(p))
 		enqueue_pushable_task(rq, p);
 }
 
-- 
2.55.0

[PATCH 2/3] sched/rt: Check eligibility of pushable task in one place

[Mel Gorman]

Two callers of enqueue_pushable_task() perform the same checks, move them
to one place to mitigate the risk of the checks made out of sync and in
preparation for the next patch.

No functional change intended.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 kernel/sched/rt.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 2587ef92efc3..cc77683f4897 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -396,6 +396,9 @@ static inline void rt_queue_pull_task(struct rq *rq)
 
 static void enqueue_pushable_task(struct rq *rq, struct task_struct *p)
 {
+	if (p->nr_cpus_allowed <= 1 || is_migration_disabled(p))
+		return;
+
 	plist_del(&p->pushable_tasks, &rq->rt.pushable_tasks);
 	plist_node_init(&p->pushable_tasks, p->prio);
 	plist_add(&p->pushable_tasks, &rq->rt.pushable_tasks);
@@ -1448,7 +1451,7 @@ enqueue_task_rt(struct rq *rq, struct task_struct *p, int flags)
 	if (task_is_blocked(p))
 		return;
 
-	if (!task_current(rq, p) && p->nr_cpus_allowed > 1 && !is_migration_disabled(p))
+	if (!task_current(rq, p))
 		enqueue_pushable_task(rq, p);
 }
 
@@ -1746,7 +1749,7 @@ static void put_prev_task_rt(struct rq *rq, struct task_struct *p, struct task_s
 	 * The previous task needs to be made eligible for pushing
 	 * if it is still active
 	 */
-	if (on_rt_rq(&p->rt) && p->nr_cpus_allowed > 1 && !is_migration_disabled(p))
+	if (on_rt_rq(&p->rt))
 		enqueue_pushable_task(rq, p);
 }
 
-- 
2.55.0

[PATCH 3/3] sched/rt: Treat migrate-disabled tasks as pushable if RT_PUSH_IPI is disabled

[Mel Gorman]

Migrate-disabled tasks are not accounted as pushable to avoid spurious
IPIs. When RT_PUSH_IPI is unavailable or disabled then there are no IPIs so
pull_rt_task() must find rqs with off-cpu migrate-disabled tasks. It then
can push the running task away allowing the migrate-disabled task to run.

Restore the original accounting of migrate-disabled tasks as pushable if
RT_PUSH_IPI is disabled.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 kernel/sched/rt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index cc77683f4897..bb640278d30e 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -396,8 +396,12 @@ static inline void rt_queue_pull_task(struct rq *rq)
 
 static void enqueue_pushable_task(struct rq *rq, struct task_struct *p)
 {
-	if (p->nr_cpus_allowed <= 1 || is_migration_disabled(p))
+	if (p->nr_cpus_allowed <= 1)
 		return;
+#ifdef HAVE_RT_PUSH_IPI
+	if (sched_feat(RT_PUSH_IPI) && is_migration_disabled(p))
+		return;
+#endif
 
 	plist_del(&p->pushable_tasks, &rq->rt.pushable_tasks);
 	plist_node_init(&p->pushable_tasks, p->prio);
-- 
2.55.0