linux-kernel - Re: [PATCH] sched/topology: Disable sched_asym

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: <20191014121648.GA53234@google.com>
Date:   Mon, 14 Oct 2019 13:16:48 +0100
From:   Quentin Perret <qperret@...gle.com>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     linux-kernel@...r.kernel.org, mingo@...nel.org,
        peterz@...radead.org, vincent.guittot@...aro.org,
        Dietmar.Eggemann@....com, morten.rasmussen@....com,
        qperret@...rret.net, stable@...r.kernel.org
Subject: Re: [PATCH] sched/topology: Disable sched_asym_cpucapacity on domain
 destruction

Hi Valentin,

On Monday 14 Oct 2019 at 12:47:10 (+0100), Valentin Schneider wrote:
> While the static key is correctly initialized as being disabled, it will
> remain forever enabled once turned on. This means that if we start with an
> asymmetric system and hotplug out enough CPUs to end up with an SMP system,
> the static key will remain set - which is obviously wrong. We should detect
> this and turn off things like misfit migration and EAS wakeups.

FWIW we already clear the EAS static key properly (based on the sd
pointer, not the static key), so this is really only for the
capacity-aware stuff.

> Having that key enabled should also mandate
> 
>   per_cpu(sd_asym_cpucapacity, cpu) != NULL
> 
> for all CPUs, but this is obviously not true with the above.
> 
> On top of that, sched domain rebuilds first lead to attaching the NULL
> domain to the affected CPUs, which means there will be a window where the
> static key is set but the sd_asym_cpucapacity shortcut points to NULL even
> if asymmetry hasn't been hotplugged out.
> 
> Disable the static key when destroying domains, and let
> build_sched_domains() (re) enable it as needed.
> 
> Cc: <stable@...r.kernel.org>
> Fixes: df054e8445a4 ("sched/topology: Add static_key for asymmetric CPU capacity optimizations")
> Signed-off-by: Valentin Schneider <valentin.schneider@....com>
> ---
>  kernel/sched/topology.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index b5667a273bf6..c49ae57a0611 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2123,7 +2123,8 @@ static void detach_destroy_domains(const struct cpumask *cpu_map)
>  {
>  	int i;
>  
> +	static_branch_disable_cpuslocked(&sched_asym_cpucapacity);
> +
>  	rcu_read_lock();
>  	for_each_cpu(i, cpu_map)
>  		cpu_attach_domain(NULL, &def_root_domain, i);

So what happens it you have mutiple root domains ? You might skip
build_sched_domains() for one of them and end up not setting the static
key when you should no ?

I suppose an alternative would be to play with static_branch_inc() /
static_branch_dec() from build_sched_domains() or something along those
lines.

Thanks,
Quentin