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

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
> 
>

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

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

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

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