[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/7] x86/smp: do not use shorthand IPI destinations in CPU hot{,un}plug contexts
On Wed, 2024-06-12 at 10:56 +0200, Jan Beulich wrote: > On 12.06.2024 10:09, Roger Pau Monné wrote: > > On Tue, Jun 11, 2024 at 09:42:39AM +0200, Jan Beulich wrote: > > > On 10.06.2024 16:20, Roger Pau Monne wrote: > > > > Due to the current rwlock logic, if the CPU calling > > > > get_cpu_maps() does so from > > > > a cpu_hotplug_{begin,done}() region the function will still > > > > return success, > > > > because a CPU taking the rwlock in read mode after having taken > > > > it in write > > > > mode is allowed. Such behavior however defeats the purpose of > > > > get_cpu_maps(), > > > > > > I'm not happy to see you still have this claim here. The behavior > > > may (appear > > > to) defeat the purpose here, but as expressed previously I don't > > > view that as > > > being a general pattern. > > > > Right. What about replacing the paragraph with: > > > > "Due to the current rwlock logic, if the CPU calling get_cpu_maps() > > does so from > > a cpu_hotplug_{begin,done}() region the function will still return > > success, > > because a CPU taking the rwlock in read mode after having taken it > > in write > > mode is allowed. Such corner case makes using get_cpu_maps() alone > > not enough to prevent using the shorthand in CPU hotplug regions." > > Thanks. > > > I think the rest is of the commit message is not controversial. > > Indeed. > > > > > --- a/xen/arch/x86/smp.c > > > > +++ b/xen/arch/x86/smp.c > > > > @@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int > > > > vector) > > > > * the system have been accounted for. > > > > */ > > > > if ( system_state > SYS_STATE_smp_boot && > > > > - !unaccounted_cpus && !disabled_cpus && > > > > + !unaccounted_cpus && !disabled_cpus && > > > > !cpu_in_hotplug_context() && > > > > /* NB: get_cpu_maps lock requires enabled interrupts. > > > > */ > > > > local_irq_is_enabled() && (cpus_locked = > > > > get_cpu_maps()) && > > > > (park_offline_cpus || > > > > > > Along with changing the condition you also want to update the > > > comment to > > > reflect the code adjustment. > > > > I've assumed the wording in the commet that says: "no CPU hotplug > > or > > unplug operations are taking place" would already cover the > > addition > > of the !cpu_in_hotplug_context() check. > > Hmm, yes, you're right. Just needs a release-ack then to go in (with > the > description adjustment). Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx> ~ Oleksii
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |