Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Apr 2023 13:47:39 +0100
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Pierre Gondois <pierre.gondois@arm.com>
CC:     <linux-kernel@vger.kernel.org>, Radu Rendec <rrendec@redhat.com>,
        Alexandre Ghiti <alexghiti@rivosinc.com>,
        Will Deacon <will@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Palmer Dabbelt <palmer@rivosinc.com>,
        Gavin Shan <gshan@redhat.com>,
        Jeremy Linton <jeremy.linton@arm.com>
Subject: Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in
 cache_leaves_are_shared()
Message-ID: <20230412-hut-unused-21d683fcb8b0@wendy>
References: <20230412071809.12670-1-pierre.gondois@arm.com>
 <20230412071809.12670-2-pierre.gondois@arm.com>
 <20230412-viewpoint-refutable-a31f3657093c@wendy>
 <d7a36615-896b-0f13-a1f6-761715ce460f@arm.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="oLQO1TaK6ViWJelZ"
Content-Disposition: inline
In-Reply-To: <d7a36615-896b-0f13-a1f6-761715ce460f@arm.com>
Precedence: bulk

--oLQO1TaK6ViWJelZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 12, 2023 at 02:34:11PM +0200, Pierre Gondois wrote:
> Hello Conor,
>=20
> On 4/12/23 13:27, Conor Dooley wrote:
> > On Wed, Apr 12, 2023 at 09:18:04AM +0200, Pierre Gondois wrote:
> > > If 'this_leaf' is a L2 cache (or higher) and 'sib_leaf' is a L1 cache,
> > > the caches are detected as shared. Indeed, cache_leaves_are_shared()
> > > only checks the cache level of 'this_leaf' when 'sib_leaf''s cache
> > > level should also be checked.
> >=20
> > I have to say, I'm a wee bit confused reading this patch - although it's
> > likely that I have just confused myself here.
> >=20
> > The comment reads "For non DT/ACPI systems, assume unique level 1 cache=
s,
> > system-wide shared caches for all other levels".
> > Does this mean all level 1 caches are unique & all level N caches are
> > shared with all other level N caches, but not with level M caches?
> > (M !=3D N; M, N > 1)
>=20
> I think the real answer to your question is in the last paragraph,
> but just in case:
>=20
> Each CPU manages the list of cacheinfo struct it has access to,
> and this list is per-CPU.
> cache_shared_cpu_map_setup() checks whether two cacheinfo struct are
> representing the same cache (for 2 CPU lists). If yes, their
> shared_cpu_map is updated.
>=20
> If there is DT/ACPI information, a cacheid/fw_token is associated
> with each cacheinfo struct. This allows to easily check when two
> struct are representing the same cache.
>=20
> Otherwise it is assumed here that L1 caches are private (so not shared)
> and other L2-N caches are shared, i.e. the interface below advertise the
> cache as available from other CPUs.
> /sys/devices/system/cpu/cpu0/cache/indexX/shared_cpu_list

Another silly question:
For two caches of level M & N; M !=3D N; M, N > 1 should they be detected
as shared in the absence of any information in DT/ACPI?
The comment (to me) reads as if they should not, but it is rather vague.

>=20
> >=20
> > Is this patches goal to make sure that if this_leaf is level 2 and
> > sib_leaf is level 1 that these are not detected as shared, since level
> > one caches are meant to be unique?
>=20
> Yes exact.
>=20
> >=20
> > The previous logic checked only this_leaf's level, and declared things
> > shared if this_leaf is not a level 1 cache.
> > What happens here if this_leaf->level =3D=3D 1 and sib_leaf->level =3D=
=3D 2?
> > That'll be detected as shared, in a contradiction of the comment above
> > it, no?
>=20
> Yes, there is a contradiction. The condition should be '&&':
>   (this_leaf->level !=3D 1) && (sib_leaf->level !=3D 1)
> I made a bad rebase and the corrected code ended up in PATCH 3/3.
> Sorry for that. I ll correct it in the v3.

Good to know I am not losing my marbles, I was trying to reconcile the
intent with the patch & without the explicit statement of what was wrong
in the commit message I found it hard!

> Thanks for the review,

nw chief.

--oLQO1TaK6ViWJelZ
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHQEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZDaoawAKCRB4tDGHoIJi
0hyeAPj39Sa1sga0eipXhsdbCWKA9HqpCkFqTOKBXtDdlLi4AQCsogwzFxJOiAZL
ENvb7+be3Jd5sudn9OHPO2TedRCLDg==
=jwas
-----END PGP SIGNATURE-----

--oLQO1TaK6ViWJelZ--