Messages in this thread | | | Date | Fri, 8 Jul 2022 09:14:59 -0700 | From | Darren Hart <> | Subject | Re: [PATCH v6 17/21] arch_topology: Limit span of cpu_clustergroup_mask() |
| |
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
| |