* [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid
@ 2022-04-11 2:36 Qing Wang
2022-04-11 8:37 ` Sudeep Holla
2022-04-11 15:07 ` Andy Shevchenko
0 siblings, 2 replies; 6+ messages in thread
From: Qing Wang @ 2022-04-11 2:36 UTC (permalink / raw)
To: Sudeep Holla, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel
Cc: Wang Qing
From: Wang Qing <wangqing@vivo.com>
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 <wangqing@vivo.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid
2022-04-11 2:36 [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid Qing Wang
@ 2022-04-11 8:37 ` Sudeep Holla
2022-04-11 9:32 ` Greg Kroah-Hartman
2022-04-11 15:07 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Sudeep Holla @ 2022-04-11 8:37 UTC (permalink / raw)
To: Qing Wang, Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Sudeep Holla, linux-kernel
Hi Qing,
On Sun, Apr 10, 2022 at 07:36:19PM -0700, Qing Wang wrote:
> From: Wang Qing <wangqing@vivo.com>
>
> 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 <sudeep.holla@arm.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid
2022-04-11 8:37 ` Sudeep Holla
@ 2022-04-11 9:32 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-04-11 9:32 UTC (permalink / raw)
To: Sudeep Holla; +Cc: Qing Wang, Rafael J. Wysocki, linux-kernel
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 <wangqing@vivo.com>
> >
> > 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 <sudeep.holla@arm.com>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid
2022-04-11 2:36 [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid Qing Wang
2022-04-11 8:37 ` Sudeep Holla
@ 2022-04-11 15:07 ` Andy Shevchenko
2022-04-12 8:58 ` Sudeep Holla
1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2022-04-11 15:07 UTC (permalink / raw)
To: Qing Wang
Cc: Sudeep Holla, Greg Kroah-Hartman, Rafael J. Wysocki,
Linux Kernel Mailing List
On Mon, Apr 11, 2022 at 12:10 PM Qing Wang <wangqing@vivo.com> wrote:
>
> From: Wang Qing <wangqing@vivo.com>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid
2022-04-11 15:07 ` Andy Shevchenko
@ 2022-04-12 8:58 ` Sudeep Holla
0 siblings, 0 replies; 6+ messages in thread
From: Sudeep Holla @ 2022-04-12 8:58 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Qing Wang, Sudeep Holla, Greg Kroah-Hartman, Rafael J. Wysocki,
Linux Kernel Mailing List
On Mon, Apr 11, 2022 at 06:07:45PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2022 at 12:10 PM Qing Wang <wangqing@vivo.com> wrote:
> >
> > From: Wang Qing <wangqing@vivo.com>
> >
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid
@ 2022-03-28 7:24 Qing Wang
0 siblings, 0 replies; 6+ messages in thread
From: Qing Wang @ 2022-03-28 7:24 UTC (permalink / raw)
To: Sudeep Holla, Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel
Cc: Wang Qing
From: Wang Qing <wangqing@vivo.com>
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 <wangqing@vivo.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-12 10:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11 2:36 [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid Qing Wang
2022-04-11 8:37 ` Sudeep Holla
2022-04-11 9:32 ` Greg Kroah-Hartman
2022-04-11 15:07 ` Andy Shevchenko
2022-04-12 8:58 ` Sudeep Holla
-- strict thread matches above, loose matches on Subject: below --
2022-03-28 7:24 Qing Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).