Given the following:

class Foo
{
public:
    void Increment()
    {
        _InterlockedIncrement(&m_value); // OSIncrementAtomic
    }

    long GetValue()
    {
        return m_value;
    }

private:
    long m_value;
};

Is a memory barrier required for the read of m_value? My understanding is that _InterlockedIncrement will generate a full memory barrier, and that ensures the value is stored before any subsequent loads occur. So that sounds safe from that aspect, but, can m_value be cached at all, i.e. can GetValue() return a stale value, even when incrementing atomically?

Jeff Preshing's excellent article(s) for reference: https://preshing.com/20120515/memory-reordering-caught-in-the-act/

CodePudding user response：

Yes, barriers should be on both sides: reading and writing. Imagine you have some write-buffer and a loading-queue where everything might be out-of-order. So you flush the write-buffer when writing your stuff but the loading-queue, you need to deal with, is on the other thread (processor) which knows nothing about your flushing. So it is always a paired process.

Also you can think about it in a compiler hat: unless a compiler is forced to serialize access it has the liberty to reorder anything it can safely (according to its view) do.

That said, it is all about serialization and not atomicity. Which is a whole another matter. You have your writing atomically _InterlockedIncrement but reading isn't atomic return m_value. So that's a race.

Also, I see your code requires atomicity but I don't see any need for serialization. You do not protect anything with your m_value. And as to the "stale" value: usually you can't guarantee that you won't have a stale value at some point of time, even with barriers. RMW-operations require the latest values but others don't. So having a barrier will help to get the latest value faster but that's it. Having your code and forgetting about the race, compiler might safely assume that you do not modify m_value and cache it. The same might be said about CPU.

All that said: just use std::atomic when you need an unprotected variable. It will make sure the value is not cached by any entity.

CodePudding user response：

Short answer: Yes.

Your "reading" should have "acquire" order, so your Increment() result will be visible in another thread when it do a "release" after an incremnet.