Re: [PATCH] sched/fair: Prefer small idle cores for forkees

[Chitti Babu Theegala]

On 1/13/2022 10:05 PM, Vincent Donnefort wrote:
> On Wed, Jan 12, 2022 at 08:09:02PM +0530, Chitti Babu Theegala wrote:
> > Newly forked threads don't have any useful utilization data yet and
> > it's not possible to forecast their impact on energy consumption.
> > update_pick_idlest These forkees (though very small, most times) end up waking big
> > cores from deep sleep for that very small durations.
> >
> > Bias all forkees to small cores to prevent waking big cores from deep
> > sleep to save power.
>
> This bias might be interesting for some workloads, but what about the
> others? (see find_energy_efficient_cpu() comment, which discusses forkees).

Yes, I agree with the find_energy_efficient_cpu() comment that we don't 
have any useful utilization data yet and hence not possible to forecast. 
However, I don't see any point in penalizing the power by waking up 
bigger cores which are in deep sleep state for very small workloads.

This patch helps lighter workloads during idle conditions w.r.t power 
POV. For active (interactive or heavier) workloads, on most big.Little 
systems' these foreground tasks get pulled into gold affined cpu-sets 
where this patch would not play any spoilsport. Even for systems with 
such cpu-sets not defined, heavy workloads might need just another 1 or 
2 scheduling windows for ramping to better freq or core.

> > Signed-off-by: Chitti Babu Theegala <quic_ctheegal@xxxxxxxxxxx>
> > ---
> >   kernel/sched/fair.c | 16 +++++++++++-----
> >   1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 6e476f6..d407bbc 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -5976,7 +5976,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p,
> >   }
> >   
> >   static struct sched_group *
> > -find_idlest_group(struct sched_domain *sd, struct task_struct *p, int this_cpu);
> > +find_idlest_group(struct sched_domain *sd, struct task_struct *p, int this_cpu, int sd_flag);
> >   
> >   /*
> >    * find_idlest_group_cpu - find the idlest CPU among the CPUs in the group.
> > @@ -6063,7 +6063,7 @@ static inline int find_idlest_cpu(struct sched_domain *sd, struct task_struct *p
> >   			continue;
> >   		}
> >   
> > -		group = find_idlest_group(sd, p, cpu);
> > +		group = find_idlest_group(sd, p, cpu, sd_flag);
> >   		if (!group) {
> >   			sd = sd->child;
> >   			continue;
> > @@ -8997,7 +8997,8 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
> >   static bool update_pick_idlest(struct sched_group *idlest,
> >   			       struct sg_lb_stats *idlest_sgs,
> >   			       struct sched_group *group,
> > -			       struct sg_lb_stats *sgs)
> > +			       struct sg_lb_stats *sgs,
> > +			       int sd_flag)
> >   {
> >   	if (sgs->group_type < idlest_sgs->group_type)
> >   		return true;
> > @@ -9034,6 +9035,11 @@ static bool update_pick_idlest(struct sched_group *idlest,
> >   		if (idlest_sgs->idle_cpus > sgs->idle_cpus)
> >   			return false;
> >   
> > +		/* Select smaller cpu group for newly woken up forkees */
> > +		if ((sd_flag & SD_BALANCE_FORK) && (idlest_sgs->idle_cpus &&
> > +			!capacity_greater(idlest->sgc->max_capacity, group->sgc->max_capacity)))
> > +			return false;
> > +
>
> Energy biased placement should probably be applied only when EAS is enabled.
>
> It's especially true here, if all CPUs have the same capacity, capacity_greater
> would be always false. So unless I missed something, we wouldn't let the group_util
> evaluation happen, would we?

True. I am uploading new version patch with a EAS enablement check in place.

> [...]