[PATCH v1 0/3] arch_topology: Correct CPU capacity scaling

[PATCH v1 0/3] arch_topology: Correct CPU capacity scaling

[Leo Yan]

This patch set is to address issues for CPU capacity scaling.

"capacity-dmips-mhz" property might be absent in all CPU nodes, and in
another situation, DT might have inconsistent binding issue, e.g. some
CPU nodes have "capacity-dmips-mhz" property and some nodes miss the
property.  Current code mixes these two cases and always rollback to CPU
capacity 1024 for these two cases.

Patches 01 and 02 in this set are used to distinguish the two different
DT binding cases, and for the inconsistent binding issue, it rolls back
to 1024 without CPU capacity scaling.

Patch 03 is to handle the case for absenting "capacity-dmips-mhz"
property in CPU nodes, the patch proceeds to do CPU capacity scaling based
on CPU maximum capacity.  Thus it can reflect the correct CPU capacity for
Arm platforms with "fast" and "slow" clusters (CPUs in two clusters have
the same raw capacity but with different maximum frequencies).

This patch set is applied on the mainline kernel with the latest commit
68453767131a ("ARM: Spectre-BHB: provide empty stub for non-config").
And tested on Arm64 Hikey960 platform (with a bit hacking to emulate
fast and slow clusters).


Leo Yan (3):
  arch_topology: Correct semantics for 'cap_parsing_failed'
  arch_topology: Handle inconsistent binding of CPU raw capacity
  arch_topology: Scale CPU capacity if without CPU raw capacity

 drivers/base/arch_topology.c | 42 +++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v1 1/3] arch_topology: Correct semantics for 'cap_parsing_failed'

[Leo Yan]

As described in the DT binding document [1]: "capacity-dmips-mhz
property is all-or-nothing: if it is specified for a cpu node, it has to
be specified for every other cpu nodes, or the system will fall back to
the default capacity value for every CPU".

In other words, we can accept that cases that "capacity-dmips-mhz"
property is specified or not specified for all CPU nodes.  The only
failure is the DT binding is inconsistent for all CPUs nodes, e.g only
part of CPUs have bound the "capacity-dmips-mhz" property.

Currently kernel only considers all CPU nodes having "capacity-dmips-mhz"
property as a parsing success; for the other two cases, one is all CPU
nodes without "capacity-dmips-mhz" property, and another is inconsistent
binding crossing CPU nodes, kernel considers them as parsing failure and
set 'cap_parsing_failed' flag as true.

This patch makes more clear for the semantics of 'cap_parsing_failed',
it only takes the inconsistent binding case as parsing failure.  So it
sets the flag 'cap_parsing_failed' to true only when the array
'raw_capacity' is not NULL and current CPU node doesn't contain the
property "capacity-dmips-mhz".  It marks 'cap_parsing_failed' as
a static global variable to allow it can be used in the source file.

[1] Documentation/devicetree/bindings/arm/cpu-capacity.txt

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/base/arch_topology.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 976154140f0b..b81777ae6425 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -234,6 +234,7 @@ static int register_cpu_capacity_sysctl(void)
 subsys_initcall(register_cpu_capacity_sysctl);
 
 static int update_topology;
+static bool cap_parsing_failed;
 
 int topology_update_cpu_topology(void)
 {
@@ -291,7 +292,6 @@ void topology_normalize_cpu_scale(void)
 bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 {
 	struct clk *cpu_clk;
-	static bool cap_parsing_failed;
 	int ret;
 	u32 cpu_capacity;
 
@@ -331,9 +331,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 			pr_err("cpu_capacity: missing %pOF raw capacity\n",
 				cpu_node);
 			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
+			cap_parsing_failed = true;
+			free_raw_capacity();
 		}
-		cap_parsing_failed = true;
-		free_raw_capacity();
 	}
 
 	return !ret;
-- 
2.25.1

[PATCH v1 2/3] arch_topology: Handle inconsistent binding of CPU raw capacity

[Leo Yan]

