[PATCH] arm64: dts: sdm845: Add CPU topology

[PATCH] arm64: dts: sdm845: Add CPU topology

[Matthias Kaehlcke]

The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
that describes this topology.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 38 ++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index c27cbd3bcb0a6..f6c0d87e663f3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -192,6 +192,44 @@
 				next-level-cache = <&L3_0>;
 			};
 		};
+
+		cpu-map {
+			cluster0 {
+				core0 {
+					cpu = <&CPU0>;
+				};
+
+				core1 {
+					cpu = <&CPU1>;
+				};
+
+				core2 {
+					cpu = <&CPU2>;
+				};
+
+				core3 {
+					cpu = <&CPU3>;
+				};
+			};
+
+			cluster1 {
+				core0 {
+					cpu = <&CPU4>;
+				};
+
+				core1 {
+					cpu = <&CPU5>;
+				};
+
+				core2 {
+					cpu = <&CPU6>;
+				};
+
+				core3 {
+					cpu = <&CPU7>;
+				};
+			};
+		};
 	};
 
 	pmu {
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Doug Anderson]

Hi,

On Mon, Jan 14, 2019 at 10:43 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> that describes this topology.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 38 ++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)

Looks right to me and matches bindings examples.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Amit Kucheria]

On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> that describes this topology.

This is partly true. There are two groups of gold and silver cores,
but AFAICT they are in a single cluster, not two separate ones. SDM845
is one of the early examples of ARM's Dynamiq architecture.

> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

I noticed that this patch sneaked through for this merge window but
perhaps we can whip up a quick fix for -rc2?

> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 38 ++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index c27cbd3bcb0a6..f6c0d87e663f3 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -192,6 +192,44 @@
>                                 next-level-cache = <&L3_0>;
>                         };
>                 };
> +
> +               cpu-map {
> +                       cluster0 {
> +                               core0 {
> +                                       cpu = <&CPU0>;
> +                               };
> +
> +                               core1 {
> +                                       cpu = <&CPU1>;
> +                               };
> +
> +                               core2 {
> +                                       cpu = <&CPU2>;
> +                               };
> +
> +                               core3 {
> +                                       cpu = <&CPU3>;
> +                               };
> +                       };
> +
> +                       cluster1 {

This shouldn't exist.

> +                               core0 {

Rename to core4, 5, etc...

> +                                       cpu = <&CPU4>;
> +                               };
> +
> +                               core1 {
> +                                       cpu = <&CPU5>;
> +                               };
> +
> +                               core2 {
> +                                       cpu = <&CPU6>;
> +                               };
> +
> +                               core3 {
> +                                       cpu = <&CPU7>;
> +                               };
> +                       };
> +               };
>         };
>
>         pmu {
> --
> 2.20.1.97.g81188d93c3-goog
>

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Amit Kucheria]

