MINOR: freq_ctr: simplify and improve the update function

update_freq_ctr_period() was still not very clean and didn't wait for
the rotation lock to be dropped before trying again, thus maintaining
the contention at a high level. In addition, the rotation update was
made in three steps, which are not very efficient in terms of bus
cycles.

Here the wait loop was reworked so that the fast path remains short
and that the contended path waits for the lock to be dropped before
attempting another write, but it only waits a relax cycle before
attempting a read. The rotation block was simplified to remove a
test that was already validated by the first loop, and so that the
retrieval of the current period, its reset and its increment are all
performed in a single atomic op and the store to the previous period
is performed immediately after.

All this results in significantly smaller code for the inline function
(~1kB total) and a shorter critical path.

diff --git a/include/haproxy/freq_ctr.h b/include/haproxy/freq_ctr.h
index 6b0cebff16e6f957019dfcc086da26397a1e0087..83fe90e09c6a80d923597ed625dc6311c8eacbcf 100644 (file)
--- a/include/haproxy/freq_ctr.h
+++ b/include/haproxy/freq_ctr.h
@@ -42,33 +42,37 @@ static inline unsigned int update_freq_ctr_period(struct freq_ctr *ctr,
        unsigned int curr_tick;
        uint32_t now_ms_tmp;
 
-       curr_tick = ctr->curr_tick;
-       do {
+       /* atomically update the counter if still within the period, even if
+        * a rotation is in progress (no big deal).
+        */
+       for (;; __ha_cpu_relax()) {
+               __ha_barrier_load();
                now_ms_tmp = global_now_ms;
+               curr_tick = ctr->curr_tick;
+
                if (now_ms_tmp - curr_tick < period)
                        return _HA_ATOMIC_ADD_FETCH(&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_cpu_relax());
-       __ha_barrier_atomic_store();
+               /* a rotation is needed */
+               if (!(curr_tick & 1) &&
+                   HA_ATOMIC_CAS(&ctr->curr_tick, &curr_tick, curr_tick | 0x1))
+                       break;
+       }
 
-       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_tmp - curr_tick >= period)) {
-                       /* we missed at least two periods */
-                       ctr->prev_ctr = 0;
-                       curr_tick = now_ms_tmp;
-               }
-               curr_tick &= ~1;
+       /* atomically switch the new period into the old one without losing any
+        * potential concurrent update.
+        */
+       _HA_ATOMIC_STORE(&ctr->prev_ctr, _HA_ATOMIC_FETCH_SUB(&ctr->curr_ctr, ctr->curr_ctr - inc));
+       curr_tick += period;
+       if (likely(now_ms_tmp - curr_tick >= period)) {
+               /* we missed at least two periods */
+               _HA_ATOMIC_STORE(&ctr->prev_ctr, 0);
+               curr_tick = now_ms_tmp;
        }
 
        /* release the lock and update the time in case of rotate. */
-       _HA_ATOMIC_STORE(&ctr->curr_tick, curr_tick);
-
-       return _HA_ATOMIC_ADD_FETCH(&ctr->curr_ctr, inc);
+       _HA_ATOMIC_STORE(&ctr->curr_tick, curr_tick & ~1);
+       return ctr->curr_ctr;
 }
 
 /* Update a 1-sec frequency counter by <inc> incremental units. It is automatically