There have a corner case is if we use below DT binding:

  cpu0: cpu@0 {
  	compatible = "arm,cortex-a53";
  	device_type = "cpu";
  	reg = <0x0 0x0>;
  	enable-method = "psci";
  };

  cpu1: cpu@1 {
  	compatible = "arm,cortex-a53";
  	device_type = "cpu";
  	reg = <0x0 0x1>;
  	enable-method = "psci";
  	capacity-dmips-mhz = <1024>;
  };

In this case, CPU0 node misses to bind "capacity-dmips-mhz" property,
and CPU1 node binds the property, this means the CPU raw capacity
binding is inconsistent across all CPUs.

This patch introduces an extra flag 'cap_property_miss' to indicate that
any previous CPU nodes miss binding for "capacity-dmips-mhz" property,
and any new CPU node contains the property, it detects the inconsistent
binding, and sets 'cap_parsing_failed' to true and frees raw capacity
array.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/base/arch_topology.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index b81777ae6425..0687576e880b 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -294,6 +294,7 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 	struct clk *cpu_clk;
 	int ret;
 	u32 cpu_capacity;
+	static bool cap_property_miss;
 
 	if (cap_parsing_failed)
 		return false;
@@ -301,6 +302,20 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 	ret = of_property_read_u32(cpu_node, "capacity-dmips-mhz",
 				   &cpu_capacity);
 	if (!ret) {
+		/*
+		 * A previous CPU node misses binding for CPU raw capacity and
+		 * current CPU node finds its property "capacity-dmips-mhz",
+		 * thus the DT binding for "capacity-dmips-mhz" is inconsistent
+		 * across all CPUs.  Set 'cap_parsing_failed' flag and drop the
+		 * CPU raw capacity values.
+		 */
+		if (cap_property_miss) {
+			pr_err("cpu_capacity: binding %pOF raw capacity\n",
+				cpu_node);
+			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
+			goto parsing_failure;
+		}
+
 		if (!raw_capacity) {
 			raw_capacity = kcalloc(num_possible_cpus(),
 					       sizeof(*raw_capacity),
@@ -331,12 +346,18 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 			pr_err("cpu_capacity: missing %pOF raw capacity\n",
 				cpu_node);
 			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
-			cap_parsing_failed = true;
-			free_raw_capacity();
+			goto parsing_failure;
+		} else {
+			cap_property_miss = true;
 		}
 	}
 
 	return !ret;
+
+parsing_failure:
+	cap_parsing_failed = true;
+	free_raw_capacity();
+	return !ret;
 }
 
 #ifdef CONFIG_CPU_FREQ
-- 
2.25.1

[PATCH v1 3/3] arch_topology: Scale CPU capacity if without CPU raw capacity

[Leo Yan]

Unlike a typical Arm big.LITTLE architecture, some Arm systems (like
Qualcomm SoC msm8996 and msm8939) have two clusters, all CPUs in two
clusters have the same micro architecture, but some CPUs are "fast" and
other are "slow".  On this kind platform, all CPUs have the same raw CPU
capacity but "fast" CPUs have higher maximum frequency than "slow" ones.

Let's see an example, there have two clusters and every cluster have 4
CPUs, every CPU has the same raw CPU capacity.  The cluster 0 has the
maximum frequency 1497.6MHz and the cluster 1 has the maximum frequency
1113.6MHz, if don't specify "capacity-dmips-mhz" in DT, the we will
get below result:

  # cat /sys/devices/system/cpu/cpu*/cpu_capacity
  1024
  1024
  1024
  1024
  1024
  1024
  1024
  1024

If "capacity-dmips-mhz" property is not specified for CPU nodes, the
kernel will fallback to default capacity value SCHED_CAPACITY_SCALE
(1024).  Though CPUs in different clusters have different maximum
frequencies, kernel skips to scale CPU capacity so that every CPU
capacity is always SCHED_CAPACITY_SCALE (1024).

