Re: [PATCH 08/20 v3] sched/fair: Add push task mechanism for fair

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] sched/fair: broken proxy execution handling in fair_push_task()
- [High] sched/fair: broken throttled task handling in cfs
  pushable_tasks
- [Medium] sched/fair: wrong util_est accounting in cpu_util() for
  fair_push_task()
- [Low] sched/fair: missing context analysis annotations in
  fair_push_tasks()
--

commit 7788037a9d21dee51af9b507ce742334ce7472a3
Author: Vincent Guittot <vincent.guittot@linaro.org>

sched/fair: Add push task mechanism for fair

This commit adds a pushable tasks mechanism to the fair scheduler so that
runnable tasks placed back on the runqueue in put_prev_task_fair() can be
evaluated and migrated via a balance callback.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8f99c8bf1a80d..e37f8b55a2934 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
[ ... ]
> @@ -10071,6 +10074,163 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>  	return target;
>  }
>  
> +DEFINE_STATIC_KEY_FALSE(sched_push_task);
> +
> +static inline bool sched_push_task_enabled(void)
> +{
> +	return static_branch_unlikely(&sched_push_task);
> +}
> +
> +static bool __check_pushable_fair_task(struct rq *rq, struct task_struct *p)
> +{
> +	if (!task_on_rq_queued(p))
> +		return false;
> +
> +	if (p->se.sched_delayed)
> +		return false;
> +
> +	if (p->nr_cpus_allowed <= 1)
> +		return false;
> +
> +	return true;
> +}

[Severity: High]
Can a task throttled by cfs bandwidth control be queued onto
rq->cfs.pushable_tasks and corrupt the plist or bypass throttling?

When throttle_cfs_rq_work() throttles a running task, it calls
dequeue_task_fair() directly rather than deactivate_task(), clearing
p->se.on_rq and setting p->throttled = true while leaving
task_on_rq_queued(p) true.  On the subsequent schedule,
put_prev_task_fair() skips __enqueue_entity() because se->on_rq is zero,
but still calls fair_add_pushable_prev().  Because
__check_pushable_fair_task() does not check p->se.on_rq or
!task_is_throttled(p), the throttled task is added to
rq->cfs.pushable_tasks.

If fair_push_task() then migrates that limbo task to another CPU whose
cfs_rq is unthrottled, enqueue_throttled_task() clears p->throttled and
lets the task run past its bandwidth quota.  Furthermore, if
dequeue_task_fair() is later called on the throttled task (for example
during sched_move_task() or an affinity change), it returns early before
reaching __dequeue_task():

kernel/sched/fair.c:dequeue_task_fair() {
	...
	if (task_is_throttled(p)) {
		dequeue_throttled_task(p, flags);
		return true;
	}

	if (!p->se.sched_delayed)
		util_est_dequeue(&rq->cfs, p);

	if (!__dequeue_task(rq, p, flags))
		return false;
	...
}

That early return skips fair_remove_pushable_task(), leaving a dangling
p->pushable_tasks node linked in rq->cfs.pushable_tasks.

[ ... ]

> +static struct task_struct *pick_next_pushable_fair_task(struct rq *rq)
> +{
> +	struct task_struct *p;
> +
> +	if (!has_pushable_tasks(rq))
> +		return NULL;
> +
> +	p = plist_first_entry(&rq->cfs.pushable_tasks,
> +			      struct task_struct, pushable_tasks);
> +
> +	WARN_ON_ONCE(rq->cpu != task_cpu(p));
> +	WARN_ON_ONCE(task_current(rq, p));
> +	WARN_ON_ONCE(p->nr_cpus_allowed <= 1);
> +	WARN_ON_ONCE(!task_on_rq_queued(p));
> +
> +	/*
> +	 * Remove task from the pushable list as we try only once after that
> +	 * the task has been put back in enqueued list.
> +	 */
> +	plist_del(&p->pushable_tasks, &rq->cfs.pushable_tasks);
> +
> +	return p;
> +}

[Severity: High]
With proxy execution enabled, can fair_push_task() migrate the actively
running mutex owner or a mutex-blocked donor task?

