From: Willy Tarreau <w@1wt.eu>
Date: Tue, 31 Oct 2017 16:54:15 +0000 (+0100)
Subject: BUG/MAJOR: threads/freq_ctr: use a memory barrier to detect changes
X-Git-Tag: v1.8-rc1~100
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a06a580941002f77fba182fd9d922670c7e2c5fc;p=thirdparty%2Fhaproxy.git

BUG/MAJOR: threads/freq_ctr: use a memory barrier to detect changes

commit 6e01286 (BUG/MAJOR: threads/freq_ctr: fix lock on freq counters)
attempted to fix the loop using volatile but that doesn't work depending
on the level of optimization, resulting in situations where the threads
could remain looping forever. Here we use memory barriers between reads
to enforce a strict ordering and the asm code produced does exactly what
the C code does and works perfectly, with a 3-digit measurement accuracy
observed during a test.
---

diff --git a/src/freq_ctr.c b/src/freq_ctr.c
index e50ce589e6..e6e0168930 100644
--- a/src/freq_ctr.c
+++ b/src/freq_ctr.c
@@ -33,15 +33,21 @@ unsigned int read_freq_ctr(struct freq_ctr *ctr)
 	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;
+	while (1) {
+		_curr = ctr->curr_ctr;
+		HA_BARRIER();
+		_past = ctr->prev_ctr;
+		HA_BARRIER();
+		_curr_sec = ctr->curr_sec;
+		HA_BARRIER();
 		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;
+		curr = ctr->curr_ctr;
+		HA_BARRIER();
+		past = ctr->prev_ctr;
+		HA_BARRIER();
+		curr_sec = ctr->curr_sec;
+		HA_BARRIER();
 		if (_curr == curr && _past == past && _curr_sec == curr_sec)
 			break;
 	}
@@ -70,15 +76,21 @@ unsigned int freq_ctr_remain(struct freq_ctr *ctr, unsigned int freq, unsigned i
 	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;
+	while (1) {
+		_curr = ctr->curr_ctr;
+		HA_BARRIER();
+		_past = ctr->prev_ctr;
+		HA_BARRIER();
+		_curr_sec = ctr->curr_sec;
+		HA_BARRIER();
 		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;
+		curr = ctr->curr_ctr;
+		HA_BARRIER();
+		past = ctr->prev_ctr;
+		HA_BARRIER();
+		curr_sec = ctr->curr_sec;
+		HA_BARRIER();
 		if (_curr == curr && _past == past && _curr_sec == curr_sec)
 			break;
 	}
@@ -111,15 +123,21 @@ unsigned int next_event_delay(struct freq_ctr *ctr, unsigned int freq, unsigned
 	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;
+	while (1) {
+		_curr = ctr->curr_ctr;
+		HA_BARRIER();
+		_past = ctr->prev_ctr;
+		HA_BARRIER();
+		_curr_sec = ctr->curr_sec;
+		HA_BARRIER();
 		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;
+		curr = ctr->curr_ctr;
+		HA_BARRIER();
+		past = ctr->prev_ctr;
+		HA_BARRIER();
+		curr_sec = ctr->curr_sec;
+		HA_BARRIER();
 		if (_curr == curr && _past == past && _curr_sec == curr_sec)
 			break;
 	}
@@ -164,15 +182,21 @@ unsigned int read_freq_ctr_period(struct freq_ctr_period *ctr, unsigned int peri
 	unsigned int _curr, _past, curr, past;
 	unsigned int remain, _curr_tick, curr_tick;
 
-	while(1) {
+	while (1) {
 		_curr = ctr->curr_ctr;
+		HA_BARRIER();
 		_past = ctr->prev_ctr;
+		HA_BARRIER();
 		_curr_tick = ctr->curr_tick;
+		HA_BARRIER();
 		if (_curr_tick & 0x1)
 			continue;
 		curr = ctr->curr_ctr;
+		HA_BARRIER();
 		past = ctr->prev_ctr;
+		HA_BARRIER();
 		curr_tick = ctr->curr_tick;
+		HA_BARRIER();
 		if (_curr == curr && _past == past && _curr_tick == curr_tick)
 			break;
 	};
@@ -205,15 +229,21 @@ unsigned int freq_ctr_remain_period(struct freq_ctr_period *ctr, unsigned int pe
 	unsigned int _curr, _past, curr, past;
 	unsigned int remain, _curr_tick, curr_tick;
 
-	while(1) {
+	while (1) {
 		_curr = ctr->curr_ctr;
+		HA_BARRIER();
 		_past = ctr->prev_ctr;
+		HA_BARRIER();
 		_curr_tick = ctr->curr_tick;
+		HA_BARRIER();
 		if (_curr_tick & 0x1)
 			continue;
 		curr = ctr->curr_ctr;
+		HA_BARRIER();
 		past = ctr->prev_ctr;
+		HA_BARRIER();
 		curr_tick = ctr->curr_tick;
+		HA_BARRIER();
 		if (_curr == curr && _past == past && _curr_tick == curr_tick)
 			break;
 	};