This patch is to scale CPU capacity even though "capacity-dmips-mhz"
property is not specified in DT.  If "capacity-dmips-mhz" property is
absent in DT binding, the array "raw_capacity" is not allocated so we
rollback to use SCHED_CAPACITY_SCALE as raw CPU capacity and proceed
to scale CPU capacity based on maximum frequency.

After apply this patch, we can get below result for up elaborated
platform:

  # cat /sys/devices/system/cpu/cpu*/cpu_capacity
  1024
  1024
  1024
  1024
  761
  761
  761
  761

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/base/arch_topology.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 0687576e880b..ef1fa2e417ea 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -267,20 +267,25 @@ void topology_normalize_cpu_scale(void)
 {
 	u64 capacity;
 	u64 capacity_scale;
+	u32 raw_cpu_capacity;
 	int cpu;
 
-	if (!raw_capacity)
+	if (cap_parsing_failed)
 		return;
 
 	capacity_scale = 1;
 	for_each_possible_cpu(cpu) {
-		capacity = raw_capacity[cpu] * per_cpu(freq_factor, cpu);
+		raw_cpu_capacity =
+			raw_capacity ? raw_capacity[cpu] : SCHED_CAPACITY_SCALE;
+		capacity = raw_cpu_capacity * per_cpu(freq_factor, cpu);
 		capacity_scale = max(capacity, capacity_scale);
 	}
 
 	pr_debug("cpu_capacity: capacity_scale=%llu\n", capacity_scale);
 	for_each_possible_cpu(cpu) {
-		capacity = raw_capacity[cpu] * per_cpu(freq_factor, cpu);
+		raw_cpu_capacity =
+			raw_capacity ? raw_capacity[cpu] : SCHED_CAPACITY_SCALE;
+		capacity = raw_cpu_capacity * per_cpu(freq_factor, cpu);
 		capacity = div64_u64(capacity << SCHED_CAPACITY_SHIFT,
 			capacity_scale);
 		topology_set_cpu_scale(cpu, capacity);
@@ -373,7 +378,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 	struct cpufreq_policy *policy = data;
 	int cpu;
 
-	if (!raw_capacity)
+	if (cap_parsing_failed)
 		return 0;
 
 	if (val != CPUFREQ_CREATE_POLICY)
@@ -412,7 +417,7 @@ static int __init register_cpufreq_notifier(void)
 	 * until we have the necessary code to parse the cpu capacity, so
 	 * skip registering cpufreq notifier.
 	 */
-	if (!acpi_disabled || !raw_capacity)
+	if (!acpi_disabled || cap_parsing_failed)
 		return -EINVAL;
 
 	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL))
-- 
2.25.1

Re: [PATCH v1 0/3] arch_topology: Correct CPU capacity scaling

[Ionela Voinescu]

Hi Leo,

On Sunday 13 Mar 2022 at 13:55:09 (+0800), Leo Yan wrote:
> This patch set is to address issues for CPU capacity scaling.
> 
> "capacity-dmips-mhz" property might be absent in all CPU nodes, and in
> another situation, DT might have inconsistent binding issue, e.g. some
> CPU nodes have "capacity-dmips-mhz" property and some nodes miss the
> property.  Current code mixes these two cases and always rollback to CPU
> capacity 1024 for these two cases.
> 
> Patches 01 and 02 in this set are used to distinguish the two different
> DT binding cases, and for the inconsistent binding issue, it rolls back
> to 1024 without CPU capacity scaling.
> 
> Patch 03 is to handle the case for absenting "capacity-dmips-mhz"
> property in CPU nodes, the patch proceeds to do CPU capacity scaling based
> on CPU maximum capacity.  Thus it can reflect the correct CPU capacity for
> Arm platforms with "fast" and "slow" clusters (CPUs in two clusters have
> the same raw capacity but with different maximum frequencies).
> 

In my opinion it's difficult to handle absent "capacity-dmips-mhz"
properties, as they can be a result of 3 scenarios: potential..
 1. bug in DT
 2. unwillingness to fill this information in DT
 3. suggestion that we're dealing with CPUs with same u-arch
    (same capacity-dmips-mhz)