In __schedule(), pick_next_task() calls put_prev_set_next_task(rq,
rq->donor, next) before find_proxy_task() resolves a blocked donor to its
mutex owner.  When a running mutex owner prev is preempted by a blocked
fair donor next that is blocked on prev, put_prev_task_fair() adds prev
to rq->cfs.pushable_tasks, and set_next_task_fair() only removes next
from pushable_tasks before queueing fair_push_tasks().  Immediately
afterwards, find_proxy_task() selects prev as rq->curr without calling
set_next_task_fair(rq, prev, ...).  When the balance callback runs
fair_push_tasks(), pick_next_pushable_fair_task() does not check
!task_on_cpu(rq, p) (unlike pick_next_pushable_task() in rt.c and
pick_next_pushable_dl_task() in deadline.c), so it triggers the
task_current(rq, p) warning and returns the actively running task to
fair_push_task() to be migrated while still executing on prev_cpu.

In addition, __check_pushable_fair_task() does not check
!task_is_blocked(p), allowing mutex-blocked donor tasks (which remain
queued on the rq) to be added to rq->cfs.pushable_tasks and pushed away
from their mutex owner's CPU while set_task_cpu() overwrites p->wake_cpu.

> +
> +static int
> +select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags);
> +
> +/*
> + * See if the non running fair tasks on this rq can be sent on other CPUs
> + * that fits better with their profile.
> + */
> +static bool fair_push_task(struct rq *rq)
> +	__releases(__rq_lockp(rq))
> +	__acquires(__rq_lockp(rq))
> +{
[ ... ]
> +	new_cpu = select_task_rq_fair(next_task, prev_cpu, 0);

[Severity: Medium]
When select_task_rq_fair() -> find_energy_efficient_cpu() runs from
fair_push_task(), does cpu_util() miscompute util_est across the
performance domain?

In fair_push_task(), next_task is already queued on prev_cpu
(task_on_rq_queued(next_task) is true and !next_task->se.sched_delayed),
so cpu_rq(prev_cpu)->cfs.avg.util_est already includes
_task_util_est(next_task).  However, cpu_util() adjusts util_est without
using add_task or sub_task:

kernel/sched/fair.c:cpu_util() {
	...
		if (dst_cpu == cpu)
			util_est += _task_util_est(p);
		else if (p && unlikely(task_on_rq_queued(p) || current == p))
			lsub_positive(&util_est, _task_util_est(p));
	...
}

When find_energy_efficient_cpu() evaluates cpu == prev_cpu with
dst_cpu == prev_cpu, dst_cpu == cpu is true and adds _task_util_est(p) a
second time.  When it evaluates any other CPU in the domain with
dst_cpu != cpu (such as dst_cpu == -1 in eenv_pd_busy_time() or a
non-target CPU in compute_energy()), task_on_rq_queued(p) is true without
checking task_cpu(p) == cpu, subtracting _task_util_est(p) from every
other CPU's util_est and skewing capacity fitness and energy delta
calculations.

[ ... ]

> +static void fair_push_tasks(struct rq *rq)
> +{
> +	/* fair_push_task() will return true if it moved a fair task */
> +	while (fair_push_task(rq))
> +		;
> +}

[Severity: Low]
Does fair_push_tasks() need __must_hold(__rq_lockp(rq)) for clang context
analysis?

kernel/sched/Makefile enables context analysis on fair.o, and
fair_push_task() is annotated with __releases(__rq_lockp(rq)) and
__acquires(__rq_lockp(rq)).  Without __must_hold(__rq_lockp(rq)) on
fair_push_tasks() (matching balance_push() and sched_core_balance() in
core.c), building with clang context analysis enabled warns that
__rq_lockp(rq) is not held on entry to fair_push_task().  Inside
fair_push_task(), reassigning cur_rq = move_queued_task(...) without
context_unsafe_alias(cur_rq) and omitting __must_hold(__rq_lockp(rq)) on
the declaration of move_queued_task() in kernel/sched/sched.h also cause
context analysis warnings in this commit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106151947.3991524-2-vincent.guittot@linaro.org?part=8