With the recent simplifications, it becomes obvious that smp_mb() isn't the
right barrier; all we need is a compiler barrier.

Include this in monitor() itself, along with an explantion.

No functional change.

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>
---
 xen/arch/x86/acpi/cpu_idle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 13040ef467a0..de020dfee87f 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -66,8 +66,12 @@ static always_inline void monitor(
     alternative_input("", "clflush (%[addr])", X86_BUG_CLFLUSH_MONITOR,
                       [addr] "a" (addr));
 
+    /*
+     * The memory clobber is a compiler barrier.  Subseqeunt reads from the
+     * monitored cacheline must not be hoisted over MONITOR.
+     */
     asm volatile ( "monitor"
-                   :: "a" (addr), "c" (ecx), "d" (edx) );
+                   :: "a" (addr), "c" (ecx), "d" (edx) : "memory" );
 }
 
 static always_inline void mwait(unsigned int eax, unsigned int ecx)
@@ -456,7 +460,6 @@ void mwait_idle_with_hints(unsigned int eax, unsigned int ecx)
                    [in_mwait] "=m" (stat->in_mwait));
 
     monitor(this_softirq_pending, 0, 0);
-    smp_mb();
 
     if ( !*this_softirq_pending )
     {
-- 
2.39.5

On 02.07.2025 16:41, Andrew Cooper wrote:
> With the recent simplifications, it becomes obvious that smp_mb() isn't the
> right barrier; all we need is a compiler barrier.
> 
> Include this in monitor() itself, along with an explantion.

Ah, here we go. As per my comment on patch 4, would this perhaps better move
ahead (which however would require a bit of an adjustment to the description)?

(Nit: explanation)

> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -66,8 +66,12 @@ static always_inline void monitor(
>      alternative_input("", "clflush (%[addr])", X86_BUG_CLFLUSH_MONITOR,
>                        [addr] "a" (addr));
>  
> +    /*
> +     * The memory clobber is a compiler barrier.  Subseqeunt reads from the

Nit: Subsequent

> +     * monitored cacheline must not be hoisted over MONITOR.
> +     */
>      asm volatile ( "monitor"
> -                   :: "a" (addr), "c" (ecx), "d" (edx) );
> +                   :: "a" (addr), "c" (ecx), "d" (edx) : "memory" );
>  }

That's heavier than we need, though. Can't we simply have a fake output
"+m" (irq_stat[cpu])? Downside being that the compiler may then set up
addressing of that operand, when the operand isn't really referenced. (As
long as __softirq_pending is the first field there, there may not be any
extra overhead, though, as %rax then would also address the unused operand.)

Yet then, is it really only reads from that cacheline that are of concern?
Isn't it - strictly speaking - also necessary that any (hypothetical) reads
done by the NOW() at the end of the function have to occur only afterwards
(and independent of there being a LOCK-ed access in cpumask_clear_cpu())?

Jan

On 03/07/2025 10:24 am, Jan Beulich wrote:
> On 02.07.2025 16:41, Andrew Cooper wrote:
>> With the recent simplifications, it becomes obvious that smp_mb() isn't the
>> right barrier; all we need is a compiler barrier.
>>
>> Include this in monitor() itself, along with an explantion.
> Ah, here we go. As per my comment on patch 4, would this perhaps better move
> ahead (which however would require a bit of an adjustment to the description)?

As said, it's not necessary in practice.

>
>> +     * monitored cacheline must not be hoisted over MONITOR.
>> +     */
>>      asm volatile ( "monitor"
>> -                   :: "a" (addr), "c" (ecx), "d" (edx) );
>> +                   :: "a" (addr), "c" (ecx), "d" (edx) : "memory" );
>>  }
> That's heavier than we need, though. Can't we simply have a fake output
> "+m" (irq_stat[cpu])?

No.  That would be wrong for one of the two callers.  Also we don't have
cpu available.

The correct value would be a round-down on addr and a cacheline-sized
sized type, but we can't do that because of -Wvla.

Nothing good can come of anything crossing the MONITOR, and ...

>  Downside being that the compiler may then set up
> addressing of that operand, when the operand isn't really referenced. (As
> long as __softirq_pending is the first field there, there may not be any
> extra overhead, though, as %rax then would also address the unused operand.)

... nothing good is going to come from trying to get clever at
optimising a constraint that doesn't actually improve code generation in
the first place.

>
> Yet then, is it really only reads from that cacheline that are of concern?
> Isn't it - strictly speaking - also necessary that any (hypothetical) reads
> done by the NOW() at the end of the function have to occur only afterwards
> (and independent of there being a LOCK-ed access in cpumask_clear_cpu())?

The NOW() and cpumask_clear_cpu() are gone, and not going to be returning.

I did put a compiler barrier in mwait() originally too, but dropped it
because I couldn't reason about it easily.

Nothing good can come of having any loads hoisted above MWAIT, but from
a programmers point of view, it's indistinguishable from e.g. taking an
SMI.  If there's a correctness issue, it's not MWAIT's fault.

~Andrew

