Re: [PATCH 1/6] x86/idle: Remove broken MWAIT implementation

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 02, 2025 at 03:41:16PM +0100, Andrew Cooper wrote:
> cpuidle_wakeup_mwait() is a TOCTOU race.  The cpumask_and() sampling
> cpuidle_mwait_flags can take a arbitrary period of time, and there's no
> guarantee that the target CPUs are still in MWAIT when writing into
> mwait_wakeup(cpu).
> 
> The consequence of the race is that we'll fail to IPI targets.  Also, there's
> no guarantee that mwait_idle_with_hints() will raise a TIMER_SOFTIRQ on it's
> way out.
> 
> The fundamental bug is that the "in_mwait" variable needs to be in the
> monitored line in order to do this in a race-free way.

I assume in_mwait being in the same monitored line is required so that
you can do an atomic exchange and ensure the remote CPU was either in
the monitor state, or just getting out of it but before processing
softirqs when the softirq is set?

That means that a CPU getting out of mwait would also need to do an
atomic exchange to clear in_mwait and fetch the pending softirqs?

> Arranging for this while keeping the old implementation is prohibitive, so
> strip it out in order to implement the new one cleanly.  In the interim, this
> causes IPIs to be used unconditionally which is safe albeit suboptimal.
> 
> Fixes: 3d521e933e1b ("cpuidle: mwait on softirq_pending & remove wakeup ipis")
> Fixes: 1adb34ea846d ("CPUIDLE: re-implement mwait wakeup process")
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.