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..c2017e714c3 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,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(p) = reinterpret_cast(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