From d51c0e8e303fbac0fccbdac3c4c21e0dcb633145 Mon Sep 17 00:00:00 2001 From: Oliver Anderson Date: Thu, 7 May 2026 11:17:29 +0200 Subject: [PATCH 1/3] memtouch: Improve safety of cleanup_memory We do not want cleanup_memory to call munmap on a null pointer or if mmap failed. Also set mem_base to be the null pointer before returning if the munmap call succeeded. Signed-off-by: Oliver Anderson On-behalf-of: SAP oliver.anderson@sap.com --- memtouch.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/memtouch.cpp b/memtouch.cpp index 9c9f287..416bd61 100644 --- a/memtouch.cpp +++ b/memtouch.cpp @@ -169,9 +169,15 @@ class WorkerThread { } void cleanup_memory() { + if (mem_base == nullptr || mem_base == MAP_FAILED) { + return; + } + if (munmap(mem_base, uint64_t(mem_size_mib) * 1024 * 1024) != 0) { printf("Unable to unmap memory\n"); + return; } + mem_base = nullptr; } bool allocate_memory() { From 2c74fa0084420c2eaf580870267ec8d6d0b9ba43 Mon Sep 17 00:00:00 2001 From: Oliver Anderson Date: Thu, 7 May 2026 11:24:36 +0200 Subject: [PATCH 2/3] memtouch: Move cleanup_memory into WorkerThread's destructor We move cleanup_memory into WorkerThread's destructor in order to enable early return in the `run` method in our next commit. Signed-off-by: Oliver Anderson On-behalf-of: SAP oliver.anderson@sap.com --- memtouch.cpp | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/memtouch.cpp b/memtouch.cpp index 416bd61..db76bde 100644 --- a/memtouch.cpp +++ b/memtouch.cpp @@ -24,13 +24,21 @@ static constexpr int DEFAULT_STAT_IVAL(1000); using namespace std; struct Statistics { - Statistics() = default; + Statistics() = default; + Statistics(const Statistics&) = delete; + Statistics& operator=(const Statistics&) = delete; Statistics(Statistics&& o) noexcept { write_rate.store(o.write_rate.load()); read_rate.store(o.read_rate.load()); } + Statistics& operator=(Statistics&& o) noexcept { + write_rate.store(o.write_rate.load()); + read_rate.store(o.read_rate.load()); + return *this; + } + std::atomic read_rate{0}; std::atomic write_rate{0}; }; @@ -49,6 +57,43 @@ class WorkerThread { page_log_ival(page_log_ival_), stats() {} + ~WorkerThread() { cleanup_memory(); } + + WorkerThread(const WorkerThread&) = delete; + WorkerThread& operator=(const WorkerThread&) = delete; + + WorkerThread(WorkerThread&& o) noexcept + : id(o.id), + run_once(o.run_once), + mem_size_mib(o.mem_size_mib), + rw_ratio(o.rw_ratio), + page_log_ival(o.page_log_ival), + mem_base(o.mem_base), + read_buffer(), + stats(std::move(o.stats)) { + o.mem_base = nullptr; + } + + WorkerThread& operator=(WorkerThread&& o) noexcept { + if (this == &o) { + return *this; + } + + cleanup_memory(); + + id = o.id; + run_once = o.run_once; + mem_size_mib = o.mem_size_mib; + rw_ratio = o.rw_ratio; + page_log_ival = o.page_log_ival; + mem_base = o.mem_base; + stats = std::move(o.stats); + + o.mem_base = nullptr; + + return *this; + } + bool pre_run() { if (not allocate_memory()) { printf("Worker %d: Unable to allocate memory\n", id); @@ -78,8 +123,6 @@ class WorkerThread { while (not terminate) { run_loop(num_pages); } - - cleanup_memory(); } int64_t measure_time_ns(std::function func) { From e21e653a27219683f5da4b77cfabe29621ffabd9 Mon Sep 17 00:00:00 2001 From: Oliver Anderson Date: Wed, 6 May 2026 19:52:17 +0200 Subject: [PATCH 3/3] memtouch: Fix UB in the signal handler The way the signal handler is currently implemented we end up writing to bool's read by other threads without the use of atomics or synchronization. This is undefined behavior: It is subject to data races and/or some compilers (either existing or to appear in the future) may end up optimizing the reads out entirely. We fix this by using a single static `std::atomic` that the worker and statistics threads read from and the signal handler writes to. The less invasive solution would be to replace each individual thread's `terminate` bool with an `std::atomic`, but I could not get the constructors to accept that. Moreover the compiler messages were rather arcane and hard to understand for someone who just started writing C++. There is probably something fancy going on with copy constructors and/or constexpr or what have you. Overall I personally prefer this simpler solution, while admitting that it is less flexible if individual thread termination should be desired in the future. I am going with the YAGNI principle here. Signed-off-by: Oliver Anderson On-behalf-of: SAP --- memtouch.cpp | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/memtouch.cpp b/memtouch.cpp index db76bde..3c32d57 100644 --- a/memtouch.cpp +++ b/memtouch.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -21,6 +22,8 @@ static constexpr uint64_t PAGE_SIZE(4096); static constexpr int PATTERN(0xff); static constexpr int DEFAULT_STAT_IVAL(1000); +static std::atomic global_terminate{false}; + using namespace std; struct Statistics { @@ -110,17 +113,17 @@ class WorkerThread { // Warmup, write every page for (uint64_t page{0}; page < num_pages; ++page) { - if (terminate) { + if (global_terminate.load(std::memory_order_relaxed)) { break; } write_page(page); } if (run_once) { - kill(); + return; } - while (not terminate) { + while (not global_terminate.load(std::memory_order_relaxed)) { run_loop(num_pages); } } @@ -230,8 +233,6 @@ class WorkerThread { return mem_base != MAP_FAILED; } - void kill() { terminate = true; } - float write_rate() const { return stats.write_rate.load(); } float read_rate() const { return stats.read_rate.load(); } @@ -243,8 +244,6 @@ class WorkerThread { unsigned rw_ratio; uint64_t page_log_ival; - bool terminate{false}; - void* mem_base{nullptr}; char read_buffer[PAGE_SIZE]; @@ -263,7 +262,7 @@ class StatisticsThread { } void run() { - while (not terminate) { + while (not global_terminate.load(std::memory_order_relaxed)) { float read_rate{0}; float write_rate{0}; @@ -283,8 +282,6 @@ class StatisticsThread { } } - void kill() { terminate = true; } - void set_interval(unsigned ival_ms) { logging_ival_ms = ival_ms; } string get_iso8601_time() { @@ -316,7 +313,6 @@ class StatisticsThread { private: vector& workers; - bool terminate{false}; unsigned logging_ival_ms{DEFAULT_STAT_IVAL}; ofstream log_file{}; @@ -329,11 +325,11 @@ StatisticsThread stat_thread(worker_storage); void sigint_handler([[maybe_unused]] int s) { printf("Terminating...\n"); - for (auto& worker : worker_storage) { - worker.kill(); - } - stat_thread.kill(); + static_assert(std::atomic::is_always_lock_free, + "Unable to ensure async-signal-handler safety: std::atomic should always " + "be lock free, but the assertion failed."); + global_terminate.store(true, std::memory_order_relaxed); } void setup_signals() {