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;
Message-ID: <b87130ba-b3c2-825c-0e2c-634e8aee8d5c@arm.com>
Date:   Wed, 12 Apr 2023 15:20:19 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v2 1/3] cacheinfo: Check sib_leaf in
 cache_leaves_are_shared()
Content-Language: en-US
To:     Conor Dooley <conor.dooley@microchip.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>
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>
 <20230412-hut-unused-21d683fcb8b0@wendy>
From:   Pierre Gondois <pierre.gondois@arm.com>
In-Reply-To: <20230412-hut-unused-21d683fcb8b0@wendy>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 4/12/23 14:47, Conor Dooley wrote:
> 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.

I think they should. The naming of cache_leaves_are_shared() might be
misleading. The function is more trying to find out if 2 cache leaves struct
are representing the same cache. So maybe renaming the function to
cache_leaves_identical() might be better ?

In cache_shared_cpu_map_setup(), cache_leaves_are_shared() is used called
on each cache leaf a sibling CPU in order to try to find a matching cache leaf.
It loops until a match is detected.

If there is a DT/ACPI, cache leaves have an id/fw_token allowing to uniquely
identify them, and trying to find a matching leaf makes sense.
If there is no DT/ACPI, it is not possible to identify whether 2 cache leaves
are representing the same cache. The desired behaviour is just:
- If this_leaf or sib_leaf is a L1 cache, then the caches are not identical
   (or shared if we use this wording)
So the meaning of cache_leaves_identical() is a bit bent for this
configuration.

> 
>>
>>>
>>> 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!

Ok, I ll try to add more details in the commit message to be clearer.

Regards,
Pierre

> 
>> Thanks for the review,
> 
> nw chief.