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

[Koichiro Den]

On Mon, Dec 09, 2024 at 12:51:59PM +0100, Thomas Gleixner wrote:
> 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.

I see, so let me address it while I'm at it.

> 
> > 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.

Hmm, could be. (By the way this was incorrect and I've fixed it here:
https://lore.kernel.org/all/20241207144721.2828390-1-koichiro.den@xxxxxxxxxxxxx/T/#mf6529a51167ffd1be52e1b67169442f486beb084

> 
> > (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.

Makes sense, I'm going to look carefully into every possible step and will
get back with v2 later. Thanks a lot for the review and discussion.

-Koichiro Den

> 
> > (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