lkml.org 
[lkml]   [2022]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2022-07-08 18:16    [W:2.596 / U:0.420 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site