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.

diff --git a/include/haproxy/freq_ctr.h b/include/haproxy/freq_ctr.h
index ce6fb952a6621f68e47e6cc805d7edf2494b42d0..38d82fe80e7820566f8912f9eb3b68d96085e0c2 100644 (file)
--- 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 a7adf0ba97aa3111e75a81b2fa74a2bd89b6767c..8fd0c90f0998f0d4eb22530aee105665765cebcc 100644 (file)
--- 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.