Re: [PATCH 07/18] sched/fair: Add push task mechanism for fair

[Shrikanth Hegde <sshegde@linux.ibm.com>]

Hi Vincent,

Disclaimer:
I haven't read the full series yet. I will try to do. Sorry in case i read
it wrong.
Thought of quickly checking the push path since i had recently worked on it.

On 10/2/26 9:14 PM, Vincent Guittot wrote:
> EAS is based on wakeup events to efficiently place tasks on the system, but
> there are cases where a task doesn't have wakeup events anymore or at a far
> too low pace. For such situation, we can take advantage of the task being
> put back in the enqueued list to check if it should be pushed on another
> CPU.
> 
> Add a push task mechanism that enables fair scheduler to push runnable
> tasks. EAS will be one user but other feature like filling idle CPUs or
> short slice tasks can also take advantage of it.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>   kernel/sched/core.c  |   4 +-
>   kernel/sched/fair.c  | 163 +++++++++++++++++++++++++++++++++++++++++++
>   kernel/sched/sched.h |   7 ++
>   3 files changed, 172 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 837dc74c9a8d..18692b752814 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2553,8 +2553,8 @@ static inline bool is_cpu_allowed(struct task_struct *p, int cpu)
>    *
>    * Returns (locked) new rq. Old rq's lock is released.
>    */
> -static struct rq *move_queued_task(struct rq *rq, struct rq_flags *rf,
> -				   struct task_struct *p, int new_cpu)
> +struct rq *move_queued_task(struct rq *rq, struct rq_flags *rf,
> +			    struct task_struct *p, int new_cpu)
>   	__must_hold(__rq_lockp(rq))
>   {
>   	lockdep_assert_rq_held(rq);
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index f12678850ce2..00078ac7fada 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8179,6 +8179,8 @@ static void dequeue_hierarchy(struct task_struct *p, int flags)
>   	}
>   }
>   
> +static void fair_remove_pushable_task(struct rq *rq, struct task_struct *p);
> +
>   /*
>    * The part of dequeue_task_fair() that is needed to dequeue delayed tasks.
>    *
> @@ -8195,6 +8197,7 @@ static bool __dequeue_task(struct rq *rq, struct task_struct *p, int flags)
>   	bool task_delayed = flags & DEQUEUE_DELAYED;
>   
>   	clear_buddies(cfs_rq, se);
> +	fair_remove_pushable_task(rq, p);
>   
>   	update_curr_eevdf(cfs_rq);
>   	update_entity_lag(cfs_rq, se);
> @@ -9799,6 +9802,157 @@ 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;
> +}
> +
> +static bool fair_check_pushable_task(struct rq *rq, struct task_struct *p, struct task_struct *next)
> +{
> +	if (!__check_pushable_fair_task(rq, p))
> +		return false;
> +
> +	return false;
> +}
> +
> +static inline int has_pushable_tasks(struct rq *rq)
> +{
> +	return !plist_head_empty(&rq->cfs.pushable_tasks);
> +}
> +

Should this use static key?

> +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;
> +}
> +
> +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)
> +{
> +	struct task_struct *next_task;
> +	int prev_cpu, new_cpu;
> +	struct rq_flags rf;
> +	struct rq *cur_rq;

You likely need below to keep clang's context tracking to be happy.

	context_unsafe_alias(rq);

> +
> +	next_task = pick_next_pushable_fair_task(rq);
> +	if (!next_task)
> +		return false;
> +
> +	if (is_migration_disabled(next_task))
> +		return true;

If migration is disabled, then it shouldn't endup on pushable_tasks?

> +
> +	/* 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.
> +	 */
> +	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;
> +
> +	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;
> +
> +	update_rq_clock(cur_rq);
> +	cur_rq = move_queued_task(cur_rq, &rf, next_task, new_cpu);
> +
> +skip_push:
> +	task_rq_unlock(cur_rq, next_task, &rf);
> +
> +	/* Restore rq state */
> +	raw_spin_rq_lock(rq);
> +	put_task_struct(next_task);
> +
> +	return true;
> +}
> +
> +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))
> +		;
> +}

Would it better to iterate through the list? This is bit difficult
to read at first.

In failure cases also, it can return true. No? for example,
is_migration_disabled(), It is needed since you need to iterate the
rq to see if there is a task to move.

