prevent memory monitor from advising away more than maxLimitPercent

Summary:
Currently, it is possible for MemoryMonitor to advise away more than maxLimitPercent_ of the cache. MemoryMonitor just checks if the amount of currently advised away memory is higher than maxLimitPercent_. It does not take into account how much memory will be advised away in total after performing the slab releases.

If MemoryMonitor is configured in a way where maxLimitPercent_ is set to 80% and in each iteration 10% of the memory is advised away, then after each 8 iterations, we will be at exactly 80%, but MemoryMonitor will still do one more iteration, resulting in 90% of the memory being advised away.

I change this behavior such that when making the decision if and how much to advise away we also take into account how much is going to be advised away. This ensures that we never advise away more than maxLimitPercent_ of the cache.

Reviewed By: pbhandar2, rlyerly

Differential Revision: D87043177

fbshipit-source-id: d05becebd76fa094667249306d6410290ca16a82

Author: AlnisM

cachelib/allocator/MemoryMonitor.cpp

@@ -247,21 +247,32 @@ void MemoryMonitor::adviseAwaySlabs() {
     return;
   }
   const auto numAdvised = cache_.getCacheMemoryStats().numAdvisedSlabs();
-  const auto advisedPercent = numAdvised * 100 / (numAdvised + totalSlabs);
-  if (advisedPercent > maxLimitPercent_) {
-    XLOGF(CRITICAL,
-          "More than {} slabs of {} ({}"
-          "%) in the item cache memory have been advised away. "
-          "This exceeds the maximum limit of {}"
-          "%. Disabling advising which may result in an OOM.",
-          numAdvised, numAdvised + totalSlabs, advisedPercent,
-          maxLimitPercent_);
-    return;
-  }
+  // allSlabs represents the total number of slabs across all pools
+  const auto allSlabs = numAdvised + totalSlabs;
+
+  // Check if we've already hit or exceeded the maximum limit
+  const auto maxNumAdvisedSlabs = (maxLimitPercent_ * allSlabs / 100);
   // Advise percentAdvisePerIteration_% of upperLimit_ - lowerLimit_
   // every iteration
-  const auto slabsToAdvise = bytesToSlabs(upperLimit_ - lowerLimit_) *
-                             percentAdvisePerIteration_ / 100;
+  auto slabsToAdvise = bytesToSlabs(upperLimit_ - lowerLimit_) *
+                       percentAdvisePerIteration_ / 100;
+  auto remainingSlabsAllowedToAdviseAway =
+      numAdvised >= maxNumAdvisedSlabs ? 0 : maxNumAdvisedSlabs - numAdvised;
+  // Clamp slabsToAdvise to stay within maxLimitPercent_
+  slabsToAdvise = std::min(slabsToAdvise, remainingSlabsAllowedToAdviseAway);
+  if (slabsToAdvise == 0) {
+    // Only log CRITICAL if we hit the max limit, not if slabsToAdvise was
+    // already 0 due to configuration (e.g., upperLimit_ == lowerLimit_)
+    if (remainingSlabsAllowedToAdviseAway == 0) {
+      XLOGF(CRITICAL,
+            "Already advised {} slabs of {} ({}"
+            "%) which is at or exceeds the maximum limit of {}"
+            "%. Disabling advising which may result in an OOM.",
+            numAdvised, allSlabs, numAdvised * 100 / allSlabs,
+            maxLimitPercent_);
+    }
+    return;
+  }
   XLOGF(DBG, "Advising away {} slabs to free {} bytes", slabsToAdvise,
         slabsToAdvise * Slab::kSize);
   updateNumSlabsToAdvise(slabsToAdvise);

cachelib/allocator/tests/AllocatorResizeTest.h

@@ -894,7 +894,7 @@ class AllocatorResizeTest : public AllocatorTest<AllocatorT> {
   }
 
   // testMemoryMonitorPerIterationAdviseReclaim
