Re: [PATCH 09/20 v3] sched/fair: Optimize push task mechanism for fair

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] sched/fair: unpinned rq lock and double clock update in
  fair_push_task()
--

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

sched/fair: Optimize push task mechanism for fair

This commit replaces the runqueue unlock and task_rq_lock() sequence in
fair_push_task() with a raw_spin_trylock() on next_task->pi_lock and
open-coded task migration to new_rq.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e37f8b55a2934..e2999d3f88e46 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10148,8 +10148,7 @@ static bool fair_push_task(struct rq *rq)
>  {
>  	struct task_struct *next_task;
>  	int prev_cpu, new_cpu;
> -	struct rq_flags rf;
> -	struct rq *cur_rq;
> +	struct rq *new_rq;
>  
>  	next_task = pick_next_pushable_fair_task(rq);
>  	if (!next_task)
> @@ -10158,38 +10157,41 @@ static bool fair_push_task(struct rq *rq)
>  	if (is_migration_disabled(next_task))
>  		return true;
>  
> -	/* We might release rq lock */
> -	get_task_struct(next_task);
> -
>  	prev_cpu = rq->cpu;
>  
>  	/*
> -	 * We need to release rq lock and take both task and rq w/o
> -	 * triggering a deadlock.
> +	 * The safe lock ordering for task and rq is task 1st then rq but we
> +	 * already get the rq so just try to get task too. If task is already
> +	 * locked, it is waiting for the rq's lock and it is about to change
> +	 * task state so skipping the push sequence in order to speed up the
> +	 * release of the lock is the best choice.
>  	 */
> -	raw_spin_rq_unlock(rq);
> -
> -	cur_rq = task_rq_lock(next_task, &rf);
> -
> -	/* Task already migrated */
> -	if (cur_rq->cpu != prev_cpu)
> -		goto skip_push;
> +	if (!raw_spin_trylock(&next_task->pi_lock))
> +		return true;
>  
>  	new_cpu = select_task_rq_fair(next_task, prev_cpu, 0);
>  
>  	/* Task doesn't need to migrate */
>  	if (new_cpu == prev_cpu)
> -		goto skip_push;
> +		goto no_push;
> +
> +	new_rq = cpu_rq(new_cpu);
>  
> -	update_rq_clock(cur_rq);
> -	cur_rq = move_queued_task(cur_rq, &rf, next_task, new_cpu);
> +	deactivate_task(rq, next_task, 0);
> +	set_task_cpu(next_task, new_cpu);
> +	raw_spin_rq_unlock(rq);
>  
> -skip_push:
> -	task_rq_unlock(cur_rq, next_task, &rf);
> +	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 state */
> +	/* Restore rq lock state */
>  	raw_spin_rq_lock(rq);

[Severity: Medium]
Does replacing move_queued_task() with open-coded locking and
deactivate_task(rq, next_task, 0) trigger double-clock-update warnings and
skip runqueue lock pinning?

fair_push_task() runs from __balance_callbacks() after the caller has
updated rq's clock (setting RQCF_UPDATED in rq->clock_update_flags) and
called rq_unpin_lock(), which leaves RQCF_UPDATED set.  Calling
deactivate_task(rq, next_task, 0) without DEQUEUE_NOCLOCK calls
dequeue_task() -> update_rq_clock(rq) while RQCF_UPDATED is still set,
triggering WARN_ON_ONCE(rq->clock_update_flags & RQCF_UPDATED) under
WARN_DOUBLE_CLOCK and setting SCX_RQ_CLK_VALID after rq_unpin_lock() has
already invalidated it.

On new_rq, using raw_spin_rq_lock() and raw_spin_rq_unlock() instead of
rq_lock()/rq_unlock() (or attach_one_task()) skips rq_pin_lock() and
rq_unpin_lock().  Because the previous lock holder on new_rq also left
RQCF_UPDATED set upon rq_unpin_lock(), activate_task() -> enqueue_task() ->
update_rq_clock(new_rq) triggers the same RQCF_UPDATED warning on new_rq,
bypasses lockdep pinning and assert_balance_callbacks_empty(), and leaves
SCX_RQ_CLK_VALID set after unlocking new_rq.

Passing DEQUEUE_NOCLOCK to deactivate_task() (since rq's clock was updated
before __balance_callbacks()) and using attach_one_task(new_rq, next_task)
would avoid both issues.

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