Re: [PATCH 5/6] x86/idle: Drop incorrect smp_mb() in mwait_idle_with_hints()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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