-  // Crate 5 pools of 50 slabs each
+  // Create 5 pools of 50 slabs each
   // fillup all pools
   // since lower limit is set to 1GB and upper limit set to 2GB, and
   // advise reclaim percent per iter value of 2, per iteration, 5 slabs
@@ -967,7 +967,7 @@ class AllocatorResizeTest : public AllocatorTest<AllocatorT> {
 
       unsigned int i;
       /* iterate for numItersToMaxAdviseAway times */
-      for (i = 1; i <= numItersToMaxAdviseAway + 1; i++) {
+      for (i = 1; i <= numItersToMaxAdviseAway; i++) {
         alloc.memMonitor_->adviseAwaySlabs();
         std::this_thread::sleep_for(std::chrono::seconds{2});
         ASSERT_EQ(alloc.allocator_->getAdvisedMemorySize(), i * perIterAdvSize);
@@ -980,7 +980,7 @@ class AllocatorResizeTest : public AllocatorTest<AllocatorT> {
       ASSERT_EQ(totalAdvisedAwayMemory, i * perIterAdvSize);
 
       // Try to reclaim back
-      for (i = 1; i <= numItersToMaxAdviseAway + 1; i++) {
+      for (i = 1; i <= numItersToMaxAdviseAway; i++) {
         alloc.memMonitor_->reclaimSlabs();
         std::this_thread::sleep_for(std::chrono::seconds{2});
         ASSERT_EQ(alloc.allocator_->getAdvisedMemorySize(),
@@ -1172,6 +1172,109 @@ class AllocatorResizeTest : public AllocatorTest<AllocatorT> {
       threads[i].join();
     }
   }
+
+  struct MemMonitorTestParams {
+    unsigned int totalSlabs;
+    unsigned int slabsToFill;
+    unsigned int maxAdvisePercent;
+    unsigned int expectedAdvisedAfterFirstIteration;
+    unsigned int expectedAdvisedAfterSecondIteration;
+  };
+
+  void setupMemMonitorTest(typename AllocatorT::Config& config) {
+    config.memMonitorConfig.mode = MemoryMonitor::TestMode;
+    config.memMonitorInterval = std::chrono::seconds(kMemoryMonitorInterval);
+    config.memMonitorConfig.maxAdvisePercentPerIter = 4;
+    config.memMonitorConfig.upperLimitGB = 2;
+    config.memMonitorConfig.lowerLimitGB = 1;
+    // With these settings, memory monitor will advise away 4% * (2GB-1GB) =
+    // 40MB = 10 slabs per iteration
+  }
+
+  void runMemMonitorTest(const MemMonitorTestParams& params) {
+    typename AllocatorT::Config config;
+    setupMemMonitorTest(config);
+    config.memMonitorConfig.maxAdvisePercent = params.maxAdvisePercent;
+    config.setCacheSize(params.totalSlabs * Slab::kSize);
+
+    AllocatorT alloc(config);
+
+    const auto numBytes = alloc.getCacheMemoryStats().ramCacheSize;
+    const std::set<uint32_t> acSizes = {512 * 1024};
+    auto poolId = alloc.addPool("test_pool", numBytes, acSizes);
+    ASSERT_NE(Slab::kInvalidPoolId, poolId);
+
+    const auto allocSizes = alloc.getPool(poolId).getAllocSizes();
+    const size_t itemsPerSlab = Slab::kSize / allocSizes[0];
+    const size_t numItems = itemsPerSlab * params.slabsToFill;
+    const uint32_t itemSize = 450 * 1024;
+
+    std::vector<typename AllocatorT::WriteHandle> handles;
+    for (size_t i = 0; i < numItems; ++i) {
+      auto handle = util::allocateAccessible(
+          alloc, poolId, folly::sformat("key_{}", i), itemSize);
+      ASSERT_NE(nullptr, handle);
+      handles.push_back(std::move(handle));
+    }
+    handles.clear();
+
+    ASSERT_EQ(alloc.getPoolStats(poolId).mpStats.allocatedSlabs(),
+              params.slabsToFill);
+    ASSERT_EQ(alloc.getPool(poolId).getNumSlabsAdvised(), 0);
+
+    alloc.memMonitor_->adviseAwaySlabs();
+    alloc.memMonitor_->checkPoolsAndAdviseReclaim();
+    ASSERT_EQ(alloc.getPool(poolId).getNumSlabsAdvised(),
+              params.expectedAdvisedAfterFirstIteration);
+
+    alloc.memMonitor_->adviseAwaySlabs();
+    alloc.memMonitor_->checkPoolsAndAdviseReclaim();
+    ASSERT_EQ(alloc.getPool(poolId).getNumSlabsAdvised(),
+              params.expectedAdvisedAfterSecondIteration);
+  }
+
+  void testMemMonitorAdvisesAwayOverLimit() {
+    // Test advising with maxAdvisePercent=51%
+    // Cache: 23 total slabs (1 for metadata, 22 usable)
+    // First iteration: advises 10 slabs (10/22 = 45%)
+    // Second iteration: advises 1 more slab (11/22 = 50% < 51%)
+    runMemMonitorTest({
+        .totalSlabs = 23,
+        .slabsToFill = 22,
+        .maxAdvisePercent = 51,
+        .expectedAdvisedAfterFirstIteration = 10,
+        .expectedAdvisedAfterSecondIteration = 11,
+    });
+  }
+
+  void testMemMonitorAdvisesAwayOverLimit2() {
+    // Test advising exactly at maxAdvisePercent boundary (50%)
+    // Cache: 21 total slabs (1 for metadata, 20 usable)
+    // First iteration: advises 10 slabs (10/20 = 50%)
+    // Second iteration: cannot advise more (would exceed 50% limit)
+    runMemMonitorTest({
+        .totalSlabs = 21,
+        .slabsToFill = 20,
+        .maxAdvisePercent = 50,
+        .expectedAdvisedAfterFirstIteration = 10,
+        .expectedAdvisedAfterSecondIteration = 10,
+    });
+  }
+
+  void testMemMonitorAdvisesAwayOverLimit3() {
+    // Test advising with maxAdvisePercent=51%
+    // Cache: 21 total slabs (1 for metadata, 20 usable)
+    // First iteration: advises 10 slabs (10/20 = 50%)
+    // Second iteration: cannot advise more because one more slab would exceed
+    // 51%
+    runMemMonitorTest({
+        .totalSlabs = 21,
+        .slabsToFill = 20,
+        .maxAdvisePercent = 51,
+        .expectedAdvisedAfterFirstIteration = 10,
+        .expectedAdvisedAfterSecondIteration = 10,
+    });
+  }
 };
 } // namespace tests
 } // namespace cachelib

cachelib/allocator/tests/AllocatorResizeTypeTest.cpp

@@ -64,6 +64,18 @@ TYPED_TEST(AllocatorResizeTest, MemoryMonitorPerIterationAdviseReclaim) {
   this->testMemoryMonitorPerIterationAdviseReclaim();
 }
 
+TYPED_TEST(AllocatorResizeTest, MemMonitorAdvisesAwayOverLimit) {
+  this->testMemMonitorAdvisesAwayOverLimit();
+}
+
+TYPED_TEST(AllocatorResizeTest, MemMonitorAdvisesAwayOverLimit2) {
+  this->testMemMonitorAdvisesAwayOverLimit2();
+}
+
+TYPED_TEST(AllocatorResizeTest, MemMonitorAdvisesAwayOverLimit3) {
+  this->testMemMonitorAdvisesAwayOverLimit3();
+}
+
 TYPED_TEST(AllocatorResizeTest, ShrinkGrowthAdviseRaceCondition) {
   this->testShrinkGrowthAdviseRaceCondition();
 }