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;
Date:   Fri, 8 Jul 2022 09:14:59 -0700
From:   Darren Hart <darren@os.amperecomputing.com>
To:     Ionela Voinescu <ionela.voinescu@arm.com>
Cc:     Sudeep Holla <sudeep.holla@arm.com>, linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        conor.dooley@microchip.com,
        valentina.fernandezalanis@microchip.com,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Qing Wang <wangqing@vivo.com>,
        Rob Herring <robh+dt@kernel.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Pierre Gondois <pierre.gondois@arm.com>,
        linux-arm-kernel@lists.infradead.org,
        linux-riscv@lists.infradead.org
Subject: Re: [PATCH v6 17/21] arch_topology: Limit span of
 cpu_clustergroup_mask()
Message-ID: <YshYAyEWhE4z/KpB@fedora>
References: <20220704101605.1318280-1-sudeep.holla@arm.com>
 <20220704101605.1318280-18-sudeep.holla@arm.com>
 <Ysd160PQdWoDIdRC@fedora>
 <YsfzXL7fp3sYVgda@arm.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YsfzXL7fp3sYVgda@arm.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?W35dQ7Bm1cnJ1lkD8cHVaDsI5fipYZZt178zroLzATSHiuf5+2hAAn+OBPXg?=
 =?us-ascii?Q?b6pPlFlPQyv9OIuiOQE3RyZs/1lShwXmbf08mdl6UQOQYOYXxWzYLf3kes4y?=
 =?us-ascii?Q?ErKQnwRd7lqVeWJI+Zmp1Y58MspLX/ozAZ8a8hnCenHXmAVpPL/Yem7Q8t8P?=
 =?us-ascii?Q?59k+Tj6N3IQOd7bBlE5Qk4OsahYhi9C/UTrlsd3s821fvKUkGR9vTJyFBrEq?=
 =?us-ascii?Q?2se3CEA2qmFVIe+omNePZeuKghHrmKTUGdokx8LfjEWLtZImMyXX4uwoVWjk?=
 =?us-ascii?Q?9VXXgJF4p1gxNcifRNpMn56ZuK/mEzrTi4pcvICk7IrCQ3tq6fSiPA4tX2Kj?=
 =?us-ascii?Q?erFHq4MZoW965CX4A821KwymuHMYRF26dkljV4e9pYgG7Ak6RuPvgiobbbMB?=
 =?us-ascii?Q?gbMvhXkDEjbGDE3P4KDfXOs7SG4jRu6B6noEDSgQXC5gm5mOTtFNXyJY5p3p?=
 =?us-ascii?Q?gKoojjlQLEKVMDc3Hn+WLVa5QNdmRL36k343mwKYSJOf8c4EuFt9UBm62Y1n?=
 =?us-ascii?Q?5nRmLp2HxPoPnI2aGDrdvnEhDbfPm6e7dVXzz43TBWdxfwcZOwe5RGETJYNW?=
 =?us-ascii?Q?VVWyp8QBPxO6DWLxJPM8EcdEQpNdTPQJYvrS5kgtokwL27HXkDZxQciY/ixq?=
 =?us-ascii?Q?aMVg2vGfWePPu510jlKXcmcyqc0EKPMhHHvfus8afsyIVSVJjX5dJUkNiNl7?=
 =?us-ascii?Q?/abMChGa5GnN5T0VqSVZzYvGKzdInNvBTImC6anbLO0QRu0mPGKKdgXbXbHE?=
 =?us-ascii?Q?BG2vGD3NXdJ+afcpQXKJrcFmgJHCYNFNW8zLLZBjJ2zLMG5C0dmjoXl0YUD8?=
 =?us-ascii?Q?bOAo+tK+ukdguSFA8KembjkJOmCtU3Zvfdl7962yoXnMIjhYlk32i/yjFzLD?=
 =?us-ascii?Q?VOs1x3YSDdE8JoP+LNpHY2+F2Ji5G1B0j7CmNiH5Vf4E1BMiXZENOC3vj4yp?=
 =?us-ascii?Q?FyAQMIbjYl0DPyfeEevRTyp9neOBRdl9X4ROD4bHqFYC2HrH14TOyEUN0cPV?=
 =?us-ascii?Q?L2o9LtFBb4WCic7+il38ZyGPXZBhdAsJhO2UHjXkDA40MjwbftUI5aZdaQgi?=
 =?us-ascii?Q?Um8lMBJuONZKLOXbJz8arfsYjsqO4Ww8aO+KXdgS9XafT3FdiKP0a1/3yP3Y?=
 =?us-ascii?Q?BODb761+hnHwJaqDJpi9qkt4fSo7KfZhZiQ1nF3obeOeDlMcHUEPKe/i+dJs?=
 =?us-ascii?Q?yEp5VsK3p/aPawaMMwkPiCMNtL8xlEZM/Nbzfsrf/W1VBvXX17QPsQZkZP4F?=
 =?us-ascii?Q?/cnU1IpxKvpCbl+Q2ALQWoDGfrk6IupVZ8mQOfoK0qTbrhHNsNJxxMeofgOo?=
 =?us-ascii?Q?pg2pV7EUDrRCWNxqVk6Q5D2/Jcee5acYq+tOZ0NYJrszd90tiKF2gQ9x+/7y?=
 =?us-ascii?Q?rcKss//rm5g0QZBxbO0M+KKA5YTaOOiE8FJSD4qV1MqRA+p+H6Cxh6B5vkt0?=
 =?us-ascii?Q?+mtPJnMvvFBSU/FNM3Uuuds35PnKAUmRp3r1mV88U9/w7Do8vHLkZBhbvaIu?=
 =?us-ascii?Q?Um4Vh3QGdtcSkzbluSFUEt8Y1Boz2qtISR5e3U1LmPVsw3VD/nTcs5h3LexV?=
 =?us-ascii?Q?rnhCdSYjj89LRMiITuaOo9BoPSEDFm2BdO4a76dHWD6mGB78V2Y2Bdo5GYUb?=
 =?us-ascii?Q?8v6iGTcZzpK65RFg+297ILk=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c8b32f8f-aed6-4d9d-1030-08da60fd0264
