BUG/MAJOR: threads/freq_ctr: fix lock on freq counters.

The wrong bit was set to keep the lock on freq counter update. And the read
functions were re-worked to use volatile.

Moreover, when a freq counter is updated, it is now rotated only if the current
counter is in the past (now.tv_sec > ctr->curr_sec). It is important with
threads because the current time (now) is thread-local. So, rounded to the
second, the time may vary by more or less 1 second. So a freq counter rotated by
one thread may be see 1 second in the future. In this case, it is updated but
not rotated.

diff --git a/include/proto/freq_ctr.h b/include/proto/freq_ctr.h
index c6ea5c24c79ca52b5c73c41bbbe3bba7cbb448fe..c10ddf2aa85bc27b133c15aff4156dc0a9e41625 100644 (file)
--- a/include/proto/freq_ctr.h
+++ b/include/proto/freq_ctr.h
@@ -34,7 +34,7 @@
  */
 static inline unsigned int update_freq_ctr(struct freq_ctr *ctr, unsigned int inc)
 {
-       unsigned int elapsed;
+       int elapsed;
        unsigned int tot_inc;
        unsigned int curr_sec;
 
@@ -42,23 +42,24 @@ static inline unsigned int update_freq_ctr(struct freq_ctr *ctr, unsigned int in
                /* remove the bit, used for the lock */
                curr_sec = ctr->curr_sec & 0x7fffffff;
        }
-       while (!HA_ATOMIC_CAS(&ctr->curr_sec, &curr_sec, curr_sec | 0x8000000));
+       while (!HA_ATOMIC_CAS(&ctr->curr_sec, &curr_sec, curr_sec | 0x80000000));
 
        elapsed = (now.tv_sec & 0x7fffffff)- curr_sec;
-       if (unlikely(elapsed)) {
+       if (unlikely(elapsed > 0)) {
                ctr->prev_ctr = ctr->curr_ctr;
                ctr->curr_ctr = 0;
                if (likely(elapsed != 1)) {
                        /* we missed more than one second */
                        ctr->prev_ctr = 0;
                }
+               curr_sec = now.tv_sec;
        }
 
        ctr->curr_ctr += inc;
        tot_inc = ctr->curr_ctr;
 
        /* release the lock and update the time in case of rotate. */
-       HA_ATOMIC_STORE(&ctr->curr_sec, now.tv_sec & 0x7fffffff);
+       HA_ATOMIC_STORE(&ctr->curr_sec, curr_sec & 0x7fffffff);
        return tot_inc;
        /* Note: later we may want to propagate the update to other counters */
 }

diff --git a/src/freq_ctr.c b/src/freq_ctr.c
index 37fb1f385e2ec7d2f38c0fff16a881450ffdf7b1..e50ce589e627a2436734e539355d42eedd5b23fa 100644 (file)
--- a/src/freq_ctr.c
+++ b/src/freq_ctr.c
@@ -30,18 +30,21 @@
  */
 unsigned int read_freq_ctr(struct freq_ctr *ctr)
 {
-       unsigned int curr, past;
-       unsigned int age, curr_sec;
-
-       do {
-               curr = ctr->curr_ctr;
-               past = ctr->prev_ctr;
-               curr_sec = ctr->curr_sec;
-
-       } while (curr != ctr->curr_ctr
-                || past != ctr->prev_ctr
-                || curr_sec !=  ctr->curr_sec
-                || (curr_sec & 0x80000000));
+       unsigned int curr, past, _curr, _past;
+       unsigned int age, curr_sec, _curr_sec;
+
+       while(1) {
+               _curr = (volatile unsigned int)ctr->curr_ctr;
+               _past = (volatile unsigned int)ctr->prev_ctr;
+               _curr_sec = (volatile unsigned int)ctr->curr_sec;
+               if (_curr_sec & 0x80000000)
+                       continue;
+               curr = (volatile unsigned int)ctr->curr_ctr;
+               past = (volatile unsigned int)ctr->prev_ctr;
+               curr_sec = (volatile unsigned int)ctr->curr_sec;
+               if (_curr == curr && _past == past && _curr_sec == curr_sec)
+                       break;
+       }
 
        age = now.tv_sec - curr_sec;
        if (unlikely(age > 1))
@@ -64,18 +67,21 @@ unsigned int read_freq_ctr(struct freq_ctr *ctr)
  */
 unsigned int freq_ctr_remain(struct freq_ctr *ctr, unsigned int freq, unsigned int pend)
 {
-       unsigned int curr, past;
-       unsigned int age, curr_sec;
-
-       do {
-               curr = ctr->curr_ctr;
-               past = ctr->prev_ctr;
-               curr_sec = ctr->curr_sec;
-
-       } while (curr != ctr->curr_ctr
-                || past != ctr->prev_ctr
-                || curr_sec !=  ctr->curr_sec
-                || (curr_sec & 0x80000000));
+       unsigned int curr, past, _curr, _past;
+       unsigned int age, curr_sec, _curr_sec;
+
+       while(1) {
+               _curr = (volatile unsigned int)ctr->curr_ctr;
+               _past = (volatile unsigned int)ctr->prev_ctr;
+               _curr_sec = (volatile unsigned int)ctr->curr_sec;
+               if (_curr_sec & 0x80000000)
+                       continue;
+               curr = (volatile unsigned int)ctr->curr_ctr;
+               past = (volatile unsigned int)ctr->prev_ctr;
+               curr_sec = (volatile unsigned int)ctr->curr_sec;
+               if (_curr == curr && _past == past && _curr_sec == curr_sec)
+                       break;
+       }
 
        age = now.tv_sec - curr_sec;
        if (unlikely(age > 1))
@@ -102,18 +108,21 @@ unsigned int freq_ctr_remain(struct freq_ctr *ctr, unsigned int freq, unsigned i
  */
 unsigned int next_event_delay(struct freq_ctr *ctr, unsigned int freq, unsigned int pend)
 {
-       unsigned int curr, past;
-       unsigned int wait, age, curr_sec;
-
-       do {
-               curr = ctr->curr_ctr;
-               past = ctr->prev_ctr;
-               curr_sec = ctr->curr_sec;
-
-       } while (curr != ctr->curr_ctr
-                || past != ctr->prev_ctr
-                || curr_sec !=  ctr->curr_sec
-                || (curr_sec & 0x80000000));
+       unsigned int curr, past, _curr, _past;
+       unsigned int wait, age, curr_sec, _curr_sec;
+
+       while(1) {
+               _curr = (volatile unsigned int)ctr->curr_ctr;
+               _past = (volatile unsigned int)ctr->prev_ctr;
+               _curr_sec = (volatile unsigned int)ctr->curr_sec;
+               if (_curr_sec & 0x80000000)
+                       continue;
+               curr = (volatile unsigned int)ctr->curr_ctr;
+               past = (volatile unsigned int)ctr->prev_ctr;
+               curr_sec = (volatile unsigned int)ctr->curr_sec;
+               if (_curr == curr && _past == past && _curr_sec == curr_sec)
+                       break;
+       }
 
        age = now.tv_sec - curr_sec;
        if (unlikely(age > 1))
@@ -152,18 +161,21 @@ unsigned int next_event_delay(struct freq_ctr *ctr, unsigned int freq, unsigned
  */
 unsigned int read_freq_ctr_period(struct freq_ctr_period *ctr, unsigned int period)
 {
-       unsigned int curr, past;
-       unsigned int remain, curr_tick;
-
-       do {
+       unsigned int _curr, _past, curr, past;
+       unsigned int remain, _curr_tick, curr_tick;
+
+       while(1) {
+               _curr = ctr->curr_ctr;
+               _past = ctr->prev_ctr;
+               _curr_tick = ctr->curr_tick;
+               if (_curr_tick & 0x1)
+                       continue;
                curr = ctr->curr_ctr;
                past = ctr->prev_ctr;
                curr_tick = ctr->curr_tick;
-
-       } while (curr != ctr->curr_ctr
-                ||  past != ctr->prev_ctr
-                || curr_tick !=  ctr->curr_tick
-                || (curr_tick & 0x1));
+               if (_curr == curr && _past == past && _curr_tick == curr_tick)
+                       break;
+       };
 
        remain = curr_tick + period - now_ms;
        if (unlikely((int)remain < 0)) {
@@ -190,18 +202,21 @@ unsigned int read_freq_ctr_period(struct freq_ctr_period *ctr, unsigned int peri
 unsigned int freq_ctr_remain_period(struct freq_ctr_period *ctr, unsigned int period,
                                    unsigned int freq, unsigned int pend)
 {
-       unsigned int curr, past;
-       unsigned int remain, curr_tick;
-
-       do {
+       unsigned int _curr, _past, curr, past;
+       unsigned int remain, _curr_tick, curr_tick;
+
+       while(1) {
+               _curr = ctr->curr_ctr;
+               _past = ctr->prev_ctr;
+               _curr_tick = ctr->curr_tick;
+               if (_curr_tick & 0x1)
+                       continue;
                curr = ctr->curr_ctr;
                past = ctr->prev_ctr;
                curr_tick = ctr->curr_tick;
-
-       } while (curr != ctr->curr_ctr
-                ||  past != ctr->prev_ctr
-                || curr_tick !=  ctr->curr_tick
-                || (curr_tick & 0x1));
+               if (_curr == curr && _past == past && _curr_tick == curr_tick)
+                       break;
+       };
 
        remain = curr_tick + period - now_ms;
        if (likely((int)remain < 0)) {