I'm not sure it's up to us to interpret suggestions in the code so I
believe treating missing information as error is the right choice, which
is how we're handling this now.

For 3. (and patch 03), isn't it easier to populate capacity-dmips-mhz to
the same value (say 1024) in DT? That is a clear message that we're
dealing with CPUs with the same u-arch.

Thanks,
Ionela.


> This patch set is applied on the mainline kernel with the latest commit
> 68453767131a ("ARM: Spectre-BHB: provide empty stub for non-config").
> And tested on Arm64 Hikey960 platform (with a bit hacking to emulate
> fast and slow clusters).
> 
> 
> Leo Yan (3):
>   arch_topology: Correct semantics for 'cap_parsing_failed'
>   arch_topology: Handle inconsistent binding of CPU raw capacity
>   arch_topology: Scale CPU capacity if without CPU raw capacity
> 
>  drivers/base/arch_topology.c | 42 +++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> -- 
> 2.25.1
> 
> 

Re: [PATCH v1 0/3] arch_topology: Correct CPU capacity scaling

[Leo Yan]

Hi Ionela,

On Mon, Mar 14, 2022 at 06:10:58PM +0000, Ionela Voinescu wrote:

[...]

> > Patch 03 is to handle the case for absenting "capacity-dmips-mhz"
> > property in CPU nodes, the patch proceeds to do CPU capacity scaling based
> > on CPU maximum capacity.  Thus it can reflect the correct CPU capacity for
> > Arm platforms with "fast" and "slow" clusters (CPUs in two clusters have
> > the same raw capacity but with different maximum frequencies).
> > 
> 
> In my opinion it's difficult to handle absent "capacity-dmips-mhz"
> properties, as they can be a result of 3 scenarios: potential..
>  1. bug in DT
>  2. unwillingness to fill this information in DT
>  3. suggestion that we're dealing with CPUs with same u-arch
>     (same capacity-dmips-mhz)

For absent "capacity-dmips-mhz" properties, I think we could divide into
two sub classes:

For all CPU nodes are absent "capacity-dmips-mhz" properties, it's
likely all CPUs have the same micro architecture, thus developers are
not necessarily to explictly set the property.

For partial CPUs absent "capacity-dmips-mhz" properties, this is an
usage issue in DT and kernel should handle this as an error and report
it.

> I'm not sure it's up to us to interpret suggestions in the code so I
> believe treating missing information as error is the right choice, which
> is how we're handling this now.

Yes, current kernel means to treat missing info as error, whatever if
all CPUs or partial CPUs are absent "capacity-dmips-mhz" properties.

> For 3. (and patch 03), isn't it easier to populate capacity-dmips-mhz to
> the same value (say 1024) in DT? That is a clear message that we're
> dealing with CPUs with the same u-arch.

"capacity-dmips-mhz" is defined as a _optional_ property in the DT
document (see devicetree/bindings/arm/cpu-capacity.txt).

Current kernel rolls back every CPU raw capacity to 1024 if DT doesn't
bind "capacity-dmips-mhz" properties, given many SoCs with same CPU
u-arch this is right thing to do; here I think kernel should proceed to
scale CPU capacity with its maximum frequency.

When I worked on a platform with a fast and a slow clusters (two clusters
have different max frequencies and with the same CPU u-arch), it's a bit
puzzle when I saw all CPU's capacities are always 1024.  In this case,
since a platform have no CPU capacity modeling, and "capacity-dmips-mhz"
property is not needed to populate in DT, but at the end the kernel
should can reflect the scaled CPU capacity correctly.

Thanks a lot for review,

Leo

Re: [PATCH v1 0/3] arch_topology: Correct CPU capacity scaling

[Sudeep Holla]

