Add metric to cacth the double free error in mmap allocator

Currently we only have a warning log for double free in mmap allocator and we shall also add a metric for production alert.
facebookincubator · Apr 9, 2024 · 4f2c6a9 · 4f2c6a9
1 parent e29cde7
commit 4f2c6a9
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 2 deletions.
diff --git a/velox/common/base/Counters.cpp b/velox/common/base/Counters.cpp
@@ -143,6 +143,12 @@ void registerVeloxMetrics() {
   DEFINE_METRIC(
       kMetricMemoryPoolReservationLeakBytes, facebook::velox::StatType::SUM);
 
+  // Tracks the count of double frees in memory allocator, indicating the
+  // possibility of buffer ownership issues when a buffer is freed more than
+  // once.
+  DEFINE_METRIC(
+      kMetricMemoryAllocatorDoubleFreeCount, facebook::velox::StatType::COUNT);
+
   /// ================== Spill related Counters =================
 
   // The number of bytes in memory to spill.

diff --git a/velox/common/base/Counters.h b/velox/common/base/Counters.h
@@ -61,6 +61,9 @@ constexpr folly::StringPiece kMetricMemoryPoolUsageLeakBytes{
 constexpr folly::StringPiece kMetricMemoryPoolReservationLeakBytes{
     "velox.memory_pool_reservation_leak_bytes"};
 
+constexpr folly::StringPiece kMetricMemoryAllocatorDoubleFreeCount{
+    "velox.memory_allocator_double_free_count"};
+
 constexpr folly::StringPiece kMetricArbitratorRequestsCount{
     "velox.arbitrator_requests_count"};
 

diff --git a/velox/common/memory/MmapAllocator.cpp b/velox/common/memory/MmapAllocator.cpp
@@ -18,7 +18,9 @@
 
 #include <sys/mman.h>
 
+#include "velox/common/base/Counters.h"
 #include "velox/common/base/Portability.h"
+#include "velox/common/base/StatsReporter.h"
 #include "velox/common/memory/Memory.h"
 
 namespace facebook::velox::memory {
@@ -877,10 +879,10 @@ MachinePageCount MmapAllocator::SizeClass::free(Allocation& allocation) {
     const int firstBit =
         (runAddress - address_) / (AllocationTraits::kPageSize * unitSize_);
     for (auto page = firstBit; page < firstBit + numPages; ++page) {
-      if (!bits::isBitSet(pageAllocated_.data(), page)) {
-        // TODO: change this to a velox failure to catch the bug.
+      if (FOLLY_UNLIKELY(!bits::isBitSet(pageAllocated_.data(), page))) {
         VELOX_MEM_LOG(ERROR)
             << "Double free: page = " << page << " sizeclass = " << unitSize_;
+        RECORD_METRIC_VALUE(kMetricMemoryAllocatorDoubleFreeCount);
         continue;
       }
       if (bits::isBitSet(pageMapped_.data(), page)) {

diff --git a/velox/docs/monitoring/metrics.rst b/velox/docs/monitoring/metrics.rst
@@ -165,6 +165,11 @@ Memory Management
    * - memory_pool_capacity_leak_bytes
      - Sum
      - The root memory pool reservation leak in bytes.
+   * - memory_allocator_double_free_count
+     - Count
+     - Tracks the count of double frees in memory allocator, indicating the
+       possibility of buffer ownership issues when a buffer is freed more
+       than once.
 
 Spilling
 --------