From 6e0128630b80d691c915cba5cec43db59d191898 Mon Sep 17 00:00:00 2001 From: Emeric Brun Date: Mon, 30 Oct 2017 18:04:28 +0100 Subject: [PATCH] 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. --- include/proto/freq_ctr.h | 9 ++-- src/freq_ctr.c | 113 ++++++++++++++++++++++----------------- 2 files changed, 69 insertions(+), 53 deletions(-) diff --git a/include/proto/freq_ctr.h b/include/proto/freq_ctr.h index c6ea5c24c..c10ddf2aa 100644 --- 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 37fb1f385..e50ce589e 100644 --- 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; + unsigned int curr, past, _curr, _past; + unsigned int age, curr_sec, _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)); + 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; + unsigned int curr, past, _curr, _past; + unsigned int age, curr_sec, _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)); + 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; + unsigned int curr, past, _curr, _past; + unsigned int wait, age, curr_sec, _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)); + 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; + unsigned int _curr, _past, curr, past; + unsigned int remain, _curr_tick, curr_tick; - do { + 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; + unsigned int _curr, _past, curr, past; + unsigned int remain, _curr_tick, curr_tick; - do { + 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)) {