watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. #13103

antoniovicente · 2020-09-15T06:06:15Z

Commit Message:
watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog.
Additional Description:
Use an atomic counter to track if the watchdog was touched since the last time the guarddog checked. This avoids expensive calls to get the current monotonic time every time the watchdog is touched, and thus reduces the overhead incurred by touching the watchdog after every event loop callback.

This PR also includes pre-requisite fixes to the Guarddog's test interlock mechanism so it operates correctly in cases where simulated monotonic time does not move forward between calls to GuardDogImpl::forceCheckForTest()
Risk Level: low, slight change to how watchdog timeouts are computed.
Testing: existing tests.
Docs Changes: n/a
Release Notes: n/a
Issue #11391

Interlocking based on time can result in confusion when using simulated time and without calls to advanceAndWait between calls to GuarddogImpl::forceCheckForTest. Signed-off-by: Antonio Vicente <avd@google.com>

…t petting of the watchdog. Signed-off-by: Antonio Vicente <avd@google.com>

KBaichoo

Looks pretty good. Thanks for doing this.

KBaichoo · 2020-09-15T15:05:01Z

source/server/guarddog_impl.cc

    : dog_(watch_dog),
+      last_touch_count_(dog_->touchCount()),


Perhaps you should set this to 0 (the last touch count)

otherwise we'll fall through if (watched_dog->last_touch_count_ != touch_count) on the first iteration of the new watchdog in step. We'll end up using const auto ltt = watched_dog->last_touch_time_; without it being explicitly set

Notice line 179-180 above, where the WatchedDog is created and touched in the following line to guarantee that the first guarddog step after creation goes down the "watched_dog->last_touch_count_ != touch_count" branch.

yanavlasov · 2020-09-15T16:38:08Z

source/server/watchdog_impl.h

  }

 private:
  const Thread::ThreadId thread_id_;
-  TimeSource& time_source_;
-  std::atomic<std::chrono::steady_clock::duration> latest_touch_time_since_epoch_;
+  std::atomic<uint64_t> touch_count_ = 0;


Don't we support 32 bit ARM? Will this work there?
I sort of expect the std::atomic vs absl::Mutex discussion here.

Can you provide more context? I assume ARM supports atomic ops. Are they particularly expensive in ARM?

It would be acceptable to make this a 32 bit type, wrap around behavior would be acceptable since we're just checking that the numbers are different. Another implementation option would be to have the guarddog reset the counter to 0 when stepping through which should prevent overflow.

run format Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

KBaichoo · 2020-09-15T18:58:43Z

source/server/watchdog_impl.h

-  }
+  // Accessors for use by GuardDogImpl to tell if the watchdog was touched recently and reset the
+  // count back to 0 when it has.
+  uint64_t touchCount() const { return touch_count_.load(); }


if we're going with uint32_t internally, perhaps we should return that?

Oops, fixed.

KBaichoo · 2020-09-15T19:25:49Z

source/server/guarddog_impl.cc

