Re: [PATCH] arch_topology: support parsing cache topology from DT

[Sudeep Holla <sudeep.holla@arm.com>]

On Thu, Apr 07, 2022 at 03:11:51AM +0000, 王擎 wrote:
> 
> >> From: wangqing <11112896@bbktel.com>
> >> 
> >> When ACPI is not enabled, we can parse cache topolopy from DT:
> >> *             cpu0: cpu@000 {
> >> *                     next-level-cache = <&L2_1>;
> >> *                     L2_1: l2-cache {
> >> *                              compatible = "cache";
> >> *                             next-level-cache = <&L3_1>;
> >> *                      };
> >> *                     L3_1: l3-cache {
> >> *                              compatible = "cache";
> >> *                      };
> >> *             };
> >> *
> >> *             cpu1: cpu@001 {
> >> *                     next-level-cache = <&L2_1>;
> >> *             };
> >> *             cpu2: cpu@002 {
> >> *                     L2_2: l2-cache {
> >> *                              compatible = "cache";
> >> *                             next-level-cache = <&L3_1>;
> >> *                     };
> >> *             };
> >> *
> >> *             cpu3: cpu@003 {
> >> *                     next-level-cache = <&L2_2>;
> >> *             };
> >> cache_topology hold the pointer describing "next-level-cache", 
> >> it can describe the cache topology of every level.
> >> 
> >> Expand the use of llc_sibling when ACPI is not enabled.
> >>
> >
> >You seem to have posted this patch as part of the series first. One patch
> >was rejected and then you post this without any history. It confuses if you
> >don't provide all the background/history.
>
> Yes, the series contains several parts, the sched_domain part was rejected
> temporary. But it has nothing to do with this patch, that's why I took it apart.

That's not correct if you plan to use it there. Currently no users so no need
to add.

> The background doesn't matter, let's focus on this patch itself.
>

It depends, some people might find it useful, so better to provide it.
One can ignore if that is not needed or if they are already aware.

> >
> >Having said that, NACK for this patch as it stands. We have
> >drivers/base/cacheinfo.c which has all the parsing of cache information.
> >IIRC we already consider LLC but highlight if anything is particularly
> >missing. I am unable to follow/understand with you commit message. 
> 
> cacheinfo.c just describes the properties of the cache, It can't describe
> the cache topology, some like cpuinfo and cpu topology.
> 

Not 100% correct. We do have info about sharing there.

> llc_sibling is not used at all if ACPI is not enabled, because llc_id
> always be -1, and cpu_coregroup_mask() always return the core_sibling.
>

You can use of_find_last_level_cache or something similar and remove load
of duplicated code you have in this patch.

> Why not get the cache topology from DT if the arch support GENERIC_ARCH_TOPOLOGY.
>

Sure but if the intended use is for scheduler, please include relevant people
as there are quite a few threads around the topic recently and disintegrating
and throwing patches at random with different set of people is not going to
help make progress. If this is intended for Arm64 platforms, I suggest to
keep these 2 in the loop as they are following few other threads and gives
them full picture of intended use-case.

Vincent Guittot <vincent.guittot@linaro.org>
Dietmar Eggemann <dietmar.eggemann@arm.com>

--
Regards,
Sudeep