24

Assume there are two threads running Thread1() and Thread2() respectively. The thread 1 just sets a global flag to tell thread 2 to quit and thread 2 periodically checks if it should quit.

volatile bool is_terminate = false;

void Thread1()
{
    is_terminate = true;
}

void Thread2()
{
    while (!is_terminate) {
        // ...
    }
}

I want to ask if the above code is safe assuming that access to is_terminate is atomic. I already know many materials state that volatile can not insure thread-safety generally. But in the situation that only one atomic variable is shared, do we really need to protect the shared variable using a lock?

Hosam Aly
  • 41,555
  • 36
  • 141
  • 182
spockwang
  • 897
  • 1
  • 6
  • 15
  • Why *not* protect the shared variable with a lock? – jalf Jul 06 '11 at 06:23
  • 2
    Can you clarify what you mean when you say the access is 'atomic'? Is it a guarantee from your platform? Does it come with ordering guarantees? – Luc Danton Jul 06 '11 at 06:25
  • 7
    Funny how the answers to this question include anything from "Not safe", "I believe it's safe", "Probably sort of safe" to "Yes, it's safe". – Frerich Raabe Jul 06 '11 at 06:32
  • 2
    @Frerich : that's probably because it's highly platform dependent. When looking at the C++ standard though, there are no guarantees - so from that point of view, there's only one valid answer. – Sander De Dycker Jul 06 '11 at 06:36
  • 3
    @jalf: Because lock is expensive and in some simple situations I want to avoid it. – spockwang Jul 06 '11 at 07:39
  • Why not use a binary semaphore instead? Platform independence? – JAB Jul 06 '11 at 14:33

9 Answers9

18

It is probably sort of thread-safe.

Thread safety tends to depend on context. Updating a bool is always thread safe, if you never read from it. And if you do read from it, then the answer depends on when you read from it, and what that read signifies.

On some CPUs, but not all, writes to an object of type bool will be atomic. x86 CPUs will generally make it atomic, but others might not. If the update isn't atomic, then adding volatile won't help you.

But the next problem is reordering. The compiler (and CPU) will carry out reads/writes to volatile variables in the order specified, without any reordering. So that's good.

But it makes no guarantee about reordering one volatile memory access relative to all the non-volatile ones. So a common example is that you define some kind of flag to protect access to a resource, you make the flag volatile, and then the compiler moves the resource access up so it happens before you check the flag. It's allowed to do that, because it's not reordering the internal ordering of two volatile accesses, but merely a volatile and a non-volatile one.

Honestly, the question I'd ask is why not just do it properly? It is possible that volatile will work in this situation, but why not save yourself the trouble, and make it clearer that it's correct? Slap a memory barrier around it instead.

