linux-kernel - Re: [PATCH v4 2/8] timers/migration: Move hierarchy setup into cpuhotplug prepare callback

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: <Zpb7vQqarZZqvtCo@pavilion.home>
Date: Wed, 17 Jul 2024 01:01:17 +0200
From: Frederic Weisbecker <frederic@...nel.org>
To: Anna-Maria Behnsen <anna-maria@...utronix.de>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/8] timers/migration: Move hierarchy setup into
 cpuhotplug prepare callback

Le Tue, Jul 16, 2024 at 04:19:20PM +0200, Anna-Maria Behnsen a écrit :
> However if the active CPU prevents from tmigr_cpu_(in)active() to walk up
> with the update not-or-half visible, nothing prevents walking up to the new
> top with a 0 childmask in tmigr_handle_remote_up() or
> tmigr_requires_handle_remote_up() if the active CPU doing the prepare is
> not the migrator. But then it looks fine because:
> 
>   * tmigr_check_migrator() should just return false
>   * The migrator is active and should eventually observe the new childmask
>     at some point in a future tick.
> 
> Split setup functionality of online callback into the cpuhotplug prepare
> callback and setup hotplug state. Change init call into early_initcall() to
> make sure boot CPU prepares everything for upcoming CPUs.

Not sure this will always be the boot CPU doing the prepare. But I think
the important part is that the target CPU must never do the prepare work.
Otherwise it may activate the old root to the new root even if the old root
is idle.

> Reorder the code,
> that all prepare related functions are close to each other and online and
> offline callbacks are also close together.
> 
> Fixes: 7ee988770326 ("timers: Implement the hierarchical pull model")
> Signed-off-by: Anna-Maria Behnsen <anna-maria@...utronix.de>
> ---
>  include/linux/cpuhotplug.h    |   1 +
>  kernel/time/timer_migration.c | 196 +++++++++++++++++++++++-------------------
>  2 files changed, 110 insertions(+), 87 deletions(-)
> 
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 7a5785f405b6..df59666a2a66 100644
> @@ -1623,7 +1689,7 @@ static int tmigr_setup_groups(unsigned int cpu, unsigned int node)
>  			continue;
>  		} else {
>  			child = stack[i - 1];
> -			tmigr_connect_child_parent(child, group);
> +			tmigr_connect_child_parent(child, group, false);

This may need a small comment: /* Will be activated at online time */

>  		}
>  
>  		/* check if uppermost level was newly created */
[...]
> -/*
> - * tmigr_trigger_active() - trigger a CPU to become active again
> - *
> - * This function is executed on a CPU which is part of cpu_online_mask, when the
> - * last active CPU in the hierarchy is offlining. With this, it is ensured that
> - * the other CPU is active and takes over the migrator duty.
> - */
> -static long tmigr_trigger_active(void *unused)
> +static int tmigr_cpu_prepare(unsigned int cpu)
>  {
> -	struct tmigr_cpu *tmc = this_cpu_ptr(&tmigr_cpu);
> +	struct tmigr_cpu *tmc = per_cpu_ptr(&tmigr_cpu, cpu);
> +	int ret = 0;
>

It would be nice to have this warning here (or in tmigr_setup_group()):

/*
 * The target CPU must never do the prepare work. Otherwise
 * it may spuriously activate the old root to the new one
 * and/or release an uninitialized childmask.
 */
WARN_ON_ONCE(cpu == raw_smp_processor_id());

And in any case:

Reviewed-by: Frederic Weisbecker <frederic@...nel.org>