Messages in this thread | | | Date | Wed, 23 Mar 2022 10:10:46 +0000 | Subject | Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier | From | Steven Price <> |
| |
Thanks for taking a look at this.
On 22/03/2022 22:58, Thomas Gleixner wrote: > On Tue, Mar 22 2022 at 15:59, Vincent Donnefort wrote: >> On 22/03/2022 15:31, Thomas Gleixner wrote: >>> On Wed, Mar 16 2022 at 15:36, Steven Price wrote: >>>> Setting the 'cpu' member of struct cpuhp_cpu_state in cpuhp_create() is >>>> too late as other callbacks can be made before that point. >>> >>> What? >>> >>> CPUHP_OFFLINE = 0, >>> CPUHP_CREATE_THREADS, >>> >>> The create threads callback is the very first callback which is invoked >>> for a to be plugged CPU on the control CPU. So which earlier callback >>> can be invoked and fail? >>> >>> Thanks, >>> >>> tglx >> >> >> CPUHP_CREATE_THREADS itself can fail, before st->cpu is set. > > Sure. But that does not explain the problem. > >> Also, that value is used outside of the callbacks (cpuhp_set_state() >> in _cpu_up()). > > And why on earth is this not spelled out in the changelog?
I apologies for that, I'm not very familiar with the code and I have to admit I have been struggling to identify exactly what is going on here. The actual issue I saw was if the callback fails then the rollback code leaves things in a messed up state. By the looks of things that callback that fails is indeed the first (CPUHP_CREATE_THREADS).
>> But indeed this description could be refined a bit. > > Indeed. But the description is not the only problem here: > > It's completely uncomprehensible from the code in _cpu_up() _WHY_ this > > st->cpu = cpu; > > assignment has to be there. > > It's non-sensical if you really think about it, right?
I entirely agree, and I did ask in my v1 posting[1] if anyone could point me to a better place to do the assignment. Vincent suggested moving it earlier in _cpu_up() which is this v2.
But it still seems out-of-place to me. I've just had a go at simply removing the 'cpu' member and it doesn't look too bad. I'll post that patch as a follow up. I'm open to other suggestions for the best way to fix this.
Thanks,
Steve
[1] https://lore.kernel.org/all/20220225134918.105796-1-steven.price@arm.com/
> That said, I'm pretty sure you can come up with: > > - a proper one time initialization of @st which solves your problem > > - a proper changelog which explains it > > Thanks, > > tglx
| |