On Tue, Mar 15, 2022 at 11:29:19AM +0800, Leo Yan wrote:
> Hi Ionela,
> 
> On Mon, Mar 14, 2022 at 06:10:58PM +0000, Ionela Voinescu wrote:
> 
> [...]
> 
> > > Patch 03 is to handle the case for absenting "capacity-dmips-mhz"
> > > property in CPU nodes, the patch proceeds to do CPU capacity scaling based
> > > on CPU maximum capacity.  Thus it can reflect the correct CPU capacity for
> > > Arm platforms with "fast" and "slow" clusters (CPUs in two clusters have
> > > the same raw capacity but with different maximum frequencies).
> > > 
> > 
> > In my opinion it's difficult to handle absent "capacity-dmips-mhz"
> > properties, as they can be a result of 3 scenarios: potential..
> >  1. bug in DT
> >  2. unwillingness to fill this information in DT
> >  3. suggestion that we're dealing with CPUs with same u-arch
> >     (same capacity-dmips-mhz)
> 
> For absent "capacity-dmips-mhz" properties, I think we could divide into
> two sub classes:
> 
> For all CPU nodes are absent "capacity-dmips-mhz" properties, it's
> likely all CPUs have the same micro architecture, thus developers are
> not necessarily to explictly set the property.
>

I completely disagree and NACK to deal with absence of the property in DT.
The binding clearly states:

"CPU capacity is a number that provides the scheduler information about CPUs
heterogeneity. Such heterogeneity can come from micro-architectural differences
(e.g., ARM big.LITTLE systems) or maximum frequency at which CPUs can run
(e.g., SMP systems with multiple frequency domains). Heterogeneity in this
context is about differing performance characteristics; this binding tries to
capture a first-order approximation of the relative performance of CPUs."

So it is clear that using same uarch can't be an excuse to miss this property.
So if you need the scheduler to be aware of this heterogeneity, better update
the DT with property. Absence will always means scheduler need not be aware
of this heterogeneity.

> For partial CPUs absent "capacity-dmips-mhz" properties, this is an
> usage issue in DT and kernel should handle this as an error and report
> it.
>

That makes sense. As I mentioned in my earlier email, we can always flag
up error in the kernel, but it would be good to catch these much earlier
in DT via schema if possible.

> > I'm not sure it's up to us to interpret suggestions in the code so I
> > believe treating missing information as error is the right choice, which
> > is how we're handling this now.
> 
> Yes, current kernel means to treat missing info as error, whatever if
> all CPUs or partial CPUs are absent "capacity-dmips-mhz" properties.
>

OK, so no change needed ? I am confused as what is missing today.

> > For 3. (and patch 03), isn't it easier to populate capacity-dmips-mhz to
> > the same value (say 1024) in DT? That is a clear message that we're
> > dealing with CPUs with the same u-arch.
>
> "capacity-dmips-mhz" is defined as a _optional_ property in the DT
> document (see devicetree/bindings/arm/cpu-capacity.txt).
>

That means that the kernel can operate without the info and nothing more
than that. We are not providing guarantee that the same performance is
possible with or without this optional property.

> Current kernel rolls back every CPU raw capacity to 1024 if DT doesn't
> bind "capacity-dmips-mhz" properties, given many SoCs with same CPU
> u-arch this is right thing to do; here I think kernel should proceed to
> scale CPU capacity with its maximum frequency.
>

As stated above, I completely disagree and once again NACK.

> When I worked on a platform with a fast and a slow clusters (two clusters
> have different max frequencies and with the same CPU u-arch), it's a bit
> puzzle when I saw all CPU's capacities are always 1024.  In this case,
> since a platform have no CPU capacity modeling, and "capacity-dmips-mhz"
> property is not needed to populate in DT, but at the end the kernel
> should can reflect the scaled CPU capacity correctly.
>

Fix the broken DT with respect to this feature. I mean DT is not broken, but
if once needs this feature then they should teach the kernel the hardware
difference with this property.

Another possible issue I can see if this is dealt within the kernel is if
on some platform for thermal or any valid hardware errata reasons, one set
of CPUs can run at max one frequency while the other is restricted at a
suitable lower frequency, it may not be good idea to mark that as difference
in cpu capacity as they are SMP CPUs just in different perf domains with
different limits. I assume the scale invariance must deal with that.
I may be wrong here but that's my understanding, happy to be corrected.