[-- Attachment #1: Type: text/plain, Size: 2932 bytes --]

On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>
> On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > that describes this topology.
>
> This is partly true. There are two groups of gold and silver cores,
> but AFAICT they are in a single cluster, not two separate ones. SDM845
> is one of the early examples of ARM's Dynamiq architecture.
>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>
> I noticed that this patch sneaked through for this merge window but
> perhaps we can whip up a quick fix for -rc2?
>

And please find attached a patch to fix this up. Andy, since this
hasn't landed yet (can we still squash this into the original patch?),
I couldn't add a Fixes tag.

Regards,
Amit

> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 38 ++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index c27cbd3bcb0a6..f6c0d87e663f3 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -192,6 +192,44 @@
> >                                 next-level-cache = <&L3_0>;
> >                         };
> >                 };
> > +
> > +               cpu-map {
> > +                       cluster0 {
> > +                               core0 {
> > +                                       cpu = <&CPU0>;
> > +                               };
> > +
> > +                               core1 {
> > +                                       cpu = <&CPU1>;
> > +                               };
> > +
> > +                               core2 {
> > +                                       cpu = <&CPU2>;
> > +                               };
> > +
> > +                               core3 {
> > +                                       cpu = <&CPU3>;
> > +                               };
> > +                       };
> > +
> > +                       cluster1 {
>
> This shouldn't exist.
>
> > +                               core0 {
>
> Rename to core4, 5, etc...
>
> > +                                       cpu = <&CPU4>;
> > +                               };
> > +
> > +                               core1 {
> > +                                       cpu = <&CPU5>;
> > +                               };
> > +
> > +                               core2 {
> > +                                       cpu = <&CPU6>;
> > +                               };
> > +
> > +                               core3 {
> > +                                       cpu = <&CPU7>;
> > +                               };
> > +                       };
> > +               };
> >         };
> >
> >         pmu {
> > --
> > 2.20.1.97.g81188d93c3-goog
> >

[-- Attachment #2: 0001-arm64-dts-sdm845-Fix-up-CPU-topology.patch --]
[-- Type: text/x-patch, Size: 1211 bytes --]

From 9e7d60bcabad7594a1da43982bbc9fda04669717 Mon Sep 17 00:00:00 2001
Message-Id: <9e7d60bcabad7594a1da43982bbc9fda04669717.1557748437.git.amit.kucheria@linaro.org>
From: Amit Kucheria <amit.kucheria@linaro.org>
Date: Mon, 13 May 2019 17:08:33 +0530
Subject: [PATCH] arm64: dts: sdm845: Fix up CPU topology

SDM845 implements ARM's Dynamiq architecture that allows the big and
LITTLE cores to exist in a single cluster sharing the L3 cache.

Fix the cpu-map to put all cpus into a single cluster.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index ef7ce63eef4e..a30fa54bfccd 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -246,22 +246,20 @@
 				core3 {
 					cpu = <&CPU3>;
 				};
-			};
 
-			cluster1 {
-				core0 {
+				core4 {
 					cpu = <&CPU4>;
 				};
 
-				core1 {
+				core5 {
 					cpu = <&CPU5>;
 				};
 
-				core2 {
+				core6 {
 					cpu = <&CPU6>;
 				};
 
-				core3 {
+				core7 {
 					cpu = <&CPU7>;
 				};
 			};
-- 
2.17.1

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Stephen Boyd]

Quoting Amit Kucheria (2019-05-13 04:54:12)
> On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> >
> > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > that describes this topology.
> >
> > This is partly true. There are two groups of gold and silver cores,
> > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > is one of the early examples of ARM's Dynamiq architecture.
> >
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >
> > I noticed that this patch sneaked through for this merge window but
> > perhaps we can whip up a quick fix for -rc2?
> >
> 
> And please find attached a patch to fix this up. Andy, since this
> hasn't landed yet (can we still squash this into the original patch?),
> I couldn't add a Fixes tag.
> 

I had the same concern. Thanks for catching this. I suspect this must
cause some problem for IPA given that it can't discern between the big
and little "power clusters"?

Either way,

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Amit Kucheria]

(cc'ing Andy's correct email address)

On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Amit Kucheria (2019-05-13 04:54:12)
> > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > >
> > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >
> > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > that describes this topology.
> > >
> > > This is partly true. There are two groups of gold and silver cores,
> > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > is one of the early examples of ARM's Dynamiq architecture.
> > >
> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > >
> > > I noticed that this patch sneaked through for this merge window but
> > > perhaps we can whip up a quick fix for -rc2?
> > >
> >
> > And please find attached a patch to fix this up. Andy, since this
> > hasn't landed yet (can we still squash this into the original patch?),
> > I couldn't add a Fixes tag.
> >
>
> I had the same concern. Thanks for catching this. I suspect this must
> cause some problem for IPA given that it can't discern between the big
> and little "power clusters"?

Both EAS and IPA, I believe. It influences the scheduler's view of the
the topology.

> Either way,
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Thanks.

Andy/Bjorn, can we squeeze this in for -rc2 as a bugfix?

Regards,
Amit

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Sudeep Holla]

On Mon, May 13, 2019 at 05:24:12PM +0530, Amit Kucheria wrote:
> On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> >
> > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > that describes this topology.
> >
> > This is partly true. There are two groups of gold and silver cores,
> > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > is one of the early examples of ARM's Dynamiq architecture.
> >
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >
> > I noticed that this patch sneaked through for this merge window but
> > perhaps we can whip up a quick fix for -rc2?
> >
> 
> And please find attached a patch to fix this up. Andy, since this
> hasn't landed yet (can we still squash this into the original patch?),
> I couldn't add a Fixes tag.
> 
> Regards,
> Amit
> 
> > > ---
> > >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 38 ++++++++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > index c27cbd3bcb0a6..f6c0d87e663f3 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > @@ -192,6 +192,44 @@
> > >                                 next-level-cache = <&L3_0>;
> > >                         };
> > >                 };
> > > +
> > > +               cpu-map {
> > > +                       cluster0 {
> > > +                               core0 {
> > > +                                       cpu = <&CPU0>;
> > > +                               };
> > > +
> > > +                               core1 {
> > > +                                       cpu = <&CPU1>;
> > > +                               };
> > > +
> > > +                               core2 {
> > > +                                       cpu = <&CPU2>;
> > > +                               };
> > > +
> > > +                               core3 {
> > > +                                       cpu = <&CPU3>;
> > > +                               };
> > > +                       };
> > > +
> > > +                       cluster1 {
> >
> > This shouldn't exist.
> >
> > > +                               core0 {
> >
> > Rename to core4, 5, etc...
> >
> > > +                                       cpu = <&CPU4>;
> > > +                               };
> > > +
> > > +                               core1 {
> > > +                                       cpu = <&CPU5>;
> > > +                               };
> > > +
> > > +                               core2 {
> > > +                                       cpu = <&CPU6>;
> > > +                               };
> > > +
> > > +                               core3 {
> > > +                                       cpu = <&CPU7>;
> > > +                               };
> > > +                       };
> > > +               };
> > >         };
> > >
> > >         pmu {
> > > --
> > > 2.20.1.97.g81188d93c3-goog
> > >

> From 9e7d60bcabad7594a1da43982bbc9fda04669717 Mon Sep 17 00:00:00 2001
> Message-Id: <9e7d60bcabad7594a1da43982bbc9fda04669717.1557748437.git.amit.kucheria@linaro.org>
> From: Amit Kucheria <amit.kucheria@linaro.org>
> Date: Mon, 13 May 2019 17:08:33 +0530
> Subject: [PATCH] arm64: dts: sdm845: Fix up CPU topology
> 
> SDM845 implements ARM's Dynamiq architecture that allows the big and
> LITTLE cores to exist in a single cluster sharing the L3 cache.
> 
> Fix the cpu-map to put all cpus into a single cluster.
>

Thanks for noticing and fixing this. I always mentioned this should
never land in mainline when Arm suggested this as hack/workaround
but it has unfortunately.

FWIW,

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Stephen Boyd]

