From 9a0f0ef5576bd72c6908339df44bf382342266de Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Wed, 8 Feb 2023 18:03:27 +0100
Subject: [PATCH 1/7] hashmap: expose helper for releasing memory pools
 independently of valgrind

Let's clean this up and export this always, so that we can later call
when we are under memory pressure.
---
 src/basic/hashmap.c | 32 ++++++++++++++++++--------------
 src/basic/hashmap.h |  2 ++
 src/basic/mempool.c |  2 --
 src/basic/mempool.h |  2 --
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c
index b9efaafa3c..c18c75d78c 100644
--- a/src/basic/hashmap.c
+++ b/src/basic/hashmap.c
@@ -274,28 +274,32 @@ static _used_ const struct hashmap_type_info hashmap_type_info[_HASHMAP_TYPE_MAX
         },
 };
 
-#if VALGRIND
-_destructor_ static void cleanup_pools(void) {
-        _cleanup_free_ char *t = NULL;
+void hashmap_cleanup_pools(void) {
+        int r;
 
-        /* Be nice to valgrind */
+        /* The pool is only allocated by the main thread, but the memory can be passed to other
+         * threads. Let's clean up if we are the main thread and no other threads are live. */
 
-        /* The pool is only allocated by the main thread, but the memory can
-         * be passed to other threads. Let's clean up if we are the main thread
-         * and no other threads are live. */
-        /* We build our own is_main_thread() here, which doesn't use C11
-         * TLS based caching of the result. That's because valgrind apparently
-         * doesn't like malloc() (which C11 TLS internally uses) to be called
-         * from a GCC destructors. */
+        /* We build our own is_main_thread() here, which doesn't use C11 TLS based caching of the
+         * result. That's because valgrind apparently doesn't like TLS to be used from a GCC destructor. */
         if (getpid() != gettid())
-                return;
+                return (void) log_debug("Not cleaning up memory pools, not in main thread.");
 
-        if (get_process_threads(0) != 1)
-                return;
+        r = get_process_threads(0);
+        if (r < 0)
+                return (void) log_debug_errno(r, "Failed to determine number of threads, not cleaning up memory pools: %m");
+        if (r != 1)
+                return (void) log_debug("Not cleaning up memory pools, running in multi-threaded process.");
 
         mempool_drop(&hashmap_pool);
         mempool_drop(&ordered_hashmap_pool);
 }
+
+#if VALGRIND
+_destructor_ static void cleanup_pools(void) {
+        /* Be nice to valgrind */
+        hashmap_cleanup_pools();
+}
 #endif
 
 static unsigned n_buckets(HashmapBase *h) {
diff --git a/src/basic/hashmap.h b/src/basic/hashmap.h
index ebb5a63eb4..1b944e93b5 100644
--- a/src/basic/hashmap.h
+++ b/src/basic/hashmap.h
@@ -443,3 +443,5 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(OrderedHashmap*, ordered_hashmap_free_free_free);
 DEFINE_TRIVIAL_CLEANUP_FUNC(IteratedCache*, iterated_cache_free);
 
 #define _cleanup_iterated_cache_free_ _cleanup_(iterated_cache_freep)
+
+void hashmap_cleanup_pools(void);
diff --git a/src/basic/mempool.c b/src/basic/mempool.c
index fff23fdbac..92d8822bd9 100644
--- a/src/basic/mempool.c
+++ b/src/basic/mempool.c
@@ -70,7 +70,6 @@ void mempool_free_tile(struct mempool *mp, void *p) {
         mp->freelist = p;
 }
 
-#if VALGRIND
 void mempool_drop(struct mempool *mp) {
         struct pool *p = mp->first_pool;
         while (p) {
@@ -80,4 +79,3 @@ void mempool_drop(struct mempool *mp) {
                 p = n;
         }
 }
-#endif
diff --git a/src/basic/mempool.h b/src/basic/mempool.h
index 539ccbdf06..80925cb95a 100644
--- a/src/basic/mempool.h
+++ b/src/basic/mempool.h
@@ -25,6 +25,4 @@ static struct mempool pool_name = { \
 
 __attribute__((weak)) bool mempool_enabled(void);
 
-#if VALGRIND
 void mempool_drop(struct mempool *mp);
-#endif

From b6ff400ffb0d164ecf538238bce50ab5510e8da5 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 14 Feb 2023 13:37:20 +0100
Subject: [PATCH 2/7] mempool: use size_t for all memory object sizes and
 counts

---
 src/basic/mempool.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/basic/mempool.h b/src/basic/mempool.h
index 80925cb95a..134e6cab17 100644
--- a/src/basic/mempool.h
+++ b/src/basic/mempool.h
@@ -10,7 +10,7 @@ struct mempool {
         struct pool *first_pool;
         void *freelist;
         size_t tile_size;
-        unsigned at_least;
+        size_t at_least;
 };
 
 void* mempool_alloc_tile(struct mempool *mp);

From 993a9314c494501f8d22c03cafda38ca17c3d40f Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 14 Feb 2023 13:38:18 +0100
Subject: [PATCH 3/7] mempool: rename local variable to match current coding
 style

---
 src/basic/mempool.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/basic/mempool.c b/src/basic/mempool.c
index 92d8822bd9..29f72a8171 100644
--- a/src/basic/mempool.c
+++ b/src/basic/mempool.c
@@ -19,15 +19,16 @@ void* mempool_alloc_tile(struct mempool *mp) {
         /* When a tile is released we add it to the list and simply
          * place the next pointer at its offset 0. */
 
+        assert(mp);
         assert(mp->tile_size >= sizeof(void*));
         assert(mp->at_least > 0);
 
         if (mp->freelist) {
-                void *r;
+                void *t;
 
-                r = mp->freelist;
-                mp->freelist = * (void**) mp->freelist;
-                return r;
+                t = mp->freelist;
+                mp->freelist = *(void**) mp->freelist;
+                return t;
         }
 
         if (_unlikely_(!mp->first_pool) ||

From 72381db942dd00f4d69b2d9a50156a62f2c350da Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 14 Feb 2023 13:40:40 +0100
Subject: [PATCH 4/7] mempool: introduce new helper pool_ptr()

This new helper returns the beginning of the usable area of the pool
object.

For now this is only used once, a later commit will use it more.
---
 src/basic/mempool.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/basic/mempool.c b/src/basic/mempool.c
index 29f72a8171..d934aa9547 100644
--- a/src/basic/mempool.c
+++ b/src/basic/mempool.c
@@ -13,6 +13,10 @@ struct pool {
         size_t n_used;
 };
 
+static void* pool_ptr(struct pool *p) {
+        return ((uint8_t*) ASSERT_PTR(p)) + ALIGN(sizeof(struct pool));
+}
+
 void* mempool_alloc_tile(struct mempool *mp) {
         size_t i;
 
@@ -54,7 +58,7 @@ void* mempool_alloc_tile(struct mempool *mp) {
 
         i = mp->first_pool->n_used++;
 
-        return ((uint8_t*) mp->first_pool) + ALIGN(sizeof(struct pool)) + i*mp->tile_size;
+        return (uint8_t*) pool_ptr(mp->first_pool) + i*mp->tile_size;
 }
 
 void* mempool_alloc0_tile(struct mempool *mp) {

From 4392b983b8e765a25c7bf081c06e8ecc37425274 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 14 Feb 2023 13:42:03 +0100
Subject: [PATCH 5/7] mempool: make mempool_free_tile() return NULL

To match how we usually do this current allocation code.

(Also, make it accept a NULL pointer, also in order to match behaviour
in the rest of our codebase)
---
 src/basic/mempool.c | 11 +++++++++--
 src/basic/mempool.h |  2 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/basic/mempool.c b/src/basic/mempool.c
index d934aa9547..999b86d5cb 100644
--- a/src/basic/mempool.c
+++ b/src/basic/mempool.c
@@ -70,9 +70,16 @@ void* mempool_alloc0_tile(struct mempool *mp) {
         return p;
 }
 
-void mempool_free_tile(struct mempool *mp, void *p) {
-        * (void**) p = mp->freelist;
+void* mempool_free_tile(struct mempool *mp, void *p) {
+        assert(mp);
+
+        if (!p)
+                return NULL;
+
+        *(void**) p = mp->freelist;
         mp->freelist = p;
+
+        return NULL;
 }
 
 void mempool_drop(struct mempool *mp) {
diff --git a/src/basic/mempool.h b/src/basic/mempool.h
index 134e6cab17..6680ba3a9b 100644
--- a/src/basic/mempool.h
+++ b/src/basic/mempool.h
@@ -15,7 +15,7 @@ struct mempool {
 
 void* mempool_alloc_tile(struct mempool *mp);
 void* mempool_alloc0_tile(struct mempool *mp);
-void mempool_free_tile(struct mempool *mp, void *p);
+void* mempool_free_tile(struct mempool *mp, void *p);
 
 #define DEFINE_MEMPOOL(pool_name, tile_type, alloc_at_least) \
 static struct mempool pool_name = { \

From a2b052b29f8bc141e94a4af95d1653a38a57eaeb Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 14 Feb 2023 13:44:51 +0100
Subject: [PATCH 6/7] mempool: rework mempool_cleanup() to only release freed
 tiles

This substantially reworks mempool_cleanup() so that it releases pools
with all freed tiles only, but keeps all pools with still-allocated
tiles around.

This is more correct, as the previous implementation just released all
pools regardless if anything was still used or not. This would make
valgrind shut up but would just hide memory leaks altogether. Moreover
if called during regular runtime of a program would result in bad memory
accesses all over.

Hence, let's add a proper implementation and only trim pools we really
know are empty.

This way we can safely call these functions later, when under memory
pressure, at any time.
---
 src/basic/hashmap.c |  6 +--
 src/basic/hashmap.h |  2 +-
 src/basic/mempool.c | 94 +++++++++++++++++++++++++++++++++++++++++----
 src/basic/mempool.h |  2 +-
 4 files changed, 92 insertions(+), 12 deletions(-)

diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c
index c18c75d78c..75119866d4 100644
--- a/src/basic/hashmap.c
+++ b/src/basic/hashmap.c
@@ -274,7 +274,7 @@ static _used_ const struct hashmap_type_info hashmap_type_info[_HASHMAP_TYPE_MAX
         },
 };
 
-void hashmap_cleanup_pools(void) {
+void hashmap_trim_pools(void) {
         int r;
 
         /* The pool is only allocated by the main thread, but the memory can be passed to other
@@ -291,8 +291,8 @@ void hashmap_cleanup_pools(void) {
         if (r != 1)
                 return (void) log_debug("Not cleaning up memory pools, running in multi-threaded process.");
 
-        mempool_drop(&hashmap_pool);
-        mempool_drop(&ordered_hashmap_pool);
+        mempool_trim(&hashmap_pool);
+        mempool_trim(&ordered_hashmap_pool);
 }
 
 #if VALGRIND
diff --git a/src/basic/hashmap.h b/src/basic/hashmap.h
index 1b944e93b5..68d9b81cf2 100644
--- a/src/basic/hashmap.h
+++ b/src/basic/hashmap.h
@@ -444,4 +444,4 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(IteratedCache*, iterated_cache_free);
 
 #define _cleanup_iterated_cache_free_ _cleanup_(iterated_cache_freep)
 
-void hashmap_cleanup_pools(void);
+void hashmap_trim_pools(void);
diff --git a/src/basic/mempool.c b/src/basic/mempool.c
index 999b86d5cb..fa319bffdb 100644
--- a/src/basic/mempool.c
+++ b/src/basic/mempool.c
@@ -3,6 +3,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 
+#include "format-util.h"
 #include "macro.h"
 #include "memory-util.h"
 #include "mempool.h"
@@ -82,12 +83,91 @@ void* mempool_free_tile(struct mempool *mp, void *p) {
         return NULL;
 }
 
-void mempool_drop(struct mempool *mp) {
-        struct pool *p = mp->first_pool;
-        while (p) {
-                struct pool *n;
-                n = p->next;
-                free(p);
-                p = n;
+static bool pool_contains(struct mempool *mp, struct pool *p, void *ptr) {
+        size_t off;
+        void *a;
+
+        assert(mp);
+        assert(p);
+
+        if (!ptr)
+                return false;
+
+        a = pool_ptr(p);
+        if ((uint8_t*) ptr < (uint8_t*) a)
+                return false;
+
+        off = (uint8_t*) ptr - (uint8_t*) a;
+        assert(off % mp->tile_size == 0);
+
+        return off < mp->tile_size * p->n_tiles;
+}
+
+static bool pool_is_unused(struct mempool *mp, struct pool *p) {
+        assert(mp);
+        assert(p);
+
+        if (p->n_used == 0)
+                return true;
+
+        /* Check if all tiles in this specific pool are in the freelist. */
+        size_t n = 0;
+        void *i = mp->freelist;
+        while (i) {
+                if (pool_contains(mp, p, i))
+                        n++;
+
+                i = *(void**) i;
+        }
+
+        assert(n <= p->n_used);
+
+        return n == p->n_used;
+}
+
+static void pool_unlink(struct mempool *mp, struct pool *p) {
+        size_t m = 0;
+
+        assert(mp);
+        assert(p);
+
+        if (p->n_used == 0)
+                return;
+
+        void **i = &mp->freelist;
+        while (*i) {
+                void *d = *i;
+
+                if (pool_contains(mp, p, d)) {
+                        *i = *(void**) d;
+                        m++;
+
+                        if (m == p->n_used)
+                                break;
+                } else
+                        i = (void**) d;
         }
 }
+
+void mempool_trim(struct mempool *mp) {
+        size_t trimmed = 0, left = 0;
+
+        assert(mp);
+
+        struct pool **p = &mp->first_pool;
+        while (*p) {
+                struct pool *d = *p;
+
+                if (pool_is_unused(mp, d)) {
+                        trimmed += d->n_tiles * mp->tile_size;
+                        pool_unlink(mp, d);
+                        *p = d->next;
+                        free(d);
+                } else {
+                        left += d->n_tiles * mp->tile_size;
+                        p = &d->next;
+                }
+        }
+
+        log_debug("Trimmed %s from memory pool %p. (%s left)", FORMAT_BYTES(trimmed), mp, FORMAT_BYTES(left));
+}
diff --git a/src/basic/mempool.h b/src/basic/mempool.h
index 6680ba3a9b..ba588af451 100644
--- a/src/basic/mempool.h
+++ b/src/basic/mempool.h
@@ -25,4 +25,4 @@ static struct mempool pool_name = { \
 
 __attribute__((weak)) bool mempool_enabled(void);
 
-void mempool_drop(struct mempool *mp);
+void mempool_trim(struct mempool *mp);

From c144696e124575358f123de4bd02ba4c687b1793 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 14 Feb 2023 13:49:17 +0100
Subject: [PATCH 7/7] tests: add test for mempool logic

---
 src/test/meson.build    |  2 +
 src/test/test-mempool.c | 92 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100644 src/test/test-mempool.c

diff --git a/src/test/meson.build b/src/test/meson.build
index f05e6a5694..1dbe12dc40 100644
--- a/src/test/meson.build
+++ b/src/test/meson.build
@@ -509,6 +509,8 @@ tests += [
 
         [files('test-tpm2.c')],
 
+        [files('test-mempool.c')],
+
         [files('test-replace-var.c')],
 
         [files('test-calendarspec.c')],
diff --git a/src/test/test-mempool.c b/src/test/test-mempool.c
new file mode 100644
index 0000000000..d3bc173fd4
--- /dev/null
+++ b/src/test/test-mempool.c
@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+
+#include "mempool.h"
+#include "random-util.h"
+#include "tests.h"
+
+struct element {
+        uint64_t value;
+};
+
+DEFINE_MEMPOOL(test_mempool, struct element, 8);
+
+TEST(mempool_trim) {
+
+#define NN 4000
+        struct element *a[NN];
+        size_t n_freed = 0;
+
+        assert_se(!test_mempool.first_pool);
+        assert_se(!test_mempool.freelist);
+
+        mempool_trim(&test_mempool);
+
+        for (size_t i = 0; i < NN; i++) {
+                assert_se(a[i] = mempool_alloc_tile(&test_mempool));
+                a[i]->value = i;
+        }
+
+        mempool_trim(&test_mempool);
+
+        /* free up to one third randomly */
+        size_t x = 0;
+        for (size_t i = 0; i < NN/3; i++) {
+                x = (x + random_u64()) % ELEMENTSOF(a);
+                assert_se(!a[x] || a[x]->value == x);
+
+                if (a[x])
+                        n_freed ++;
+
+                a[x] = mempool_free_tile(&test_mempool, a[x]);
+        }
+
+        mempool_trim(&test_mempool);
+
+        /* free definitely at least one third */
+        for (size_t i = 2; i < NN; i += 3) {
+                assert_se(!a[i] || a[i]->value == i);
+                if (a[i])
+                        n_freed ++;
+                a[i] = mempool_free_tile(&test_mempool, a[i]);
+        }
+
+        mempool_trim(&test_mempool);
+
+        /* Allocate another set of tiles, which will fill up the free list and allocate some new tiles */
+        struct element *b[NN];
+        for (size_t i = 0; i < NN; i++) {
+                assert_se(b[i] = mempool_alloc_tile(&test_mempool));
+                b[i]->value = ~(uint64_t) i;
+        }
+
+        mempool_trim(&test_mempool);
+
+        /* free everything from the original set*/
+
+        for (size_t i = 0; i < NN; i += 1) {
+                assert_se(!a[i] || a[i]->value == i);
+                if (a[i])
+                        n_freed ++;
+                a[i] = mempool_free_tile(&test_mempool, a[i]);
+        }
+
+        mempool_trim(&test_mempool);
+
+        /* and now everything from the second set too */
+
+        for (size_t i = 0; i < NN; i += 1) {
+                assert_se(!b[i] || b[i]->value == ~(uint64_t) i);
+                if (b[i])
+                        n_freed ++;
+                b[i] = mempool_free_tile(&test_mempool, b[i]);
+        }
+
+        assert_se(n_freed == NN * 2);
+
+        mempool_trim(&test_mempool);
+
+        assert_se(!test_mempool.first_pool);
+        assert_se(!test_mempool.freelist);
+}
+
+DEFINE_TEST_MAIN(LOG_DEBUG);