Message-ID: <ZCVm+93vj4oiQDO3@BLR-5CG11610CF.amd.com>
Date:   Thu, 30 Mar 2023 16:09:55 +0530
From:   "Gautham R. Shenoy" <gautham.shenoy@....com>
To:     Libo Chen <libo.chen@...cle.com>
Cc:     mingo@...hat.com, peterz@...radead.org, vincent.guittot@...aro.org,
        juri.lelli@...hat.com, dietmar.eggemann@....com, mgorman@...e.de,
        linux-kernel@...r.kernel.org,
        Daniel Jordan <daniel.m.jordan@...cle.com>
Subject: Re: [PATCH 1/1] sched/fair: Fix inaccurate tally of ttwu_move_affine

Hello Libo,

On Thu, Aug 25, 2022 at 02:13:17AM -0700, Libo Chen wrote:
>

Sorry, looks like this message got burried under the pile.

> 
> On 8/25/22 00:30, Gautham R. Shenoy wrote:
> > On Wed, Aug 10, 2022 at 03:33:13PM -0700, Libo Chen wrote:
> > > There are scenarios where non-affine wakeups are incorrectly counted as
> > > affine wakeups by schedstats.
> > > 
> > > When wake_affine_idle() returns prev_cpu which doesn't equal to
> > > nr_cpumask_bits, it will slip through the check: target == nr_cpumask_bits
> > > in wake_affine() and be counted as if target == this_cpu in schedstats.
> > > 
> > > Replace target == nr_cpumask_bits with target != this_cpu to make sure
> > > affine wakeups are accurately tallied.
> > > 
> > > Fixes: 806486c377e33 (sched/fair: Do not migrate if the prev_cpu is idle)
> > > Suggested-by: Daniel Jordan <daniel.m.jordan@...cle.com>
> > > Signed-off-by: Libo Chen <libo.chen@...cle.com>
> > > ---
> > >   kernel/sched/fair.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index da388657d5ac..b179da4f8105 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -6114,7 +6114,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p,
> > >   		target = wake_affine_weight(sd, p, this_cpu, prev_cpu, sync);
> > >   	schedstat_inc(p->stats.nr_wakeups_affine_attempts);
> > > -	if (target == nr_cpumask_bits)
> > > +	if (target != this_cpu)
> > >   		return prev_cpu;
> > 
> > This seems to be the right thing to do. However,..
> > 
> > if this_cpu and prev_cpu were in the same LLC and we pick prev_cpu,
> > technically is it still not an affine wakeup?
> > 
> I think schedstats like ttwu_move_affine/ttwu_wake_remote is defined within
> a sched domain, so if the waking CPU and the previous CPU are in the same MC
> domain, then picking the previous CPU is a remote wakeup
> within that MC. If the two candidate CPUs are from two different NUMA nodes,
> then picking the waking CPU is an affine wakeup within that NUMA domain.
> Correct me if I am wrong, this definition is consistent across
> all levels of sched domains.

Yes, the definition of ttwu_wake_remote in the lowest sched-domain
containing both the prev_cpu and this_cpu, is target_cpu != this_cpu.
This is fairly unambiguous.


>From the code, the definition of an ttwu_move_affine is to capture an
_attempt_ to chose the this_cpu as the target_cpu in the lowest
sched-domain containing both prev CPU and this_cpu. It is merely an
attempt since the actual target_CPU is selected by SIS and could be
any idle CPU in the LLC of the prev/this_cpu. 

ttwu_move_affine makes sense for sched-domains higher than the LLC
domain since we move the task to the LLC of this_cpu away from the LLC
of the prev_cpu (This is possible on AMD processors which contains
multiple LLC domains within a NUMA node). Having given it some more
thought, I am not sure how to interpret this metric for the LLC domain
and lower ones, since the eventual target CPU may not even be "closer"
to this_cpu.

> 
> But I do understand that when two candidate CPUs are within an LLC,
>     a) all the fast-path wakeups should be affine wakeups if your definition
> of an affine wakeup is a wakeup to the same LLC of the waker.
>     b) select_idle_sibling() may pick a CPU in that LLC other than the two
> candidate CPUs which makes the affine/remote stats here useless even if we
> are consistent with ttwu_move_affine/ttwu_wake_remote
>        definition.

Fair enough.

> 
> I personally think it's just too much trouble to add additional code in the
> kernel to, let's say, treat all wakeups within an LLC as ttwu_move_affine.
> It's a lot easier to do that when you process schedstats data,
> whether you want to treat all wakeups in LLC domains as affine wakeups or
> ignore ttwu_move_affine/ttwu_wake_remote stats from LLC domains.


I agree.

I think that having your fix is the right thing, since currently the
move_affine data in schedstats isn't accurate when wake_affine_idle()
or wake_affine_weight() picks the prev_cpu, especially when prev_cpu
and this_cpu are in sched-domains higher than the LLC. Thus today we
overcount affine wakeups which is incorrect.

So,
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@....com>

--
Thanks and Regards
gautham.

Powered by blists - more mailing lists