On Fri, May 13, 2022 at 10:34:00AM +0200, Dietmar Eggemann wrote:
> package_id should represent socket in DT. Free it for possible socket
> dts parsing and use the so far unused llc_id / llc_sibling cpumask to
> decode 1. level clusters used in pre-DynamIQ big/little systems.
>
> cpu_coregroup_mask() will return llc_sibling isntead of core_mask in
> this case.
>
> This will let the cluster_id / cluster_sibling cpumask be available for
> 2. level clusters (e.g. to map L2 sharing in Armv9 A510 complexes).
>
> The corresponding sched domain CLS is similarly used in ACPI (servers)
> to map e.g. Kunpeng920 L3-tags or Ampere Altra's SCU bounderies.
>
> Lighlty tested on qemu-system-aarch64 with 1x8 (cluster-x-core) and
> 2x4 cpu-map.
>
> Signed-off-by: Dietmar Eggemann <[email protected]>
> ---
>
> Related to: https://lkml.kernel.org/r/[email protected]
>
> drivers/base/arch_topology.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index f73b836047cf..ac1488990cc8 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -490,7 +490,7 @@ static int __init get_cpu_for_node(struct device_node *node)
> return cpu;
> }
>
> -static int __init parse_core(struct device_node *core, int package_id,
> +static int __init parse_core(struct device_node *core, int llc_id,
> int core_id)
> {
> char name[20];
> @@ -506,7 +506,8 @@ static int __init parse_core(struct device_node *core, int package_id,
> leaf = false;
> cpu = get_cpu_for_node(t);
> if (cpu >= 0) {
> - cpu_topology[cpu].package_id = package_id;
> + cpu_topology[cpu].package_id = 0;
> + cpu_topology[cpu].llc_id = llc_id;
> cpu_topology[cpu].core_id = core_id;
> cpu_topology[cpu].thread_id = i;
> } else if (cpu != -ENODEV) {
> @@ -527,7 +528,8 @@ static int __init parse_core(struct device_node *core, int package_id,
> return -EINVAL;
> }
>
> - cpu_topology[cpu].package_id = package_id;
> + cpu_topology[cpu].package_id = 0;
While the above looks good and matches with what I am attempting to do
as well ...
> + cpu_topology[cpu].llc_id = llc_id;
This looks wrong for simple reason that this is derived incorrectly from
the cpu-map while there is no guarantee that it matches the last level
cache ID on the system as we didn't parse the cache topology for this.
So I disagree with this change as it might conflict with the actual and
correct llc_id.
--
Regards,
Sudeep
On 13/05/2022 11:03, Sudeep Holla wrote:
> On Fri, May 13, 2022 at 10:34:00AM +0200, Dietmar Eggemann wrote:
[...]
>> @@ -527,7 +528,8 @@ static int __init parse_core(struct device_node *core, int package_id,
>> return -EINVAL;
>> }
>>
>> - cpu_topology[cpu].package_id = package_id;
>> + cpu_topology[cpu].package_id = 0;
>
> While the above looks good and matches with what I am attempting to do
> as well ...
>
>> + cpu_topology[cpu].llc_id = llc_id;
>
> This looks wrong for simple reason that this is derived incorrectly from
> the cpu-map while there is no guarantee that it matches the last level
> cache ID on the system as we didn't parse the cache topology for this.
> So I disagree with this change as it might conflict with the actual and
> correct llc_id.
It might not match the LLC, that's true. Something we have already today
in Android for DynamIQ clusters with big/Little. People using 1. level
clusters to group CPUs according to uArch.
My point is we manage to get:
SMT - cpu_smt_mask()
CLS - cpu_clustergroup_mask()
MC - cpu_coregroup_mask()
DIE - cpu_cpu_mask()
covered in ACPI with the cpu_topology[] structure and if we want CLS on
DT we have to save cluster_id for the 2. level (DT) cluster.
And that's why I proposed to (ab)use llc_id to form the MC mask.
I'm not currently aware of another solution to get CLS somehow elegantly
into a DT system.
On Fri, May 13, 2022 at 12:42:00PM +0200, Dietmar Eggemann wrote:
> On 13/05/2022 11:03, Sudeep Holla wrote:
> > On Fri, May 13, 2022 at 10:34:00AM +0200, Dietmar Eggemann wrote:
>
> [...]
>
> >> @@ -527,7 +528,8 @@ static int __init parse_core(struct device_node *core, int package_id,
> >> return -EINVAL;
> >> }
> >>
> >> - cpu_topology[cpu].package_id = package_id;
> >> + cpu_topology[cpu].package_id = 0;
> >
> > While the above looks good and matches with what I am attempting to do
> > as well ...
> >
> >> + cpu_topology[cpu].llc_id = llc_id;
> >
> > This looks wrong for simple reason that this is derived incorrectly from
> > the cpu-map while there is no guarantee that it matches the last level
> > cache ID on the system as we didn't parse the cache topology for this.
> > So I disagree with this change as it might conflict with the actual and
> > correct llc_id.
>
> It might not match the LLC, that's true. Something we have already today
> in Android for DynamIQ clusters with big/Little. People using 1. level
> clusters to group CPUs according to uArch.
Not sure if that is the correct representation of h/w cluster on those
platforms, but if they want to misguide OS with the f/w(DT in this case)
well that's their choice.
The main point is we need to get the exact h/w topology information and
then we can decide how to present the below masks as required by the
scheduler for its sched domains.
> My point is we manage to get:
>
> SMT - cpu_smt_mask()
> CLS - cpu_clustergroup_mask()
> MC - cpu_coregroup_mask()
> DIE - cpu_cpu_mask()
>
> covered in ACPI with the cpu_topology[] structure and if we want CLS on
> DT we have to save cluster_id for the 2. level (DT) cluster.
>
I am not sure on the above point. Even with ACPI PPTT we are just setting
cluster_id based on first or leaf level of the clusters ignoring the
nesting ATM. And that's exactly what I am trying to get with this series[1]
> And that's why I proposed to (ab)use llc_id to form the MC mask.
>
Sure, it is already supported IIUC by cpu_coregroup_mask in arch_topology.c
We just need to make sure llc_id is set correctly in case of DT. Now if
you are saying llc_sibling is not what you need but something else, then
we may need to add that new mask and update cpu_coregroup_mask to choose
that based on certain condition which I believe is bit complicated.
> I'm not currently aware of another solution to get CLS somehow elegantly
> into a DT system.
Will grouping of CPUs into cluster they belong not good enough for CLS ?
I thought that should suffice based on what we have in cpu_clustergroup_mask
(i.e. cluster sibling mask)
--
Regards,
Sudeep