jalf
  • 243,077
  • 51
  • 345
  • 550
  • 1
    +1: I remember reading somewhere that your "may work" scenario is true for MSVC 2005, but not later versions, because on that particular version a `volatile` pretty much acts as a memory barrier. So, moral of the story is, you can't be sure, and it is better to protect the access using compiler primitives. – Praetorian Jul 06 '11 at 06:35
  • This is actually **false**. A processor may cache the value of the variable if the architecture is not *cache-coherent* which will make further accesses return the previous value even though there was a write in the meanwhile. Atomicity is **not** everything, you have to consider visibility too. And while a bool is probably atomic, volatile does not guarantees visibility. – Giovanni Funchal Jul 06 '11 at 06:37
  • Yeah, VC2005 and later "upgrades" `volatile` to protect the value with memory barriers. But that's not required by the standard, and so you can't expect other compilers to do the same (and you can't be sure the *next* VC will do it either), so I'd play it safe and use something that's guaranteed to do the right thing. – jalf Jul 06 '11 at 06:37
  • 1
    @Giovanni: how does that make what I say **false** (in bold, even)? Yes, there are details I left out; many, varied details, not just cache coherence. But I fail to see how that makes any of the things I *did* say false. – jalf Jul 06 '11 at 06:38
  • Ok I did read your answer too quickly, the point I was trying to make is that atomicity isn't everything. – Giovanni Funchal Jul 06 '11 at 06:42
  • @Giovanni: That's true. But it's a start :) – jalf Jul 06 '11 at 06:43
  • 2
    My intention is that in this simple situation locking access to the shared variable is not worth the cost. – spockwang Jul 06 '11 at 06:44
  • @wbb indeed, but locking will guarantee side effects such as if you are using `is_terminate` to communicate that a buffer was filled and the other thread may read from it, then locks will guarantee that the whole buffer is written safely to the memory, while atomic accesses won't (surprisingly, isn't it?). – Giovanni Funchal Jul 06 '11 at 06:46
  • @wbb: a: how can you be sure without measuring it, that a lock would be costlier than making it `volatile`? b: how can you be sure that this cost would be a problem? 3: you don't need to use a full-blown lock. A memory barrier, which prevents reordering, and ensures that earlier writes are made visible, would suffice. – jalf Jul 06 '11 at 06:53
  • 1
    @jalf: I think in the situation which I post in the question memory reordering is not a problem. After thread 1 writes is_teriminate some time later thread 2 will see the change eventually and will terminate. is_terminate is not intended to protect resources but a flag to tell the thread to quit. – spockwang Jul 06 '11 at 07:43
  • 1
    @wbb: if "eventually" is good enough for you, then yes, it should be safe. But notice how many "if"'s you're stacking on top of each others now. The next time you or someone else reads this code, they're going to have to stop and pick this apart, to try to figure out if it *is* safe, or if you were just on drugs when you wrote it. Why not go with something more readable, more explicit, and just as fast? Something that indicates to the reader of the code "I have actually considered the thread safety of this, and here's the solution"? – jalf Jul 06 '11 at 07:46
  • By the way, please use backticks (`\``) to surround inline code, rather than `` tags, which, as you might have noticed, don't work. – jalf Jul 06 '11 at 07:47
  • @Giovanni, are there any non-cache-coherent architectures? Last I read was that they were too hard to develop for so really aren't used, so much that the term NUMA has basically come to mean ccNUMA. – edA-qa mort-ora-y Jul 06 '11 at 09:22
  • Yes there are. In most platforms, the DMA is non-coherent with respect to the CPU. Embedded powerPCs and multicore Amigas are CPU/CPU non-coherent. – Giovanni Funchal Jul 06 '11 at 13:03
11

It is not thread safe.

If the threads, for example, are run on CPUs with separate caches there are no language rules saying that the caches are to be synchronized when writing a volatile variable. The other thread may not see the change for a very long time, if ever.

To answer in another way:

If volatile is enough to be thread safe, why is C++0x adding an entire chapter with atomic operations?

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2047.html

Bo Persson
  • 90,663
  • 31
  • 146
  • 203
  • 4
    Not quite true. The CPU itself synchronizes the caches all the time. There's no way a value can be in one CPU's cache, without other cores seeing it. – jalf Jul 06 '11 at 06:33
  • @jalf - You are talking x86. What about Itanium? – Bo Persson Jul 06 '11 at 06:36
  • @jalf Bo is using the plural where you're using the singular for 'CPU'. – Luc Danton Jul 06 '11 at 06:37
  • @Bo: that's true. Most common CPUs, but not all, have coherent caches. – jalf Jul 06 '11 at 06:41
  • @Bo, I have checked the same code on HP-UX IA64 11.31 and gcc 4.3.1 and it even works without volatile. I mean setting a boolean in one thread means that another thread will see it quickly. –  Jul 06 '11 at 06:41
  • 2
    @skwllsp: just running the code guarantees nothing. Will it work with the *next* compiler version too? Will it work if you run it again tomorrow? What guarantees are provided by either the CPU or the compiler? That's what matters when you consider thread safety – jalf Jul 06 '11 at 06:43
  • Its not the CPU itself, its the multitasking. OS does context switching which flushes the cache in most modern processors. No guarantee about future/embedded processors though. – Giovanni Funchal Jul 06 '11 at 06:44
  • @jalf, My point was that @Bo just mentioned Itanium and I wanted to tell him that basicly that he said was not true about Itanium. I know what you are telling me. –  Jul 06 '11 at 06:47
