views:

231

answers:

3

The naming of this function seems like this is some complicated stuff going on. When exactly does one know that this is the way to go instead of doing something like this:

Preparation CRITICAL_SECTION cs; int *p = malloc(sizeof(int)); // Allocation Site InitializeCriticalSection(&cs); // HINT for first Write

Thread #1 { *p = 1; // First Write }

Thread #2 { EnterCriticalSection(&cs); *p = 2; // Second Write LeaveCriticalSection(&cs); }

I have a write that gets done in one thread:

Run()
{
// some code
m_bIsTerminated = TRUE;
// some more code
}

Then, I have a read that gets done in another thread (potentially at the same time):

Terminate()
{
// some code
if( m_bIsTerminated )
{
m_dwThreadId = 0;
m_hThread = NULL;
m_evExit.SetEvent();
return;
}
// even more code
}

What's the best solution to solve this race condition? Are critical sections the way to go or is the use of InterlockedExchangeAdd() more useful?

+3  A: 

In your case, there's no race condition. The variable is never reset back to FALSE, is it? It's just a "please die" switch for the thread, right? Then no need for synchronization of any kind.

The InterlockedXXX family of functions makes use of Intel CPU's atomic 3-operand commands (XADD and CMPXCNG). So they're much cheaper than a critical section. And the one you want for thread-safe assignment is InterlockedCompareExchange().

UPD: and the mark the variable as volatile.

Seva Alekseyev
There is a problem. The value may sit in one core's cache and the other core may take quite some time to see the updated value.
Omnifarious
Rather than volatile, use a memory barrier after updating the variable. That has the semantics you actually want (perform all writes *now*). Volatile isn't ideal as 1) it doesn't prevent writes from being reordered, and 2) it forces *all* writes to the variable to be written through immediately, which you don't necessarily need.
jalf
A: 

Make sure m_bIsTerminated is marked as volatile, and you should be ok. Although it seems pretty weird to me that you'd // some more code after setting "is termianted" to true. What exactly does that variable indicate?

Your "race condition" is that your various elements of // more code can execute in different orders. Your variable doesn't help that. Is your goal to get them to execute in a deterministic order? If yes, you'd need a condition variable to wait on one thread and set in another. If you just don't want them executing concurrently, a critical section would be fine.

Terry Mahaffey
It indicates that the current thread has been signaled to terminate, then it returns the thread back to pool while other threads continue to run unless they have been signaled to terminate as well.
Brian T Hannan
volatile is sufficent in this example, where one thread is reading and one thread is writing.
Terry Mahaffey
You're right - it doesn't matter the exact order that the other thread sees the write, just that it does actually see it.
Michael
A: 

InterlockedExchangeAdd is used to add a value to an integer as an atomic operation, meaning that you won't have to use a critical section. This also removes the risk of a deadlock if one of your threads throws an exception - you need to make sure that you don't keep any lock of any kind as that would prevent other threads from acquiring that lock.

For your scenario you can definitely use an Interlocked...- function, but I would use an event (CreateEvent, SetEvent, WaitForSingleObject), probably because I often find myself needing to wait for more than one object (you can wait for zero seconds in your scenario).

Upd: Using volatile for the variable may work, however it isn't recommended, see: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2016.html and http://www-949.ibm.com/software/rational/cafe/blogs/ccpp-parallel-multicore/tags/c%2B%2B0x for instance.

If you want to be portable, take a look at boost::thread.

villintehaspam
I am using events for normal thread start and finish events, but this is for when you want to terminate immediately before the thread finishes it's work. I want to make sure that it terminates gracefully and puts the thread back in the pool so it can be used later, if necessary.
Brian T Hannan
Those calls are Windows specific.
Omnifarious
@Omnifarious: Since the OP mentions InterlockedExchangeAdd and the question is tagged Windows, I think it is fair to assume that the question is Windows specific as well.
villintehaspam
@Brian: I'm not sure I follow. Are you looking for something that will exit faster than your normal exit? There is nothing that prevents you from using events for this unless I've misunderstood your question.
villintehaspam
I guess it is Windows-specific now b/c Neil Butterworth decided to edit my original posting. The original question did not mention anything about Windows. In the future, if possible, we want to make our code portable to Linux and MAC systems as well. But for now we are sticking with Windows and trying to do as little windows-specific things as possible.
Brian T Hannan
@villintehasspam: I am not the developer of this project, I am a tester and I noticed that there could be a possible race condition using Intel Parallel Studio. We are not looking to change the entire infrastructure of the code with all the other events that happen. I am simply asking about this one condition.
Brian T Hannan
Ok, I have more information for clarity for you. We have a Run() and Terminate() function and m_bIsTerminated is originally FALSE. We create and run the thread and if we call terminate while the thread is still running it will set m_bIsTerminated. Meanwhile, in Run() there is a check inside the look to see whether m_bIsTerminated is set, then it sets the thread id to zero, makes the thread handle NULL and calls m_evExit.SetEvent() to set the exit event.
Brian T Hannan
After reading a bit, it seems that the volatile keyword isn't really meant to be used for this type of thing either, so using that isn't really portable. I would recommend that you use an event or an Interlocked...-function to do what you want. You could also take a look at boost::thread ( http://www.boost.org/doc/libs/1_41_0/doc/html/thread.html ), which is portable.
villintehaspam
I've updated my answer with some links on why volatile isn't recommended.
villintehaspam