From 017f18c3b02a44d4879e838d652d658d84e7c11b Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Mon, 1 Jun 2026 14:19:42 -0500 Subject: [PATCH 1/2] Fix hdrHeap/hdrStrHeap allocator inuse metric underflow 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 --- src/iocore/eventsystem/ProxyAllocator.cc | 8 ++ src/proxy/hdrs/unit_tests/test_HdrHeap.cc | 95 +++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/src/iocore/eventsystem/ProxyAllocator.cc b/src/iocore/eventsystem/ProxyAllocator.cc index 7b6f4dd1d7c..8f9d93cfc33 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(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 66e3bcacd0f..d465c03c26d 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 +#include +#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,87 @@ 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(p) = reinterpret_cast(l.freelist); + l.freelist = p; + l.allocated++; + a.increment_for_free(); +} +} // 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. + bool const saved_disable = cmd_disable_pfreelist; + cmd_disable_pfreelist = 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); + } + + cmd_disable_pfreelist = saved_disable; +} + +#endif // TS_USE_ALLOCATOR_METRICS From 1b241227e22482b3570197cb1eadd95eb3a4ce6e Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Mon, 1 Jun 2026 14:25:27 -0500 Subject: [PATCH 2/2] Restore cmd_disable_pfreelist via RAII guard in test A failing REQUIRE()/CHECK() throws and would skip the manual restore of the cmd_disable_pfreelist harness flag, contaminating later tests in the same process. Use an RAII guard that preserves and restores the original int value on scope exit. Co-Authored-By: Claude Opus 4.8 --- src/proxy/hdrs/unit_tests/test_HdrHeap.cc | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/proxy/hdrs/unit_tests/test_HdrHeap.cc b/src/proxy/hdrs/unit_tests/test_HdrHeap.cc index d465c03c26d..c2017e714c3 100644 --- a/src/proxy/hdrs/unit_tests/test_HdrHeap.cc +++ b/src/proxy/hdrs/unit_tests/test_HdrHeap.cc @@ -160,6 +160,14 @@ freelist_push(Allocator &a, ProxyAllocator &l, void *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 @@ -183,9 +191,9 @@ TEST_CASE("allocator inuse stays balanced across freelist reuse", "[proxy][hdrhe }; // The unit test harness disables per-thread freelists; enable them so that - // thread_alloc() exercises the freelist-reuse path that regressed. - bool const saved_disable = cmd_disable_pfreelist; - cmd_disable_pfreelist = false; + // 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; @@ -220,8 +228,6 @@ TEST_CASE("allocator inuse stays balanced across freelist reuse", "[proxy][hdrhe c.allocator->free_void(p); CHECK(inuse->load() == baseline); } - - cmd_disable_pfreelist = saved_disable; } #endif // TS_USE_ALLOCATOR_METRICS