linux-kernel - Re: [PATCH v6 17/21] arch_topology: Limit span of cpu_clustergroup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ysd160PQdWoDIdRC@fedora>
Date:   Thu, 7 Jul 2022 17:10:19 -0700
From:   Darren Hart <darren@...amperecomputing.com>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        conor.dooley@...rochip.com,
        valentina.fernandezalanis@...rochip.com,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Qing Wang <wangqing@...o.com>,
        Rob Herring <robh+dt@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Ionela Voinescu <ionela.voinescu@....com>,
        Pierre Gondois <pierre.gondois@....com>,
        linux-arm-kernel@...ts.infradead.org,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v6 17/21] arch_topology: Limit span of
 cpu_clustergroup_mask()

On Mon, Jul 04, 2022 at 11:16:01AM +0100, Sudeep Holla wrote:
> From: Ionela Voinescu <ionela.voinescu@....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?

> 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.

> 
> While previously the scheduling domain builder code would have removed MC
> as redundant and kept CLS if SCHED_CLUSTER was enabled and the
> cpu_coregroup_mask() and cpu_clustergroup_mask() spanned the same CPUs,
> now CLS will be removed and MC kept.
> 

This is not desireable for all systems, particular those which don't
have an L3 but do share other resources - such as the snoop filter in
the case of the Ampere Altra.

While not universally supported, we agreed in the discussion on the
above patch to allow systems to define clusters independently from the
L3 as an LLC since this is also independently defined in PPTT.

Going back to my first comment - does this fix an existing system with a
valid topology? It's not clear to me what that would look like. The
Ampere Altra presents a cluster level in PPTT because that is the
desireable topology for the system. If it's not desirable for another
system to have the cluster topology - shouldn't it not present that
layer to the kernel in the first place?

Thanks,

> Cc: Darren Hart <darren@...amperecomputing.com>
> Acked-by: Vincent Guittot <vincent.guittot@...aro.org>
> Tested-by: Ionela Voinescu <ionela.voinescu@....com>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@....com>
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> ---
>  drivers/base/arch_topology.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index e384afb6cac7..591c1f8e15e2 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -686,6 +686,14 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
>  
>  const struct cpumask *cpu_clustergroup_mask(int cpu)
>  {
> +	/*
> +	 * Forbid cpu_clustergroup_mask() to span more or the same CPUs as
> +	 * cpu_coregroup_mask().
> +	 */
> +	if (cpumask_subset(cpu_coregroup_mask(cpu),
> +			   &cpu_topology[cpu].cluster_sibling))
> +		return get_cpu_mask(cpu);
> +
>  	return &cpu_topology[cpu].cluster_sibling;
>  }
>  
> -- 
> 2.37.0
> 

-- 
Darren Hart
Ampere Computing / OS and Kernel