On Fri, 2022-02-04 at 23:49 +1300, Barry Song wrote:
> On Fri, Feb 4, 2022 at 11:28 PM Barry Song <[email protected]> wrote:
> > On Fri, Feb 4, 2022 at 8:33 PM Srikar Dronamraju
> > <[email protected]> wrote:
> > > * Barry Song <[email protected]> [2022-02-02 09:20:32]:
> > >
> > > > On Tue, Feb 1, 2022 at 10:39 PM Srikar Dronamraju
> > > > <[email protected]> wrote:
> > > > > * Barry Song <[email protected]> [2022-01-28 07:40:15]:
> > > > >
> > > > > > On Fri, Jan 28, 2022 at 8:13 PM Srikar Dronamraju
> > > > > > <[email protected]> wrote:
> > > > > > > * Barry Song <[email protected]> [2022-01-28 09:21:08]:
> > > > > > >
> > > > > > > > On Fri, Jan 28, 2022 at 4:41 AM Gautham R. Shenoy
> > > > > > > > <[email protected]> wrote:
> > > > > > > > > On Wed, Jan 26, 2022 at 04:09:47PM +0800, Yicong Yang
> > > > > > > > > wrote:
> > > > > > > > > > From: Barry Song <[email protected]>
> > > > > > > > > >
> > > > > > 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.
Tim
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 <[email protected]> wrote:
> > > On Fri, Feb 4, 2022 at 8:33 PM Srikar Dronamraju
> > > <[email protected]> wrote:
> > > > * Barry Song <[email protected]> [2022-02-02 09:20:32]:
> > > >
> > > > > On Tue, Feb 1, 2022 at 10:39 PM Srikar Dronamraju
> > > > > <[email protected]> wrote:
> > > > > > * Barry Song <[email protected]> [2022-01-28 07:40:15]:
> > > > > >
> > > > > > > On Fri, Jan 28, 2022 at 8:13 PM Srikar Dronamraju
> > > > > > > <[email protected]> wrote:
> > > > > > > > * Barry Song <[email protected]> [2022-01-28 09:21:08]:
> > > > > > > >
> > > > > > > > > On Fri, Jan 28, 2022 at 4:41 AM Gautham R. Shenoy
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > On Wed, Jan 26, 2022 at 04:09:47PM +0800, Yicong Yang
> > > > > > > > > > wrote:
> > > > > > > > > > > From: Barry Song <[email protected]>
> > > > > > > > > > >
> > > > > > > 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.
thanks,
Chenyu
> Tim
>
On Sun, Feb 6, 2022 at 6:16 AM Chen Yu <[email protected]> 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 <[email protected]> wrote:
> > > > On Fri, Feb 4, 2022 at 8:33 PM Srikar Dronamraju
> > > > <[email protected]> wrote:
> > > > > * Barry Song <[email protected]> [2022-02-02 09:20:32]:
> > > > >
> > > > > > On Tue, Feb 1, 2022 at 10:39 PM Srikar Dronamraju
> > > > > > <[email protected]> wrote:
> > > > > > > * Barry Song <[email protected]> [2022-01-28 07:40:15]:
> > > > > > >
> > > > > > > > On Fri, Jan 28, 2022 at 8:13 PM Srikar Dronamraju
> > > > > > > > <[email protected]> wrote:
> > > > > > > > > * Barry Song <[email protected]> [2022-01-28 09:21:08]:
> > > > > > > > >
> > > > > > > > > > On Fri, Jan 28, 2022 at 4:41 AM Gautham R. Shenoy
> > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > On Wed, Jan 26, 2022 at 04:09:47PM +0800, Yicong Yang
> > > > > > > > > > > wrote:
> > > > > > > > > > > > From: Barry Song <[email protected]>
> > > > > > > > > > > >
> > > > > > > > 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