Re: [PATCH v3] topology: make core_mask include at least cluster_siblings

From: Dietmar Eggemann <dietmar.eggemann@arm.com>
To: Darren Hart <darren@os.amperecomputing.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>,
	Will Deacon <will@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux Arm <linux-arm-kernel@lists.infradead.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Valentin Schneider <Valentin.Schneider@arm.com>,
	"D . Scott Phillips" <scott@os.amperecomputing.com>,
	Ilkka Koskinen <ilkka@os.amperecomputing.com>,
	stable@vger.kernel.org, Barry Song <21cnbao@gmail.com>
Subject: Re: [PATCH v3] topology: make core_mask include at least cluster_siblings
Date: Wed, 16 Mar 2022 15:48:50 +0100	[thread overview]
Message-ID: <eb33745a-9d63-89b1-1245-9d1e0e04a169@arm.com> (raw)
In-Reply-To: <Yi9zUuroS1vHWexY@fedora>

- Barry Song <song.bao.hua@hisilicon.com> (always get undelivered mail
  returned to sender)
+ Barry Song <21cnbao@gmail.com>

On 14/03/2022 17:54, Darren Hart wrote:
> On Mon, Mar 14, 2022 at 05:35:05PM +0100, Dietmar Eggemann wrote:
>> On 09/03/2022 19:26, Darren Hart wrote:
>>> On Wed, Mar 09, 2022 at 01:50:07PM +0100, Dietmar Eggemann wrote:
>>>> On 08/03/2022 18:49, Darren Hart wrote:
>>>>> On Tue, Mar 08, 2022 at 05:03:07PM +0100, Dietmar Eggemann wrote:
>>>>>> On 08/03/2022 12:04, Vincent Guittot wrote:
>>>>>>> On Tue, 8 Mar 2022 at 11:30, Will Deacon <will@kernel.org> wrote:

[...]

> Ultimately, this delivers the same result. I do think it imposes more complexity
> for everyone to address what as far as I'm aware only affect the one system.
> 
> I don't think the term "Cluster" has a clear and universally understood
> definition, so I don't think it's a given that "CLS should be sub-SD of MC". I

I agree, the term 'cluster' is overloaded but default_topology[] clearly
says (with direction up means smaller SD spans).

  #ifdef CONFIG_SCHED_CLUSTER
        { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
  #endif

  #ifdef CONFIG_SCHED_MC
        { cpu_coregroup_mask, cpu_core_flags, SD_INIT_NAME(MC) },
  #endif

In ACPI code we have `cluster_node = fetch_pptt_node(... ,
cpu_node->parent) but then the cache information (via
llc_id/llc_sibling) can change things which make this less easy to grasp.

> think this has been assumed, and that assumption has mostly held up, but this is
> an abstraction, and the abstraction should follow the physical topologies rather
> than the other way around in my opinion. If that's the primary motivation for
> this approach, I don't think it justifies the additional complexity.
> 
> All told, I prefer the 2 line change contained within cpu_coregroup_mask() which
> handles the one known exception with minimal impact. It's easy enough to come
> back to this to address more cases with a more complex solution if needed in the
> future - but I prefer to introduce the least amount of complexity as possible to
> address the known issues, especially if the end result is the same and the cost
> is paid by the affected systems.
> 
> Thanks,

Yeah, I can see your point. It's the smaller hack. My solution just
prevents us to manipulate the coregroup mask only to get the MC layer
degenerated by the core topology code. But people might say that's a
clever thing to do here. So I'm fine with your original solution as well.

[...]