4

First, volatile is used for disabling compile optimization in c/c++. see this for understanding volatile.

The core of atomic is word align and size of is_terminate, if size of is_terminate is less than machine native size and aligned, then R and W of it is atomic.

In your context, with or without volatile, thread2 may read old value after thread1 modified it, but thread2 can read it eventually.

If eventually-read is OK for you, then your codes are thread safety.

Chang
  • 3,953
  • 2
  • 30
  • 43
2

it's safe because one thread is only reading and one is only writing.

The threads aren't really sharing that flag, one is reading, one is writing. You can't have a race because the other thread will never write a bad result, and the reading thread will never read a bad result. simple.

marinara
  • 538
  • 1
  • 5
  • 10
  • Simple? Can you be sure that the reading thread will immediately see the write from the other thread? The reading thread might quit a few iterations too late otherwise. Is that safe? – jalf Jul 06 '11 at 06:32
  • 3
    jalf: No one said anything about requiring a strictly timed effect. From the context of the question, it's clearly a simple shutdown flag to cancel some operation on a worker thread. – Inverse Jul 08 '11 at 15:41
  • 1
    BTW, THE ITANIUM IS ONLY CACHE- INCOHERENT FOR SELF-MODIFYING CODE. THE EXAMPLE IS NOT SELF-MODIFYING. Still think i'm wrong? – marinara Jul 08 '11 at 21:26
1

No, it is not. It could be thread safe if the value access was atomic, but in C++ you can't assume that variables access is thread-safe unless you use some compiler-specific constructs or synchronization primitives.

sharptooth
  • 167,383
  • 100
  • 513
  • 979
  • Why change in answer even if OP has already mentioned: `if above code is safe ?` He might have platform / implementation support for that. – iammilind Jul 06 '11 at 06:29
1

It is still not safe. You should use synchronizaton to access is_terminate Access to the bool is not guaranteed to be an atomic operation.

Alok Save
  • 202,538
  • 53
  • 430
  • 533
1

I believe that this code is safe, until both the threads are not writing the bool (already you have mentioned that value access is atomic).

iammilind
  • 68,093
  • 33
  • 169
  • 336
1

The big problem with assuming that the volatile keyword imposes any kind of thread safety, is that the C or C++ standards have no concept of threads in the abstract machine they describe.

The guarantees that the standard imposes on the volatile keyword, are only valid within a thread - not between multiple threads.

This leaves implementors with full liberty to do whatever they please when it comes to threads. If they chose to implement the volatile keyword to be safe across threads, then you're lucky. More often than not, that's not the case though.

Sander De Dycker
  • 16,053
  • 1
  • 35
  • 40
  • 1
    An even bigger problem with the `volatile` keyword is that many compilers don't do `volatile` right, threaded or not. For example, see http://www.cs.utah.edu/~regehr/papers/emsoft08-preprint.pdf and http://www.eetimes.com/design/embedded/4008879/When-good-compilers-go-bad-or-What-you-see-is-not-what-you-execute . – David Hammen Jul 06 '11 at 08:02
  • @David : that's interesting, but I'm not really surprised, because it's quite hard to implement `volatile` correctly, while still trying to keep a good performance. It would be interesting to see how these results for gcc compare to other compilers though. – Sander De Dycker Jul 06 '11 at 08:18
-1

This code isn't seems to be thread safe. Reason can be explained easily.

Problem lies in below code line

"is_terminate = true;"

Even if access to "is_terminate" is atomic, above statement is not atomic. This statement includes more than 1 operations. Like load "is_terminate" and update "is_terminate".

Now gotcha is if is_terminate is loaded and not updated and thread switches to another one. Now thread 2 expected result to be true but it won't get it.

Make sure "is_terminate = true;" is atomic. So lock it.

Hope it helps.

Hem
  • 97
  • 2
  • 10