From: Willy Tarreau <w@1wt.eu>
Date: Sat, 17 Apr 2021 16:06:57 +0000 (+0200)
Subject: MINOR: pools: always use atomic ops to maintain counters
X-Git-Tag: v2.4-dev17~64
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2b5579f6da2b24bfcc7fefb4ba06d4a4aba2eb05;p=thirdparty%2Fhaproxy.git

MINOR: pools: always use atomic ops to maintain counters

A part of the code cannot be factored out because it still uses non-atomic
inc/dec for pool->used and pool->allocated as these are located under the
pool's lock. While it can make sense in terms of bus cycles, it does not
make sense in terms of code normalization. Further, some operations were
still performed under a lock that could be totally removed via the use of
atomic ops.

There is still one occurrence in pool_put_to_shared_cache() in the locked
code where pool_free_area() is called under the lock, which must absolutely
be fixed.
---

diff --git a/include/haproxy/pool.h b/include/haproxy/pool.h
index 19e596552d..a44062556f 100644
--- a/include/haproxy/pool.h
+++ b/include/haproxy/pool.h
@@ -217,11 +217,11 @@ static inline void *pool_get_from_shared_cache(struct pool_head *pool)
 	void *p;
 
 	HA_SPIN_LOCK(POOL_LOCK, &pool->lock);
-	if ((p = pool->free_list) != NULL) {
+	if ((p = pool->free_list) != NULL)
 		pool->free_list = *POOL_LINK(pool, p);
-		pool->used++;
-	}
 	HA_SPIN_UNLOCK(POOL_LOCK, &pool->lock);
+	if (p)
+		_HA_ATOMIC_INC(&pool->used);
 
 #ifdef DEBUG_MEMORY_POOLS
 	if (p) {
@@ -238,11 +238,11 @@ static inline void *pool_get_from_shared_cache(struct pool_head *pool)
 static inline void pool_put_to_shared_cache(struct pool_head *pool, void *ptr)
 {
 #ifndef DEBUG_UAF /* normal pool behaviour */
+	_HA_ATOMIC_DEC(&pool->used);
 	HA_SPIN_LOCK(POOL_LOCK, &pool->lock);
-	pool->used--;
 	if (pool_is_crowded(pool)) {
 		pool_free_area(ptr, pool->size + POOL_EXTRA);
-		pool->allocated--;
+		_HA_ATOMIC_DEC(&pool->allocated);
 	} else {
 		*POOL_LINK(pool, ptr) = (void *)pool->free_list;
 		pool->free_list = (void *)ptr;
@@ -253,11 +253,9 @@ static inline void pool_put_to_shared_cache(struct pool_head *pool, void *ptr)
 	/* ensure we crash on double free or free of a const area*/
 	*(uint32_t *)ptr = 0xDEADADD4;
 	pool_free_area(ptr, pool->size + POOL_EXTRA);
-	HA_SPIN_LOCK(POOL_LOCK, &pool->lock);
-	pool->allocated--;
-	pool->used--;
+	_HA_ATOMIC_DEC(&pool->allocated);
+	_HA_ATOMIC_DEC(&pool->used);
 	swrate_add(&pool->needed_avg, POOL_AVG_SAMPLES, pool->used);
-	HA_SPIN_UNLOCK(POOL_LOCK, &pool->lock);
 #endif /* DEBUG_UAF */
 }
 
diff --git a/src/pool.c b/src/pool.c
index f309b84879..f98092225b 100644
--- a/src/pool.c
+++ b/src/pool.c
@@ -215,7 +215,6 @@ void pool_flush(struct pool_head *pool)
 {
 	struct pool_free_list cmp, new;
 	void **next, *temp;
-	int removed = 0;
 
 	if (!pool)
 		return;
@@ -232,11 +231,10 @@ void pool_flush(struct pool_head *pool)
 	while (next) {
 		temp = next;
 		next = *POOL_LINK(pool, temp);
-		removed++;
 		pool_free_area(temp, pool->size + POOL_EXTRA);
+		_HA_ATOMIC_DEC(&pool->allocated);
 	}
 	pool->free_list = next;
-	_HA_ATOMIC_SUB(&pool->allocated, removed);
 	/* here, we should have pool->allocate == pool->used */
 }
 
@@ -300,9 +298,9 @@ void pool_flush(struct pool_head *pool)
 			break;
 		}
 		pool->free_list = *POOL_LINK(pool, temp);
-		pool->allocated--;
 		HA_SPIN_UNLOCK(POOL_LOCK, &pool->lock);
 		pool_free_area(temp, pool->size + POOL_EXTRA);
+		_HA_ATOMIC_DEC(&pool->allocated);
 	}
 	/* here, we should have pool->allocated == pool->used */
 }
@@ -327,8 +325,8 @@ void pool_gc(struct pool_head *pool_ctx)
 		       (int)(entry->allocated - entry->used) > (int)entry->minavail) {
 			temp = entry->free_list;
 			entry->free_list = *POOL_LINK(entry, temp);
-			entry->allocated--;
 			pool_free_area(temp, entry->size + POOL_EXTRA);
+			_HA_ATOMIC_DEC(&entry->allocated);
 		}
 	}