Re: [RFC PATCH 3/6] common/Throttle: throttle in FIFO order

[Gregory Farnum <gregory.farnum@xxxxxxxxxxxxx>]

On Thu, Feb 2, 2012 at 10:31 AM, Jim Schutt <jaschut@xxxxxxxxxx> wrote:
> 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

Awesome. Applied to master in commit:83432af2adce75676b734d2b99dd88372ede833a.
-Greg
--
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