lkml.org 
[lkml]   [2023]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] sched/fair: Introduce SIS_PAIR to wakeup task on local idle core first
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

\
 
 \ /
  Last update: 2023-05-18 06:18    [W:0.165 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site