+      if (watched_dog->dog_->touchCount() > 0) {
+        // Watchdog was touched since guarddog last checked; update time and reset the count.
+        watched_dog->dog_->resetTouchCount();
+        watched_dog->last_touch_time_ = now;


So we'll lose some accuracy since the last touch time is set here instead of when the watchdog is actually touched. Is this ok?

Should we then rename it to something else?

It is still a timestamp from which the most recent touch was detected. I agree that there's a change in precision; watchdog miss/megamiss/kill/multikill would be delayed by up to min(miss_timeout / 2, megamiss_timeout / 2, kill_timeout / 2, multikill_timeout / 2).

changed to last_checkin_ for consistency after your recent change

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-09-17T05:20:12Z

/retest

repokitteh-read-only · 2020-09-17T05:20:17Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13103 (comment) was created by @antoniovicente.

see: more, trace.

mattklein123 · 2020-09-18T22:37:33Z

Can you merge main and see if that fixes CI? Not sure what is broken here.

/wait

Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123

LGTM with a few optional drive by comments.

mattklein123 · 2020-09-21T15:07:12Z

source/server/guarddog_impl.cc

+  Thread::LockGuard guard(mutex_);
+  if (!run_thread_) {
+    return;


It's not clear to me why this lock now needs to be held for the entire function. Can you possibly add some more comments? Should there by GUARDED_BY annotations to make this more clear? Maybe this is just to simplify the logic?

Added a comment. This is part of the fix to test interlock; we need a full run of the step function after forceCheckForTest. Performance shouldn't suffer since the use of mutex_ in the step() function should never be contended; the other uses of this mutex are GuardDogImpl::start, stop and forceCheckForTest.

mattklein123 · 2020-09-21T15:08:45Z

source/server/guarddog_impl.cc

+      if (watched_dog->dog_->touchCount() > 0) {
+        // Watchdog was touched since the guard dog last checked; update time and reset the count.
+        watched_dog->dog_->resetTouchCount();


perf nit: I think unless you do a relaxed load it's probably faster to just always do a reset and return the previous count, and then branch on that. Up to you.

I changed the watchdog API so it provides a single method that returns if the dog was touched recently and reset touch state. I also changed the type to bool since it's more appropriate and switched to a looser memory order for performance.

Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123

LGTM with small comment but needs format fix.

/wait

mattklein123 · 2020-09-22T17:31:34Z

source/server/watchdog_impl.h

+  void touch() override {
+    // Set touched_ if not already set.
+    bool expected = false;
+    touched_.compare_exchange_strong(expected, true, std::memory_order_release,


I'm not an atomics expert, but I'm pretty sure you can use store with memory_order_relaxed on this one, and use memory_order_relaxed on the exchange above.

The whole thing is eventually consistent so I'm not sure any ordering matters and that would probably be faster?

The only thing I'm not sure of is whether this would effect test determinism, however, if you are using locks to serialize the tests that should force consistency of all previous operations.

I'm no expert either. After thinking about it some more, I think you're totally right - relaxed memory order is sufficient here. Switched to that.

Fix spelling Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-09-23T00:26:19Z

LGTM with small comment but needs format fix.

/wait

Oops, spellcheck issue. Need to remember to run that before pushing changes.

Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123

Sorry one more comment otherwise LGTM.

/wait

mattklein123 · 2020-09-23T16:13:40Z

source/server/watchdog_impl.h


  // Server::WatchDog
  void startWatchdog(Event::Dispatcher& dispatcher) override;
  void touch() override {
    // Set touched_ if not already set.
    bool expected = false;
-    touched_.compare_exchange_strong(expected, true, std::memory_order_release,
-                                     std::memory_order_acquire);
+    touched_.compare_exchange_strong(expected, true, std::memory_order_relaxed);


I don't think you need the CAS here. I think you can just do a relaxed store. It doesn't matter if it's already set?

Correct. I'm using CAS to avoid write in cases where touched_ is already true, since atomic reads are cheaper than atomic writes.

OK sounds good.

mattklein123

Nice!

stale · 2020-10-04T10:16:16Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123 · 2020-10-15T22:37:48Z

Merge main once #13598 merges. Thanks!

/wait

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente · 2020-10-20T15:36:27Z

Merged master, I see an old approval from you. Let me know if you have any newer comments or I should just merge in the PR.

mattklein123 · 2020-10-20T15:41:37Z

Go ahead and merge!

* master: (22 commits) ci: various improvements (envoyproxy#13660) dns: fix defunct fd bug in apple resolver (envoyproxy#13641) build: support ppc64le with wasm (envoyproxy#13657) [fuzz] Added random load balancer fuzz (envoyproxy#13400) dependencies: compute and check release dates via GitHub API. (envoyproxy#13582) mac ci: try ignoring update failure (envoyproxy#13658) watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. (envoyproxy#13103) typos: fix a couple 'enovy' mispellings (envoyproxy#13645) lua: Expose stream info downstreamLocalAddress and downstreamDirectRemoteAddress for Lua filter (envoyproxy#13536) tap: fix upstream streamed transport socket taps (envoyproxy#13638) Revert "delay health checks until transport socket secrets are ready. (envoyproxy#13516)" (envoyproxy#13639) Watchdog: use abort action as a default if killing is enabled. (envoyproxy#13523) [fuzz] Fixed divide by zero bug (envoyproxy#13545) wasm: flip the meaning of the "repository" in envoy_wasm_cc_binary(). (envoyproxy#13621) fix: record recovered local address (envoyproxy#13581) docs: fix incorrect compressor filter doc (envoyproxy#13611) docs: clean up docs for azp migration (envoyproxy#13558) wasm: fix building Wasm example. (envoyproxy#13619) test: Refactor flood tests into a separate test file (envoyproxy#13556) wasm: re-enable tests with precompiled modules. (envoyproxy#13583) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

antoniovicente added 2 commits September 15, 2020 01:48

test: Change the GuarddogImpl's interlock so it is time agnostic.

bd92bc2

Interlocking based on time can result in confusion when using simulated time and without calls to advanceAndWait between calls to GuarddogImpl::forceCheckForTest. Signed-off-by: Antonio Vicente <avd@google.com>

watchdog: Optimize WatchdogImpl::touch in preparation to more frequen…

e6afe7b

…t petting of the watchdog. Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente requested a review from KBaichoo September 15, 2020 06:06

antoniovicente mentioned this pull request Sep 15, 2020

watchdog: Touch Watchdog before executing most event loop callbacks to avoid spurious misses when there are many queued callbacks. #13104

Merged

asraa assigned KBaichoo Sep 15, 2020

KBaichoo reviewed Sep 15, 2020

View reviewed changes

yanavlasov reviewed Sep 15, 2020

View reviewed changes

prefer fetch_add over ++

08ee875

run format Signed-off-by: Antonio Vicente <avd@google.com>

yanavlasov self-assigned this Sep 15, 2020

address review comments

3b8b2a9

Signed-off-by: Antonio Vicente <avd@google.com>

KBaichoo reviewed Sep 15, 2020

View reviewed changes

antoniovicente added 3 commits September 15, 2020 17:43

fix return type.

6b623b6

Signed-off-by: Antonio Vicente <avd@google.com>

fix spelling

3f962a1

Signed-off-by: Antonio Vicente <avd@google.com>

fix spelling more

bcad016

Signed-off-by: Antonio Vicente <avd@google.com>

KBaichoo previously approved these changes Sep 16, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into timeless_watchdog

d436eaf

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed KBaichoo’s stale review via d436eaf September 16, 2020 22:42

yanavlasov previously approved these changes Sep 17, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 18, 2020

Merge remote-tracking branch 'upstream/master' into timeless_watchdog

c971f62

Signed-off-by: Antonio Vicente <avd@google.com>

repokitteh-read-only bot removed the waiting label Sep 18, 2020

mattklein123 previously approved these changes Sep 21, 2020

View reviewed changes

antoniovicente added 2 commits September 22, 2020 01:39

Switch to atomic<bool> and acq_rel memory_order instead of seq_cst

1d5b149

Signed-off-by: Antonio Vicente <avd@google.com>

add comments

e9db878

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed stale reviews from mattklein123 and yanavlasov via e9db878 September 22, 2020 15:54

KBaichoo previously approved these changes Sep 22, 2020

View reviewed changes

mattklein123 requested changes Sep 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 22, 2020

Switch to relaxed memory order

5a3ecaf

Fix spelling Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed KBaichoo’s stale review via 5a3ecaf September 23, 2020 00:23

repokitteh-read-only bot removed the waiting label Sep 23, 2020

Kick CI

3b6313a

Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123 requested changes Sep 23, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 23, 2020

mattklein123 self-assigned this Sep 23, 2020

antoniovicente removed the waiting label Sep 23, 2020

mattklein123 approved these changes Sep 23, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 4, 2020

antoniovicente added 2 commits October 6, 2020 02:43

Merge remote-tracking branch 'upstream/master' into timeless_watchdog

8e54372

Signed-off-by: Antonio Vicente <avd@google.com>

Merge remote-tracking branch 'upstream/master' into timeless_watchdog

39dbb11

Signed-off-by: Antonio Vicente <avd@google.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 12, 2020

repokitteh-read-only bot added the waiting label Oct 15, 2020

Merge remote-tracking branch 'upstream/master' into timeless_watchdog

fc49f33

Signed-off-by: Antonio Vicente <avd@google.com>

repokitteh-read-only bot removed the waiting label Oct 16, 2020

Kick CI

9ee42f5

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente merged commit 4797e7a into envoyproxy:master Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. #13103

watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. #13103

antoniovicente commented Sep 15, 2020

KBaichoo left a comment

KBaichoo Sep 15, 2020

antoniovicente Sep 15, 2020

yanavlasov Sep 15, 2020

antoniovicente Sep 15, 2020

antoniovicente Sep 15, 2020

KBaichoo Sep 15, 2020

antoniovicente Sep 16, 2020

KBaichoo Sep 15, 2020

antoniovicente Sep 16, 2020

antoniovicente Sep 16, 2020

antoniovicente commented Sep 17, 2020

repokitteh-read-only bot commented Sep 17, 2020

mattklein123 commented Sep 18, 2020

mattklein123 left a comment

mattklein123 Sep 21, 2020

antoniovicente Sep 22, 2020

mattklein123 Sep 21, 2020

antoniovicente Sep 22, 2020

mattklein123 left a comment

mattklein123 Sep 22, 2020

antoniovicente Sep 23, 2020

antoniovicente commented Sep 23, 2020

mattklein123 left a comment

mattklein123 Sep 23, 2020

antoniovicente Sep 23, 2020

mattklein123 Sep 23, 2020

mattklein123 left a comment

stale bot commented Oct 4, 2020

mattklein123 commented Oct 15, 2020

antoniovicente commented Oct 20, 2020

mattklein123 commented Oct 20, 2020

watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. #13103

watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. #13103

Conversation

antoniovicente commented Sep 15, 2020

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente commented Sep 17, 2020

repokitteh-read-only bot commented Sep 17, 2020

mattklein123 commented Sep 18, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente commented Sep 23, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

stale bot commented Oct 4, 2020

mattklein123 commented Oct 15, 2020

antoniovicente commented Oct 20, 2020

mattklein123 commented Oct 20, 2020