Possible memory order issue

[AngelicosPhosphoros]

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.
}

[AngelicosPhosphoros]

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.

[Amanieu]

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.

[AngelicosPhosphoros]

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

[Amanieu]

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

[AngelicosPhosphoros]

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?

[Amanieu]

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.

[AngelicosPhosphoros]

Thank you for answering my questions.