From 35c5041b506c959e455e67fab837cf6b99e5a2d0 Mon Sep 17 00:00:00 2001 From: Armin Weatherwax Date: Tue, 2 Mar 2010 14:40:59 +0100 Subject: Robin Cornelius, Aleric Inglewood: SNOW-196 missing mutexes for texture fetch Applys a trylock() abort system to allow the worker thread to avoid deadlocks due to a race between mQueueMutex and mWorkerMutex. --- linden/doc/contributions.txt | 2 ++ linden/indra/llcommon/llthread.cpp | 11 +++++++++++ linden/indra/llcommon/llthread.h | 3 ++- linden/indra/newview/lltexturefetch.cpp | 28 ++++++++++++++++++---------- linden/indra/newview/lltexturefetch.h | 9 ++++----- 5 files changed, 37 insertions(+), 16 deletions(-) diff --git a/linden/doc/contributions.txt b/linden/doc/contributions.txt index a1ec128..14807e1 100644 --- a/linden/doc/contributions.txt +++ b/linden/doc/contributions.txt @@ -40,6 +40,7 @@ Aimee Trescothick Alejandro Rosenthal VWR-1184 Aleric Inglewood + SNOW-196 SNOW-408 VWR-10759 VWR-10837 @@ -473,6 +474,7 @@ Ringo Tuxing CT-321 Robin Cornelius SNOW-108 + SNOW-196 VWR-2488 VWR-9557 Ryozu Kojima diff --git a/linden/indra/llcommon/llthread.cpp b/linden/indra/llcommon/llthread.cpp index b39ffb6..60ccd5c 100644 --- a/linden/indra/llcommon/llthread.cpp +++ b/linden/indra/llcommon/llthread.cpp @@ -322,6 +322,17 @@ bool LLMutex::isLocked() } } +//trys to grab a lock and if sucessful returns true +bool LLMutex::tryLock() +{ + apr_status_t status = apr_thread_mutex_trylock(mAPRMutexp); + if (APR_STATUS_IS_EBUSY(status)) + { + return false; + } + return true; +} + //============================================================================ LLCondition::LLCondition(apr_pool_t *poolp) : diff --git a/linden/indra/llcommon/llthread.h b/linden/indra/llcommon/llthread.h index 721b6e7..d773fa4 100644 --- a/linden/indra/llcommon/llthread.h +++ b/linden/indra/llcommon/llthread.h @@ -134,7 +134,8 @@ public: void lock(); // blocks void unlock(); bool isLocked(); // non-blocking, but does do a lock/unlock so not free - + bool tryLock(); //non-blocking, but not free, returns true if grabed lock + protected: apr_thread_mutex_t *mAPRMutexp; apr_pool_t *mAPRPoolp; diff --git a/linden/indra/newview/lltexturefetch.cpp b/linden/indra/newview/lltexturefetch.cpp index 8ff9e9f..3a5665c 100644 --- a/linden/indra/newview/lltexturefetch.cpp +++ b/linden/indra/newview/lltexturefetch.cpp @@ -744,13 +744,17 @@ bool LLTextureFetchWorker::doWork(S32 param) } else if (mSentRequest == UNSENT) { - // Add this to the network queue and sit here. - // LLTextureFetch::update() will send off a request which will change our state - mRequestedSize = mDesiredSize; - mRequestedDiscard = mDesiredDiscard; - mSentRequest = QUEUED; - mFetcher->addToNetworkQueue(this); - setPriority(LLWorkerThread::PRIORITY_LOW | mWorkPriority); + if(mFetcher->mQueueMutex.tryLock()) + { + // Add this to the network queue and sit here. + // LLTextureFetch::update() will send off a request which will change our state + mRequestedSize = mDesiredSize; + mRequestedDiscard = mDesiredDiscard; + mSentRequest = QUEUED; + mFetcher->addToNetworkQueue(this); + setPriority(LLWorkerThread::PRIORITY_LOW | mWorkPriority); + mFetcher->mQueueMutex.unlock(); + } return false; } else @@ -800,7 +804,7 @@ bool LLTextureFetchWorker::doWork(S32 param) // Set the throttle to the entire bandwidth, assuming UDP packets will get priority // when they are needed F32 max_bandwidth = mFetcher->mMaxBandwidth; - if ((mFetcher->getHTTPQueueSize() >= HTTP_QUEUE_MAX_SIZE) || + if ((mFetcher->getNumHTTPRequests() >= HTTP_QUEUE_MAX_SIZE) || (mFetcher->getTextureBandwidth() > max_bandwidth)) { // Make normal priority and return (i.e. wait until there is room in the queue) @@ -1469,12 +1473,13 @@ void LLTextureFetch::deleteRequest(const LLUUID& id, bool cancel) } } +// Need to hold mQueueMutex when entering this function SNOW-196 +// Snowglobe ToDo fix holding mQueueMutex over the entire function, we only need +// it for the mRequestMap access // protected void LLTextureFetch::addToNetworkQueue(LLTextureFetchWorker* worker) { - mQueueMutex.lock(); bool is_worker_in_request_map = (mRequestMap.find(worker->mID) != mRequestMap.end()); - mQueueMutex.unlock(); LLMutexLock lock(&mNetworkQueueMutex); if (is_worker_in_request_map) @@ -1916,6 +1921,7 @@ bool LLTextureFetch::receiveImageHeader(const LLHost& host, const LLUUID& id, U8 { // llwarns << "Received header for non active worker: " << id << llendl; ++mBadPacketCount; + LLMutexLock lock2(&mNetworkQueueMutex); mCancelQueue[host].insert(id); return false; } @@ -1944,6 +1950,7 @@ bool LLTextureFetch::receiveImageHeader(const LLHost& host, const LLUUID& id, U8 if (!res) { ++mBadPacketCount; + LLMutexLock lock2(&mNetworkQueueMutex); mCancelQueue[host].insert(id); } else @@ -1988,6 +1995,7 @@ bool LLTextureFetch::receiveImagePacket(const LLHost& host, const LLUUID& id, U1 if (!res) { ++mBadPacketCount; + LLMutexLock lock2(&mNetworkQueueMutex); mCancelQueue[host].insert(id); return false; } diff --git a/linden/indra/newview/lltexturefetch.h b/linden/indra/newview/lltexturefetch.h index c48f609..5eca0ba 100644 --- a/linden/indra/newview/lltexturefetch.h +++ b/linden/indra/newview/lltexturefetch.h @@ -76,8 +76,8 @@ public: S32 getFetchState(const LLUUID& id, F32& decode_progress_p, F32& requested_priority_p, U32& fetch_priority_p, F32& fetch_dtime_p, F32& request_dtime_p); void dump(); - S32 getNumRequests() { return mRequestMap.size(); } - S32 getNumHTTPRequests() { return mHTTPTextureQueue.size(); } + S32 getNumRequests() const { LLMutexLock lock(&mQueueMutex); return mRequestMap.size(); } + S32 getNumHTTPRequests() const { LLMutexLock lock(&mNetworkQueueMutex); return mHTTPTextureQueue.size(); } // Public for access by callbacks void lockQueue() { mQueueMutex.lock(); } @@ -91,7 +91,6 @@ protected: void removeFromNetworkQueue(LLTextureFetchWorker* worker, bool cancel); void addToHTTPQueue(const LLUUID& id); void removeFromHTTPQueue(const LLUUID& id); - S32 getHTTPQueueSize() { return (S32)mHTTPTextureQueue.size(); } void removeRequest(LLTextureFetchWorker* worker, bool cancel); // Called from worker thread (during doWork) void processCurlRequests(); @@ -110,8 +109,8 @@ public: S32 mBadPacketCount; private: - LLMutex mQueueMutex; - LLMutex mNetworkQueueMutex; + mutable LLMutex mQueueMutex; + mutable LLMutex mNetworkQueueMutex; LLTextureCache* mTextureCache; LLImageDecodeThread* mImageDecodeThread; -- cgit v1.1