Re: [PATCH 15/20 v3] sched/fair: Enable push task for preempt short

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] sched/fair: enabling sched_push_task pushes throttled or active
  proxy tasks
- [Medium] sched/topology: sched_push_task not enabled at boot in
  sched_init_domains()
- [Medium] sched/topology: sched_push_task never disabled when
  PREEMPT_SHORT is off
- [Medium] sched/fair: missing rq_lock() and clock flag handling in
  fair_push_task()
--

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

sched/fair: Enable push task for preempt short

This commit enables the sched_push_task static branch in
partition_sched_domains_locked() when the PREEMPT_SHORT scheduler feature
is set.

> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 919d0fb00bd9f..5481f2472e175 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -3497,6 +3497,9 @@ static void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new
>  	sched_energy_set(has_eas);
>  #endif
>  
> +	if (sched_feat(PREEMPT_SHORT) && !static_branch_likely(&sched_push_task))
> +		static_branch_enable_cpuslocked(&sched_push_task);

[Severity: High]
Can enabling sched_push_task cause throttled or proxy-executing tasks to be
queued onto rq->cfs.pushable_tasks and pushed to another CPU?

In put_prev_task_fair(), fair_add_pushable_prev() is called even when
se->on_rq is 0, and __check_pushable_fair_task() only checks
task_on_rq_queued(p) without checking p->se.on_rq, !task_is_throttled(p),
!task_is_blocked(p), or !task_on_cpu(rq, p):

kernel/sched/fair.c:put_prev_task_fair() {
	...
	if (se->on_rq)
		__enqueue_entity(cfs_rq, se);

	if (next) {
		if (next != prev)
			fair_add_pushable_prev(rq, prev, next);
	...
}

Under CONFIG_CFS_BANDWIDTH, throttle_cfs_rq_work() dequeues a task with
DEQUEUE_SLEEP | DEQUEUE_THROTTLE, clearing p->se.on_rq and setting
p->throttled while leaving p->on_rq == TASK_ON_RQ_QUEUED.  When that task
schedules out, put_prev_task_fair() can add it to rq->cfs.pushable_tasks.
If fair_push_task() then migrates it to an unthrottled cfs_rq,
activate_task() -> enqueue_task_fair() -> enqueue_throttled_task() clears
p->throttled and bypasses the CFS bandwidth quota; if the throttled task is
instead dequeued by sched_move_task() or do_set_cpus_allowed(),
dequeue_task_fair() returns early via dequeue_throttled_task() without
calling fair_remove_pushable_task(), leaving p->pushable_tasks corruptly
linked on rq->cfs.pushable_tasks.

Similarly, under CONFIG_SCHED_PROXY_EXEC, proxy_needs_return() calls
dequeue_block_task(rq, p, TASK_WAKING) before proxy_reset_donor() calls
put_prev_set_next_task(), which can link the waking task onto
rq->cfs.pushable_tasks before try_to_wake_up() migrates it to another CPU,
and switching rq->donor away from a mutex owner that remains rq->curr
allows fair_push_task() to deactivate and migrate a task that is actively
executing on the CPU.

[Severity: Medium]
Does fair_push_task() miss RQCF_UPDATED clearing and rq_lock()/rq_unlock()
pinning when migrating a task?

kernel/sched/fair.c:fair_push_task() {
	...
	deactivate_task(rq, next_task, 0);
	set_task_cpu(next_task, new_cpu);
	raw_spin_rq_unlock(rq);

	raw_spin_rq_lock(new_rq);
	WARN_ON_ONCE(task_cpu(next_task) != new_cpu);
	activate_task(new_rq, next_task, 0);
	wakeup_preempt(new_rq, next_task, 0);
	raw_spin_rq_unlock(new_rq);

	/* Restore rq lock state */
	raw_spin_rq_lock(rq);
}

When __balance_callbacks() runs fair_push_task() at the end of
__schedule(), rq_unpin_lock(rq, &rf) has already unpinned rq->__lock and
invalidated the sched_ext clock, but RQCF_UPDATED remains set on
rq->clock_update_flags.  Calling deactivate_task(rq, next_task, 0) without
DEQUEUE_NOCLOCK (or clearing RQCF_UPDATED) calls update_rq_clock(rq) again,
triggering WARN_DOUBLE_CLOCK and re-setting SCX_RQ_CLK_VALID on
rq->scx.flags after rq_unpin_lock().  In addition, locking new_rq with
raw_spin_rq_lock() and raw_spin_rq_unlock() instead of rq_lock() and
rq_unlock() skips rq_pin_lock() and rq_unpin_lock() on new_rq, triggering
WARN_DOUBLE_CLOCK in activate_task() and leaving SCX_RQ_CLK_VALID set on
new_rq after unlock.

[Severity: Medium]
Does sched_push_task remain disabled after boot on systems that only build
domains through sched_init_domains()?

During boot, smp_init() brings secondary CPUs online before
sched_smp_initialized is set, and sched_init_smp() then initializes domains
via sched_init_domains() -> build_sched_domains() without calling
partition_sched_domains_locked().  On systems that do not later rebuild
scheduler domains (for example via Energy Model registration, ITMT, CPU
hotplug, or cpuset updates), sched_push_task is never enabled even though
PREEMPT_SHORT is on by default.

[Severity: Medium]
What disables sched_push_task (or stops check_pushable_short_task()) when
PREEMPT_SHORT is disabled at runtime via /sys/kernel/debug/sched/features?

partition_sched_domains_locked() only enables sched_push_task and has no
corresponding disable path when !sched_feat(PREEMPT_SHORT), and
check_pushable_short_task() does not test sched_feat(PREEMPT_SHORT).  Also,
note that fair_remove_pushable_task() gates plist_del() on
sched_push_task_enabled(), so disabling the static branch while tasks are
queued on rq->cfs.pushable_tasks would skip unlinking them on dequeue.

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