Quoting Amit Kucheria (2019-05-16 04:54:45)
> (cc'ing Andy's correct email address)
> 
> On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Amit Kucheria (2019-05-13 04:54:12)
> > > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > > >
> > > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > >
> > > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > > that describes this topology.
> > > >
> > > > This is partly true. There are two groups of gold and silver cores,
> > > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > > is one of the early examples of ARM's Dynamiq architecture.
> > > >
> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > >
> > > > I noticed that this patch sneaked through for this merge window but
> > > > perhaps we can whip up a quick fix for -rc2?
> > > >
> > >
> > > And please find attached a patch to fix this up. Andy, since this
> > > hasn't landed yet (can we still squash this into the original patch?),
> > > I couldn't add a Fixes tag.
> > >
> >
> > I had the same concern. Thanks for catching this. I suspect this must
> > cause some problem for IPA given that it can't discern between the big
> > and little "power clusters"?
> 
> Both EAS and IPA, I believe. It influences the scheduler's view of the
> the topology.

And EAS and IPA are OK with the real topology? I'm just curious if
changing the topology to reflect reality will be a problem for those
two.

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Bjorn Andersson]

On Thu 16 May 04:54 PDT 2019, Amit Kucheria wrote:

> (cc'ing Andy's correct email address)
> 
> On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Amit Kucheria (2019-05-13 04:54:12)
> > > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > > >
> > > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > >
> > > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > > that describes this topology.
> > > >
> > > > This is partly true. There are two groups of gold and silver cores,
> > > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > > is one of the early examples of ARM's Dynamiq architecture.
> > > >
> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > >
> > > > I noticed that this patch sneaked through for this merge window but
> > > > perhaps we can whip up a quick fix for -rc2?
> > > >
> > >
> > > And please find attached a patch to fix this up. Andy, since this
> > > hasn't landed yet (can we still squash this into the original patch?),
> > > I couldn't add a Fixes tag.
> > >
> >
> > I had the same concern. Thanks for catching this. I suspect this must
> > cause some problem for IPA given that it can't discern between the big
> > and little "power clusters"?
> 
> Both EAS and IPA, I believe. It influences the scheduler's view of the
> the topology.
> 
> > Either way,
> >
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> 
> Thanks.
> 
> Andy/Bjorn, can we squeeze this in for -rc2 as a bugfix?
> 

Yes, I've picked this up among a few other fixes.

Regards,
Bjorn

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Quentin Perret]

On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
> Quoting Amit Kucheria (2019-05-16 04:54:45)
> > (cc'ing Andy's correct email address)
> > 
> > On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Amit Kucheria (2019-05-13 04:54:12)
> > > > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > > > >
> > > > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > > >
> > > > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > > > that describes this topology.
> > > > >
> > > > > This is partly true. There are two groups of gold and silver cores,
> > > > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > > > is one of the early examples of ARM's Dynamiq architecture.
> > > > >
> > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > >
> > > > > I noticed that this patch sneaked through for this merge window but
> > > > > perhaps we can whip up a quick fix for -rc2?
> > > > >
> > > >
> > > > And please find attached a patch to fix this up. Andy, since this
> > > > hasn't landed yet (can we still squash this into the original patch?),
> > > > I couldn't add a Fixes tag.
> > > >
> > >
> > > I had the same concern. Thanks for catching this. I suspect this must
> > > cause some problem for IPA given that it can't discern between the big
> > > and little "power clusters"?
> > 
> > Both EAS and IPA, I believe. It influences the scheduler's view of the
> > the topology.
> 
> And EAS and IPA are OK with the real topology? I'm just curious if
> changing the topology to reflect reality will be a problem for those
> two.

FWIW, neither EAS nor IPA depends on this. Not the upstream version of
EAS at least (which is used in recent Android kernels -- 4.19+).

But doing this is still required for other things in the scheduler (the
so-called 'capacity-awareness' code). So until we have a better
solution, this patch is doing the right thing.

I hope that helps.

Thanks,
Quentin

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Vincent Guittot]

Hi Quentin,

