Is this C++11 seqlock implementation correct? My code: https://ideone.com/DZeIZv

#include &lt;atomic&gt;

template &lt;class T&gt; class Seqlock {
    std::atomic&lt;int&gt; seq_;
    T val_;

public:
    Seqlock(T value = T())
        : val_(value) {
    }

    // concurrent calls are NOT allowed
    void store(T value) {
        const int seq0 = seq_.load(std::memory_order_relaxed);
        seq_.store(seq0 + 1, std::memory_order_relaxed);

        std::atomic_thread_fence(std::memory_order_release);

        val_ = value;
        std::atomic_thread_fence(std::memory_order_release);

        seq_.store(seq0 + 2, std::memory_order_relaxed);
    }

    // concurrent calls are allowed
    T load() const {
        for (;;) {
            const int seq0 = seq_.load(std::memory_order_relaxed);
            if (seq0 &amp; 1) {
                // cpu_relax()
                continue;
            }

            std::atomic_thread_fence(std::memory_order_acquire);

            T ret = val_;
            std::atomic_thread_fence(std::memory_order_acquire);

            const int seq1 = seq_.load(std::memory_order_relaxed);
            if (seq0 == seq1) {
                return ret;
            }
        }
    }
};


Is this seqlock implementation correct across all architectures, at least when T is an integral type? Can it be improved?

References I was following:


https://elixir.bootlin.com/linux/latest/source/include/linux/seqlock.h
https://www.hpl.hp.com/techreports/2012/HPL-2012-68.pdf
https://github.com/rigtorp/Seqlock/
My guideline on memory orders is "Just say no"; I guarantee that if you're using them, you're using them wrong. :)  So I won't attempt to find the exact bug, I'll just assume they all say seq_cst. The only thing I'll say about your memory orders is, when you write

    seq_.store(seq0 + 1, std::memory_order_relaxed);
    std::atomic_thread_fence(std::memory_order_release);


can you explain how that's different from

    seq_.store(seq0 + 1, std::memory_order_release);


?



Seqlock(T value = T())


This constructor should be explicit; otherwise you're accidentally permitting

Seqlock&lt;int&gt; s = 42;


and in fact because of C++17 CTAD you're also permitting

Seqlock s = 42;


Rule of thumb: make all constructors explicit except for those you want the compiler to be able to call implicitly (i.e., copy and move constructors).



In T load() const, you load seq1 at the bottom of the loop, and then go around again and immediately load seq0 from the same location. You could just have set seq0 = seq1; i.e.

T load() const {
    int seq1 = seq_.load(std::memory_order_relaxed);
    while (true) {
        int seq0 = seq1;
        if (seq0 &amp; 1) {
            // cpu_relax();
        } else {
            std::atomic_thread_fence(std::memory_order_acquire);
            T ret = val_;
            std::atomic_thread_fence(std::memory_order_acquire);
            seq1 = seq_.load(std::memory_order_relaxed);
            if (seq0 == seq1) {
                return ret;
            }
        }
    }
}