Date:   Wed, 23 Feb 2022 09:19:17 +0100
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Darren Hart <darren@...amperecomputing.com>
Cc:     Will Deacon <will@...nel.org>,
        "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Arm <linux-arm-kernel@...ts.infradead.org>,
        Catalin Marinas <Catalin.Marinas@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Valentin Schneider <Valentin.Schneider@....com>,
        "D . Scott Phillips" <scott@...amperecomputing.com>,
        Ilkka Koskinen <ilkka@...amperecomputing.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] arm64: smp: Skip MC domain for SoCs without shared cache

On Wed, 23 Feb 2022 at 04:08, Darren Hart <darren@...amperecomputing.com> wrote:
>
> On Wed, Feb 16, 2022 at 06:52:29PM +0100, Vincent Guittot wrote:
> > On Wed, 16 Feb 2022 at 17:24, Darren Hart <darren@...amperecomputing.com> wrote:
> > >
> > > On Wed, Feb 16, 2022 at 09:30:49AM +0100, Vincent Guittot wrote:
> > > > On Tue, 15 Feb 2022 at 21:05, Darren Hart <darren@...amperecomputing.com> wrote:
> > > > >
> > > > > On Tue, Feb 15, 2022 at 07:19:45PM +0100, Vincent Guittot wrote:
> > > > > > On Tue, 15 Feb 2022 at 18:32, Darren Hart <darren@...amperecomputing.com> wrote:
> > > > > > >
> > > > > > > On Tue, Feb 15, 2022 at 06:09:08PM +0100, Vincent Guittot wrote:
> > > > > > > > On Tue, 15 Feb 2022 at 17:46, Will Deacon <will@...nel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Feb 15, 2022 at 08:44:23AM -0800, Darren Hart wrote:
> > > > > > > > > > On Tue, Feb 15, 2022 at 04:38:59PM +0000, Will Decon wrote:
> > > > > > > > > > > On Fri, Feb 11, 2022 at 03:20:51AM +0000, Song Bao Hua (Barry Song) wrote:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Darren Hart [mailto:darren@...amperecomputing.com]
> > > > > > > > > > > > > Sent: Friday, February 11, 2022 2:43 PM
> > > > > > > > > > > > > To: LKML <linux-kernel@...r.kernel.org>; Linux Arm
> > > > > > > > > > > > > <linux-arm-kernel@...ts.infradead.org>
> > > > > > > > > > > > > Cc: Catalin Marinas <catalin.marinas@....com>; Will Deacon <will@...nel.org>;
> > > > > > > > > > > > > Peter Zijlstra <peterz@...radead.org>; Vincent Guittot
> > > > > > > > > > > > > <vincent.guittot@...aro.org>; Song Bao Hua (Barry Song)
> > > > > > > > > > > > > <song.bao.hua@...ilicon.com>; Valentin Schneider
> > > > > > > > > > > > > <valentin.schneider@....com>; D . Scott Phillips
> > > > > > > > > > > > > <scott@...amperecomputing.com>; Ilkka Koskinen
> > > > > > > > > > > > > <ilkka@...amperecomputing.com>; stable@...r.kernel.org
> > > > > > > > > > > > > Subject: [PATCH] arm64: smp: Skip MC domain for SoCs without shared cache
> > > > > > > > > > > > >
> > > > > > > > > > > > > SoCs such as the Ampere Altra define clusters but have no shared
> > > > > > > > > > > > > processor-side cache. As of v5.16 with CONFIG_SCHED_CLUSTER and
> > > > > > > > > > > > > CONFIG_SCHED_MC, build_sched_domain() will BUG() with:
> > > > > > > > > > > > >
> > > > > > > > > > > > > BUG: arch topology borken
> > > > > > > > > > > > >      the CLS domain not a subset of the MC domain
> > > > > > > > > > > > >
> > > > > > > > > > > > > for each CPU (160 times for a 2 socket 80 core Altra system). The MC
> > > > > > > > > > > > > level cpu mask is then extended to that of the CLS child, and is later
> > > > > > > > > > > > > removed entirely as redundant.
> > > > > > > > > > > > >
> > > > > > > > > > > > > This change detects when all cpu_coregroup_mask weights=1 and uses an
> > > > > > > > > > > > > alternative sched_domain_topology equivalent to the default if
> > > > > > > > > > > > > CONFIG_SCHED_MC were disabled.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The final resulting sched domain topology is unchanged with or without
> > > > > > > > > > > > > CONFIG_SCHED_CLUSTER, and the BUG is avoided:
> > > > > > > > > > > > >
> > > > > > > > > > > > > For CPU0:
> > > > > > > > > > > > >
> > > > > > > > > > > > > With CLS:
> > > > > > > > > > > > > CLS  [0-1]
> > > > > > > > > > > > > DIE  [0-79]
> > > > > > > > > > > > > NUMA [0-159]
> > > > > > > > > > > > >
> > > > > > > > > > > > > Without CLS:
> > > > > > > > > > > > > DIE  [0-79]
> > > > > > > > > > > > > NUMA [0-159]
> > > > > > > > > > > > >
> > > > > > > > > > > > > Cc: Catalin Marinas <catalin.marinas@....com>
> > > > > > > > > > > > > Cc: Will Deacon <will@...nel.org>
> > > > > > > > > > > > > Cc: Peter Zijlstra <peterz@...radead.org>
> > > > > > > > > > > > > Cc: Vincent Guittot <vincent.guittot@...aro.org>
> > > > > > > > > > > > > Cc: Barry Song <song.bao.hua@...ilicon.com>
> > > > > > > > > > > > > Cc: Valentin Schneider <valentin.schneider@....com>
> > > > > > > > > > > > > Cc: D. Scott Phillips <scott@...amperecomputing.com>
> > > > > > > > > > > > > Cc: Ilkka Koskinen <ilkka@...amperecomputing.com>
> > > > > > > > > > > > > Cc: <stable@...r.kernel.org> # 5.16.x
> > > > > > > > > > > > > Signed-off-by: Darren Hart <darren@...amperecomputing.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Darrent,
> > > > > > > > > > > > What kind of resources are clusters sharing on Ampere Altra?
> > > > > > > > > > > > So on Altra, cpus are not sharing LLC? Each LLC is separate
> > > > > > > > > > > > for each cpu?
> > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  arch/arm64/kernel/smp.c | 32 ++++++++++++++++++++++++++++++++
> > > > > > > > > > > > >  1 file changed, 32 insertions(+)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > > > > > > > > > > > > index 27df5c1e6baa..0a78ac5c8830 100644
> > > > > > > > > > > > > --- a/arch/arm64/kernel/smp.c
> > > > > > > > > > > > > +++ b/arch/arm64/kernel/smp.c
> > > > > > > > > > > > > @@ -715,9 +715,22 @@ void __init smp_init_cpus(void)
> > > > > > > > > > > > >         }
> > > > > > > > > > > > >  }
> > > > > > > > > > > > >
> > > > > > > > > > > > > +static struct sched_domain_topology_level arm64_no_mc_topology[] = {
> > > > > > > > > > > > > +#ifdef CONFIG_SCHED_SMT
> > > > > > > > > > > > > +       { cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
> > > > > > > > > > > > > +#endif
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +#ifdef CONFIG_SCHED_CLUSTER
> > > > > > > > > > > > > +       { cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
> > > > > > > > > > > > > +#endif
> > > > > > > > > > > > > +       { cpu_cpu_mask, SD_INIT_NAME(DIE) },
> > > > > > > > > > > > > +       { NULL, },
> > > > > > > > > > > > > +};
> > > > > > > > > > > > > +
> > > > > > > > > > > > >  void __init smp_prepare_cpus(unsigned int max_cpus)
> > > > > > > > > > > > >  {
> > > > > > > > > > > > >         const struct cpu_operations *ops;
> > > > > > > > > > > > > +       bool use_no_mc_topology = true;
> > > > > > > > > > > > >         int err;
> > > > > > > > > > > > >         unsigned int cpu;
> > > > > > > > > > > > >         unsigned int this_cpu;
> > > > > > > > > > > > > @@ -758,6 +771,25 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> > > > > > > > > > > > >
> > > > > > > > > > > > >                 set_cpu_present(cpu, true);
> > > > > > > > > > > > >                 numa_store_cpu_info(cpu);
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +               /*
> > > > > > > > > > > > > +                * Only use no_mc topology if all cpu_coregroup_mask weights=1
> > > > > > > > > > > > > +                */
> > > > > > > > > > > > > +               if (cpumask_weight(cpu_coregroup_mask(cpu)) > 1)
> > > > > > > > > > > > > +                       use_no_mc_topology = false;
> > > > > > > > > > > >
> > > > > > > > > > > > This seems to be wrong? If you have 5 cpus,
> > > > > > > > > > > > Cpu0 has cpu_coregroup_mask(cpu)== 1, cpu1-4
> > > > > > > > > > > > has cpu_coregroup_mask(cpu)== 4, for cpu0, you still
> > > > > > > > > > > > need to remove MC, but for cpu1-4, you will need
> > > > > > > > > > > > CLS and MC both?
> > > > > > > > > > >
> > > > > > > > > > > What is the *current* behaviour on such a system?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > As I understand it, any system that uses the default topology which has
> > > > > > > > > > a cpus_coregroup weight of 1 and a child (cluster, smt, ...) weight > 1
> > > > > > > > > > will behave as described above by printing the following for each CPU
> > > > > > > > > > matching this criteria:
> > > > > > > > > >
> > > > > > > > > >   BUG: arch topology borken
> > > > > > > > > >         the [CLS,SMT,...] domain not a subset of the MC domain
> > > > > > > > > >
> > > > > > > > > > And then extend the MC domain cpumask to match that of the child and continue
> > > > > > > > > > on.
> > > > > > > > > >
> > > > > > > > > > That would still be the behavior for this type of system after this
> > > > > > > > > > patch is applied.
> > > > > > > > >
> > > > > > > > > That's what I thought, but in that case applying your patch is a net
> > > > > > > > > improvement: systems either get current or better behaviour.
> > > > > > > >
> > > > > > > > CLUSTER level is normally defined as a intermediate group of the MC
> > > > > > > > level and both levels have the scheduler flag SD_SHARE_PKG_RESOURCES
> > > > > > > > flag
> > > > > > > >
> > > > > > > > In the case of Ampere altra, they consider that CPUA have a CLUSTER
> > > > > > > > level which SD_SHARE_PKG_RESOURCES with another CPUB but the next and
> > > > > > > > larger MC level then says that CPUA doesn't SD_SHARE_PKG_RESOURCES
> > > > > > > > with CPUB which seems to be odd because the SD_SHARE_PKG_RESOURCES has
> > > > > > > > not disappeared Looks like there is a mismatch in topology description
> > > > > > >
> > > > > > > Hi Vincent,
> > > > > > >
> > > > > > > Agree. Where do you think this mismatch exists?
> > > > > >
> > > > > > I think that the problem comes from that the default topology order is
> > > > > > assumed to be :
> > > > > > SMT
> > > > > > CLUSTER shares pkg resources i.e. cache
> > > > > > MC
> > > > > > DIE
> > > > > > NUMA
> > > > > >
> > > > > > but in your case, you want a topology order like :
> > > > > > SMT
> > > > > > MC
> > > > > > CLUSTER shares SCU
> > > > > > DIE
> > > > > > NUMA
> > > > >
> > > > > Given the fairly loose definition of some of these domains and the
> > > > > freedom to adjust flags with custom topologies, I think it's difficult
> > > > > to say it needs to be this or that. As you point out, this stems from an
> > > > > assumption in the default topology, so eliding the MC level within the
> > > > > current set of abstractions for a very targeted topology still seems
> > > > > reasonable to address the BUG in the very near term in a contained way.
> > > >
> > > > But if another SoC comes with a valid MC then a CLUSTER, this proposal
> > > > will not work.
> > > >
> > > > Keep in mind that the MC level will be removed/degenerate when
> > > > building because it is useless in your case so the scheduler topology
> > > > will still be the same at the end but it will support more case. That
> > > > why I think you should keep MC level
> > >
> > > Hi Vincent,
> > >
> > > Thanks for reiterating, I don't think I quite understood what you were
> > > suggesting before. Is the following in line with what you were thinking?
> > >
> > > I am testing a version of this patch which uses a topology like this instead:
> > >
> > > MC
> > > CLS
> > > DIE
> > > NUMA
> > >
> > > (I tested without an SMT domain since the trigger is still MC weight==1, so
> > > there is no valid topology that includes an SMT level under these conditions).
> > >
> > > Which results in no BUG output and a final topology on Altra of:
> > >
> > > CLS
> > > DIE
> > > NUMA
> > >
> > > Which so far seems right (I still need to do some testing and review the sched
> > > debug data).
> > >
> > > If we take this approach, I think to address your concern about other systems
> > > with valid MCs, we would need a different trigger that MC weight == 1 to use
> > > this alternate topology. Do you have a suggestion on what to trigger this on?
> >
> > AFAICT, this CLUSTER level is only supported by ACPI. In
> > parse_acpi_topology() you should be able to know if cluster level is
> > above or below the level returned by acpi_find_last_cache_level() and
> > set the correct topology table accordingly
> >
>
> Thanks Vincent,
>
> This made sense as a place to start to me. The more I dug into the ACPI PPTT
> code, I kept running into conflicts with the API which would require extending
> it in ways that seems contrary to its intent. e.g. the exposed API uses Kernel
> logical CPUs rather than the topology ids (needed for working with processor
> containers). The cpu_topology masks haven't been populated yet, and
> acpi_find_last_cache_level is decoupled from the CPU topology level. So what
> we're really testing for is if the cluster cpumask is a subset of the coregroup
> cpumask or not, and it made the most sense to me to keep that in smp.c after the
> cpumasks have been updated and stored.

I'm not sure why you want to compare cpumask when you can directly
compare topology level which is exactly what we are looking for in
order to correctly order the scheduler topology. I was expecting
something like the below to be enough. acpi_find_cluster_level() needs
to be created and should be similar to
find_acpi_cpu_topology_cluster() but return level instead of id. The
main advantage is that everything is contained in topology.c which
makes sense as we are playing with topology

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 9ab78ad826e2..4dac0491b7e3 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -84,6 +84,7 @@ static bool __init acpi_cpu_is_threaded(int cpu)
 int __init parse_acpi_topology(void)
 {
        int cpu, topology_id;
+       bool default_cluster_topology = true;

        if (acpi_disabled)
                return 0;
@@ -119,8 +120,16 @@ int __init parse_acpi_topology(void)
                        if (cache_id > 0)
                                cpu_topology[cpu].llc_id = cache_id;
                }
+
+               if (default_cluster_topology &&
+                   (i < acpi_find_cluster_level(cpu))) {
+                       default_cluster_topology = false;
+               }
        }

+       if (!default_cluster_topology)
+               set_sched_topology(arm64_no_mc_topology);
+
        return 0;
 }
 #endif

>
> I'll send v2 out for review shortly using this approach.

Ok.

>
> --
> Darren Hart
> Ampere Computing / OS and Kernel

Powered by blists - more mailing lists