On Wed, 5 Jun 2019 at 19:21, Quentin Perret <quentin.perret@arm.com> wrote:
>
> On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
> > Quoting Amit Kucheria (2019-05-16 04:54:45)
> > > (cc'ing Andy's correct email address)
> > >
> > > On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Amit Kucheria (2019-05-13 04:54:12)
> > > > > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > > > > >
> > > > > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > > > >
> > > > > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > > > > that describes this topology.
> > > > > >
> > > > > > This is partly true. There are two groups of gold and silver cores,
> > > > > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > > > > is one of the early examples of ARM's Dynamiq architecture.
> > > > > >
> > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > >
> > > > > > I noticed that this patch sneaked through for this merge window but
> > > > > > perhaps we can whip up a quick fix for -rc2?
> > > > > >
> > > > >
> > > > > And please find attached a patch to fix this up. Andy, since this
> > > > > hasn't landed yet (can we still squash this into the original patch?),
> > > > > I couldn't add a Fixes tag.
> > > > >
> > > >
> > > > I had the same concern. Thanks for catching this. I suspect this must
> > > > cause some problem for IPA given that it can't discern between the big
> > > > and little "power clusters"?
> > >
> > > Both EAS and IPA, I believe. It influences the scheduler's view of the
> > > the topology.
> >
> > And EAS and IPA are OK with the real topology? I'm just curious if
> > changing the topology to reflect reality will be a problem for those
> > two.
>
> FWIW, neither EAS nor IPA depends on this. Not the upstream version of
> EAS at least (which is used in recent Android kernels -- 4.19+).
>
> But doing this is still required for other things in the scheduler (the
> so-called 'capacity-awareness' code). So until we have a better
> solution, this patch is doing the right thing.

I'm not sure to catch what you mean ?
Which so-called 'capacity-awareness' code are you speaking about ? and
what is the problem ?

Regards,
Vincent

>
> I hope that helps.
>
> Thanks,
> Quentin

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Quentin Perret]

Hi Vincent,

On Thursday 06 Jun 2019 at 09:05:16 (+0200), Vincent Guittot wrote:
> Hi Quentin,
> 
> On Wed, 5 Jun 2019 at 19:21, Quentin Perret <quentin.perret@arm.com> wrote:
> >
> > On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
> > > Quoting Amit Kucheria (2019-05-16 04:54:45)
> > > > (cc'ing Andy's correct email address)
> > > >
> > > > On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Amit Kucheria (2019-05-13 04:54:12)
> > > > > > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > > > > > >
> > > > > > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > > > > >
> > > > > > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > > > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > > > > > that describes this topology.
> > > > > > >
> > > > > > > This is partly true. There are two groups of gold and silver cores,
> > > > > > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > > > > > is one of the early examples of ARM's Dynamiq architecture.
> > > > > > >
> > > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > > >
> > > > > > > I noticed that this patch sneaked through for this merge window but
> > > > > > > perhaps we can whip up a quick fix for -rc2?
> > > > > > >
> > > > > >
> > > > > > And please find attached a patch to fix this up. Andy, since this
> > > > > > hasn't landed yet (can we still squash this into the original patch?),
> > > > > > I couldn't add a Fixes tag.
> > > > > >
> > > > >
> > > > > I had the same concern. Thanks for catching this. I suspect this must
> > > > > cause some problem for IPA given that it can't discern between the big
> > > > > and little "power clusters"?
> > > >
> > > > Both EAS and IPA, I believe. It influences the scheduler's view of the
> > > > the topology.
> > >
> > > And EAS and IPA are OK with the real topology? I'm just curious if
> > > changing the topology to reflect reality will be a problem for those
> > > two.
> >
> > FWIW, neither EAS nor IPA depends on this. Not the upstream version of
> > EAS at least (which is used in recent Android kernels -- 4.19+).
> >
> > But doing this is still required for other things in the scheduler (the
> > so-called 'capacity-awareness' code). So until we have a better
> > solution, this patch is doing the right thing.
> 
> I'm not sure to catch what you mean ?
> Which so-called 'capacity-awareness' code are you speaking about ? and
> what is the problem ?

I'm talking about the wake-up path. ATM select_idle_sibling() is totally
unaware of capacity differences. In its current form, this function
basically assumes that all CPUs in a given sd_llc have the same
capacity, which would be wrong if we had a single MC level for SDM845.
So, until select_idle_sibling() is 'fixed' to be capacity-aware, we need
two levels of sd for asymetric systems (including DynamIQ) so the
wake_cap() story actually works.

I hope that clarifies it :)

Thanks,
Quentin

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Vincent Guittot]

