Re: [PATCH for-4.19 3/9] xen/cpu: ensure get_cpu_maps() returns false if CPU operations are underway

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.05.2024 11:01, 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(),
> as it should always return false when called with a CPU hot{,un}plug operation
> is in progress.

I'm not sure I can agree with this. The CPU doing said operation ought to be
aware of what it is itself doing. And all other CPUs will get back false from
get_cpu_maps().

>  Otherwise the logic in send_IPI_mask() for example is wrong,
> as it could decide to use the shorthand even when a CPU operation is in
> progress.

It's also not becoming clear what's wrong there: As long as a CPU isn't
offline enough to not be in cpu_online_map anymore, it may well need to still
be the target of IPIs, and targeting it with a shorthand then is still fine.

In any event this would again affect only the CPU leading the CPU operation,
which should clearly know at which point(s) it is okay to send IPIs. Are we
actually sending any IPIs from within CPU-online or CPU-offline paths?
Together with the earlier paragraph the critical window would be between the
CPU being taken off of cpu_online_map and the CPU actually going "dead" (i.e.
on x86: its LAPIC becoming unresponsive to other than INIT/SIPI). And even
then the question would be what bad, if any, would happen to that CPU if an
IPI was still targeted at it by way of using the shorthand. I'm pretty sure
it runs with IRQs off at that time, so no ordinary IRQ could be delivered.

> Adjust the logic in get_cpu_maps() to return false when the CPUs lock is
> already hold in write mode by the current CPU, as read_trylock() would
> otherwise return true.
> 
> Fixes: 868a01021c6f ('rwlock: allow recursive read locking when already 
> locked in write mode')

I'm puzzled by this as well: Prior to that and the change referenced by its
Fixes: tag, recursive spin locks were used. For the purposes here that's the
same as permitting read locking even when the write lock is already held by
the local CPU.

Jan