--
Regards,
Sudeep

Re: [PATCH v1 0/3] arch_topology: Correct CPU capacity scaling

[Leo Yan]

Hi Sudeep,

On Tue, Mar 15, 2022 at 10:08:28AM +0000, Sudeep Holla wrote:

[...]

> > > In my opinion it's difficult to handle absent "capacity-dmips-mhz"
> > > properties, as they can be a result of 3 scenarios: potential..
> > >  1. bug in DT
> > >  2. unwillingness to fill this information in DT
> > >  3. suggestion that we're dealing with CPUs with same u-arch
> > >     (same capacity-dmips-mhz)
> > 
> > For absent "capacity-dmips-mhz" properties, I think we could divide into
> > two sub classes:
> > 
> > For all CPU nodes are absent "capacity-dmips-mhz" properties, it's
> > likely all CPUs have the same micro architecture, thus developers are
> > not necessarily to explictly set the property.
> >
> 
> I completely disagree and NACK to deal with absence of the property in DT.
> The binding clearly states:
> 
> "CPU capacity is a number that provides the scheduler information about CPUs
> heterogeneity. Such heterogeneity can come from micro-architectural differences
> (e.g., ARM big.LITTLE systems) or maximum frequency at which CPUs can run
> (e.g., SMP systems with multiple frequency domains). Heterogeneity in this
> context is about differing performance characteristics; this binding tries to
> capture a first-order approximation of the relative performance of CPUs."
> 
> So it is clear that using same uarch can't be an excuse to miss this property.
> So if you need the scheduler to be aware of this heterogeneity, better update
> the DT with property. Absence will always means scheduler need not be aware
> of this heterogeneity.

Okay, understood your point and I am respect that.

> > For partial CPUs absent "capacity-dmips-mhz" properties, this is an
> > usage issue in DT and kernel should handle this as an error and report
> > it.
> >
> 
> That makes sense. As I mentioned in my earlier email, we can always flag
> up error in the kernel, but it would be good to catch these much earlier
> in DT via schema if possible.
> 
> > > I'm not sure it's up to us to interpret suggestions in the code so I
> > > believe treating missing information as error is the right choice, which
> > > is how we're handling this now.
> > 
> > Yes, current kernel means to treat missing info as error, whatever if
> > all CPUs or partial CPUs are absent "capacity-dmips-mhz" properties.
> >
> 
> OK, so no change needed ? I am confused as what is missing today.

The different understanding between us is for the case when all CPUs
absent "capacity-dmips-mhz" properties, seems to me we can take it as
the same thing as all CPUs with binding "capacity-dmips-mhz" = 1024.

Maybe I am is bit obsessive on this :)

> > > For 3. (and patch 03), isn't it easier to populate capacity-dmips-mhz to
> > > the same value (say 1024) in DT? That is a clear message that we're
> > > dealing with CPUs with the same u-arch.
> >
> > "capacity-dmips-mhz" is defined as a _optional_ property in the DT
> > document (see devicetree/bindings/arm/cpu-capacity.txt).
> 
> That means that the kernel can operate without the info and nothing more
> than that. We are not providing guarantee that the same performance is
> possible with or without this optional property.
> 
> > Current kernel rolls back every CPU raw capacity to 1024 if DT doesn't
> > bind "capacity-dmips-mhz" properties, given many SoCs with same CPU
> > u-arch this is right thing to do; here I think kernel should proceed to
> > scale CPU capacity with its maximum frequency.
> 
> As stated above, I completely disagree and once again NACK.
> 
> > When I worked on a platform with a fast and a slow clusters (two clusters
> > have different max frequencies and with the same CPU u-arch), it's a bit
> > puzzle when I saw all CPU's capacities are always 1024.  In this case,
> > since a platform have no CPU capacity modeling, and "capacity-dmips-mhz"
> > property is not needed to populate in DT, but at the end the kernel
> > should can reflect the scaled CPU capacity correctly.
> >
> 
> Fix the broken DT with respect to this feature. I mean DT is not broken, but
> if once needs this feature then they should teach the kernel the hardware
> difference with this property.
> 
> Another possible issue I can see if this is dealt within the kernel is if
> on some platform for thermal or any valid hardware errata reasons, one set
> of CPUs can run at max one frequency while the other is restricted at a
> suitable lower frequency, it may not be good idea to mark that as difference
> in cpu capacity as they are SMP CPUs just in different perf domains with
> different limits. I assume the scale invariance must deal with that.
> I may be wrong here but that's my understanding, happy to be corrected.

After looked a bit for the code, the short answer is we don't need to
adjust "capacity-dmips-mhz" for any thermal capping or CPU frequency
limit.

Since "capacity-dmips-mhz"'s unit is DMIPS/MHz, it's a modeling value
(e.g. generated by using Dhrystone, sysbench, etc).  This is why for
the same micro architecture CPUs, we don't need to do any profiling
and would be fine to directly set as 1024 for all CPUs (no matter the
maximum frequency).

In the kernel, there have two scale invariants: one is CPU capacity
invariant, my understanding is it can allow us to compare capacity
across CPUs; another is CPU frequency invariant, it's used to scale
capacity for different OPPs on a CPU.

So "capacity-dmips-mhz" is used to calculate CPU capacity invariant,
the formual is:

  cpu_scale(cpu) = capacity-dmips-mhz(cpu) * policy(cpu)->cpuinfo.max_freq

policy(cpu)->cpuinfo.max_freq is the maximum frequency when register OPP
table, it's no matter with thermal capping or CPU frequency limit.

Thanks,
Leo

Re: [PATCH v1 0/3] arch_topology: Correct CPU capacity scaling

[Sudeep Holla]

Hi Leo,

On Mon, Mar 14, 2022 at 06:10:58PM +0000, Ionela Voinescu wrote:
> Hi Leo,
> 
> On Sunday 13 Mar 2022 at 13:55:09 (+0800), Leo Yan wrote:
> > This patch set is to address issues for CPU capacity scaling.
> > 
> > "capacity-dmips-mhz" property might be absent in all CPU nodes, and in
> > another situation, DT might have inconsistent binding issue, e.g. some
> > CPU nodes have "capacity-dmips-mhz" property and some nodes miss the
> > property.  Current code mixes these two cases and always rollback to CPU
> > capacity 1024 for these two cases.
> >

Ideally the schema can be made to catch such issues. While I understand
that it is work in progress, we can flag the error in the code to handle that.
Rollback to 1024 seems correct default behaviour to me.

> > Patches 01 and 02 in this set are used to distinguish the two different
> > DT binding cases, and for the inconsistent binding issue, it rolls back
> > to 1024 without CPU capacity scaling.
> >
> > Patch 03 is to handle the case for absenting "capacity-dmips-mhz"
> > property in CPU nodes, the patch proceeds to do CPU capacity scaling based
> > on CPU maximum capacity.  Thus it can reflect the correct CPU capacity for
> > Arm platforms with "fast" and "slow" clusters (CPUs in two clusters have
> > the same raw capacity but with different maximum frequencies).
> >

NACK for the approach. Just fix the DT.

>
> In my opinion it's difficult to handle absent "capacity-dmips-mhz"
> properties, as they can be a result of 3 scenarios: potential..
>  1. bug in DT
>  2. unwillingness to fill this information in DT
>  3. suggestion that we're dealing with CPUs with same u-arch
>     (same capacity-dmips-mhz)
>
> I'm not sure it's up to us to interpret suggestions in the code so I
> believe treating missing information as error is the right choice, which
> is how we're handling this now.
>

+1 for all the points above and are very much valid.

> For 3. (and patch 03), isn't it easier to populate capacity-dmips-mhz to
> the same value (say 1024) in DT? That is a clear message that we're
> dealing with CPUs with the same u-arch.
>

Indeed.

-- 
Regards,
Sudeep