<div dir="auto"><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 31, 2022, 11:10 AM Zebediah Figura <<a href="mailto:zfigura@codeweavers.com">zfigura@codeweavers.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
On 5/30/22 20:23, Jin-oh Kang wrote:<br>
> On Tue, May 31, 2022, 7:52 AM Zebediah Figura wrote:<br>
> <br>
>> On 5/30/22 14:20, Jinoh Kang wrote:<br>
>>> +<br>
>>> +    /* GetOverlappedResultEx() skips waiting for the event/file handle<br>
>> if the<br>
>>> +     * Status field indicates completion, and returns the result from<br>
>> the<br>
>>> +     * Information field.<br>
>>> +     *<br>
>>> +     * Hence, we have to ensure that writes to the Information field<br>
>> are seen<br>
>>> +     * from the other threads *before* writes to the Status field.<br>
>>> +     *<br>
>>> +     * Otherwise, a race condition results: if writing the Status field<br>
>> has<br>
>>> +     * been completed but the subsequent update to the Information<br>
>> field has<br>
>>> +     * not, the application may end up reading stale value from it.<br>
>>> +     *<br>
>>> +     * The race condition can be especially problematic with<br>
>> applications that<br>
>>> +     * use the HasOverlappedIoCompleted() macro in a tight loop to<br>
>> busy-wait<br>
>>> +     * for completion of overlapped I/O.<br>
>>> +     */<br>
>>> +    MemoryBarrier();  /* TODO: consider replacing with a store-release.<br>
>> */<br>
>>> +    *(volatile NTSTATUS *)status_ptr = status;<br>
>>>    }<br>
>>><br>
>>>    static inline client_ptr_t iosb_client_ptr( IO_STATUS_BLOCK *io )<br>
>><br>
>> Unpaired MemoryBarrier() is pretty much always wrong. (In fact, I think<br>
>> unpaired anything would be, here.)<br>
>><br>
> <br>
> By pairing, do you mean that another MemoryBarrier() should follow the<br>
> status write?<br>
<br>
No. We need another memory barrier in GetOverlappedResult().<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Ah yes, of course. I'm sending it in a separate patch set, though. Thanks for the reminder.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The point of a memory barrier is that if you have two pairs of memory <br>
operations (read or write) which must be perceived as happening "in <br>
order" across threads, you place a memory barrier in the middle of each <br>
pair. That is, if thread 1 executes ops A and B, and thread 2 executes <br>
ops C and D, you need a barrier between A and B, but also a barrier <br>
between C and D.<br>
<br>
The following image (stolen and adapted from the Linux kernel <br>
documentation) may be illustrative:<br>
<br>
         CPU 1                       CPU 2<br>
         ===============             ===============<br>
         WRITE_ONCE(a, 1);           x = READ_ONCE(b);<br>
         <write barrier>  <--------> <read barrier><br>
         WRITE_ONCE(b, 2);           y = READ_ONCE(a);<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">That said, the *_ONCE macros would be quite helpful for Wine too. Ideally, however, we should just use the stdatomic.h primitives whenever possible (old GCC has bugs in them though), shimming in our own implementation on older compilers.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
You need to do this because the compiler—and, more importantly, the <br>
CPU—is free to reorder the instructions on *both* sides. If, as in the <br>
above example, you need "x == 2" to imply "y == 1", the write barrier <br>
alone is not enough to achieve this, because the CPU can *also* reorder <br>
the reads.<br>
<br>
The same applies to atomic operations that aren't just RMW. E.g. a <br>
store-release (to Status) would be correct here, and more efficient than <br>
a barrier, but it has to be paired with a read-acquire (also of Status) <br>
in GetOverlappedResult().<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">The problem is that the Wine codebase doesn't have one (yet).  Otherwise an acquire/release barrier pair would be much more efficient than a full barrier (at least on weak-memory-order architectures).</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
</blockquote></div></div></div>