On 02/02/2012 10:53 AM, Gregory Farnum wrote:
I went to merge this but then had a question on part of it (below). On Wed, Feb 1, 2012 at 7:54 AM, Jim Schutt<jaschut@xxxxxxxxxx> wrote:Under heavy write load from many clients, many reader threads will be waiting in the policy throttler, all on a single condition variable. When a wakeup is signalled, any of those threads may receive the signal. This increases the variance in the message processing latency, and in extreme cases can significantly delay a message. This patch causes threads to exit a throttler in the same order they entered. Signed-off-by: Jim Schutt<jaschut@xxxxxxxxxx> --- src/common/Throttle.h | 42 ++++++++++++++++++++++++++++-------------- 1 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/common/Throttle.h b/src/common/Throttle.h index 10560bf..ca72060 100644 --- a/src/common/Throttle.h +++ b/src/common/Throttle.h @@ -3,23 +3,31 @@ #include "Mutex.h" #include "Cond.h" +#include<list> class Throttle { - int64_t count, max, waiting; + int64_t count, max; uint64_t sseq, wseq; Mutex lock; - Cond cond; + list<Cond*> cond; public: - Throttle(int64_t m = 0) : count(0), max(m), waiting(0), sseq(0), wseq(0), + Throttle(int64_t m = 0) : count(0), max(m), sseq(0), wseq(0), lock("Throttle::lock") { assert(m>= 0); } + ~Throttle() { + while (!cond.empty()) { + Cond *cv = cond.front(); + delete cv; + cond.pop_front(); + } + } private: void _reset_max(int64_t m) { - if (m< max) - cond.SignalOne(); + if (m< max&& !cond.empty()) + cond.front()->SignalOne(); max = m; } bool _should_wait(int64_t c) { @@ -28,19 +36,24 @@ private: ((c< max&& count + c> max) || // normally stay under max (c>= max&& count> max)); // except for large c } + bool _wait(int64_t c) { bool waited = false; - if (_should_wait(c)) { - waiting += c; + if (_should_wait(c) || !cond.empty()) { // always wait behind other waiters. + Cond *cv = new Cond; + cond.push_back(cv); do { + if (cv != cond.front()) + cond.front()->SignalOne(); // wake up the oldest.What's this extra wakeup for? Unless I'm missing something it's always going to be gratuitous. :/
I think it was a poorly thought-out attempt at defensive programming. Now that I'm thinking about it harder, I agree it is gratuitous. Thanks -- Jim
waited = true; - cond.Wait(lock); - } while (_should_wait(c)); - waiting -= c; + cv->Wait(lock); + } while (_should_wait(c) || cv != cond.front()); + delete cv; + cond.pop_front(); // wake up the next guy - if (waiting) - cond.SignalOne(); + if (!cond.empty()) + cond.front()->SignalOne(); } return waited; } @@ -101,7 +114,7 @@ public: bool get_or_fail(int64_t c = 1) { assert (c>= 0); Mutex::Locker l(lock); - if (_should_wait(c)) return false; + if (_should_wait(c) || !cond.empty()) return false; count += c; return true; } @@ -110,7 +123,8 @@ public: assert(c>= 0); Mutex::Locker l(lock); if (c) { - cond.SignalOne(); + if (!cond.empty()) + cond.front()->SignalOne(); count -= c; assert(count>= 0); //if count goes negative, we failed somewhere! } -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
-- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
