linux-kernel - Re: [PATCH v3 2/3] sched/topology: Rework CPU capacity asymmetry detection

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: <YJ0H5+P4uHJVh5qF@hirez.programming.kicks-ass.net>
Date:   Thu, 13 May 2021 13:05:11 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Beata Michalska <beata.michalska@....com>
Cc:     linux-kernel@...r.kernel.org, mingo@...hat.com,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        valentin.schneider@....com, dietmar.eggemann@....com,
        corbet@....net, rdunlap@...radead.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 2/3] sched/topology: Rework CPU capacity asymmetry
 detection

On Mon, May 10, 2021 at 05:30:57PM +0100, Beata Michalska wrote:
> Currently the CPU capacity asymmetry detection, performed through
> asym_cpu_capacity_level, tries to identify the lowest topology level
> at which the highest CPU capacity is being observed, not necessarily
> finding the level at which all possible capacity values are visible
> to all CPUs, which might be bit problematic for some possible/valid
> asymmetric topologies i.e.:
> 
> DIE      [                                ]
> MC       [                       ][       ]
> 
> CPU       [0] [1] [2] [3] [4] [5]  [6] [7]
> Capacity  |.....| |.....| |.....|  |.....|
> 	     L	     M       B        B
> 
> Where:
>  arch_scale_cpu_capacity(L) = 512
>  arch_scale_cpu_capacity(M) = 871
>  arch_scale_cpu_capacity(B) = 1024

Low, High
Small, Big

But you appear to have picked: Low and Big, which just doesn't make any
sense. (Worse, L can also be Large :-)

> In this particular case, the asymmetric topology level will point
> at MC, as all possible CPU  masks for that level do cover the CPU
> with the highest capacity. It will work just fine for the first
> cluster, not so much for the second one though (consider the
> find_energy_efficient_cpu which might end up attempting the energy
> aware wake-up for a domain that does not see any asymmetry at all)
> 
> Rework the way the capacity asymmetry levels are being detected,
> to point to the lowest topology level( for a given CPU), where full
> range of available CPU capacities is visible to all CPUs within given
> domain. As a result, the per-cpu sd_asym_cpucapacity might differ
> across the domains. This will have an impact on EAS wake-up placement
> in a way that it might see different rage of CPUs to be considered,
> depending on the given current and target CPUs.
> 
> Additionally, those levels, where any range of asymmetry (not
> necessarily full) is being detected will get identified as well.
> The selected asymmetric topology level will be denoted by
> SD_ASYM_CPUCAPACITY_FULL sched domain flag whereas the 'sub-levels'
> would receive the already used SD_ASYM_CPUCAPACITY flag. This allows
> maintaining the current behaviour for asymmetric topologies, with
> misfit migration operating correctly on lower levels, if applicable,
> as any asymmetry is enough to trigger the misfit migration.
> The logic there relies on the SD_ASYM_CPUCAPACITY flag and does not
> relate to the full asymmetry level denoted by the sd_asym_cpucapacity
> pointer.

My head hurts trying to untangle this.

Would it not be much easier to have a cpumask for each type and then
have something like:


enum asym_type {
	full,
	asym,
	none,
};

static struct cpumask *asym_masks[];
static int asym_nr;

enum asym_type classify_asym_domain(struct sched_domain *sd)
{
	int i, n = 0;

	for (i = 0; i < asym_nr; i++) {
		if (cpumask_intersects(sched_domain_span(sd), asym_mask[i]))
			n++;
	}

	WARN_ON_ONCE(!n);

	if (n == 1)
		return none;
	if (n == asym_nr)
		return full;
	return asym;
}

Or am I missing the point?