From a1ecbca0a50ccb273bb5cdc9f031408d782a7bcf Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 17 Mar 2021 19:10:23 +0100 Subject: [PATCH] BUG/MINOR: freq_ctr/threads: make use of the last updated global time The freq counters were using the thread's own time as the start of the current period. The problem is that in case of contention, it was occasionally possible to perform non-monotonic updates on the edge of the next second, because if the upfront thread updates a counter first, it causes a rotation, then the second thread loses the race from its older time, and tries again, and detects a different time again, but in the past so it only updates the counter, then a third thread on the new date would detect a change again, thus provoking a rotation again. The effect was triple: - rare loss of stored values during certain transitions from one period to the next one, causing counters to report 0 - half of the threads forced to go through the slow path every second - difficult convergence when using many threads where the CAS can fail a lot and we can observe N(N-1) attempts for N threads to complete This patch fixes this issue in two ways: - first, it now makes use og the monotonic global_now value which also happens to be volatile and to carry the latest known time; this way time will never jump backwards anymore and only the first thread updates it on transition, the other ones do not need to. - second, re-read the time in the loop after each failure, because if the date changed in the counter, it means that one thread knows a more recent one and we need to update. In this case if it matches the new current second, the fast path is usable. This patch relies on previous patch "MINOR: time: export the global_now variable" and must be backported as far as 1.8. --- include/haproxy/freq_ctr.h | 26 +++++++++++++++----------- src/freq_ctr.c | 12 ++++++------ 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/include/haproxy/freq_ctr.h b/include/haproxy/freq_ctr.h index ce6fb952a6..38d82fe80e 100644 --- a/include/haproxy/freq_ctr.h +++ b/include/haproxy/freq_ctr.h @@ -36,6 +36,7 @@ static inline unsigned int update_freq_ctr(struct freq_ctr *ctr, unsigned int in { int elapsed; unsigned int curr_sec; + uint32_t now_tmp; /* we manipulate curr_ctr using atomic ops out of the lock, since @@ -47,16 +48,17 @@ static inline unsigned int update_freq_ctr(struct freq_ctr *ctr, unsigned int in * same uncertainty as well. */ curr_sec = ctr->curr_sec; - if (curr_sec == (now.tv_sec & 0x7fffffff)) - return _HA_ATOMIC_ADD(&ctr->curr_ctr, inc); - do { + now_tmp = global_now >> 32; + if (curr_sec == (now_tmp & 0x7fffffff)) + return _HA_ATOMIC_ADD(&ctr->curr_ctr, inc); + /* remove the bit, used for the lock */ curr_sec &= 0x7fffffff; } while (!_HA_ATOMIC_CAS(&ctr->curr_sec, &curr_sec, curr_sec | 0x80000000)); __ha_barrier_atomic_store(); - elapsed = (now.tv_sec & 0x7fffffff)- curr_sec; + elapsed = (now_tmp & 0x7fffffff) - curr_sec; if (unlikely(elapsed > 0)) { ctr->prev_ctr = ctr->curr_ctr; _HA_ATOMIC_SUB(&ctr->curr_ctr, ctr->prev_ctr); @@ -64,7 +66,7 @@ static inline unsigned int update_freq_ctr(struct freq_ctr *ctr, unsigned int in /* we missed more than one second */ ctr->prev_ctr = 0; } - curr_sec = now.tv_sec; + curr_sec = now_tmp; } /* release the lock and update the time in case of rotate. */ @@ -82,25 +84,27 @@ static inline unsigned int update_freq_ctr_period(struct freq_ctr_period *ctr, unsigned int period, unsigned int inc) { unsigned int curr_tick; + uint32_t now_ms_tmp; curr_tick = ctr->curr_tick; - if (now_ms - curr_tick < period) - return _HA_ATOMIC_ADD(&ctr->curr_ctr, inc); - do { + now_ms_tmp = (uint32_t)global_now / 1000; + if (now_ms_tmp - curr_tick < period) + return _HA_ATOMIC_ADD(&ctr->curr_ctr, inc); + /* remove the bit, used for the lock */ curr_tick &= ~1; } while (!_HA_ATOMIC_CAS(&ctr->curr_tick, &curr_tick, curr_tick | 0x1)); __ha_barrier_atomic_store(); - if (now_ms - curr_tick >= period) { + if (now_ms_tmp - curr_tick >= period) { ctr->prev_ctr = ctr->curr_ctr; _HA_ATOMIC_SUB(&ctr->curr_ctr, ctr->prev_ctr); curr_tick += period; - if (likely(now_ms - curr_tick >= period)) { + if (likely(now_ms_tmp - curr_tick >= period)) { /* we missed at least two periods */ ctr->prev_ctr = 0; - curr_tick = now_ms; + curr_tick = now_ms_tmp; } curr_tick &= ~1; } diff --git a/src/freq_ctr.c b/src/freq_ctr.c index a7adf0ba97..8fd0c90f09 100644 --- a/src/freq_ctr.c +++ b/src/freq_ctr.c @@ -51,7 +51,7 @@ unsigned int read_freq_ctr(struct freq_ctr *ctr) break; } - age = now.tv_sec - curr_sec; + age = (global_now >> 32) - curr_sec; if (unlikely(age > 1)) return 0; @@ -94,7 +94,7 @@ unsigned int freq_ctr_remain(struct freq_ctr *ctr, unsigned int freq, unsigned i break; } - age = now.tv_sec - curr_sec; + age = (global_now >> 32) - curr_sec; if (unlikely(age > 1)) curr = 0; else { @@ -141,7 +141,7 @@ unsigned int next_event_delay(struct freq_ctr *ctr, unsigned int freq, unsigned break; } - age = now.tv_sec - curr_sec; + age = (global_now >> 32) - curr_sec; if (unlikely(age > 1)) curr = 0; else { @@ -169,7 +169,7 @@ unsigned int next_event_delay(struct freq_ctr *ctr, unsigned int freq, unsigned /* Reads a frequency counter taking history into account for missing time in * current period. The period has to be passed in number of ticks and must * match the one used to feed the counter. The counter value is reported for - * current date (now_ms). The return value has the same precision as one input + * current global date. The return value has the same precision as one input * data sample, so low rates over the period will be inaccurate but still * appropriate for max checking. One trick we use for low values is to specially * handle the case where the rate is between 0 and 1 in order to avoid flapping @@ -206,7 +206,7 @@ unsigned int read_freq_ctr_period(struct freq_ctr_period *ctr, unsigned int peri break; }; - remain = curr_tick + period - now_ms; + remain = curr_tick + period - (uint32_t)global_now / 1000; if (unlikely((int)remain < 0)) { /* We're past the first period, check if we can still report a * part of last period or if we're too far away. @@ -253,7 +253,7 @@ unsigned int freq_ctr_remain_period(struct freq_ctr_period *ctr, unsigned int pe break; }; - remain = curr_tick + period - now_ms; + remain = curr_tick + period - (uint32_t)global_now / 1000; if (likely((int)remain < 0)) { /* We're past the first period, check if we can still report a * part of last period or if we're too far away.