| Message ID | 1308767187-10376-4-git-send-email-jaschut@sandia.gov (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 22, 2011 at 11:26 AM, Jim Schutt <jaschut@sandia.gov> wrote: > This will enable SimpleMessenger to send keepalives to clients while > blocked in the policy throttler. > > Signed-off-by: Jim Schutt <jaschut@sandia.gov> > --- > src/common/Throttle.h | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 40 insertions(+), 0 deletions(-) This looks good. Only a small nitpick-- it's better not to pass utime_t by value, since it's a struct. There's a bunch of other code doing it, but it's better to pass a const reference. Also, I guess we need to rebase this because g_clock has been replaced by ceph_clock_now() in master. Should be easy though. cheers, Colin > > diff --git a/src/common/Throttle.h b/src/common/Throttle.h > index 8fd2625..5a028cc 100644 > --- a/src/common/Throttle.h > +++ b/src/common/Throttle.h > @@ -27,6 +27,9 @@ private: > ((c < max && count + c > max) || // normally stay under max > (c >= max && count > max)); // except for large c > } > + > + /* Returns true if it had to block/wait, false otherwise. > + */ > bool _wait(int64_t c) { > bool waited = false; > if (_should_wait(c)) { > @@ -44,6 +47,28 @@ private: > return waited; > } > > + /* Returns true if it timed out while blocked/waiting, > + * false otherwise. > + */ > + bool _timed_wait(utime_t interval, int64_t c) { > + if (_should_wait(c)) { > + utime_t timeout_at = g_clock.now() + interval; > + waiting += c; > + do { > + if (cond.WaitUntil(lock, timeout_at)) { > + waiting -= c; > + return true; > + } > + } while (_should_wait(c)); > + waiting -= c; > + > + // wake up the next guy > + if (waiting) > + cond.SignalOne(); > + } > + return false; > + } > + > public: > int64_t get_current() { > Mutex::Locker l(lock); > @@ -79,6 +104,21 @@ public: > count += c; > } > > + /* Returns true if it got the requested amount within > + * the specified interval, false otherwise. > + */ > + bool timed_get(utime_t interval, int64_t c = 1, int64_t m = 0) { > + assert(c >= 0); > + Mutex::Locker l(lock); > + if (m) { > + assert(m > 0); > + _reset_max(m); > + } > + if (_timed_wait(interval, c)) return false; > + count += c; > + return true; > + } > + > /* Returns true if it successfully got the requested amount, > * or false if it would block. > */ > -- > 1.7.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Colin, Colin McCabe wrote: > On Wed, Jun 22, 2011 at 11:26 AM, Jim Schutt <jaschut@sandia.gov> wrote: >> This will enable SimpleMessenger to send keepalives to clients while >> blocked in the policy throttler. >> >> Signed-off-by: Jim Schutt <jaschut@sandia.gov> >> --- >> src/common/Throttle.h | 40 ++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 40 insertions(+), 0 deletions(-) > > This looks good. Only a small nitpick-- it's better not to pass > utime_t by value, since it's a struct. There's a bunch of other code > doing it, but it's better to pass a const reference. Also, I guess we > need to rebase this because g_clock has been replaced by > ceph_clock_now() in master. Should be easy though. Given Sage's much cleaner approach to fixing the osd reset issue, is this change useful on its own? If so, I'm happy to fix it up and resubmit. Let me know. -- Jim > > cheers, > Colin > > >> diff --git a/src/common/Throttle.h b/src/common/Throttle.h >> index 8fd2625..5a028cc 100644 >> --- a/src/common/Throttle.h >> +++ b/src/common/Throttle.h >> @@ -27,6 +27,9 @@ private: >> ((c < max && count + c > max) || // normally stay under max >> (c >= max && count > max)); // except for large c >> } >> + >> + /* Returns true if it had to block/wait, false otherwise. >> + */ >> bool _wait(int64_t c) { >> bool waited = false; >> if (_should_wait(c)) { >> @@ -44,6 +47,28 @@ private: >> return waited; >> } >> >> + /* Returns true if it timed out while blocked/waiting, >> + * false otherwise. >> + */ >> + bool _timed_wait(utime_t interval, int64_t c) { >> + if (_should_wait(c)) { >> + utime_t timeout_at = g_clock.now() + interval; >> + waiting += c; >> + do { >> + if (cond.WaitUntil(lock, timeout_at)) { >> + waiting -= c; >> + return true; >> + } >> + } while (_should_wait(c)); >> + waiting -= c; >> + >> + // wake up the next guy >> + if (waiting) >> + cond.SignalOne(); >> + } >> + return false; >> + } >> + >> public: >> int64_t get_current() { >> Mutex::Locker l(lock); >> @@ -79,6 +104,21 @@ public: >> count += c; >> } >> >> + /* Returns true if it got the requested amount within >> + * the specified interval, false otherwise. >> + */ >> + bool timed_get(utime_t interval, int64_t c = 1, int64_t m = 0) { >> + assert(c >= 0); >> + Mutex::Locker l(lock); >> + if (m) { >> + assert(m > 0); >> + _reset_max(m); >> + } >> + if (_timed_wait(interval, c)) return false; >> + count += c; >> + return true; >> + } >> + >> /* Returns true if it successfully got the requested amount, >> * or false if it would block. >> */ >> -- >> 1.7.1 >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to majordomo@vger.kernel.org >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 23, 2011 at 8:53 AM, Jim Schutt <jaschut@sandia.gov> wrote: > Hi Colin, > > Colin McCabe wrote: >> >> On Wed, Jun 22, 2011 at 11:26 AM, Jim Schutt <jaschut@sandia.gov> wrote: >>> >>> This will enable SimpleMessenger to send keepalives to clients while >>> blocked in the policy throttler. >>> >>> Signed-off-by: Jim Schutt <jaschut@sandia.gov> >>> --- >>> src/common/Throttle.h | 40 ++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 40 insertions(+), 0 deletions(-) >> >> This looks good. Only a small nitpick-- it's better not to pass >> utime_t by value, since it's a struct. There's a bunch of other code >> doing it, but it's better to pass a const reference. Also, I guess we >> need to rebase this because g_clock has been replaced by >> ceph_clock_now() in master. Should be easy though. > > Given Sage's much cleaner approach to fixing the osd reset > issue, is this change useful on its own? If so, I'm happy > to fix it up and resubmit. I have a feeling that adding timeouts to the Throttler would be useful in general. I guess we'll probably wait until we have a user for the function first, though. We should definitely pull the first (cleanup) patch for Throttle.h, though. sincerely, Colin > > Let me know. > > -- Jim > >> >> cheers, >> Colin >> >> >>> diff --git a/src/common/Throttle.h b/src/common/Throttle.h >>> index 8fd2625..5a028cc 100644 >>> --- a/src/common/Throttle.h >>> +++ b/src/common/Throttle.h >>> @@ -27,6 +27,9 @@ private: >>> ((c < max && count + c > max) || // normally stay under max >>> (c >= max && count > max)); // except for large c >>> } >>> + >>> + /* Returns true if it had to block/wait, false otherwise. >>> + */ >>> bool _wait(int64_t c) { >>> bool waited = false; >>> if (_should_wait(c)) { >>> @@ -44,6 +47,28 @@ private: >>> return waited; >>> } >>> >>> + /* Returns true if it timed out while blocked/waiting, >>> + * false otherwise. >>> + */ >>> + bool _timed_wait(utime_t interval, int64_t c) { >>> + if (_should_wait(c)) { >>> + utime_t timeout_at = g_clock.now() + interval; >>> + waiting += c; >>> + do { >>> + if (cond.WaitUntil(lock, timeout_at)) { >>> + waiting -= c; >>> + return true; >>> + } >>> + } while (_should_wait(c)); >>> + waiting -= c; >>> + >>> + // wake up the next guy >>> + if (waiting) >>> + cond.SignalOne(); >>> + } >>> + return false; >>> + } >>> + >>> public: >>> int64_t get_current() { >>> Mutex::Locker l(lock); >>> @@ -79,6 +104,21 @@ public: >>> count += c; >>> } >>> >>> + /* Returns true if it got the requested amount within >>> + * the specified interval, false otherwise. >>> + */ >>> + bool timed_get(utime_t interval, int64_t c = 1, int64_t m = 0) { >>> + assert(c >= 0); >>> + Mutex::Locker l(lock); >>> + if (m) { >>> + assert(m > 0); >>> + _reset_max(m); >>> + } >>> + if (_timed_wait(interval, c)) return false; >>> + count += c; >>> + return true; >>> + } >>> + >>> /* Returns true if it successfully got the requested amount, >>> * or false if it would block. >>> */ >>> -- >>> 1.7.1 >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >>> the body of a message to majordomo@vger.kernel.org >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/common/Throttle.h b/src/common/Throttle.h index 8fd2625..5a028cc 100644 --- a/src/common/Throttle.h +++ b/src/common/Throttle.h @@ -27,6 +27,9 @@ private: ((c < max && count + c > max) || // normally stay under max (c >= max && count > max)); // except for large c } + + /* Returns true if it had to block/wait, false otherwise. + */ bool _wait(int64_t c) { bool waited = false; if (_should_wait(c)) { @@ -44,6 +47,28 @@ private: return waited; } + /* Returns true if it timed out while blocked/waiting, + * false otherwise. + */ + bool _timed_wait(utime_t interval, int64_t c) { + if (_should_wait(c)) { + utime_t timeout_at = g_clock.now() + interval; + waiting += c; + do { + if (cond.WaitUntil(lock, timeout_at)) { + waiting -= c; + return true; + } + } while (_should_wait(c)); + waiting -= c; + + // wake up the next guy + if (waiting) + cond.SignalOne(); + } + return false; + } + public: int64_t get_current() { Mutex::Locker l(lock); @@ -79,6 +104,21 @@ public: count += c; } + /* Returns true if it got the requested amount within + * the specified interval, false otherwise. + */ + bool timed_get(utime_t interval, int64_t c = 1, int64_t m = 0) { + assert(c >= 0); + Mutex::Locker l(lock); + if (m) { + assert(m > 0); + _reset_max(m); + } + if (_timed_wait(interval, c)) return false; + count += c; + return true; + } + /* Returns true if it successfully got the requested amount, * or false if it would block. */
This will enable SimpleMessenger to send keepalives to clients while blocked in the policy throttler. Signed-off-by: Jim Schutt <jaschut@sandia.gov> --- src/common/Throttle.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-)