On Thu, 6 Jun 2019 at 09:49, Quentin Perret <quentin.perret@arm.com> wrote:
>
> Hi Vincent,
>
> On Thursday 06 Jun 2019 at 09:05:16 (+0200), Vincent Guittot wrote:
> > Hi Quentin,
> >
> > On Wed, 5 Jun 2019 at 19:21, Quentin Perret <quentin.perret@arm.com> wrote:
> > >
> > > On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
> > > > Quoting Amit Kucheria (2019-05-16 04:54:45)
> > > > > (cc'ing Andy's correct email address)
> > > > >
> > > > > On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Amit Kucheria (2019-05-13 04:54:12)
> > > > > > > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > > > > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > > > > > > that describes this topology.
> > > > > > > >
> > > > > > > > This is partly true. There are two groups of gold and silver cores,
> > > > > > > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > > > > > > is one of the early examples of ARM's Dynamiq architecture.
> > > > > > > >
> > > > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > > > >
> > > > > > > > I noticed that this patch sneaked through for this merge window but
> > > > > > > > perhaps we can whip up a quick fix for -rc2?
> > > > > > > >
> > > > > > >
> > > > > > > And please find attached a patch to fix this up. Andy, since this
> > > > > > > hasn't landed yet (can we still squash this into the original patch?),
> > > > > > > I couldn't add a Fixes tag.
> > > > > > >
> > > > > >
> > > > > > I had the same concern. Thanks for catching this. I suspect this must
> > > > > > cause some problem for IPA given that it can't discern between the big
> > > > > > and little "power clusters"?
> > > > >
> > > > > Both EAS and IPA, I believe. It influences the scheduler's view of the
> > > > > the topology.
> > > >
> > > > And EAS and IPA are OK with the real topology? I'm just curious if
> > > > changing the topology to reflect reality will be a problem for those
> > > > two.
> > >
> > > FWIW, neither EAS nor IPA depends on this. Not the upstream version of
> > > EAS at least (which is used in recent Android kernels -- 4.19+).
> > >
> > > But doing this is still required for other things in the scheduler (the
> > > so-called 'capacity-awareness' code). So until we have a better
> > > solution, this patch is doing the right thing.
> >
> > I'm not sure to catch what you mean ?
> > Which so-called 'capacity-awareness' code are you speaking about ? and
> > what is the problem ?
>
> I'm talking about the wake-up path. ATM select_idle_sibling() is totally
> unaware of capacity differences. In its current form, this function
> basically assumes that all CPUs in a given sd_llc have the same
> capacity, which would be wrong if we had a single MC level for SDM845.
> So, until select_idle_sibling() is 'fixed' to be capacity-aware, we need
> two levels of sd for asymetric systems (including DynamIQ) so the
> wake_cap() story actually works.
>
> I hope that clarifies it :)

hmm... does this justifies this wrong topology ?
select_idle_sibling() is called only when system is overloaded and
scheduler disables the EAS path
In this case, the scheduler looks either for an idle cpu or for evenly
spreading the loads
This is maybe not always optimal and should probably be fixed but
doesn't justifies a wrong topology description IMHO


>
> Thanks,
> Quentin

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Quentin Perret]

+CC Morten who had an interest in this

On Thursday 06 Jun 2019 at 10:20:12 (+0200), Vincent Guittot wrote:
> On Thu, 6 Jun 2019 at 09:49, Quentin Perret <quentin.perret@arm.com> wrote:
> >
> > Hi Vincent,
> >
> > On Thursday 06 Jun 2019 at 09:05:16 (+0200), Vincent Guittot wrote:
> > > Hi Quentin,
> > >
> > > On Wed, 5 Jun 2019 at 19:21, Quentin Perret <quentin.perret@arm.com> wrote:
> > > >
> > > > On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
> > > > > Quoting Amit Kucheria (2019-05-16 04:54:45)
> > > > > > (cc'ing Andy's correct email address)
> > > > > >
> > > > > > On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > >
> > > > > > > Quoting Amit Kucheria (2019-05-13 04:54:12)
> > > > > > > > On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > > > > > > > > > ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > > > > > > > > > that describes this topology.
> > > > > > > > >
> > > > > > > > > This is partly true. There are two groups of gold and silver cores,
> > > > > > > > > but AFAICT they are in a single cluster, not two separate ones. SDM845
> > > > > > > > > is one of the early examples of ARM's Dynamiq architecture.
> > > > > > > > >
> > > > > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > > > > >
> > > > > > > > > I noticed that this patch sneaked through for this merge window but
> > > > > > > > > perhaps we can whip up a quick fix for -rc2?
> > > > > > > > >
> > > > > > > >
> > > > > > > > And please find attached a patch to fix this up. Andy, since this
> > > > > > > > hasn't landed yet (can we still squash this into the original patch?),
> > > > > > > > I couldn't add a Fixes tag.
> > > > > > > >
> > > > > > >
> > > > > > > I had the same concern. Thanks for catching this. I suspect this must
> > > > > > > cause some problem for IPA given that it can't discern between the big
> > > > > > > and little "power clusters"?
> > > > > >
> > > > > > Both EAS and IPA, I believe. It influences the scheduler's view of the
> > > > > > the topology.
> > > > >
> > > > > And EAS and IPA are OK with the real topology? I'm just curious if
> > > > > changing the topology to reflect reality will be a problem for those
> > > > > two.
> > > >
> > > > FWIW, neither EAS nor IPA depends on this. Not the upstream version of
> > > > EAS at least (which is used in recent Android kernels -- 4.19+).
> > > >
> > > > But doing this is still required for other things in the scheduler (the
> > > > so-called 'capacity-awareness' code). So until we have a better
> > > > solution, this patch is doing the right thing.
> > >
> > > I'm not sure to catch what you mean ?
> > > Which so-called 'capacity-awareness' code are you speaking about ? and
> > > what is the problem ?
> >
> > I'm talking about the wake-up path. ATM select_idle_sibling() is totally
> > unaware of capacity differences. In its current form, this function
> > basically assumes that all CPUs in a given sd_llc have the same
> > capacity, which would be wrong if we had a single MC level for SDM845.
> > So, until select_idle_sibling() is 'fixed' to be capacity-aware, we need
> > two levels of sd for asymetric systems (including DynamIQ) so the
> > wake_cap() story actually works.
> >
> > I hope that clarifies it :)
> 
> hmm... does this justifies this wrong topology ?
> select_idle_sibling() is called only when system is overloaded and
> scheduler disables the EAS path
> In this case, the scheduler looks either for an idle cpu or for evenly
> spreading the loads

