Re: [RFC PATCH v2 1/3] sched/fair: introduce new scheduler group type group_parked

[Tobias Huschle]

On 18/02/2025 06:44, Shrikanth Hegde wrote:
[...]
> > @@ -1352,6 +1352,9 @@ bool sched_can_stop_tick(struct rq *rq)
> >       if (rq->cfs.h_nr_queued > 1)
> >           return false;
> > +    if (rq->cfs.nr_running > 0 && arch_cpu_parked(cpu_of(rq)))
> > +        return false;
> > +
>
> you mean rq->cfs.h_nr_queued or rq->nr_running ?

cfs.h_nr_queued is probably more sensible, will use that.

[...]
> > @@ -11259,6 +11293,8 @@ static inline void calculate_imbalance(struct 
> > lb_env *env, struct sd_lb_stats *s
> >    * avg_load : Only if imbalance is significant enough.
> >    * nr_idle :  dst_cpu is not busy and the number of idle CPUs is quite
> >    *            different in groups.
> > + * nr_task :  balancing can go either way depending on the number of 
> > running tasks
> > + *            per group
> >    */
>
> This comment on nr_task can be removed as it is not present in the list.

Consider it gone.

[...]
> > @@ -11766,7 +11822,7 @@ static int sched_balance_rq(int this_cpu, 
> > struct rq *this_rq,
> >       ld_moved = 0;
> >       /* Clear this flag as soon as we find a pullable task */
> >       env.flags |= LBF_ALL_PINNED;
> > -    if (busiest->nr_running > 1) {
> > +    if (busiest->nr_running > 1 || arch_cpu_parked(busiest->cpu)) {
>
> Since there is reliance on active balance if there is single task, it 
> think above isn't needed. Is there any usecase for it?

Seems to work without that check. I have no particular use case in mind.

> >           /*
> >            * Attempt to move tasks. If sched_balance_find_src_group 
> > has found
> >            * an imbalance but busiest->nr_running <= 1, the group is
> > @@ -12356,6 +12412,11 @@ static void nohz_balancer_kick(struct rq *rq)
> >       if (time_before(now, nohz.next_balance))
> >           goto out;
> > +    if (!idle_cpu(rq->cpu)) {
> > +        flags = NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
> > +        goto out;
> > +    }
> > +
>
> This could be agrressive. Note when the code comes here, it is not idle. 
> It would bail out early if it is idle.

It seems like we can do without this one as well.

> >       if (rq->nr_running >= 2) {
> >           flags = NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
> >           goto out;
> > @@ -12767,6 +12828,9 @@ static int sched_balance_newidle(struct rq 
> > *this_rq, struct rq_flags *rf)
> >       update_misfit_status(NULL, this_rq);
> > +    if (arch_cpu_parked(this_cpu))
> > +        return 0;
> > +
> >       /*
> >        * There is a task waiting to run. No need to search for one.
> >        * Return 0; the task will be enqueued when switching to idle.
> > diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> > index 456d339be98f..7efd76a30be7 100644
> > --- a/kernel/sched/syscalls.c
> > +++ b/kernel/sched/syscalls.c
> > @@ -214,6 +214,9 @@ int idle_cpu(int cpu)
> >           return 0;
> >   #endif
> > +    if (arch_cpu_parked(cpu))
> > +        return 0;
> > +
> >       return 1;
> >   }