Re: [PATCH] cpu/hotplug: ensure the starting section runs fully regardless of target

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Dec 09 2024 at 13:19, Koichiro Den wrote:
> Now I'm wondering whether we should go further..
> Because in PREPARE section, things are not fully symmetric, so
> there is a problem like an example below:
>
>     E.g.
>
>     (1) writing <some state in between> into 'target' and then (2) bringing
>     the the cpu fully online again by writing a large value leaves
>     hrtimer_cpu_base's 'online' field at 0 because hrtimers:prepare does not
>     re-run.
>
>            - hrtimers:prepare (CPUHP_HRTIMERS_PREPARE)
>            - ...
>     ------ - <some state in between>
>      ^  :  - ...
>      :  :  - hrtimers:dying (CPUHP_AP_HRTIMERS_DYING)
>     (1)(2)
>      :  :
>      :  v

That got broken by me with commit:

  5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier")

and want's to be fixed.

> While I understand this is still a debug option, it seems to me that there are
> several approaches to consider here. I'm inclined toward (a).
>
> (a). prohibit writing all halfway states in PREPARE+STARTING sections, i.e.
>
>      --- a/kernel/cpu.c
>      +++ b/kernel/cpu.c
>      @@ -2759,7 +2759,8 @@ static ssize_t target_store(struct device *dev,
>                      return ret;
>      
>       #ifdef CONFIG_CPU_HOTPLUG_STATE_CONTROL
>      -       if (target < CPUHP_OFFLINE || target > CPUHP_ONLINE)
>      +       if (target != CPUHP_OFFLINE || target > CPUHP_ONLINE ||
>      +           target < CPUHP_AP_OFFLINE_IDLE)
>                      return -EINVAL;
>       #else
>              if (target != CPUHP_OFFLINE && target != CPUHP_ONLINE)

That's lame.

> (b). make all fully symmetric. (I'm not sure whether it could be possible)

There is no requirement to make everything symmetric as there are
prepare steps which just allocate memory and do some basic
initialization. So I rather go through the steps and keep the ability to
invoke them fine granular in all directions (except for the atomic AP
states which started this discussion.

> (c). add all safety catch at sysfs interface. (For the above example, once
>      it goes down to <some state in between>, disallow to go up without
>      once going down to a state earlier than hrtimers:prepare beforehand.
>      I guess this would mess up source code unnecessarily though.)

That's unmaintainable.

Thanks,

        tglx