From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C15CFC77B6E for ; Wed, 12 Apr 2023 13:20:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229671AbjDLNUy (ORCPT ); Wed, 12 Apr 2023 09:20:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46990 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229817AbjDLNUr (ORCPT ); Wed, 12 Apr 2023 09:20:47 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 8D2AE8682 for ; Wed, 12 Apr 2023 06:20:27 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0E656D75; Wed, 12 Apr 2023 06:21:10 -0700 (PDT) Received: from [10.34.100.129] (pierre123.nice.arm.com [10.34.100.129]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0D4723F73F; Wed, 12 Apr 2023 06:20:23 -0700 (PDT) Message-ID: 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 Cc: linux-kernel@vger.kernel.org, Radu Rendec , Alexandre Ghiti , Will Deacon , Greg Kroah-Hartman , "Rafael J. Wysocki" , Sudeep Holla , Palmer Dabbelt , Gavin Shan , Jeremy Linton References: <20230412071809.12670-1-pierre.gondois@arm.com> <20230412071809.12670-2-pierre.gondois@arm.com> <20230412-viewpoint-refutable-a31f3657093c@wendy> <20230412-hut-unused-21d683fcb8b0@wendy> From: Pierre Gondois In-Reply-To: <20230412-hut-unused-21d683fcb8b0@wendy> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.