Re: [RFC PATCH] sched/fair: Introduce SIS_PAIR to wakeup task on local idle core first

[Chen Yu <yu.c.chen@intel.com>]

Hi Prateek,
On 2023-05-18 at 09:00:38 +0530, K Prateek Nayak wrote:
> Hello Chenyu,
> 
> I'll do some light testing with some benchmarks and share results on the
> thread but meanwhile I have a few observations with the implementation.
>
Thanks for reviewing this change.
> On 5/17/2023 10:27 PM, Chen Yu wrote:
> > On 2023-05-16 at 13:51:26 +0200, Mike Galbraith wrote:
> >> On Tue, 2023-05-16 at 16:41 +0800, Chen Yu wrote:
> >> [..snip..]
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 7d2613ab392c..572d663065e3 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7126,6 +7126,23 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> >  	    asym_fits_cpu(task_util, util_min, util_max, target))
> >  		return target;
> >  
> > +	/*
> > +	 * If the waker and the wakee are good friends to each other,
> > +	 * putting them within the same SMT domain could reduce C2C
> > +	 * overhead. But this only applies when there is no idle core
> > +	 * available. SMT idle sibling should be prefered to wakee's
> > +	 * previous CPU, because the latter could still have the risk of C2C
> > +	 * overhead.
> > +	 *
> > +	 */
> > +	if (sched_feat(SIS_PAIR) && sched_smt_active() && !has_idle_core &&
> 
> "has_idle_core" is not populated at this point and will always be false
> from the initialization. Should there be a:
> 
> 	has_idle_core = test_idle_cores(? /* Which CPU? */);
Yes you are right, I have 2 patches, the first one is to check has_idle_core
in the beginning but I forgot to send it out but only the second one.
> 	if (sched_feat(SIS_PAIR) ...) {
> 		...
> 	}
> 	has_idle_core = false;
> 
> ?: "has_idle_core" is currently used in select_idle_sibling() from the
> perspective of the target MC. Does switching target to current core
> (which may not be on the same MC) if target MC does not have an idle core
> make sense in all scenarios?
Right, we should check whether target equals to current CPU. Since I tested
with 1 socket online, this issue did not expose
> 
> > +	    current->last_wakee == p && p->last_wakee == current) {
> > +		i = select_idle_smt(p, smp_processor_id());
> 
> Also wondering if asym_fits_cpu() check is needed in some way here.
> Consider a case where waker is on a weaker capacity CPU but wakee
> previously ran on a stronger capacity CPU. It might be worthwhile
> to wake the wakee on previous CPU if the current CPU does not fit
> the task's utilization and move the pair to the CPU with larger
> capacity during the next wakeup. wake_affine_weight() would select
> a target based on load and capacity consideration but here we
> switch the wakeup target to a thread on the current core.
> 
> Wondering if the capacity details already considered in the path?
> 
Good point, I guess what you mean is that, target could be other CPU rather than
the current one, there should be a check if the target equals to current CPU.
Let me refine the patch and have a test.

thanks,
Chenyu
> > +
> > +		if ((unsigned int)i < nr_cpumask_bits)
> > +			return i;
> > +	}
> > +
> >  	/*
> >  	 * If the previous CPU is cache affine and idle, don't be stupid:
> >  	 */
> > diff --git a/kernel/sched/features.h b/kernel/sched/features.h
> > index ee7f23c76bd3..86b5c4f16199 100644
> > --- a/kernel/sched/features.h
> > +++ b/kernel/sched/features.h
> > @@ -62,6 +62,7 @@ SCHED_FEAT(TTWU_QUEUE, true)
> >   */
> >  SCHED_FEAT(SIS_PROP, false)
> >  SCHED_FEAT(SIS_UTIL, true)
> > +SCHED_FEAT(SIS_PAIR, true)
> >  
> >  /*
> >   * Issue a WARN when we do multiple update_rq_clock() calls
> 
> --
> Thanks and Regards,
> Prateek