Skip to content

Commit

Permalink
Add metric to cacth the double free error in mmap allocator (#8648)
Browse files Browse the repository at this point in the history
Summary:
Currently we only have a warning log for double free in mmap allocator
and we shall also add a metric for production alert.

Pull Request resolved: #8648

Reviewed By: bikramSingh91

Differential Revision: D53341067

Pulled By: xiaoxmeng

fbshipit-source-id: 3d0979a1621ebae1180b3d59cd98345fd0c4dff8
  • Loading branch information
xiaoxmeng authored and facebook-github-bot committed Apr 10, 2024
1 parent 89de5d3 commit c4cd265
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 3 deletions.
7 changes: 6 additions & 1 deletion velox/common/base/Counters.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ void registerVeloxMetrics() {
// The distribution of a root memory pool's initial capacity in range of [0,
// 256MB] with 32 buckets. It is configured to report the capacity at P50,
// P90, P99, and P100 percentiles.

DEFINE_HISTOGRAM_METRIC(
kMetricMemoryPoolInitialCapacityBytes,
8L << 20,
Expand All @@ -163,6 +162,12 @@ void registerVeloxMetrics() {
DEFINE_HISTOGRAM_METRIC(
kMetricMemoryPoolCapacityGrowCount, 8, 0, 256, 50, 90, 99, 100);

// 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.
Expand Down
3 changes: 3 additions & 0 deletions velox/common/base/Counters.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,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"};

Expand Down
6 changes: 4 additions & 2 deletions velox/common/memory/MmapAllocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)) {
Expand Down
5 changes: 5 additions & 0 deletions velox/docs/monitoring/metrics.rst
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,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
--------
Expand Down

0 comments on commit c4cd265

Please sign in to comment.