From 433f35987fc8d89757d8b1cf15756410a4652d09 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 13 Nov 2024 09:35:12 -0500 Subject: [PATCH 1/3] Do not clobber the Total line in mgr:mem report Since 2022 commit a7508376, Squid reported incorrect aggregate "Total" line stats. The line itself was mislabeled by repeating the name of the last MemPools::pools pool (which may vary with traffic patterns!), confusing admins and wreaking havoc on mgr:mem analysis scripts. --- src/mem/PoolChunked.cc | 1 - src/mem/PoolMalloc.cc | 1 - src/mem/Stats.cc | 12 +++++++----- src/mem/Stats.h | 1 - src/mem/old_api.cc | 2 +- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/mem/PoolChunked.cc b/src/mem/PoolChunked.cc index c397d00ffc2..50362e3db89 100644 --- a/src/mem/PoolChunked.cc +++ b/src/mem/PoolChunked.cc @@ -437,7 +437,6 @@ MemPoolChunked::getStats(Mem::PoolStats &stats) clean((time_t) 555555); /* don't want to get chunks released before reporting */ - stats.pool = this; stats.label = label; stats.meter = &meter; stats.obj_size = objectSize; diff --git a/src/mem/PoolMalloc.cc b/src/mem/PoolMalloc.cc index 7a33eab94cb..f409594c2d8 100644 --- a/src/mem/PoolMalloc.cc +++ b/src/mem/PoolMalloc.cc @@ -61,7 +61,6 @@ MemPoolMalloc::deallocate(void *obj) size_t MemPoolMalloc::getStats(Mem::PoolStats &stats) { - stats.pool = this; stats.label = label; stats.meter = &meter; stats.obj_size = objectSize; diff --git a/src/mem/Stats.cc b/src/mem/Stats.cc index 418557d0cfa..b8e768b9fb1 100644 --- a/src/mem/Stats.cc +++ b/src/mem/Stats.cc @@ -16,11 +16,6 @@ Mem::GlobalStats(PoolStats &stats) { MemPools::GetInstance().flushMeters(); - stats.meter = &TheMeter; - stats.label = "Total"; - stats.obj_size = 1; - stats.overhead += sizeof(MemPools); - /* gather all stats for Totals */ size_t pools_inuse = 0; for (const auto pool: MemPools::GetInstance().pools) { @@ -29,5 +24,12 @@ Mem::GlobalStats(PoolStats &stats) stats.overhead += sizeof(Allocator *); } + // Reset PoolStats::meter, label, and obj_size data members after getStats() + // calls in the above loop set them. TODO: Refactor to remove these members. + stats.meter = &TheMeter; + stats.label = "Total"; + stats.obj_size = 1; + stats.overhead += sizeof(MemPools); + return pools_inuse; } diff --git a/src/mem/Stats.h b/src/mem/Stats.h index a0af35e222e..fba1d290f0c 100644 --- a/src/mem/Stats.h +++ b/src/mem/Stats.h @@ -17,7 +17,6 @@ namespace Mem class PoolStats { public: - Allocator *pool = nullptr; const char *label = nullptr; PoolMeter *meter = nullptr; int obj_size = 0; diff --git a/src/mem/old_api.cc b/src/mem/old_api.cc index d6fc8058fee..7565d2c811a 100644 --- a/src/mem/old_api.cc +++ b/src/mem/old_api.cc @@ -586,7 +586,7 @@ Mem::Report(std::ostream &stream) PoolStats mp_stats; pool->getStats(mp_stats); - if (mp_stats.pool->meter.gb_allocated.count > 0) + if (pool->meter.gb_allocated.count > 0) usedPools.emplace_back(mp_stats); else ++not_used; From a644ade205aff767e78d1d31e94ea40bcf07b90d Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 14 Nov 2024 09:10:40 -0500 Subject: [PATCH 2/3] fixup: Attempt to clarify "these members" Context: https://github.com/squid-cache/squid/pull/1945#discussion_r1841251677 --- src/mem/Stats.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mem/Stats.cc b/src/mem/Stats.cc index b8e768b9fb1..751f3a03d90 100644 --- a/src/mem/Stats.cc +++ b/src/mem/Stats.cc @@ -25,10 +25,11 @@ Mem::GlobalStats(PoolStats &stats) } // Reset PoolStats::meter, label, and obj_size data members after getStats() - // calls in the above loop set them. TODO: Refactor to remove these members. + // calls in the above loop set them. TODO: Refactor to remove these data members. stats.meter = &TheMeter; stats.label = "Total"; stats.obj_size = 1; + stats.overhead += sizeof(MemPools); return pools_inuse; From 5f6793b66f5f79c73f1a5378b9aa44d103990d1c Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 15 Nov 2024 21:33:53 -0500 Subject: [PATCH 3/3] fixup: Removed TODO about removing problematic members. Context: https://github.com/squid-cache/squid/pull/1945#discussion_r1844719126 --- src/mem/Stats.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mem/Stats.cc b/src/mem/Stats.cc index 751f3a03d90..66604bd645a 100644 --- a/src/mem/Stats.cc +++ b/src/mem/Stats.cc @@ -25,7 +25,7 @@ Mem::GlobalStats(PoolStats &stats) } // Reset PoolStats::meter, label, and obj_size data members after getStats() - // calls in the above loop set them. TODO: Refactor to remove these data members. + // calls in the above loop set them. stats.meter = &TheMeter; stats.label = "Total"; stats.obj_size = 1;