views:

214

answers:

2

I still feel a bit unsafe about the topic and hope you folks can help me -

For passing data (configuration or results) between a worker thread polling something and a controlling thread interested in the most recent data, I've ended up using more or less the following pattern repeatedly:

Mutex m;
tData * stage;       // temporary, accessed concurrently

// send data, gives up ownership, receives old stage if any
tData * Send(tData * newData)
{
   ScopedLock lock(m);
   swap(newData, stage);              
   return newData;
}

// receiving thread fetches latest data here
tData * Fetch(tData * prev)
{
  ScopedLock lock(m);
  if (stage != 0)
  {
     // ... release prev
     prev = stage;
     stage = 0;
  }
  return prev;  // now current
}

Note: This is not supposed to be a full producer-consumer queue, only the msot recent data is relevant. Also, I've skimmed ressource management somewhat here.

When necessary I'm using two such stages: one to send config changes to the worker, and for sending back results.

Now, my questions

assuming that ScopedLock implements a full memory barrier:

  • do stage and/or workerData need to be volatile?
  • is volatile necessary for tData members?
  • can I use smart pointers instead of the raw pointers - say boost::shared_ptr?
  • Anything else that can go wrong?
  • I am basically trying to avoid "volatile infection" spreading into tData, and minimize lock contention (a lock free implementation seems possible, too). However, I'm not sure if this is the easiest solution.

ScopedLock acts as a full memory barrier. Since all this is more or less platform dependent, let's say Visual C++ x86 or x64, though differences/notes for other platforms are welcome, too.


(a prelimenary "thanks but" for recommending libraries such as Intel TBB - I am trying to understand the platform issues here)

A: 

Well here's one problem:

Your send function needs to pass in newData by reference (or pointer to pointer). Otherwise the result of the swap never makes it back to the caller.

You won't need volatile just means that the data is always read from memory everytime it's accessed. Since your program is always changing the value of stage, the compiler will know whats going on and everything will be fine. You only use volatile if you have something outside of your program changing the value. E.g, you have a serial port that is sending data to a place in memory and you have your program polling that memory for updates. Every time you poll that memory, you have to check the memory, not the cache, and that's where you'd use volatile.

miked
"return newData" should make sure that the swapped value gets back to the caller.
Mark B
Yeah, but stage is getting lost. The comment there says transfers ownership, so it seems like no one is freeing the data to stage. It may be right, but it just seem suspect to me.
miked
+2  A: 

You don't need volatile here. Use volatile only if the value can change due to something outside of your program, such as if the variable represents a memory-mapped hardware register. The values here are only modified inside your program, so you can trust the compiler to know when it can and can't cache the values.

If you need to make sure the worker and controller aren't accessing the shared data at the same time, I would recommend that you use a mutex instead. In both your Send and Fetch functions, simply lock the mutex, manipulate stage, and release the mutex. I don't know what system libraries you have available, but there's a good description of POSIX mutexes (from pthreads) here. The Win32 version (albeit with less explanation) is available here. Other libraries will use different names, but the concept is the same.

bta
I've used "Mutex" + "ScopedLock" as an abstraction (in practice it's usually a wrapper around a Win32 CRITICAL_SECTION).
peterchen
Your code snippet locks the mutex, but never releases the lock. Add in some unlock calls after you are done manipulating the shared variable and your code should be fine.
bta