X-MS-Exchange-CrossTenant-AuthSource: MWHPR01MB2638.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jul 2022 16:15:01.2107
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kPmffw12xws8NcE3/7sN5virnA2muCNqa+fnhKmWBECsgS1MvqmiFOZgNZVb+h+OUKdgCoRcw6L1s8zPdzsxvNyhDxSjO7VdHvQ6H3pg62y6lOfVSrg8IP6mFZGDu7o3
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR01MB2722
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Jul 08, 2022 at 10:05:32AM +0100, Ionela Voinescu wrote:
> Hi Darren,
> 
> On Thursday 07 Jul 2022 at 17:10:19 (-0700), Darren Hart wrote:
> > On Mon, Jul 04, 2022 at 11:16:01AM +0100, Sudeep Holla wrote:
> > > From: Ionela Voinescu <ionela.voinescu@arm.com>
> > 
> > Hi Sudeep and Ionela,
> > 
> > > 
> > > Currently the cluster identifier is not set on DT based platforms.
> > > The reset or default value is -1 for all the CPUs. Once we assign the
> > > cluster identifier values correctly, the cluster_sibling mask will be
> > > populated and returned by cpu_clustergroup_mask() to contribute in the
> > > creation of the CLS scheduling domain level, if SCHED_CLUSTER is
> > > enabled.
> > > 
> > > To avoid topologies that will result in questionable or incorrect
> > > scheduling domains, impose restrictions regarding the span of clusters,
> > 
> > Can you provide a specific example of a valid topology that results in
> > the wrong thing currently?
> > 
> 
> When CONFIG_SCHED_CLUSTER=y, all typical big.LITTLE platforms will end up
> having a CLS level instead of MC, with an extra flag for the CLS level:
> SD_PREFER_SIBLING. Additional to this, potentially broken cluster
> descriptions in DT (let's say clusters spanning more CPUs than the LLC
> domain) will result in broken scheduler topologies.

You addressed my primary concern below, thank you. Re this point, I was
concerned that we were prioritizing correcting "broken cluster
descriptions" over "correct, but unusual cluster descriptions". Your
solutions seems to elegantly address both.

> 
> This drew our attention that the span of clusters should be restricted
> to ensure they always span less CPUs than LLC, if LLC information exists
> and LLC spans more than 1 core. But the Ampere Altra functionality you
> introduced is maintained. I'll detail this below.
> 
> > > as presented to scheduling domains building code: cluster_sibling should
> > > not span more or the same CPUs as cpu_coregroup_mask().
> > > 
> > > This is needed in order to obtain a strict separation between the MC and
> > > CLS levels, and maintain the same domains for existing platforms in
> > > the presence of CONFIG_SCHED_CLUSTER, where the new cluster information
> > > is redundant and irrelevant for the scheduler.
> > 
> > Unfortunately, I believe this changes the behavior for the existing
> > Ampere Altra systems, resulting in degraded performance particularly
> > latency sensitive workloads by effectively reverting:
> > 
> >   db1e59483d topology: make core_mask include at least cluster_siblings
> > 
> > and ensuring the clustergroup_mask will return with just one CPU for the
> > condition the above commit addresses.
> > 
> 
> It does not change the functionality on Ampere Altra. cpu_coregroup_mask
> will still return 2 CPUs (cluster span). The difference is that
> cpu_clustergroup_mask will see that cpu_coregroup_masks spans the same
> CPUs and it will return a single CPU. This results in the CLS level
> being invalidated, and the MC level maintained. But MC will span 2 CPUs,
> instead of 1, which was the case before your fix. This is alright as
> MC and CLS have the same flags so the existing functionality is fully
> maintained.

Ah, of course. I missed the combined impact of my earlier change plus
yours, which is to first expand MC and then to collapse CLS. It's a
little round about for the Altra, but that seems reasonable as it's a
bit of a corner case in terms topologies.

Thank you for the explanation.

-- 
Darren Hart
Ampere Computing / OS and Kernel