Yeah but a big task can end up being placed on a little CPU even if a
big CPU is idle. So, it's definitely sub-optimal.

> This is maybe not always optimal and should probably be fixed but
> doesn't justifies a wrong topology description IMHO

I totally agree, the proper fix is to get select_idle_sibling() capacity
aware. And actually, it is true the DT should represent the HW, so
perhaps Amit's fix is the right thing to do so we don't let those hacky
topologies creep into mainline ... I just want to make sure we are fully
aware and OK with the fact that the performance on this platform might
be a little inconsistent until select_idle_sibling() is fixed.

Thanks,
Quentin

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Dietmar Eggemann]

On 6/6/19 10:20 AM, Vincent Guittot wrote:
> On Thu, 6 Jun 2019 at 09:49, Quentin Perret <quentin.perret@arm.com> wrote:
>>
>> Hi Vincent,
>>
>> On Thursday 06 Jun 2019 at 09:05:16 (+0200), Vincent Guittot wrote:
>>> Hi Quentin,
>>>
>>> On Wed, 5 Jun 2019 at 19:21, Quentin Perret <quentin.perret@arm.com> wrote:
>>>>
>>>> On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
>>>>> Quoting Amit Kucheria (2019-05-16 04:54:45)
>>>>>> (cc'ing Andy's correct email address)
>>>>>>
>>>>>> On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
>>>>>>>
>>>>>>> Quoting Amit Kucheria (2019-05-13 04:54:12)
>>>>>>>> On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>>>>>>>>>>
>>>>>>>>>> The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
>>>>>>>>>> ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
>>>>>>>>>> that describes this topology.
>>>>>>>>>
>>>>>>>>> This is partly true. There are two groups of gold and silver cores,
>>>>>>>>> but AFAICT they are in a single cluster, not two separate ones. SDM845
>>>>>>>>> is one of the early examples of ARM's Dynamiq architecture.
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>>>>>>>>
>>>>>>>>> I noticed that this patch sneaked through for this merge window but
>>>>>>>>> perhaps we can whip up a quick fix for -rc2?
>>>>>>>>>
>>>>>>>>
>>>>>>>> And please find attached a patch to fix this up. Andy, since this
>>>>>>>> hasn't landed yet (can we still squash this into the original patch?),
>>>>>>>> I couldn't add a Fixes tag.
>>>>>>>>
>>>>>>>
>>>>>>> I had the same concern. Thanks for catching this. I suspect this must
>>>>>>> cause some problem for IPA given that it can't discern between the big
>>>>>>> and little "power clusters"?
>>>>>>
>>>>>> Both EAS and IPA, I believe. It influences the scheduler's view of the
>>>>>> the topology.
>>>>>
>>>>> And EAS and IPA are OK with the real topology? I'm just curious if
>>>>> changing the topology to reflect reality will be a problem for those
>>>>> two.
>>>>
>>>> FWIW, neither EAS nor IPA depends on this. Not the upstream version of
>>>> EAS at least (which is used in recent Android kernels -- 4.19+).
>>>>
>>>> But doing this is still required for other things in the scheduler (the
>>>> so-called 'capacity-awareness' code). So until we have a better
>>>> solution, this patch is doing the right thing.
>>>
>>> I'm not sure to catch what you mean ?
>>> Which so-called 'capacity-awareness' code are you speaking about ? and
>>> what is the problem ?
>>
>> I'm talking about the wake-up path. ATM select_idle_sibling() is totally
>> unaware of capacity differences. In its current form, this function
>> basically assumes that all CPUs in a given sd_llc have the same
>> capacity, which would be wrong if we had a single MC level for SDM845.
>> So, until select_idle_sibling() is 'fixed' to be capacity-aware, we need
>> two levels of sd for asymetric systems (including DynamIQ) so the
>> wake_cap() story actually works.
>>
>> I hope that clarifies it :)
> 
> hmm... does this justifies this wrong topology ?
> select_idle_sibling() is called only when system is overloaded and
> scheduler disables the EAS path
> In this case, the scheduler looks either for an idle cpu or for evenly
> spreading the loads
> This is maybe not always optimal and should probably be fixed but
> doesn't justifies a wrong topology description IMHO

The big/Little cluster detection in wake_cap() doesn't work anymore with 
DynamIQ w/o Phanton (DIE) domain. So the decision of going sis() or slow 
path is IMHO broken.
But I support the idea of not introducing Phantom Domains in device tree 
and fix wake_cap() instead.

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Vincent Guittot]

