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

[Frederic Weisbecker <frederic@kernel.org>]

Le Mon, Jul 01, 2024 at 12:18:38PM +0200, Anna-Maria Behnsen a écrit :
> To prevent those races, the setup of the hierarchy is moved into the
> cpuhotplug prepare callback. The prepare callback is not executed by the
> CPU which will come online, it is executed by the CPU which prepares
> onlining of the other CPU. This CPU will not go idle and so it is ensured,
> that the formerly top level group cannot go idle and change top level group
> state and the propagation could be done without a risk. The direction for
> the updates is now forced to look like "from bottom to top".

You might want to elaborate a bit on those last two sentences. For example:

"""
This CPU is active while it is connecting the formerly top level to the new
one. This prevents from (A) to happen and it also prevents from any further walk
above the formerly top level until that active CPU becomes inactive, releasing
the new ->parent and ->childmask updates to be visible by any subsequent walk
up above the formerly top level hierarchy. This prevents from (B) to happen.
"""

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.

But still it can be a good idea to mention that somewhere.

> @@ -1540,22 +1600,22 @@ static void tmigr_connect_child_parent(struct tmigr_group *child,
>  	 *   child to the new parent. So tmigr_connect_child_parent() is
>  	 *   executed with the formerly top level group (child) and the newly
>  	 *   created group (parent).
> +	 *
> +	 * * It is ensured, that the child is active, as this setup path is

First comma is not needed.

> +	 *   executed in hotplug prepare callback. This is exectued by an
> +	 *   already connected and !idle CPU. Even if all other CPUs go idle,
> +	 *   the CPU executing the setup will be responsible up to current top
> +	 *   level group.

And the next time it goes inactive, it will release the new childmask and parent
to subsequent walkers through this @child.

> +       * Therefore propagate active state unconditionally.

As for the change itself:

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

Thanks.