Re: [PATCH 11/20 v3] sched/fair: Add force 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: invalid context-analysis lock annotations in
  tick_push_task()
--

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

sched/fair: Add force push task mechanism for fair

This commit adds tick_push_task() to task_tick_fair() so a single runnable
task on a CPU can be pushed to a better CPU via active load balance on
the scheduler tick.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3bcb663ebcde2..f2a0d2c2bb632 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10233,6 +10233,75 @@ static void fair_add_pushable_prev(struct rq *rq, struct task_struct *prev,
>  		__fair_add_pushable_task(rq, prev);
>  }
>  
> +static int active_load_balance_cpu_stop(void *data);
> +
> +/*
> + * See if the alone task running on the CPU should migrate on a better than
> + * the local one.
> + */
> +static inline bool tick_push_task(struct task_struct *p, struct rq *rq, struct rq_flags *rf)
> +	__releases(__rq_lockp(rq))
> +	__acquires(__rq_lockp(rq))
> +{
> +	int new_cpu, cpu = cpu_of(rq);
> +
> +	if (!sched_push_task_enabled())
> +		return false;

[ ... ]

> +	preempt_disable();
> +	rq_unpin_lock(rq, rf);
> +	raw_spin_rq_unlock(rq);
> +	stop_one_cpu_nowait(cpu,
> +			    active_load_balance_cpu_stop, rq,
> +			    &rq->active_balance_work);
> +	preempt_enable();
> +	raw_spin_rq_lock(rq);
> +	rq_repin_lock(rq, rf);
> +
> +	return true;
> +}
> +
>  /*
>   * select_task_rq_fair: Select a target runqueue for the task.
>   * There are 2 ways to select the target runqueue:
> @@ -15713,8 +15782,10 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, struct rq_fl
>  
>  	task_tick_cache(rq, curr);
>  
> -	update_misfit_status(curr, rq);
> -	check_update_overutilized_status(task_rq(curr));
> +	if (!tick_push_task(curr, rq, rf)) {
> +		update_misfit_status(curr, rq);
> +		check_update_overutilized_status(task_rq(curr));
> +	}
>  
>  	task_tick_core(rq, curr);
>  }

[Severity: Medium]
Should tick_push_task() be annotated with __must_hold(__rq_lockp(rq))
instead of pairing __releases(__rq_lockp(rq)) with
__acquires(__rq_lockp(rq))?

Under CONFIG_WARN_CONTEXT_ANALYSIS=y (enabled for fair.o via
CONTEXT_ANALYSIS_fair.o := y), __releases() requires the lock to be held
on entry and unlocked on exit, while __acquires() requires the lock to be
unlocked on entry and held on exit.  Applying both to __rq_lockp(rq) on a
function that enters and exits with the rq lock held (including on the
early return paths that never drop the lock) triggers Clang
-Wthread-safety warnings.

Like sched_balance_newidle(), tick_push_task() and its caller
task_tick_fair() need __must_hold(__rq_lockp(rq)) (or
lockdep_assert_rq_held(rq) in task_tick_fair()) to satisfy context
analysis.

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