Re: [PATCH v3 2/2] sched/fair: Choose the CPU where short task is running during wake up

[Chen Yu]

Hi Yicong,
On 2022-12-06 at 21:02:11 +0800, Yicong Yang wrote:
[...]
> > +++ b/kernel/sched/fair.c
> > @@ -6246,6 +6246,11 @@ wake_affine_idle(int this_cpu, int prev_cpu, int sync)
> >  	if (available_idle_cpu(prev_cpu))
> >  		return prev_cpu;
> >  
> > +	/* The only running task is a short duration one. */
> > +	if (cpu_rq(this_cpu)->nr_running == 1 &&
> > +	    is_short_task((struct task_struct *)cpu_curr(this_cpu)))
> > +		return this_cpu;
> > +
> 
> Is it necessary to check the ttwu pending state here and below?
>
My understanding is that, ttwu_pending will be set on this_cpu if
1) this_cpu is idle, or
2) waker on another LLC domain wants to wake up the wakee on this_cpu,
see ttwu_queue_cond().
For 1), the nr_running is 1, so it is not idle. For 2) the
chance to do a cross LLC wake up is relatively low with current patch
applied. Besides, I was trying to make this proposal a dynamic version
of WF_SYNC, since the latter does not check ttwu_pending, I did
not add this check as well.(for now)+
> >  	return nr_cpumask_bits;
> >  }
> >  
> > @@ -6612,6 +6617,11 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
> >  		time = cpu_clock(this);
> >  	}
> >  
> > +	if (!has_idle_core && cpu_rq(target)->nr_running == 1 &&
> > +	    is_short_task((struct task_struct *)cpu_curr(target)) &&
> > +	    is_short_task(p))
> > +		return target;
> > +
> 
> A short running task doesn't means a low utilization (you also mentioned in Patch 1/2).
> So should we concern that we may overload the target?
> 
The overloaded target might be expected in this case IMO. Because for a ping-pong scheduling
pair, we want to saturate the target CPU to eliminate the idle time. And this strategy
only takes effect when !has_idle_core.
> btw, we're doing no scanning here so I may think it'll be more consistent to put this part
> in select_idle_siblings(), considering we've already have some similiar judgement for the
> prev_cpu, recent_used_cpu, etc. there.
> 
Got it, I can change it in next version.
> Still doing some test, will reply the results once I get them.
Thanks for the test, we can tune this patch when we have the data.

thanks,
Chenyu
> 
> Thanks,
> Yicong
> 
> >  	if (sched_feat(SIS_UTIL)) {
> >  		sd_share = rcu_dereference(per_cpu(sd_llc_shared, target));
> >  		if (sd_share) {
> >