Re: [PATCH] arch_topology: Do not set llc_sibling if llc_id is invalid

[Sudeep Holla]

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@xxxxxxxx> wrote:
> >
> > From: Wang Qing <wangqing@xxxxxxxx>
> >
> > 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