5

I have two threads trying to lock the same boost::mutex. One of those threads is continuously processing some data, and the other is periodically displaying the current state. The processing thread, according to my intention, releases the lock very frequently and reacquires it, so that the display thread can tap in and acquire it whenever it needs it. So, obviously, I would like the display thread to acquire the lock next time it's released by the process thread. However, it doesn't do that, instead, it waits for the lock and only acquires it after many lock-release cycles from the process thread.

Please inspect the minimal example illustrating my problem:

#include <boost/thread.hpp>
#include <iostream>

using namespace std;
using namespace boost;

mutex mut;

void process() {
        double start = time(0);
        while(1) {
                unique_lock<mutex> lock(mut);
                this_thread::sleep(posix_time::milliseconds(10));
                std::cout<<".";
                if(time(0)>start+10) break;
        }
}

int main() {

        thread t(process);

        while(!t.timed_join(posix_time::seconds(1))) {
                posix_time::ptime mst1 = posix_time::microsec_clock::local_time();
                cout<<endl<<"attempting to lock"<<endl;
                cout.flush();

                unique_lock<mutex> lock(mut);

                posix_time::ptime mst2 = posix_time::microsec_clock::local_time();
                posix_time::time_duration msdiff = mst2 - mst1;
                cout << std::endl<<"acquired lock in: "<<msdiff.total_milliseconds() << endl;
                cout.flush();
        }

}

Compiled with: g++ mutextest.cpp -lboost_thread -pthread

When I run the executable, a sample output is like this:

...................................................................................................
attempting to lock
....................................................................................................................................................................................................................................................................................................................................................................................................................................
acquired lock in: 4243
...................................................................................................
attempting to lock
........................................................................................................
acquired lock in: 1049
...................................................................................................
attempting to lock
........................................................................................................................
acquired lock in: 1211
....................................

As you can see, in the worst case, the display thread waits for 424 lock-release cycles before it comes round to catching the lock.

I'm obviously using the mutex in a wrong way, but what is the usual way to solve this?

enobayram
  • 4,650
  • 23
  • 36

5 Answers5

4

It's not that you're using the mutex wrong, just that threading doesn't do what you expect here. The OS decides which thread runs when (this is known as "scheduling"), and there's nothing in the code that forces a thread switch at the end of the loop; the thread keeps going, and reacquires the lock. One thing to try is to add a call to this_thread::yield() after releasing the lock (in this case, at the top of the loop, before relocking); that will suggest to the scheduler that it's appropriate for another thread to run. If you really want tightly synchronized interleaving, threads won't do it; instead, write a higher-level function that calls your two functions one after the other.

Pete Becker
  • 74,985
  • 8
  • 76
  • 165
  • `yield()` seemed to somewhat help, but it's still bad. With `yield()` before the `unique_lock` construction in the processing thread, I still see up to ~300 lock-release iterations before the display thread catches the lock. I don't really want super-tight interleaving since it's just a display, but I'd appreciate if it didn't wait for 300 freaking iterations :) – enobayram Apr 05 '13 at 16:05
  • Yuk. Try calling `sleep`; when a thread sleeps, any reasonable OS will run some other thread. – Pete Becker Apr 05 '13 at 16:15
  • I've tried that too, if you sleep for 0 milliseconds, than it's as if the sleep doesn't exist, otherwise you can sleep at least for 10 milliseconds and that's simply too much to waste for such a nuisance :/ I mean, it's not like I'm asking for a theoretical impossibility, the lock granting implementation simply needs to give it to the earliest candidate. – enobayram Apr 05 '13 at 16:20
1

If the update thread has nothing else to do it can wait for the mutex to become available.

Look at boost::condition_variable. You can read about it here http://www.boost.org/doc/libs/1_53_0/doc/html/thread/synchronization.html and here Using boost condition variables

If having the update thread go to sleep would be a problem -- it is on many GUI systems and you don't specify which one you are using -- consider posting a message from the processing thread to to updating thread. (again the details depend on your platform.) The message may either contain the information necessary to update the display, or be a notification that "now would be a good time to look."

If you do use a condition code, the processing thread should probably yield after signalling the condition and before reacquiring the lock to open up a large window for the updating thread.

Community
  • 1
  • 1
Dale Wilson
  • 9,166
  • 3
  • 34
  • 52
  • Note that my update thread is already waiting for the mutex to become available, but the problem is that it doesn't acquire the lock for a very long time even after it is available. If the GUI system really matters, my display thread uses OpenGL to do some screen drawing. – enobayram Apr 05 '13 at 16:10
  • The processing thread posting a message to the update thread will not help, really. The display thread can't "take a look" without acquiring the lock, and that's where the problem is anyway. – enobayram Apr 05 '13 at 16:11
