Date:   Sun, 6 Feb 2022 13:26:15 +1300
From:   Barry Song <21cnbao@...il.com>
To:     Chen Yu <yu.c.chen@...el.com>
Cc:     Tim Chen <tim.c.chen@...ux.intel.com>,
        Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
        "Gautham R. Shenoy" <gautham.shenoy@....com>,
        Yicong Yang <yangyicong@...ilicon.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        LAK <linux-arm-kernel@...ts.infradead.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        prime.zeng@...wei.com,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        ego@...ux.vnet.ibm.com, Linuxarm <linuxarm@...wei.com>,
        Barry Song <song.bao.hua@...ilicon.com>,
        Guodong Xu <guodong.xu@...aro.org>
Subject: Re: [PATCH v2 2/2] sched/fair: Scan cluster before scanning LLC in
 wake-up path

On Sun, Feb 6, 2022 at 6:16 AM Chen Yu <yu.c.chen@...el.com> wrote:
>
> On Fri, Feb 04, 2022 at 09:41:21AM -0800, Tim Chen wrote:
> > On Fri, 2022-02-04 at 23:49 +1300, Barry Song wrote:
> > > On Fri, Feb 4, 2022 at 11:28 PM Barry Song <21cnbao@...il.com> wrote:
> > > > On Fri, Feb 4, 2022 at 8:33 PM Srikar Dronamraju
> > > > <srikar@...ux.vnet.ibm.com> wrote:
> > > > > * Barry Song <21cnbao@...il.com> [2022-02-02 09:20:32]:
> > > > >
> > > > > > On Tue, Feb 1, 2022 at 10:39 PM Srikar Dronamraju
> > > > > > <srikar@...ux.vnet.ibm.com> wrote:
> > > > > > > * Barry Song <21cnbao@...il.com> [2022-01-28 07:40:15]:
> > > > > > >
> > > > > > > > On Fri, Jan 28, 2022 at 8:13 PM Srikar Dronamraju
> > > > > > > > <srikar@...ux.vnet.ibm.com> wrote:
> > > > > > > > > * Barry Song <21cnbao@...il.com> [2022-01-28 09:21:08]:
> > > > > > > > >
> > > > > > > > > > On Fri, Jan 28, 2022 at 4:41 AM Gautham R. Shenoy
> > > > > > > > > > <gautham.shenoy@....com> wrote:
> > > > > > > > > > > On Wed, Jan 26, 2022 at 04:09:47PM +0800, Yicong Yang
> > > > > > > > > > > wrote:
> > > > > > > > > > > > From: Barry Song <song.bao.hua@...ilicon.com>
> > > > > > > > > > > >
> > > > > > > > I am sorry I didn't get your question. Currently the code
> > > > > > > > works as below:
> > > > > > > > if task A wakes up task B, and task A is in LLC0 and task B
> > > > > > > > is in LLC1.
> > > > > > > > we will scan the cluster of A before scanning the whole
> > > > > > > > LLC0, in this case,
> > > > > > > > cluster of A is the closest sibling, so it is the better
> > > > > > > > choice than other CPUs
> > > > > > > > which are in LLC0 but not in the cluster of A.
> > > > > > >
> > > > > > > Yes, this is right.
> > > > > > >
> > > > > > > > But we do scan all cpus of LLC0
> > > > > > > > afterwards if we fail to find an idle CPU in the cluster.
> > > > > > >
> > > > > > > However my reading of the patch, before we can scan other
> > > > > > > clusters within
> > > > > > > the LLC (aka LLC0), we have a check in scan cluster which
> > > > > > > says
> > > > > > >
> > > > > > >         /* Don't ping-pong tasks in and out cluster
> > > > > > > frequently */
> > > > > > >         if (cpus_share_resources(target, prev_cpu))
> > > > > > >            return target;
> > > > > > >
> > > > > > > My reading of this is, ignore other clusters (at this point,
> > > > > > > we know there
> > > > > > > are no idle CPUs in this cluster. We don't know if there are
> > > > > > > idle cpus in
> > > > > > > them or not) if the previous CPU and target CPU happen to be
> > > > > > > from the same
> > > > > > > cluster. This effectively means we are given preference to
> > > > > > > cache over idle
> > > > > > > CPU.
> > > > > >
> > > > > > Note we only ignore other cluster while prev_cpu and target are
> > > > > > in same
> > > > > > cluster. if the condition is false, we are not ignoring other
> > > > > > cpus. typically,
> > > > > > if waker is the target, and wakee is the prev_cpu, that means
> > > > > > if they are
> > > > > > already in one cluster, we don't stupidly spread them in
> > > > > > select_idle_cpu() path
> > > > > > as benchmark shows we are losing. so, yes, we are giving
> > > > > > preference to
> > > > > > cache over CPU.
> > > > >
> > > > > We already figured out that there are no idle CPUs in this
> > > > > cluster. So dont
> > > > > we gain performance by picking a idle CPU/core in the
> > > > > neighbouring cluster.
> > > > > If there are no idle CPU/core in the neighbouring cluster, then
> > > > > it does make
> > > > > sense to fallback on the current cluster.
> > > >
> > > >
> >
> > We may need to take into consideration the utilization and
> > load average for the source and target cluster to make
> > better decision of whether it is worth placing the
> > task in the next cluster.  If the load of the target
> > cluster is too high, it is not worth pushing the task there.
> >
> > Those stats can be gathered during load balancing without adding
> > overhead in the hot task wakeup path.
> >
> > Chen Yu played around with cutting off the idle CPU search
> > in a LLC based on such stats and he saw some good
> > improvements over the default.
> >
> Yes, we used the sum of percpu util_avg to estimate if the LLC domain
> is overloaded. If it is too busy, skip searching for an idle cpu/core in
> that LLC domain. The util_avg is a metric of accumulated historic
> activity, which might be more accurate than instantaneous metrics(such as
> rq->nr_running) on calculating the probability of find an idle cpu.
> So far this change has shown some benefits in several microbenchmarks and
> OLTP benchmark when the system is quite busy. That change has introduced a
> per-LLC-domain flag to indicate whether the LLC domain is oveloaded,
> it seems that this flag could also be extended for cluster domain.
> Maybe I could post the draft patch to see if it would be helpful for this
> cluster patch serie.

yes. please send. my feeling is that select_idle_cpu() can select an "idle"cpu
which is actually very busy, but can be in "idle" state for a very
short period. it
is not always correct to get this kind of "idle" cpu. It could be
better to be still.
I am not quite sure your patch is directly related with clusters, but we will
try to figure out some connection, maybe we can integrate your patch into
this series afterwards.

>
> thanks,
> Chenyu
> > Tim
> >

Thanks
Barry

Powered by blists - more mailing lists