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

[Frederic Weisbecker]

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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>