On 03.07.2025 14:37, Andrew Cooper wrote:
> On 03/07/2025 10:24 am, Jan Beulich wrote:
>> On 02.07.2025 16:41, Andrew Cooper wrote:
>>> With the recent simplifications, it becomes obvious that smp_mb() isn't the
>>> right barrier; all we need is a compiler barrier.
>>>
>>> Include this in monitor() itself, along with an explantion.
>> Ah, here we go. As per my comment on patch 4, would this perhaps better move
>> ahead (which however would require a bit of an adjustment to the description)?
> 
> As said, it's not necessary in practice.

As said where? All you say here is that a memory barrier is needed. Perhaps
my use of "ahead" was ambiguous? I meant "move ahead in the series", not
"move ahead in code". Apart from this as a possibility I fear I don't
understand.

>>> +     * monitored cacheline must not be hoisted over MONITOR.
>>> +     */
>>>      asm volatile ( "monitor"
>>> -                   :: "a" (addr), "c" (ecx), "d" (edx) );
>>> +                   :: "a" (addr), "c" (ecx), "d" (edx) : "memory" );
>>>  }
>> That's heavier than we need, though. Can't we simply have a fake output
>> "+m" (irq_stat[cpu])?
> 
> No.  That would be wrong for one of the two callers.

How that? MONITOR behaves the same in all cases, doesn't it? And it's
the same piece of data in both cases the address of which is passed in
(&softirq_pending(smp_processor_id())).

>  Also we don't have cpu available.

smp_processor_id()? Or else have a pointer to the full array entry passed
in? We could also specify the entire array, just that that's not easily
expressable afaict.

> The correct value would be a round-down on addr and a cacheline-sized
> sized type, but we can't do that because of -Wvla.

But that's what irq_stat[cpu] is (or at least claims to be, by the element
type having the __cacheline_aligned attribute).

> Nothing good can come of anything crossing the MONITOR, and ...
> 
>>  Downside being that the compiler may then set up
>> addressing of that operand, when the operand isn't really referenced. (As
>> long as __softirq_pending is the first field there, there may not be any
>> extra overhead, though, as %rax then would also address the unused operand.)
> 
> ... nothing good is going to come from trying to get clever at
> optimising a constraint that doesn't actually improve code generation in
> the first place.
> 
>> Yet then, is it really only reads from that cacheline that are of concern?
>> Isn't it - strictly speaking - also necessary that any (hypothetical) reads
>> done by the NOW() at the end of the function have to occur only afterwards
>> (and independent of there being a LOCK-ed access in cpumask_clear_cpu())?
> 
> The NOW() and cpumask_clear_cpu() are gone, and not going to be returning.

I just used the former as example and mentioned the latter because it would
serialize memory accesses irrespective of any barriers we add.

> I did put a compiler barrier in mwait() originally too, but dropped it
> because I couldn't reason about it easily.

Which I understand.

> Nothing good can come of having any loads hoisted above MWAIT, but from
> a programmers point of view, it's indistinguishable from e.g. taking an
> SMI.  If there's a correctness issue, it's not MWAIT's fault.

Well, yes, but what in the code is it that tells the compiler not to? Up
to and including LTO, should we ever get that to work again. This
specifically may be why mwait() may need to gain one, despite not itself
dealing with any memory (operands).

Jan

On 03/07/2025 2:30 pm, Jan Beulich wrote:
> On 03.07.2025 14:37, Andrew Cooper wrote:
>> On 03/07/2025 10:24 am, Jan Beulich wrote:
>>> On 02.07.2025 16:41, Andrew Cooper wrote:
>>>> With the recent simplifications, it becomes obvious that smp_mb() isn't the
>>>> right barrier; all we need is a compiler barrier.
>>>>
>>>> Include this in monitor() itself, along with an explantion.
>>> Ah, here we go. As per my comment on patch 4, would this perhaps better move
>>> ahead (which however would require a bit of an adjustment to the description)?
>> As said, it's not necessary in practice.
> As said where? All you say here is that a memory barrier is needed. Perhaps
> my use of "ahead" was ambiguous? I meant "move ahead in the series", not
> "move ahead in code". Apart from this as a possibility I fear I don't
> understand.

I did take it to mean "ahead in the series".

Your comment in patch 4 talks about alternative(), alternative_io() and
barriers.  I stated (admittedly without reference) that the barrier
between two alternatives() doesn't matter because of their volatileness.

It can move in the series, because it is genuinely independent and
unrelated to patch 4 AFAICT.


>> Nothing good can come of having any loads hoisted above MWAIT, but from
>> a programmers point of view, it's indistinguishable from e.g. taking an
>> SMI.  If there's a correctness issue, it's not MWAIT's fault.
> Well, yes, but what in the code is it that tells the compiler not to? Up
> to and including LTO, should we ever get that to work again. This
> specifically may be why mwait() may need to gain one, despite not itself
> dealing with any memory (operands).

In practice, mwait() is surrounded by spec_ctrl_{enter,exit}_idle() and
nothing is crossing those.  I'm going to leave the mwait side of things
alone for now.

But even with LTO, if hoisting a read causes a problem, that's a bug in
whatever got hoisted, not in MWAIT.

~Andrew