Possible memory order issue

Hi!

I read a code of create and noticed this segment:

            // We need to use a volatile read here because the data may be
            // concurrently modified by a writer. We also use MaybeUninit in
            // case we read the data in the middle of a modification.
            let result = unsafe { ptr::read_volatile(self.data.get() as *mut MaybeUninit<T>) };

            // Make sure the seq2 read occurs after reading the data. What we
            // ideally want is a load(Release), but the Release ordering is not
            // available on loads.
            fence(Ordering::Acquire);

            // If the sequence number is the same then the data wasn't modified
            // while we were reading it, and can be returned.
            let seq2 = self.seq.load(Ordering::Relaxed);
            if seq1 == seq2 {
                return unsafe { result.assume_init() };
            }

Shouldn't fence here use AcqRel? While using Acquire indeed prevents moving second atomic load up, it doesn't prevent moving read_volatile down. So CPU may reorder your code like this:

seq1 = load_atomic;
seq2 = load_atomic;
if seq1 == seq2 {
   return load value; // This is valid for moving down because acquire doesn't prevent moving down.
}

Same thing with a fence here:

seqlock/src/lib.rs

Line 162 in b72653f

fence(Ordering::Release);

It doesn't prevent writes to the underlying memory from moving before updating an atomic.

It may be not noticed so far because x86-64 have a guaranteed store order but other platforms could do such moves.

IMHO, you should use AcqRel fences in both cases.

The code actually works, but it's a bit subtle. Essentially the fence(Acquire) is there to effectively give the volatile load "acquire" semantics. This ensures it always executes before the seq2 load.

But does volatile guarantee that CPU wouldn't reorder instructions? AFAIK, it only affects the compiler.

Volatile on its own doesn't guarantee this, that's what the fence is for.

Do I understand correctly that volatile read followed by a acquire fence hace same effect as acquire load, and therefore it would happen before any reads after fence?

It's a bit of a gray area in the language, but it works in practice. The proper solution is to use something like rust-lang/rfcs#3301, but that doesn't exist in the language yet.

Thank you for answering my questions.