Fix fence on non-x86 arch and miri #16
Conversation
|
At least crossbeam and event-listener also have the same issue, but fixing them is probably more complex... |
|
I feel a bit uncomfortable with this commit. Admittedly, I don't know what exactly is the role of the fence here. This fence does not exist in Dmitry Vyukov's original implementation of the queue, so I guess it was added as part of the modifications that ensure that this queue is linearisable (unlike the original queue). That being said, if the cross-platform solution is indeed to place the load before the fence (this, I do not know) then I am pretty sure that the intel specialization that uses a I did look at https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html but could not see where it states that |
| @@ -461,7 +464,11 @@ fn full_fence() { | |||
| // x86 platforms is going to optimize this away. | |||
| let a = AtomicUsize::new(0); | |||
| let _ = a.compare_exchange(0, 1, Ordering::SeqCst, Ordering::SeqCst); | |||
| // On x86, `lock cmpxchg; mov` is fine. See also https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html. | |||
| load_op() | |||
There was a problem hiding this comment.
FWIW, this is still Rust code -- so if Miri complains when running this branch of the code (which I suspect it will, since a SC RMW before a load cannot replace a fence after a load), then this code is still wrong.
When you write Rust code, the hardware memory model is all but irrelevant for program correctness. Only the Rust memory model counts.
EDIT: Oh I see this got reverted in #18.
| @@ -461,7 +464,11 @@ fn full_fence() { | |||
| // x86 platforms is going to optimize this away. | |||
There was a problem hiding this comment.
The fact that you are hoping that "sane" compilers for particular targets are going to treat the memory model differently, is a big red flag. The memory model is target-independent, and a whole bunch of optimizations run on this code (including its use of atomics) before any target-specific concerns are applied.
Inline assembly is the only correct choice here.
EDIT: Oh I see this got reverted in #18.
I would usually expect that to be the case -- a relaxed load followed by an acquire-or-stronger fence can induce a synchronization edge. But I don't know the context for this particular code. Does something break, or perf go down badly, if the fence is moved after the load? |
The problem seems to be that the original author of this code confused fence in the x86 hardware memory model with atomic fence in the C++ memory model. (On x86,
lock cmpxchg; mov (load from memory)is fine. See also https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html. On C++ memory model and many architectures, fence for load should beload; fence)Fixes bevyengine/bevy#5164
FYI @cbeuw