Sign in to follow this  
kiske1

Boost thread mutex

Recommended Posts

Hi there... I'm very new to multithreaded programming, and I've a question regarding the boost thread library. I have this class (only relevant code):
class cBall
{
private:
	cBall();
	static cBall* m_pGlobalBall;

        boost::shared_ptr<boost::thread> m_BallThread;
        boost::mutex	m_BallMutex;
        bool		m_bThreadStop;


        [... cut ...]
public:
	~cBall();

	void	StartUpdateThread();		
	void	JoinUpdateThread();		
	void	StopUpdateThread();

	void	Render();	
        
        [... cut ...]
};					

And there's the implementation

void cBall::UpdatePosition()
{
	do
	{
           boost::mutex::scoped_lock l(m_BallMutex);

           [do some calculation]

	    if (m_bDataRead)
	    {
                m_vPosizioneReale = m_vPosizione;
		m_bDataRead = FALSE;
	    }           

	}
	while(!m_bThreadStop);

}

void cBall::StartUpdateThread()
{
	m_BallThread = boost::shared_ptr<boost::thread>(new boost::thread(boost::bind(&cBall::UpdatePosition, this)));
	m_bThreadStop = FALSE;
}

void cBall::JoinUpdateThread()
{
	m_BallThread->join();
}

void cBall::StopUpdateThread()
{
	m_bThreadStop = TRUE;
	m_BallThread->join();
}

void cBall::Render()
{
	if (!m_bDataRead)
	{
		boost::mutex::scoped_lock l(m_BallMutex);

		m_NodeBall->setPosition(m_VPosizioneReale);
		m_bDataRead = TRUE;
	}

}

The "render" function runs on the main thread. When I get to ball set position I get wrong data in m_VPosizioneReale... I'm sure I'm doing something wrong, but I don't know where... Many thanks in advance... Juri

Share this post


Link to post
Share on other sites
In cBall::Render(), you're reading from m_bDataRead without first locking the mutex.
Locking or unlocking the mutex will cause a memory barrier, which prevents the CPU from re-ordering read/write instructions and screwing up multi-threaded data sharing.

Without a memory barrier, the CPU can re-order this code:
                m_vPosizioneReale = m_vPosizione;
m_bDataRead = FALSE;
to look like this instead:
		m_bDataRead = FALSE;
m_vPosizioneReale = m_vPosizione;
which will break your program.

[EDIT] Actually, this shouldn't be a problem for you.
Even if the wrong value of m_bDataRead is read, both threads should be placing a barrier after writing to and before reading from m_VPosizioneReale (so it's value should always be valid/up-to-date, even if m_bDataRead is invalid).

Share this post


Link to post
Share on other sites
Many thanks for the answers...

The volatile keyword solved the data corruption, but the animation is not smooth as I expected.

The calculations done in the UpdatePosition worked previously well in single threaded app (the UpdatePosistion where called once per frame, just before the render).

Now the object movement is not smooth, it seems just strange, because I've checked the timings and the UpdatePosition procedure cycle takes about 0.00129 seconds, and the rendering cycle is on 50/60 fps, so ~0.02 seconds....

Share this post


Link to post
Share on other sites
Quote:
Original post by swiftcoder
volatile probably isn't doing what you think it does.


Thanks for the interesting link... I was confused by MSDN too, becuase in this page MSDN LINK they say:

Quote:
"The volatile keyword is a type qualifier used to declare that an object can be modified in the program by something such as the operating system, the hardware, or a concurrently executing thread."


For my problem I've now removed the volatile keyword but I've the same stutter movement of the object.

As I have one thread that writes the data and another one that reads it, maybe should I use the boost shared_mutex for read/write operations?
In case could anyone post a little example that fits my case? I've tried to read the boost documentation, but I can't find the right way to implement the shared mutex within my code...

Once again thanks

Share this post


Link to post
Share on other sites
Quote:
Original post by kiske1
I have this class (only relevant code):


class cBall
{
private:
cBall();
static cBall* m_pGlobalBall;


Try really hard to avoid class-/file-/function-static and global objects of any kind in threaded code.

Quote:

And there's the implementation


// ...

void cBall::StartUpdateThread()
{
m_BallThread = boost::shared_ptr<boost::thread>(new boost::thread(boost::bind(&cBall::UpdatePosition, this)));
m_bThreadStop = FALSE;
}



These two statements are in the wrong order. It's entirely possible that one iteration of the loop in UpdatePosition will occur before m_bThreadStop is set to FALSE. Perhaps this is unrelated to your current problem, but it is a bug nonetheless.

You can also write the thread creation line somewhat more succinctly:


m_BallThread.reset(new boost::thread(boost::bind(&cBall::UpdatePosition, this)));



Quote:


// ...

void cBall::StopUpdateThread()
{
m_bThreadStop = TRUE;
m_BallThread->join();
}



m_bThreadStop should be protected by the mutex as it is read from and written to by different threads. If you don't do this, old values may stay resident in one of the various layers of cache.

Quote:


// ...

void cBall::Render()
{
if (!m_bDataRead)
{
boost::mutex::scoped_lock l(m_BallMutex);

m_NodeBall->setPosition(m_VPosizioneReale);
m_bDataRead = TRUE;
}

}



You have the same issue again here with m_bDataRead.

You need to have a very clear idea of the invariants that your mutex is protecting. All the levels of the cache must have a consistent view of your shared data for the threads to be properly coordinated.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this