On Thu, 6 Jun 2019 at 10:34, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>
> On 6/6/19 10:20 AM, Vincent Guittot wrote:
> > On Thu, 6 Jun 2019 at 09:49, Quentin Perret <quentin.perret@arm.com> wrote:
> >>
> >> Hi Vincent,
> >>
> >> On Thursday 06 Jun 2019 at 09:05:16 (+0200), Vincent Guittot wrote:
> >>> Hi Quentin,
> >>>
> >>> On Wed, 5 Jun 2019 at 19:21, Quentin Perret <quentin.perret@arm.com> wrote:
> >>>>
> >>>> On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
> >>>>> Quoting Amit Kucheria (2019-05-16 04:54:45)
> >>>>>> (cc'ing Andy's correct email address)
> >>>>>>
> >>>>>> On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >>>>>>>
> >>>>>>> Quoting Amit Kucheria (2019-05-13 04:54:12)
> >>>>>>>> On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> >>>>>>>>>
> >>>>>>>>> On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >>>>>>>>>>
> >>>>>>>>>> The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> >>>>>>>>>> ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> >>>>>>>>>> that describes this topology.
> >>>>>>>>>
> >>>>>>>>> This is partly true. There are two groups of gold and silver cores,
> >>>>>>>>> but AFAICT they are in a single cluster, not two separate ones. SDM845
> >>>>>>>>> is one of the early examples of ARM's Dynamiq architecture.
> >>>>>>>>>
> >>>>>>>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>>>>>>>>
> >>>>>>>>> I noticed that this patch sneaked through for this merge window but
> >>>>>>>>> perhaps we can whip up a quick fix for -rc2?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> And please find attached a patch to fix this up. Andy, since this
> >>>>>>>> hasn't landed yet (can we still squash this into the original patch?),
> >>>>>>>> I couldn't add a Fixes tag.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I had the same concern. Thanks for catching this. I suspect this must
> >>>>>>> cause some problem for IPA given that it can't discern between the big
> >>>>>>> and little "power clusters"?
> >>>>>>
> >>>>>> Both EAS and IPA, I believe. It influences the scheduler's view of the
> >>>>>> the topology.
> >>>>>
> >>>>> And EAS and IPA are OK with the real topology? I'm just curious if
> >>>>> changing the topology to reflect reality will be a problem for those
> >>>>> two.
> >>>>
> >>>> FWIW, neither EAS nor IPA depends on this. Not the upstream version of
> >>>> EAS at least (which is used in recent Android kernels -- 4.19+).
> >>>>
> >>>> But doing this is still required for other things in the scheduler (the
> >>>> so-called 'capacity-awareness' code). So until we have a better
> >>>> solution, this patch is doing the right thing.
> >>>
> >>> I'm not sure to catch what you mean ?
> >>> Which so-called 'capacity-awareness' code are you speaking about ? and
> >>> what is the problem ?
> >>
> >> I'm talking about the wake-up path. ATM select_idle_sibling() is totally
> >> unaware of capacity differences. In its current form, this function
> >> basically assumes that all CPUs in a given sd_llc have the same
> >> capacity, which would be wrong if we had a single MC level for SDM845.
> >> So, until select_idle_sibling() is 'fixed' to be capacity-aware, we need
> >> two levels of sd for asymetric systems (including DynamIQ) so the
> >> wake_cap() story actually works.
> >>
> >> I hope that clarifies it :)
> >
> > hmm... does this justifies this wrong topology ?
> > select_idle_sibling() is called only when system is overloaded and
> > scheduler disables the EAS path
> > In this case, the scheduler looks either for an idle cpu or for evenly
> > spreading the loads
> > This is maybe not always optimal and should probably be fixed but
> > doesn't justifies a wrong topology description IMHO
>
> The big/Little cluster detection in wake_cap() doesn't work anymore with
> DynamIQ w/o Phanton (DIE) domain. So the decision of going sis() or slow
> path is IMHO broken.

That's probably not the right thread to discuss this further but i'm
not sure to understand why wake_cap() doesn't work as it compares the
capacity_orig of local cpu and prev cpu which are the same whatever
the sche domainœ

> But I support the idea of not introducing Phantom Domains in device tree
> and fix wake_cap() instead.

Re: [PATCH] arm64: dts: sdm845: Add CPU topology

[Morten Rasmussen]

