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

[Jan Beulich <jbeulich@xxxxxxxx>]

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