> +
> +static DEFINE_PER_CPU(struct balance_callback, fair_push_head);
> +
> +static inline void fair_queue_push_tasks(struct rq *rq)
> +{
> +	if (!sched_push_task_enabled() || !has_pushable_tasks(rq))
> +		return;
> +
> +	queue_balance_callback(rq, &per_cpu(fair_push_head, rq->cpu), fair_push_tasks);
> +}
> +
> +static void fair_remove_pushable_task(struct rq *rq, struct task_struct *p)
> +{
> +	if (sched_push_task_enabled())
> +		plist_del(&p->pushable_tasks, &rq->cfs.pushable_tasks);
> +}
> +
> +static void __fair_add_pushable_task(struct rq *rq, struct task_struct *p)
> +{
> +	plist_del(&p->pushable_tasks, &rq->cfs.pushable_tasks);
> +	plist_node_init(&p->pushable_tasks, p->prio);
> +	plist_add(&p->pushable_tasks, &rq->cfs.pushable_tasks);
> +}
> +
> +static void fair_add_pushable_prev(struct rq *rq, struct task_struct *prev, struct task_struct *next)
> +{
> +	if (sched_push_task_enabled() && fair_check_pushable_task(rq, prev, next))
> +		__fair_add_pushable_task(rq, prev);
> +}
> +
>   /*
>    * select_task_rq_fair: Select a target runqueue for the task.
>    * There are 2 ways to select the target runqueue:
> @@ -10272,6 +10426,12 @@ static void put_prev_task_fair(struct rq *rq, struct task_struct *prev, struct t
>   	cfs_rq->curr = NULL;
>   	if (se->on_rq)
>   		__enqueue_entity(cfs_rq, se);
> +
> +	/*
> +	 * The previous task might be eligible for being pushed on another cpu
> +	 * if it is still active.
> +	 */
> +	fair_add_pushable_prev(rq, prev, next);
>   }
>   
>   /*
> @@ -15379,6 +15539,7 @@ static void set_next_task_fair(struct rq *rq, struct task_struct *p, enum snt_e
>   		goto repick;
>   
>   	clear_buddies(cfs_rq, se);
> +	fair_remove_pushable_task(rq, p);
>   
>   	if (on_rq)
>   		__dequeue_entity(cfs_rq, se);
> @@ -15423,6 +15584,7 @@ static void set_next_task_fair(struct rq *rq, struct task_struct *p, enum snt_e
>   
>   	update_misfit_status(p, rq);
>   	sched_fair_update_stop_tick(rq, p);
> +	fair_queue_push_tasks(rq);
>   
>   repick:
>   	/*
> @@ -15439,6 +15601,7 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
>   {
>   	cfs_rq->tasks_timeline = RB_ROOT_CACHED;
>   	cfs_rq->zero_vruntime = (u64)(-(1LL << 20));
> +	plist_head_init(&cfs_rq->pushable_tasks);
>   	raw_spin_lock_init(&cfs_rq->removed.lock);
>   }
>   
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index e025d2a6c302..74130bd2a2c8 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -721,6 +721,8 @@ struct cfs_rq {
>   		unsigned long	runnable_avg;
>   	} removed;
>   
> +	struct plist_head	pushable_tasks;
> +
>   #ifdef CONFIG_FAIR_GROUP_SCHED
>   	u64			last_update_tg_load_avg;
>   	unsigned long		tg_load_avg_contrib;
> @@ -3877,6 +3879,8 @@ static inline bool sched_energy_enabled(void) { return false; }
>   
>   #endif /* !(CONFIG_ENERGY_MODEL && CONFIG_CPU_FREQ_GOV_SCHEDUTIL) */
>   
> +DECLARE_STATIC_KEY_FALSE(sched_push_task);
> +
>   #ifdef CONFIG_MEMBARRIER
>   
>   /*
> @@ -4209,6 +4213,9 @@ void move_queued_task_locked(struct rq *src_rq, struct rq *dst_rq, struct task_s
>   	wakeup_preempt(dst_rq, task, 0);
>   }
>   
> +extern struct rq *move_queued_task(struct rq *rq, struct rq_flags *rf,
> +				   struct task_struct *p, int new_cpu);
> +
>   static inline
>   bool task_is_pushable(struct rq *rq, struct task_struct *p, int cpu)
>   {