On Thu, Jun 06, 2019 at 10:44:58AM +0200, Vincent Guittot wrote:
> On Thu, 6 Jun 2019 at 10:34, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> >
> > On 6/6/19 10:20 AM, Vincent Guittot wrote:
> > > On Thu, 6 Jun 2019 at 09:49, Quentin Perret <quentin.perret@arm.com> wrote:
> > >>
> > >> Hi Vincent,
> > >>
> > >> On Thursday 06 Jun 2019 at 09:05:16 (+0200), Vincent Guittot wrote:
> > >>> Hi Quentin,
> > >>>
> > >>> On Wed, 5 Jun 2019 at 19:21, Quentin Perret <quentin.perret@arm.com> wrote:
> > >>>>
> > >>>> On Friday 17 May 2019 at 14:55:19 (-0700), Stephen Boyd wrote:
> > >>>>> Quoting Amit Kucheria (2019-05-16 04:54:45)
> > >>>>>> (cc'ing Andy's correct email address)
> > >>>>>>
> > >>>>>> On Wed, May 15, 2019 at 2:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >>>>>>>
> > >>>>>>> Quoting Amit Kucheria (2019-05-13 04:54:12)
> > >>>>>>>> On Mon, May 13, 2019 at 4:31 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > >>>>>>>>>
> > >>>>>>>>> On Tue, Jan 15, 2019 at 12:13 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >>>>>>>>>>
> > >>>>>>>>>> The 8 CPU cores of the SDM845 are organized in two clusters of 4 big
> > >>>>>>>>>> ("gold") and 4 little ("silver") cores. Add a cpu-map node to the DT
> > >>>>>>>>>> that describes this topology.
> > >>>>>>>>>
> > >>>>>>>>> This is partly true. There are two groups of gold and silver cores,
> > >>>>>>>>> but AFAICT they are in a single cluster, not two separate ones. SDM845
> > >>>>>>>>> is one of the early examples of ARM's Dynamiq architecture.
> > >>>>>>>>>
> > >>>>>>>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > >>>>>>>>>
> > >>>>>>>>> I noticed that this patch sneaked through for this merge window but
> > >>>>>>>>> perhaps we can whip up a quick fix for -rc2?
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>> And please find attached a patch to fix this up. Andy, since this
> > >>>>>>>> hasn't landed yet (can we still squash this into the original patch?),
> > >>>>>>>> I couldn't add a Fixes tag.
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> I had the same concern. Thanks for catching this. I suspect this must
> > >>>>>>> cause some problem for IPA given that it can't discern between the big
> > >>>>>>> and little "power clusters"?
> > >>>>>>
> > >>>>>> Both EAS and IPA, I believe. It influences the scheduler's view of the
> > >>>>>> the topology.
> > >>>>>
> > >>>>> And EAS and IPA are OK with the real topology? I'm just curious if
> > >>>>> changing the topology to reflect reality will be a problem for those
> > >>>>> two.
> > >>>>
> > >>>> FWIW, neither EAS nor IPA depends on this. Not the upstream version of
> > >>>> EAS at least (which is used in recent Android kernels -- 4.19+).
> > >>>>
> > >>>> But doing this is still required for other things in the scheduler (the
> > >>>> so-called 'capacity-awareness' code). So until we have a better
> > >>>> solution, this patch is doing the right thing.
> > >>>
> > >>> I'm not sure to catch what you mean ?
> > >>> Which so-called 'capacity-awareness' code are you speaking about ? and
> > >>> what is the problem ?
> > >>
> > >> I'm talking about the wake-up path. ATM select_idle_sibling() is totally
> > >> unaware of capacity differences. In its current form, this function
> > >> basically assumes that all CPUs in a given sd_llc have the same
> > >> capacity, which would be wrong if we had a single MC level for SDM845.
> > >> So, until select_idle_sibling() is 'fixed' to be capacity-aware, we need
> > >> two levels of sd for asymetric systems (including DynamIQ) so the
> > >> wake_cap() story actually works.
> > >>
> > >> I hope that clarifies it :)
> > >
> > > hmm... does this justifies this wrong topology ?

No, it doesn't. It relies heavily on how nested clusters are interpreted
too, so it is quite fragile.

> > > select_idle_sibling() is called only when system is overloaded and
> > > scheduler disables the EAS path
> > > In this case, the scheduler looks either for an idle cpu or for evenly
> > > spreading the loads
> > > This is maybe not always optimal and should probably be fixed but
> > > doesn't justifies a wrong topology description IMHO
> >
> > The big/Little cluster detection in wake_cap() doesn't work anymore with
> > DynamIQ w/o Phanton (DIE) domain. So the decision of going sis() or slow
> > path is IMHO broken.
> 
> That's probably not the right thread to discuss this further but i'm
> not sure to understand why wake_cap() doesn't work as it compares the
> capacity_orig of local cpu and prev cpu which are the same whatever
> the sche domainœ

We have had this discussion a couple of times over the last couple of
years. The story, IIRC, is that when we introduced capacity awareness in
the wake-up path (wake_cap()) we realised (I think it was actually you)
that we could use select_idle_sibling() in cases where we know that the
search space is limited to cpus with sufficient capacity so we didn't
have to take the long route through find_idlest_cpu(). Back then, big
and little were grouped by clusters so it was "safe" to use
select_idle_sibling() on cpu or prev_cpu if they have sufficient
capacity.

With DynamiQ the true topology on many systems is just one cluster and
hence using select_idle_sibling() there means search space includes all
cpu types which isn't "safe" if you have a task requiring more capacity
than can be offered by any cpu in the system. We need to use the
find_idlest_cpu() path on more cases than we do today.

All the code is there I think, we just have to tweak some conditions. I
can try to come up with a simple fix we can discuss and refine as
necessary.

Morten