From: Wang Qing <[email protected]>
When ACPI is not enabled, cpuid_topo->llc_id = cpu_topo->llc_id = -1, which
will set llc_sibling 0xff(...), this is misleading.
Don't set llc_sibling(default 0) if we don't know the cache topology.
Signed-off-by: Wang Qing <[email protected]>
---
drivers/base/arch_topology.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 1d6636e..5c3a642
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -684,7 +684,7 @@ void update_siblings_masks(unsigned int cpuid)
for_each_online_cpu(cpu) {
cpu_topo = &cpu_topology[cpu];
- if (cpuid_topo->llc_id == cpu_topo->llc_id) {
+ if (cpu_topo->llc_id != -1 && cpuid_topo->llc_id == cpu_topo->llc_id) {
cpumask_set_cpu(cpu, &cpuid_topo->llc_sibling);
cpumask_set_cpu(cpuid, &cpu_topo->llc_sibling);
}
--
2.7.4
On Mon, Apr 11, 2022 at 09:37:36AM +0100, Sudeep Holla wrote:
> Hi Qing,
>
> On Sun, Apr 10, 2022 at 07:36:19PM -0700, Qing Wang wrote:
> > From: Wang Qing <[email protected]>
> >
> > When ACPI is not enabled, cpuid_topo->llc_id = cpu_topo->llc_id = -1, which
> > will set llc_sibling 0xff(...), this is misleading.
> >
> > Don't set llc_sibling(default 0) if we don't know the cache topology.
> >
>
> Makes sense to me and thanks for splitting the patch and pointing this out
> clearly. Your earlier patches mixed other things and made it hard to highlight
> this one.
>
> Reviewed-by: Sudeep Holla <[email protected]>
>
> Hi Greg,
>
> Can you pick this up ? IMO It can go as fix in -rc as it is kind of
> misleading though I am not sure if it is breaking any platform.
Will do, thanks.
greg k-h
Hi Qing,
On Sun, Apr 10, 2022 at 07:36:19PM -0700, Qing Wang wrote:
> From: Wang Qing <[email protected]>
>
> When ACPI is not enabled, cpuid_topo->llc_id = cpu_topo->llc_id = -1, which
> will set llc_sibling 0xff(...), this is misleading.
>
> Don't set llc_sibling(default 0) if we don't know the cache topology.
>
Makes sense to me and thanks for splitting the patch and pointing this out
clearly. Your earlier patches mixed other things and made it hard to highlight
this one.
Reviewed-by: Sudeep Holla <[email protected]>
Hi Greg,
Can you pick this up ? IMO It can go as fix in -rc as it is kind of
misleading though I am not sure if it is breaking any platform.
--
Regards,
Sudeep
On Mon, Apr 11, 2022 at 06:07:45PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2022 at 12:10 PM Qing Wang <[email protected]> wrote:
> >
> > From: Wang Qing <[email protected]>
> >
> > When ACPI is not enabled, cpuid_topo->llc_id = cpu_topo->llc_id = -1, which
> > will set llc_sibling 0xff(...), this is misleading.
>
> Shouldn't it be a Fixes tag then?
>
I thought about that. One the file has moved and lot of refactoring before the
move after the code was first introduced. Since no one has seen any issues as
the mask matches all the CPUs on a single chip SoC and this is not user visible,
I didn't push for the fixes tag.
Anyways the original commit introducing the feature is
Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings")
which was merged in v4.18 if I read git log correctly.
I am happy to backport if needed.
> > Don't set llc_sibling(default 0) if we don't know the cache topology.
>
> ...
>
> > - if (cpuid_topo->llc_id == cpu_topo->llc_id) {
> > + if (cpu_topo->llc_id != -1 && cpuid_topo->llc_id == cpu_topo->llc_id) {
>
> I'm wondering if more strict check is better here, i.e.
>
> if (cpu_topo->llc_id >= 0 && cpuid_topo->llc_id ==
> cpu_topo->llc_id) {
>
Yes I would agree. But I think Qing is just following other similar checks in
the file. All such ids are initialised to -1 and are assigned only valid
values >= 0. I am OK to keep it as is to keep it aligned with other similar
checks.
--
Regards,
Sudeep
On Mon, Apr 11, 2022 at 12:10 PM Qing Wang <[email protected]> wrote:
>
> From: Wang Qing <[email protected]>
>
> When ACPI is not enabled, cpuid_topo->llc_id = cpu_topo->llc_id = -1, which
> will set llc_sibling 0xff(...), this is misleading.
Shouldn't it be a Fixes tag then?
> Don't set llc_sibling(default 0) if we don't know the cache topology.
...
> - if (cpuid_topo->llc_id == cpu_topo->llc_id) {
> + if (cpu_topo->llc_id != -1 && cpuid_topo->llc_id == cpu_topo->llc_id) {
I'm wondering if more strict check is better here, i.e.
if (cpu_topo->llc_id >= 0 && cpuid_topo->llc_id ==
cpu_topo->llc_id) {
> cpumask_set_cpu(cpu, &cpuid_topo->llc_sibling);
> cpumask_set_cpu(cpuid, &cpu_topo->llc_sibling);
> }
--
With Best Regards,
Andy Shevchenko