This is an automated email from the ASF dual-hosted git repository.
moonchen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 113f5fbb16 Fix hdrHeap/hdrStrHeap allocator inuse metric underflow
(#13218)
113f5fbb16 is described below
commit 113f5fbb1667df56799e9878c71d6cd286695a49
Author: Mo Chen <[email protected]>
AuthorDate: Tue Jun 16 13:08:27 2026 -0500
Fix hdrHeap/hdrStrHeap allocator inuse metric underflow (#13218)
The hdrHeap and hdrStrHeap allocators are plain Allocator globals whose
objects are allocated with no constructor arguments, so THREAD_ALLOC
resolves to the non-templated thread_alloc(Allocator &, ProxyAllocator &)
overload. Unlike the templated overload, it did not call
increment_for_alloc() when reusing an object from the per-thread
freelist, while THREAD_FREE always decremented inuse on the way in.
The counted frees therefore outran the counted allocs and
proxy.process.allocator.inuse.{hdrHeap,hdrStrHeap} marched negative,
wrapping around as a huge uint64 value. Other allocators use
ClassAllocator (the templated path) and were unaffected.
Account for the freelist reuse in the non-templated overload so the two
paths stay symmetric, and add a regression test that cycles a block
through the freelist and asserts inuse stays balanced and never dips
below its starting value.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
---
src/iocore/eventsystem/ProxyAllocator.cc | 8 +++
src/proxy/hdrs/unit_tests/test_HdrHeap.cc | 101 ++++++++++++++++++++++++++++++
2 files changed, 109 insertions(+)
diff --git a/src/iocore/eventsystem/ProxyAllocator.cc
b/src/iocore/eventsystem/ProxyAllocator.cc
index 7b6f4dd1d7..8f9d93cfc3 100644
--- a/src/iocore/eventsystem/ProxyAllocator.cc
+++ b/src/iocore/eventsystem/ProxyAllocator.cc
@@ -34,6 +34,14 @@ thread_alloc(Allocator &a, ProxyAllocator &l)
void *v = l.freelist;
l.freelist = *static_cast<void **>(l.freelist);
--(l.allocated);
+#if TS_USE_ALLOCATOR_METRICS
+ // Reusing an item from the per-thread freelist is an allocation as far as
+ // the metrics are concerned. THREAD_FREE decremented inuse when the item
+ // was pushed onto the freelist, so without this the counted frees exceed
+ // the counted allocs and inuse underflows. The templated thread_alloc()
+ // below already does this; the two paths must stay symmetric.
+ a.increment_for_alloc();
+#endif
return v;
}
return a.alloc_void();
diff --git a/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
b/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
index 66e3bcacd0..c2017e714c 100644
--- a/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
+++ b/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
@@ -23,6 +23,17 @@
#include "proxy/hdrs/HdrHeap.h"
#include "proxy/hdrs/URL.h"
+#include "tscore/ink_config.h"
+
+#if TS_USE_ALLOCATOR_METRICS
+#include "tscore/Allocator.h"
+#include "tsutil/Metrics.h"
+#include "iocore/eventsystem/ProxyAllocator.h"
+
+#include <algorithm>
+#include <cstdint>
+#endif
+
/**
This test is designed to test numerous pieces of the HdrHeaps including
allocations,
demotion of rw heaps to ronly heaps, and finally the coalesce and evacuate
behaviours.
@@ -130,3 +141,93 @@ TEST_CASE("HdrHeap", "[proxy][hdrheap]")
// Clean up
heap->destroy();
}
+
+#if TS_USE_ALLOCATOR_METRICS
+
+extern Allocator hdrHeapAllocator;
+extern Allocator strHeapAllocator;
+
+namespace
+{
+// Mirror the per-thread-freelist branch of the THREAD_FREE macro: push the
block
+// onto the freelist and account the free with the same metered call the macro
+// uses (UPDATE_FREE_METRICS).
+void
+freelist_push(Allocator &a, ProxyAllocator &l, void *p)
+{
+ *reinterpret_cast<char **>(p) = reinterpret_cast<char *>(l.freelist);
+ l.freelist = p;
+ l.allocated++;
+ a.increment_for_free();
+}
+
+// RAII guard that restores cmd_disable_pfreelist on scope exit, so a failing
+// REQUIRE()/CHECK() does not leak the flipped flag into later tests.
+struct PfreelistGuard {
+ int saved;
+ explicit PfreelistGuard(bool disable) : saved{cmd_disable_pfreelist} {
cmd_disable_pfreelist = disable; }
+ ~PfreelistGuard() { cmd_disable_pfreelist = saved; }
+};
+} // namespace
+
+// Regression test for the hdrHeap/hdrStrHeap allocator inuse underflow. These
two
+// allocators are plain Allocator globals whose objects are allocated with no
+// constructor arguments, so THREAD_ALLOC resolves to the non-templated
+// thread_alloc(Allocator &, ProxyAllocator &). That overload used to skip the
+// metric increment when reusing an object from the per-thread freelist, while
+// THREAD_FREE always decremented it on the way in. The counted frees therefore
+// outran the counted allocs and proxy.process.allocator.inuse.* marched
negative,
+// wrapping around as a huge uint64 value.
+TEST_CASE("allocator inuse stays balanced across freelist reuse",
"[proxy][hdrheap][metrics]")
+{
+ struct AllocCase {
+ const char *metric;
+ Allocator *allocator;
+ };
+
+ AllocCase const cases[] = {
+ {"proxy.process.allocator.inuse.hdrHeap", &hdrHeapAllocator},
+ {"proxy.process.allocator.inuse.hdrStrHeap", &strHeapAllocator},
+ };
+
+ // The unit test harness disables per-thread freelists; enable them so that
+ // thread_alloc() exercises the freelist-reuse path that regressed. The guard
+ // restores the harness setting even if an assertion below throws.
+ PfreelistGuard const pfreelist_guard{false};
+
+ for (auto const &c : cases) {
+ ts::Metrics::IdType id;
+ auto *inuse = ts::Metrics::Gauge::lookup(c.metric, &id);
+ REQUIRE(inuse != nullptr);
+
+ ProxyAllocator l;
+ int64_t const baseline = inuse->load();
+ int64_t low = baseline;
+
+ // One live block, allocated from the global pool (a counted allocation).
+ void *p = thread_alloc(*c.allocator, l);
+ REQUIRE(p != nullptr);
+
+ // Cycle the single block through the freelist. Each iteration frees it
+ // (inuse--) then reuses it (which must inuse++). Without the fix the reuse
+ // does not re-increment and inuse marches negative.
+ for (int i = 0; i < 1000; ++i) {
+ freelist_push(*c.allocator, l, p);
+ low = std::min(low, inuse->load());
+ p = thread_alloc(*c.allocator, l);
+ REQUIRE(p != nullptr);
+ low = std::min(low, inuse->load());
+ }
+
+ // Exactly one block is still outstanding, and inuse never dipped below the
+ // value we started from.
+ CHECK(inuse->load() == baseline + 1);
+ CHECK(low >= baseline);
+
+ // Release the outstanding block back to the global pool, restoring inuse.
+ c.allocator->free_void(p);
+ CHECK(inuse->load() == baseline);
+ }
+}
+
+#endif // TS_USE_ALLOCATOR_METRICS
