Synchronisation in C++ 11 with std::atomic

Question

I have the following code that runs fine on Intel processors but produces weird data on ARM processors.
I suspect that this is a synchronisation problem.

Basically I have a producer thread calling setLiveData(...) periodically and a consumer thread calling getLiveData(...) periodically as well.

.h file

class DataHandler{
public:
...
private:

    LiveDataValue lastValues_;
    bool lastValuesValid;
};

.cpp file

bool DataHandler::getLiveData(LiveDataValue *val)
{
    if(this->lastValuesValid){
        *val = this->lastValues_;
        return true;
    }else
        return false;
}

void DataHandler::setLiveData(LiveDataValue val)
{
    this->lastValuesValid = false;
    this->lastValues = val;
    this->lastValuesValid = true;
}

Just by reading the code, I think that I need to make sure that setLiveData is atomic in the sense that a consumer thread can't call getLiveData(...) while a producer thread is in the middle of setLiveData(...)

I've found this answer and tried to used it fixing the code:

.h file

class DataHandler{
public:
...
private:

    LiveDataValue lastValues_;
    std::atomic<bool> lastValuesValid;
};

.cpp file

bool DataHandler::getLiveData(LiveDataValue *val)
{
   while (!this->lastValuesValid.load(std::memory_order_acquire))
   {
       std::this_thread::yield();
   }

    if(this->lastValuesValid){
        *val = this->lastValues_;
        return true;
    }else
        return false;
}

void DataHandler::setLiveData(LiveDataValue val)
{
    this->lastValuesValid_.store(false, std::memory_order_release);
    this->lastValues = val;
    this->lastValuesValid_.store(true, std::memory_order_release);
}

My problem is that I never exit the while loop in getLiveData called by the reader thread. Why is that?

EDIT : LiveDataValue is a complex union typedef, not detailed here.

Answer 1

Your problem is that your code doesn't synchronize, not that your loop doesn't end.

if(this->lastValuesValid){
    *val = this->lastValues_;
    return true;
}else
    return false;

you can check if last values are valid, get true, and by the time you assign they aren't valid. Any check of validity isn't going to hold immediately afterwards, it simply tells you that at some in the past point they where valid.

template<class T>
struct mutex_guarded {
  template<class F>
  void read( F&& f ) const {
    auto l = std::unique_lock<std::mutex>(m);
    f(t);
  }
  template<class F>
  void write( F&& f ) {
    auto l = std::unique_lock<std::mutex>(m);
    f(t);
  }
private:
  mutable std::mutex m;
  T t;
};

this is a simple wrapper to serialize access to some data of arbitrary type.

class DataHandler{
public:
...
private:

  struct Data {
    LiveDataHolder lastValues_;
    bool lastValuesValid_ = false;
  };
  mutex_guarded<Data> data_;
};

then

bool DataHandler::getLiveData(LiveDataValue *val) const
{
  bool bRet = false;
  data_.read([&](Data const& data_){
    bRet = data_.lastValuesValid_;
    if (!bRet) return;
    *val = data_.lastValues;
  });
  return bRet;
}

void DataHandler::setLiveData(LiveDataValue val)
{
  data_.write([&](Data & data_){
    data_.lastValues = std::move(val);
    data_.lastValuesValid = true;
  });
}

will atomically modify both the valid and the values fields.

Everything done in .read(lambda) and .write(lambda) is done while guarded by locking a mutex. The lambda is passed either a T const& or a T& depending on if it is a read or a write operation, and there is no other way to access the guarded data.

(Extending this to support reader/writer locks is relatively easy, but keep it simple is a good rule of thumb, so I just wrote it with a mutex)