Since the cacheinfo LLC information is used directly in arch_topology,
there is no need to parse and store the LLC ID information only for
ACPI systems in the CPU topology.
Remove the redundant LLC ID from the generic CPU arch_topology information.
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/base/arch_topology.c | 1 -
include/linux/arch_topology.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 4c486e4e6f2f..76c702c217c5 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -747,7 +747,6 @@ void __init reset_cpu_topology(void)
cpu_topo->core_id = -1;
cpu_topo->cluster_id = -1;
cpu_topo->package_id = -1;
- cpu_topo->llc_id = -1;
clear_cpu_topology(cpu);
}
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 58cbe18d825c..a07b510e7dc5 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -68,7 +68,6 @@ struct cpu_topology {
int core_id;
int cluster_id;
int package_id;
- int llc_id;
cpumask_t thread_sibling;
cpumask_t core_sibling;
cpumask_t cluster_sibling;
--
2.36.1
Currently the cluster identifier is not set on the DT based platforms.
The reset or default value is -1 for all the CPUs. Once we assign the
cluster identifier values correctly that imay result in getting the thread
siblings wrongs as the core identifiers can be same for 2 different CPUs
belonging to 2 different cluster.
So, in order to get the thread sibling cpumasks correct, we need to
update them only if the cores they belong are in the same cluster within
the socket. Let us skip updation of the thread sibling cpumaks if the
cluster identifier doesn't match.
This change won't affect even if the cluster identifiers are not set
currently but will avoid any breakage once we set the same correctly.
Signed-off-by: Sudeep Holla <[email protected]>
---
drivers/base/arch_topology.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 76c702c217c5..59dc2c80c439 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -703,15 +703,17 @@ void update_siblings_masks(unsigned int cpuid)
if (cpuid_topo->package_id != cpu_topo->package_id)
continue;
- if (cpuid_topo->cluster_id == cpu_topo->cluster_id &&
- cpuid_topo->cluster_id != -1) {
+ cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
+ cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
+
+ if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
+ continue;
+
+ if (cpuid_topo->cluster_id != -1) {
cpumask_set_cpu(cpu, &cpuid_topo->cluster_sibling);
cpumask_set_cpu(cpuid, &cpu_topo->cluster_sibling);
}
- cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
- cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
-
if (cpuid_topo->core_id != cpu_topo->core_id)
continue;
--
2.36.1
Hi Sudeep,
On 5/25/22 4:14 PM, Sudeep Holla wrote:
> Since the cacheinfo LLC information is used directly in arch_topology,
> there is no need to parse and store the LLC ID information only for
> ACPI systems in the CPU topology.
>
> Remove the redundant LLC ID from the generic CPU arch_topology information.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/base/arch_topology.c | 1 -
> include/linux/arch_topology.h | 1 -
> 2 files changed, 2 deletions(-)
>
How about merge the changes to PATCH[08/16]? I don't see why we need put
the changes into separate patches.
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 4c486e4e6f2f..76c702c217c5 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -747,7 +747,6 @@ void __init reset_cpu_topology(void)
> cpu_topo->core_id = -1;
> cpu_topo->cluster_id = -1;
> cpu_topo->package_id = -1;
> - cpu_topo->llc_id = -1;
>
> clear_cpu_topology(cpu);
> }
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index 58cbe18d825c..a07b510e7dc5 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -68,7 +68,6 @@ struct cpu_topology {
> int core_id;
> int cluster_id;
> int package_id;
> - int llc_id;
> cpumask_t thread_sibling;
> cpumask_t core_sibling;
> cpumask_t cluster_sibling;
>
Thanks,
Gavin
On Wed, Jun 01, 2022 at 11:35:20AM +0800, Gavin Shan wrote:
> Hi Sudeep,
>
> On 5/25/22 4:14 PM, Sudeep Holla wrote:
> > Since the cacheinfo LLC information is used directly in arch_topology,
> > there is no need to parse and store the LLC ID information only for
> > ACPI systems in the CPU topology.
> >
> > Remove the redundant LLC ID from the generic CPU arch_topology information.
> >
> > Signed-off-by: Sudeep Holla <[email protected]>
> > ---
> > drivers/base/arch_topology.c | 1 -
> > include/linux/arch_topology.h | 1 -
> > 2 files changed, 2 deletions(-)
> >
>
> How about merge the changes to PATCH[08/16]? I don't see why we need put
> the changes into separate patches.
>
It took a while to remember as I was with the same opinion as yours but
decided to split them for one reason: to keep arch specific change in a
separate patch(if that becomes a need due to some conflict or some other
non-technical reason)
--
Regards,
Sudeep
On 5/25/22 4:14 PM, Sudeep Holla wrote:
> Currently the cluster identifier is not set on the DT based platforms.
> The reset or default value is -1 for all the CPUs. Once we assign the
> cluster identifier values correctly that imay result in getting the thread
> siblings wrongs as the core identifiers can be same for 2 different CPUs
> belonging to 2 different cluster.
>
> So, in order to get the thread sibling cpumasks correct, we need to
> update them only if the cores they belong are in the same cluster within
> the socket. Let us skip updation of the thread sibling cpumaks if the
> cluster identifier doesn't match.
>
> This change won't affect even if the cluster identifiers are not set
> currently but will avoid any breakage once we set the same correctly.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/base/arch_topology.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
Reviewed-by: Gavin Shan <[email protected]>
Tested-by: Gavin Shan <[email protected]>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 76c702c217c5..59dc2c80c439 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -703,15 +703,17 @@ void update_siblings_masks(unsigned int cpuid)
> if (cpuid_topo->package_id != cpu_topo->package_id)
> continue;
>
> - if (cpuid_topo->cluster_id == cpu_topo->cluster_id &&
> - cpuid_topo->cluster_id != -1) {
> + cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
> + cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
> +
> + if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
> + continue;
> +
> + if (cpuid_topo->cluster_id != -1) {
> cpumask_set_cpu(cpu, &cpuid_topo->cluster_sibling);
> cpumask_set_cpu(cpuid, &cpu_topo->cluster_sibling);
> }
>
> - cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
> - cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
> -
> if (cpuid_topo->core_id != cpu_topo->core_id)
> continue;
>
>
On 5/25/22 4:14 PM, Sudeep Holla wrote:
> Since the cacheinfo LLC information is used directly in arch_topology,
> there is no need to parse and store the LLC ID information only for
> ACPI systems in the CPU topology.
>
> Remove the redundant LLC ID from the generic CPU arch_topology information.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> ---
> drivers/base/arch_topology.c | 1 -
> include/linux/arch_topology.h | 1 -
> 2 files changed, 2 deletions(-)
>
Reviewed-by: Gavin Shan <[email protected]>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 4c486e4e6f2f..76c702c217c5 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -747,7 +747,6 @@ void __init reset_cpu_topology(void)
> cpu_topo->core_id = -1;
> cpu_topo->cluster_id = -1;
> cpu_topo->package_id = -1;
> - cpu_topo->llc_id = -1;
>
> clear_cpu_topology(cpu);
> }
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index 58cbe18d825c..a07b510e7dc5 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -68,7 +68,6 @@ struct cpu_topology {
> int core_id;
> int cluster_id;
> int package_id;
> - int llc_id;
> cpumask_t thread_sibling;
> cpumask_t core_sibling;
> cpumask_t cluster_sibling;
>
Hi Sudeep,
On 6/1/22 8:06 PM, Sudeep Holla wrote:
> On Wed, Jun 01, 2022 at 11:35:20AM +0800, Gavin Shan wrote:
>> On 5/25/22 4:14 PM, Sudeep Holla wrote:
>>> Since the cacheinfo LLC information is used directly in arch_topology,
>>> there is no need to parse and store the LLC ID information only for
>>> ACPI systems in the CPU topology.
>>>
>>> Remove the redundant LLC ID from the generic CPU arch_topology information.
>>>
>>> Signed-off-by: Sudeep Holla <[email protected]>
>>> ---
>>> drivers/base/arch_topology.c | 1 -
>>> include/linux/arch_topology.h | 1 -
>>> 2 files changed, 2 deletions(-)
>>>
>>
>> How about merge the changes to PATCH[08/16]? I don't see why we need put
>> the changes into separate patches.
>>
>
> It took a while to remember as I was with the same opinion as yours but
> decided to split them for one reason: to keep arch specific change in a
> separate patch(if that becomes a need due to some conflict or some other
> non-technical reason)
>
Ok. Thanks for the explanation, which sounds reasonable to me.
Thanks,
Gavin