From: Darren Hart <darren@os.amperecomputing.com> To: Dietmar Eggemann <dietmar.eggemann@arm.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 08:20:32 -0700 [thread overview] Message-ID: <YjIAQKwfa3/vr/kU@fedora> (raw) In-Reply-To: <eb33745a-9d63-89b1-1245-9d1e0e04a169@arm.com> On Wed, Mar 16, 2022 at 03:48:50PM +0100, Dietmar Eggemann wrote: > - 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 > Right, understood. It is a clear expectation of the current Sched Domain topology abstraction. > 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. > > [...] Thanks Dietmar, Sudeep, do we have sufficient consensus to pull in this patch? Thanks, -- Darren Hart Ampere Computing / OS and Kernel
WARNING: multiple messages have this Message-ID (diff)
From: Darren Hart <darren@os.amperecomputing.com> To: Dietmar Eggemann <dietmar.eggemann@arm.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 08:20:32 -0700 [thread overview] Message-ID: <YjIAQKwfa3/vr/kU@fedora> (raw) In-Reply-To: <eb33745a-9d63-89b1-1245-9d1e0e04a169@arm.com> On Wed, Mar 16, 2022 at 03:48:50PM +0100, Dietmar Eggemann wrote: > - 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 > Right, understood. It is a clear expectation of the current Sched Domain topology abstraction. > 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. > > [...] Thanks Dietmar, Sudeep, do we have sufficient consensus to pull in this patch? Thanks, -- Darren Hart Ampere Computing / OS and Kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-16 15:20 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-04 17:01 [PATCH v3] topology: make core_mask include at least cluster_siblings Darren Hart 2022-03-04 17:01 ` Darren Hart 2022-03-08 10:30 ` Will Deacon 2022-03-08 10:30 ` Will Deacon 2022-03-08 10:45 ` Sudeep Holla 2022-03-08 10:45 ` Sudeep Holla 2022-03-08 11:04 ` Vincent Guittot 2022-03-08 11:04 ` Vincent Guittot 2022-03-08 16:03 ` Dietmar Eggemann 2022-03-08 16:03 ` Dietmar Eggemann 2022-03-08 17:49 ` Darren Hart 2022-03-08 17:49 ` Darren Hart 2022-03-09 12:50 ` Dietmar Eggemann 2022-03-09 12:50 ` Dietmar Eggemann 2022-03-09 18:26 ` Darren Hart 2022-03-09 18:26 ` Darren Hart 2022-03-14 9:37 ` Dietmar Eggemann 2022-03-14 9:37 ` Dietmar Eggemann 2022-03-14 16:56 ` Darren Hart 2022-03-14 16:56 ` Darren Hart 2022-03-16 14:42 ` Dietmar Eggemann 2022-03-16 14:42 ` Dietmar Eggemann 2022-03-14 16:35 ` Dietmar Eggemann 2022-03-14 16:35 ` Dietmar Eggemann 2022-03-14 16:54 ` Darren Hart 2022-03-14 16:54 ` Darren Hart 2022-03-16 14:48 ` Dietmar Eggemann 2022-03-16 14:48 ` Dietmar Eggemann 2022-03-16 15:20 ` Darren Hart [this message] 2022-03-16 15:20 ` Darren Hart 2022-03-16 15:55 ` Sudeep Holla 2022-03-16 15:55 ` Sudeep Holla 2022-03-21 14:30 ` Will Deacon 2022-03-21 14:30 ` Will Deacon 2022-03-21 15:56 ` Greg Kroah-Hartman 2022-03-21 15:56 ` Greg Kroah-Hartman 2022-03-14 21:29 ` [PATCH] arch_topology: Swap MC & CLS SD mask if MC weight==1 & kernel test robot 2022-03-14 21:29 ` kernel test robot 2022-03-14 23:02 ` kernel test robot 2022-03-14 23:02 ` kernel test robot 2022-03-17 6:10 ` [PATCH v3] topology: make core_mask include at least cluster_siblings Barry Song 2022-03-17 6:10 ` Barry Song
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YjIAQKwfa3/vr/kU@fedora \ --to=darren@os.amperecomputing.com \ --cc=21cnbao@gmail.com \ --cc=Catalin.Marinas@arm.com \ --cc=Valentin.Schneider@arm.com \ --cc=dietmar.eggemann@arm.com \ --cc=gregkh@linuxfoundation.org \ --cc=ilkka@os.amperecomputing.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=peterz@infradead.org \ --cc=rafael@kernel.org \ --cc=scott@os.amperecomputing.com \ --cc=stable@vger.kernel.org \ --cc=sudeep.holla@arm.com \ --cc=vincent.guittot@linaro.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.