From fe16126accdc7cc7f289d759a281d967c7eafbee Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 28 Apr 2021 17:31:22 +0200 Subject: [PATCH] BUG/MEDIUM: time: fix updating of global_now upon clock drift During commit 7e4a557f6 ("MINOR: time: change the global timeval and the the global tick at once") the approach made sure that the new now_ms was always higher than or equal to global_now_ms, but by forgetting the old value. This can cause the first update to global_now_ms to fail if it's already out of sync, going back into the loop, and the subsequent call would then succeed due to commit 4d01f3dcd ("MINOR: time: avoid overwriting the same values of global_now"). And if it goes out of sync, it will fail to update forever, as observed by Ashley Penney in github issue #1194, causing incorrect freq counters calculations everywhere. One possible trigger for this issue is one thread spinning for a few milliseconds while the other ones continue to work. The issue really is that old_now_ms ought not to be modified in the loop as it's used for the CAS. But we don't need to structurally guarantee that global_now_ms grows monotonically as it's computed from the new global_now which is already verified for this via the __tv_islt() test. Thus, dropping any corrections on global_now_ms in the loop is the correct way to proceed as long as this one is always updated to follow global_now. No backport is needed, this is only for 2.4-dev. --- src/time.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/time.c b/src/time.c index 4ebde37..d6ab185 100644 --- a/src/time.c +++ b/src/time.c @@ -236,7 +236,6 @@ void tv_update_date(int max_wait, int interrupted) do { tmp_now.tv_sec = (unsigned int)(old_now >> 32); tmp_now.tv_usec = old_now & 0xFFFFFFFFU; - old_now_ms = __tv_to_ms(&tmp_now); if (__tv_islt(&now, &tmp_now)) now = tmp_now; @@ -246,8 +245,6 @@ void tv_update_date(int max_wait, int interrupted) */ new_now = ((ullong)now.tv_sec << 32) + (uint)now.tv_usec; now_ms = __tv_to_ms(&now); - if (tick_is_lt(now_ms, old_now_ms)) - now_ms = old_now_ms; /* let's try to update the global (both in timeval * and ms forms) or loop again. -- 1.7.10.4