Re: [PATCH 4/6] x86/idle: Implement a new MWAIT IPI-elision algorithm

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jul 03, 2025 at 06:48:23PM +0100, Andrew Cooper wrote:
> On 03/07/2025 5:36 pm, Roger Pau Monné wrote:
> > On Wed, Jul 02, 2025 at 03:41:19PM +0100, Andrew Cooper wrote:
> >> In order elide IPIs, we must be able to identify whether a target CPU is in
> >> MWAIT at the point it is woken up.  i.e. the store to wake it up must also
> >> identify the state.
> >>
> >> Create a new in_mwait variable beside __softirq_pending, so we can use a
> >> CMPXCHG to set the softirq while also observing the status safely.  Implement
> >> an x86 version of arch_pend_softirq() which does this.
> >>
> >> In mwait_idle_with_hints(), advertise in_mwait, with an explanation of
> >> precisely what it means.  X86_BUG_MONITOR can be accounted for simply by not
> >> advertising in_mwait.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Anthony PERARD <anthony.perard@vates.tech>
> >> CC: Michal Orzel <michal.orzel@amd.com>
> >> CC: Julien Grall <julien@xen.org>
> >> CC: Stefano Stabellini <sstabellini@kernel.org>
> >>
> >> This is modelled after Linux's TIF_NEED_RESCHED (single bit equivelent of all
> >> of __softirq_pending), and TIF_POLLING_NRFLAG (arch-neutral "in_mwait").
> >>
> >> In Linux, they're both in the same flags field, which adds complexity.  In
> >> Xen, __softirq_pending is already unsigned long for everything other than x86,
> >> so adding an arch-neutral "in_mwait" would need wider changes.
> >> ---
> >>  xen/arch/x86/acpi/cpu_idle.c       | 20 +++++++++++++++++-
> >>  xen/arch/x86/include/asm/hardirq.h | 14 +++++++++++-
> >>  xen/arch/x86/include/asm/softirq.h | 34 ++++++++++++++++++++++++++++++
> >>  3 files changed, 66 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> >> index caa0fef0b3b1..13040ef467a0 100644
> >> --- a/xen/arch/x86/acpi/cpu_idle.c
> >> +++ b/xen/arch/x86/acpi/cpu_idle.c
> >> @@ -439,7 +439,21 @@ __initcall(cpu_idle_key_init);
> >>  void mwait_idle_with_hints(unsigned int eax, unsigned int ecx)
> >>  {
> >>      unsigned int cpu = smp_processor_id();
> >> -    const unsigned int *this_softirq_pending = &softirq_pending(cpu);
> >> +    irq_cpustat_t *stat = &irq_stat[cpu];
> >> +    const unsigned int *this_softirq_pending = &stat->__softirq_pending;
> >> +
> >> +    /*
> >> +     * By setting in_mwait, we promise to other CPUs that we'll notice changes
> >> +     * to __softirq_pending without being sent an IPI.  We achieve this by
> >> +     * either not going to sleep, or by having hardware notice on our behalf.
> >> +     *
> >> +     * Some errata exist where MONITOR doesn't work properly, and the
> >> +     * workaround is to force the use of an IPI.  Cause this to happen by
> >> +     * simply not advertising outselves as being in_mwait.
> >> +     */
> >> +    alternative_io("movb $1, %[in_mwait]",
> >> +                   "", X86_BUG_MONITOR,
> >> +                   [in_mwait] "=m" (stat->in_mwait));
> >>  
> >>      monitor(this_softirq_pending, 0, 0);
> >>      smp_mb();
> >> @@ -452,6 +466,10 @@ void mwait_idle_with_hints(unsigned int eax, unsigned int ecx)
> >>          mwait(eax, ecx);
> >>          spec_ctrl_exit_idle(info);
> >>      }
> >> +
> >> +    alternative_io("movb $0, %[in_mwait]",
> >> +                   "", X86_BUG_MONITOR,
> >> +                   [in_mwait] "=m" (stat->in_mwait));
> > Isn't it a bit overkill to use alternatives here when you could have a
> > conditional based on a global variable to decide whether to set/clear
> > in_mwait?
> 
> I view it differently.  Why should the common case suffer overhead
> (extra memory read and conditional branch) to work around 3 buggy pieces
> of hardware in a common path?

TBH I don't think the extra read and conditional would make any
difference in this specific path performance wise.  Either the CPU is
going to sleep and has nothing to do, or the cost of getting back from
idle will shadow the overhead of an extra read and conditional.

It's all a question of taste I guess.  If it was me I would set/clear
stat->in_mwait unconditionally in mwait_idle_with_hints(), and then in
arch_pend_softirq() I would return:

return new & (1UL << 32) || force_mwait_ipi_wakeup;

Or AFAICT you could possibly skip the cmpxchg() in the
force_mwait_ipi_wakeup case in arch_pend_softirq(), and just do:

if ( force_mwait_ipi_wakeup )
    return test_and_set_bit(nr, &softirq_pending(cpu));

Because in that case Xen doesn't care about the in_mwait status.  It
would be an optimization for the buggy hardware that already has to
deal with the extra cost of always sending an IPI.

> >> +        };
> >> +        uint64_t softirq_mwait_raw;
> >> +    };
> > This could be a named union type ...
> >
> >> +
> >>      unsigned int __local_irq_count;
> >>      unsigned int nmi_count;
> >>      unsigned int mce_count;
> >> diff --git a/xen/arch/x86/include/asm/softirq.h b/xen/arch/x86/include/asm/softirq.h
> >> index e4b194f069fb..069e5716a68d 100644
> >> --- a/xen/arch/x86/include/asm/softirq.h
> >> +++ b/xen/arch/x86/include/asm/softirq.h
> >> @@ -1,6 +1,8 @@
> >>  #ifndef __ASM_SOFTIRQ_H__
> >>  #define __ASM_SOFTIRQ_H__
> >>  
> >> +#include <asm/system.h>
> >> +
> >>  #define NMI_SOFTIRQ            (NR_COMMON_SOFTIRQS + 0)
> >>  #define TIME_CALIBRATE_SOFTIRQ (NR_COMMON_SOFTIRQS + 1)
> >>  #define VCPU_KICK_SOFTIRQ      (NR_COMMON_SOFTIRQS + 2)
> >> @@ -9,4 +11,36 @@
> >>  #define HVM_DPCI_SOFTIRQ       (NR_COMMON_SOFTIRQS + 4)
> >>  #define NR_ARCH_SOFTIRQS       5
> >>  
> >> +/*
> >> + * Ensure softirq @nr is pending on @cpu.  Return true if an IPI can be
> >> + * skipped, false if the IPI cannot be skipped.
> >> + *
> >> + * We use a CMPXCHG covering both __softirq_pending and in_mwait, in order to
> >> + * set softirq @nr while also observing in_mwait in a race-free way.
> >> + */
> >> +static always_inline bool arch_pend_softirq(unsigned int nr, unsigned int cpu)
> >> +{
> >> +    uint64_t *ptr = &irq_stat[cpu].softirq_mwait_raw;
> >> +    uint64_t old, new;
> > ... so that you also use it here?
> 
> No, it cant.  The of softirq_pending() in common code requires no
> intermediate field names, and I'm not untangling that mess in a series
> wanting backporting.

Oh, I see.  Never mind then, something for a later cleanup if
anything.

Thanks, Roger.