Futures-rs: AtomicWake memory order regression

[carllerche]

Introduced by #1316

The patch introduces a load with SeqCst assuming sequential consistency from the write as well, but the write is done with AcqRel semantics which introduces a potential deadlock.

this load assumes it will always see the latest write from here.

However, that would require the compare_and_swap to use a SeqCst ordering.

This can lead to deadlocks where the notify doesn't see the registered task.

[cramertj]

From the SeqCst docs:

Like Acquire/Release/AcqRel (for load, store, and load-with-store operations, respectively) with the additional guarantee that all threads see all sequentially consistent operations in the same order.

I think the "with the additional guarantee that all threads see all sequentially consistent operations in the same order". Because only the load is SeqCst, it does not have to have a sequentially consistent ordering with respect to the compare_and_swap (which is AcqRel). I think you're correct, and would accept a PR to either revert or switch the compare_and_swap to SeqCst.

[cramertj]

I've been looking at this and I'm also reasonably confident that SeqCst isn't enough either, since it only imposes a total ordering between sequentially consistent operations, not to third elements unrelated to the atomic variable being updated. The operation that is wanted here is a load(Release) (guarantee that my writes were released to other threads before any of them observed the change away from the value I observed with my load). However, that's not a thing that exists in LLVM nor any architecture I'm aware of. I'm planning to revert for now. cc @stjepang

[hcpl]

cc @stjepang

cc @stepancheg, right?

[cramertj]

Heh, yup. My "@st" + autocomplete was not sufficient XD

[stepancheg]

@cramertj

I'm also reasonably confident that SeqCst isn't enough either

Are you sure? My naive understanding is that any SeqCst operation is a full memory barrier, all other threads will see all my changes on their next SeqCst.

(I couldn't understand your explanation sorry)

[cramertj]

@stepancheg The LLVM docs say:

SequentiallyConsistent (seq_cst in IR) provides Acquire semantics for loads and Release semantics for stores. Additionally, it guarantees that a total ordering exists between all SequentiallyConsistent operations.

It's guaranteeing a total order of SeqCst ops, not of other operations that fall between the SeqCst ops, nor is it giving Release semantics for loads.