diff --git a/ddprof-lib/src/main/cpp/linearAllocator.cpp b/ddprof-lib/src/main/cpp/linearAllocator.cpp index cc12da403..22ea16706 100644 --- a/ddprof-lib/src/main/cpp/linearAllocator.cpp +++ b/ddprof-lib/src/main/cpp/linearAllocator.cpp @@ -46,6 +46,14 @@ LinearAllocator::~LinearAllocator() { } void LinearAllocator::clear() { + // Both pointers are NULL only after detachChunks() could not allocate a + // replacement chunk, which leaves the allocator deliberately unusable rather + // than risking a double free. Bail out here so the whole function is + // null-safe: it dereferences _reserve immediately below and _tail at the end, + // so guarding either one alone would leave the other exposed. + if (_tail == NULL || _reserve == NULL) { + return; + } // OS::safeAlloc/safeFree use raw syscalls not intercepted by TSan, so TSan // never clears shadow memory on munmap. Add explicit acquire/release around // every plain prev-field read so the happens-before chain from freeChunk's @@ -102,6 +110,10 @@ void LinearAllocator::clear() { _reserve = current; _tail = current; } + // _tail is kept (not freed) but its contents are discarded here, so its + // consumed bytes must be un-recorded explicitly -- freeChunk() won't run + // for this one. + NativeMem::record(NM_CALLTRACE, -(long long)(_tail->offs - sizeof(Chunk))); _tail->offs = sizeof(Chunk); // DON'T UNPOISON HERE - let alloc() do it on-demand! @@ -162,15 +174,17 @@ void LinearAllocator::freeChunks(ChunkList& chunks) { __tsan_acquire(current); #endif Chunk* prev = current->prev; + // Capture before safeFree unmaps the chunk -- reading current->offs after + // that would touch freed memory. This is exactly the byte count alloc() + // recorded into NM_CALLTRACE for this chunk, so the decrement is exact. + long long used = (long long)(current->offs - sizeof(Chunk)); #ifdef TSAN_ENABLED __tsan_release(current); #endif OS::safeFree(current, chunks.chunk_size); Counters::decrement(LINEAR_ALLOCATOR_BYTES, chunks.chunk_size); Counters::decrement(LINEAR_ALLOCATOR_CHUNKS); - // The LinearAllocator's only user is call-trace storage, so all of its - // chunk memory is attributed to the CALLTRACE category. - NativeMem::record(NM_CALLTRACE, -(long long)chunks.chunk_size); + NativeMem::record(NM_CALLTRACE, -used); current = prev; } @@ -195,6 +209,11 @@ void *LinearAllocator::alloc(size_t size) { if (__sync_bool_compare_and_swap(&chunk->offs, offs, offs + size)) { void* allocated_ptr = (char *)chunk + offs; + // The LinearAllocator's only user is call-trace storage, so all of + // its bump-allocated bytes are attributed to the CALLTRACE category. + // A relaxed atomic add, safe to call from the sampling signal handler. + NativeMem::record(NM_CALLTRACE, (long long)size); + // ASAN UNPOISONING: Unpoison ONLY the allocated region on-demand // This allows ASan to detect use-after-free of memory that was cleared // but not yet reallocated @@ -264,12 +283,30 @@ Chunk *LinearAllocator::allocateChunk(Chunk *current) { Counters::increment(LINEAR_ALLOCATOR_BYTES, _chunk_size); Counters::increment(LINEAR_ALLOCATOR_CHUNKS); - NativeMem::record(NM_CALLTRACE, (long long)_chunk_size); + // NM_CALLTRACE is NOT recorded here. Recording the full chunk size at + // reservation time would count virtual capacity, not residency: the + // chunk is mmap'd whole but filled incrementally by alloc()'s bump + // pointer, and reserveChunk() eagerly reserves the next chunk at 50% + // fill of the current one, so there is always at least one chunk that's + // fully counted but mostly or entirely untouched. alloc() instead + // records exactly the bytes actually handed out, which -- because the + // bump pointer advances linearly and every returned pointer is written + // into immediately by the caller -- tracks touched (resident) bytes + // directly, with no separate residency measurement needed. } return chunk; } void LinearAllocator::freeChunk(Chunk *current) { + // allocateChunk() returns NULL when the mmap fails, and detachChunks() stores + // that NULL into _tail, so the destructor's freeChunk(_tail) can be reached + // with nothing to free. Bail out rather than dereference it below. + if (current == NULL) { + return; + } + // Capture before safeFree unmaps the chunk -- see freeChunks() for why this + // exactly reverses what alloc() recorded for this chunk. + long long used = (long long)(current->offs - sizeof(Chunk)); // Release TSan ownership before munmap so the sanitizer knows this thread is // done with the memory. The mmap(MAP_FIXED) re-map in allocateChunk() resets // the shadow for whichever thread later reuses this VA (after OS VA reuse), so @@ -280,7 +317,7 @@ void LinearAllocator::freeChunk(Chunk *current) { OS::safeFree(current, _chunk_size); Counters::decrement(LINEAR_ALLOCATOR_BYTES, _chunk_size); Counters::decrement(LINEAR_ALLOCATOR_CHUNKS); - NativeMem::record(NM_CALLTRACE, -(long long)_chunk_size); + NativeMem::record(NM_CALLTRACE, -used); } void LinearAllocator::reserveChunk(Chunk *current) { diff --git a/ddprof-lib/src/main/cpp/nativeMem.h b/ddprof-lib/src/main/cpp/nativeMem.h index 2706f5629..fc89449f4 100644 --- a/ddprof-lib/src/main/cpp/nativeMem.h +++ b/ddprof-lib/src/main/cpp/nativeMem.h @@ -13,10 +13,17 @@ // per-category live byte gauges partition the total: sum(category) == total, // with no double counting. // -// The "reserved vs used vs wasted" breakdowns exposed by the existing counters -// (CALLTRACE_STORAGE_BYTES is the used slice of the CALLTRACE arena; -// DICTIONARY_ARENA_WASTE_BYTES is the wasted slice of the DICTIONARY arena) are -// a separate, nested dimension. They are intentionally NOT summed in here. +// These gauges track memory that is actually touched (and therefore resident), +// not address space that has merely been reserved. NM_CALLTRACE in particular +// counts the bytes LinearAllocator::alloc() has bump-allocated, NOT the +// capacity of the 8 MiB chunks backing them -- see linearAllocator.cpp. +// +// Consequently NM_CALLTRACE is no longer an "arena reserved" figure with +// CALLTRACE_STORAGE_BYTES as its used slice: both now count touched bytes, and +// their small difference is the call-trace hash tables (also bump-allocated), +// not chunk slack. Do not compute arena waste as the difference between them. +// DICTIONARY_ARENA_WASTE_BYTES remains a genuine nested waste figure for the +// DICTIONARY arena. None of these nested counters are summed in here. #define DD_NATIVE_MEM_CATEGORY_TABLE(X) \ X(CALLTRACE, "calltrace") \ X(DICTIONARY, "dictionary") \ diff --git a/ddprof-lib/src/test/cpp/linearAllocator_nativemem_ut.cpp b/ddprof-lib/src/test/cpp/linearAllocator_nativemem_ut.cpp new file mode 100644 index 000000000..8a0299127 --- /dev/null +++ b/ddprof-lib/src/test/cpp/linearAllocator_nativemem_ut.cpp @@ -0,0 +1,101 @@ +#include "gtest/gtest.h" +#include "linearAllocator.h" +#include "nativeMem.h" + +// NM_CALLTRACE must track bytes actually handed out by alloc() -- i.e. touched, +// resident bytes -- not the virtual capacity of the mmap'd chunks backing them. +// The distinction is large: chunks are 8 MiB (CALL_TRACE_CHUNK) and alloc() +// eagerly reserves the next one at 50% fill, so a capacity-based counter +// over-reports by up to two chunks' worth of untouched address space. + +static const size_t CHUNK_SIZE = 1024 * 1024; + +class LinearAllocatorNativeMemTest : public ::testing::Test { +protected: + long long _baseline[NM_NUM_CATEGORIES]; + + // Static-duration objects elsewhere in the binary record into these same + // categories before main() runs; restore their baseline in TearDown so + // their destructors don't underflow a category this fixture zeroed. + // (Same rationale as NativeMemTest in nativeMem_ut.cpp.) + void SetUp() override { + for (int c = 0; c < NM_NUM_CATEGORIES; c++) { + _baseline[c] = NativeMem::live((NativeMemCategory)c); + } + NativeMem::reset(); + } + void TearDown() override { + NativeMem::reset(); + for (int c = 0; c < NM_NUM_CATEGORIES; c++) { + NativeMem::setLive((NativeMemCategory)c, _baseline[c]); + } + } +}; + +TEST_F(LinearAllocatorNativeMemTest, ReportsBumpedBytesNotChunkCapacity) { + const size_t ALLOC_SIZE = 128; + const int NUM_ALLOCS = 100; + + LinearAllocator allocator(CHUNK_SIZE); + // Constructing the allocator mmaps a chunk but touches nothing in it. + EXPECT_EQ(0, NativeMem::live(NM_CALLTRACE)) + << "a freshly reserved chunk is untouched and must not be counted"; + + for (int i = 0; i < NUM_ALLOCS; i++) { + ASSERT_NE(nullptr, allocator.alloc(ALLOC_SIZE)); + } + + EXPECT_EQ((long long)(ALLOC_SIZE * NUM_ALLOCS), NativeMem::live(NM_CALLTRACE)) + << "live bytes must equal exactly the bytes alloc() handed out"; + EXPECT_LT(NativeMem::live(NM_CALLTRACE), (long long)CHUNK_SIZE) + << "must not have jumped to whole-chunk granularity"; +} + +TEST_F(LinearAllocatorNativeMemTest, CrossingIntoASecondChunkStaysByteAccurate) { + // An allocation size that does not divide the chunk evenly, driven past the + // point where reserveChunk() pre-reserves the next chunk, so the reserved + // chunk's untouched capacity would show up in a capacity-based counter. + const size_t ALLOC_SIZE = 3000; + const int NUM_ALLOCS = (int)(CHUNK_SIZE / ALLOC_SIZE) + 50; + + LinearAllocator allocator(CHUNK_SIZE); + long long handed_out = 0; + for (int i = 0; i < NUM_ALLOCS; i++) { + if (allocator.alloc(ALLOC_SIZE) != nullptr) { + handed_out += (long long)ALLOC_SIZE; + } + } + + EXPECT_EQ(handed_out, NativeMem::live(NM_CALLTRACE)) + << "byte-accurate across a chunk boundary, with a chunk pre-reserved"; +} + +TEST_F(LinearAllocatorNativeMemTest, ClearReturnsLiveToZero) { + LinearAllocator allocator(CHUNK_SIZE); + for (int i = 0; i < 50; i++) { + ASSERT_NE(nullptr, allocator.alloc(256)); + } + ASSERT_GT(NativeMem::live(NM_CALLTRACE), 0); + + allocator.clear(); + + EXPECT_EQ(0, NativeMem::live(NM_CALLTRACE)) + << "clear() discards all chunk contents, including the retained _tail's"; +} + +TEST_F(LinearAllocatorNativeMemTest, FreeChunksReturnsLiveToZero) { + LinearAllocator allocator(CHUNK_SIZE); + for (int i = 0; i < 50; i++) { + ASSERT_NE(nullptr, allocator.alloc(256)); + } + long long before_detach = NativeMem::live(NM_CALLTRACE); + ASSERT_GT(before_detach, 0); + + ChunkList detached = allocator.detachChunks(); + EXPECT_EQ(before_detach, NativeMem::live(NM_CALLTRACE)) + << "detaching moves ownership but frees nothing yet"; + + LinearAllocator::freeChunks(detached); + EXPECT_EQ(0, NativeMem::live(NM_CALLTRACE)) + << "freeing the detached chunks un-records exactly what alloc() recorded"; +}