1

You may want to have a look at boost::condition_variable, specially at the methods wait() and notify_one() or notify_all(). The method wait() will block the current thread until a the lock is released. The methods notify_one() and notify_all() notify one or all threads that are waiting for the lock to continue execution.

FKaria
  • 1,012
  • 13
  • 14
  • I've already tried the `condition_variable` but it doesn't work. `notify_x` methods bring other threads to a "ready" state, but they don't force a wake-up. Either way, you need the lock in the first place before you wait for it, my problem is not being able to acquire the lock. – enobayram Apr 05 '13 at 16:08
1

As I see it, the principle of mutexes is not fairness, but correctness. Mutexes themselves cannot control the scheduler. One thing that bothers me is the reason why you have chosen to create 2 threads, which use such coarse-grained locking. In theory you are running two things in parallel, but in fact you are making them interdependent/serial.

Pete's idea seem much nicer (one function running these draw & update) and you still can use threads inside each of inner functions, not having to worry about contention and fairness so much.

If you really wish to have two threads running in parallel, then I can give you just a few tips:

  • spinlock using atomics and forget about mutex,
  • go into implementation-defined land and set the thread priorities, or
  • make the locking much more fine-grained.

Neither of these is problem-proof, unfortunately.

Red XIII
  • 5,771
  • 4
  • 28
  • 29
  • Thanks for your suggestions, they all have useful cases, but not mine: A spinlock is simply too wasteful, I want to stay portable and the locking is as fine grained as it could theoretically be. – enobayram Apr 06 '13 at 06:14
  • As for joining them in a single function; I can see that it would be good approach for the minimal example, but the real application is much more complicated. besides, I'm doing other things in the display thread as well. I extract the necessary information from the world model and I release the lock while the display thread handles the OpenGL calls. – enobayram Apr 06 '13 at 06:16
  • Finally, I'm also using the multiple threads model as a way of abstraction, it really simplifies my design. – enobayram Apr 06 '13 at 06:16
  • @enobayram On the contrary, I believe that one function (that delegates tasks to some operators, allowing them to run some tasks in parallel) is great for more complex scenarios, not a minimal one. Obviously it requires careful synchronization, but being able to say: `auto a = runA(); auto b = runB(); runC(); joinA(); joinB();` nicely decomposes the whole complexity. – Red XIII Apr 06 '13 at 10:47
0

I've solved the problem using conditions, as suggested by Dale Wilson an FKaria, but I've used it in the opposite direction. So, the process thread checks for a pause flag, and when it's set, it waits for a condition, therefore releasing the lock. The display thread controls the pause flag, and it also resumes the process thread by notifying it through the condition. Code: (The code is mostly the same, I've marked the new lines with //New)

#include <boost/thread.hpp>
#include <boost/thread/condition.hpp>
#include <iostream>

using namespace std;
using namespace boost;

mutex mut;
condition cond;

volatile bool shouldPause = false; //New

void process() {
        double start = time(0);
        while(1) {
                unique_lock<mutex> lock(mut);

                if(shouldPause) cond.wait(mut); //New

                this_thread::sleep(posix_time::milliseconds(10));
                std::cout<<".";
                if(time(0)>start+10) break;
        }
}

int main() {

        thread t(process);

        while(!t.timed_join(posix_time::seconds(1))) {
                posix_time::ptime mst1 = posix_time::microsec_clock::local_time();
                cout<<endl<<"attempting to lock"<<endl;
                cout.flush();
                shouldPause = true; // New
                unique_lock<mutex> lock(mut);

                posix_time::ptime mst2 = posix_time::microsec_clock::local_time();
                posix_time::time_duration msdiff = mst2 - mst1;
                cout << std::endl<<"acquired lock in: "<<msdiff.total_milliseconds() << endl;
                cout.flush();

                shouldPause = false; // New
                cond.notify_all(); // New
        }

}

Now the output is precisely as I would like it to be:

...................................................................................................
attempting to lock
.
acquired lock in: 9
...................................................................................................
attempting to lock
.
acquired lock in: 8
...................................................................................................
attempting to lock
.
acquired lock in: 9
...................................................................................................
attempting to lock
.
acquired lock in: 8
...................................................................................................
attempting to lock
.
acquired lock in: 9
...................................................................................................
attempting to lock
.
acquired lock in: 9
...................................................................................................
attempting to lock
.
acquired lock in: 9
...................................................................................................
attempting to lock
.
acquired lock in: 9
...................................................................................................
attempting to lock
.
acquired lock in: 8
...................................................................................................
attempting to lock
.
acquired lock in: 9
..........................
enobayram
  • 4,650
  • 23
  • 36