Messages in this thread | | | Date | Wed, 12 Apr 2023 13:47:39 +0100 | From | Conor Dooley <> | Subject | Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in cache_leaves_are_shared() |
| |
On Wed, Apr 12, 2023 at 02:34:11PM +0200, Pierre Gondois wrote: > Hello Conor, > > 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. > > > > I have to say, I'm a wee bit confused reading this patch - although it's > > likely that I have just confused myself here. > > > > The comment reads "For non DT/ACPI systems, assume unique level 1 caches, > > 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 != N; M, N > 1) > > I think the real answer to your question is in the last paragraph, > but just in case: > > 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. > > 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. > > 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 != 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.
> > > > > 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? > > Yes exact. > > > > > 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 == 1 and sib_leaf->level == 2? > > That'll be detected as shared, in a contradiction of the comment above > > it, no? > > Yes, there is a contradiction. The condition should be '&&': > (this_leaf->level != 1) && (sib_leaf->level != 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. [unhandled content-type:application/pgp-signature] | |