diff --git a/tcmalloc/huge_page_filler.h b/tcmalloc/huge_page_filler.h index b79ab51f2..a0baedb03 100644 --- a/tcmalloc/huge_page_filler.h +++ b/tcmalloc/huge_page_filler.h @@ -1919,7 +1919,6 @@ inline Length HugePageFiller::HandleReleaseFree( PageTracker* tracker) { RemoveFromFillerList(tracker); Length released_length = tracker->ReleaseFree(unback_); - subrelease_stats_.total_pages_subreleased += released_length; unmapped_ += released_length; unmapping_unaccounted_ += released_length; AddToFillerList(tracker); @@ -1939,7 +1938,6 @@ inline Length HugePageFiller::HandleUnbackedHugePage( PageTracker* tracker, const PageBitmap& unbacked) { RemoveFromFillerList(tracker); Length unmapped_length = tracker->MarkSubreleased(unbacked); - subrelease_stats_.total_pages_subreleased += unmapped_length; unmapped_ += unmapped_length; unmapping_unaccounted_ += unmapped_length; AddToFillerList(tracker); diff --git a/tcmalloc/huge_page_filler_test.cc b/tcmalloc/huge_page_filler_test.cc index 33f9f6cb9..4d2421f88 100644 --- a/tcmalloc/huge_page_filler_test.cc +++ b/tcmalloc/huge_page_filler_test.cc @@ -842,7 +842,7 @@ TEST_F(FillerTest, ReleaseFreePagesWhenAnyPageIsSwappedRespectsClock) { TreatHugepageTrackers(EnableCollapse::kDisabled, EnableUnfilteredCollapse::kDisabled, ReleaseStalePages::kDisabled, &pageflags, &residency); - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(1)); + EXPECT_EQ(ReleasePages(Length(0)), Length(1)); EXPECT_EQ(GetHugePageTreatmentStats().treated_pages_subreleased, 1); DeleteVector(p1); } @@ -989,7 +989,7 @@ TEST_F(FillerTest, ReleaseFreePagesWhenAnyPageIsSwapped) { ReleaseStalePages::kDisabled, &pageflags, &residency); // We expect to release the two free pages, since the second native page is // swapped. We expect to log this correctly. - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(2)); + EXPECT_EQ(ReleasePages(Length(0)), Length(2)); EXPECT_EQ(GetHugePageTreatmentStats().treated_pages_subreleased, 2); std::string buffer = PrintToString(1024 * 1024, [&](Printer& printer) { PageHeapSpinLockHolder l; @@ -1004,12 +1004,49 @@ TEST_F(FillerTest, ReleaseFreePagesWhenAnyPageIsSwapped) { TreatHugepageTrackers(EnableCollapse::kDisabled, EnableUnfilteredCollapse::kDisabled, ReleaseStalePages::kDisabled, &pageflags, &residency); - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(2)); + EXPECT_EQ(ReleasePages(Length(0)), Length(0)); EXPECT_EQ(GetHugePageTreatmentStats().treated_pages_subreleased, 0); DeleteVector(p1); } +// Checks that pages released due to swapped page treatment are not double +// counted in subrelease_stats. +TEST_F(FillerTest, SubreleaseStatsNoDoubleCountSwapped) { + const Length kAlloc = kPagesPerHugePage; + std::vector p1 = AllocateVector(kAlloc - Length(2)); + ASSERT_TRUE(!p1.empty()); + + FakePageFlags pageflags; + FakeResidency residency; + for (const auto& pa : p1) { + pageflags.MarkHugePageBacked(pa.p.start_addr(), + /*is_hugepage_backed=*/false); + + Bitmap unbacked, swapped; + swapped.SetRange(/*index=*/1, /*n=*/1); + residency.SetUnbackedAndSwappedBitmaps(pa.p.start_addr(), unbacked, + swapped); + pageflags.SetStaleBitmap(pa.p.start_addr(), {}); + } + + ASSERT_EQ(filler_.size(), NHugePages(1)); + TreatHugepageTrackers(EnableCollapse::kDisabled, + EnableUnfilteredCollapse::kDisabled, + ReleaseStalePages::kDisabled, &pageflags, &residency); + + EXPECT_EQ(ReleasePages(Length(0)), Length(2)); + EXPECT_EQ(filler_.subrelease_stats().num_pages_subreleased, Length(2)); + EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(0)); + + PAlloc extra = Allocate(Length(1)); + EXPECT_EQ(filler_.subrelease_stats().num_pages_subreleased, Length(0)); + EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(2)); + + Delete(extra); + DeleteVector(p1); +} + // Checks that we don't release pages when there aren't any pages that are // swapped. TEST_F(FillerTest, ReleaseNoFreePages) { @@ -1089,7 +1126,7 @@ TEST_F(FillerTest, CheckAllocationsComeFromIntactHugepage) { ReleaseStalePages::kDisabled, &pageflags, &residency); // There should be two pages released, from p1's hugepage. - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(2)); + EXPECT_EQ(ReleasePages(Length(0)), Length(2)); EXPECT_EQ(GetHugePageTreatmentStats().treated_pages_subreleased, 2); // We make an allocation. We expect it to come from the same hugepage as // the elements of p3, since this hugepage has not been subreleased from, @@ -1916,7 +1953,7 @@ TEST_F(FillerTestWithSubreleaseUnbacked, SubreleaseUnbackedPages) { TreatHugepageTrackers(EnableCollapse::kDisabled, EnableUnfilteredCollapse::kDisabled, ReleaseStalePages::kDisabled, &pageflags, &residency); - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(1)); + EXPECT_EQ(ReleasePages(Length(0)), Length(1)); EXPECT_EQ(GetHugePageTreatmentStats().treated_pages_unbacked_subreleased, 1); std::string buffer = PrintToString(1024 * 1024, [&](Printer& printer) { PageHeapSpinLockHolder l; @@ -1928,6 +1965,42 @@ TEST_F(FillerTestWithSubreleaseUnbacked, SubreleaseUnbackedPages) { DeleteVector(p1); } +// Checks that pages released due to unbacked page treatment are not double +// counted in subrelease_stats. +TEST_F(FillerTestWithSubreleaseUnbacked, SubreleaseStatsNoDoubleCountUnbacked) { + randomize_density_ = false; + const Length kAlloc = kPagesPerHugePage - Length(1); + std::vector p1 = AllocateVector(kAlloc); + ASSERT_TRUE(!p1.empty()); + + FakePageFlags pageflags; + FakeResidency residency; + for (const auto& pa : p1) { + pageflags.MarkHugePageBacked(pa.p.start_addr(), + /*is_hugepage_backed=*/false); + Bitmap unbacked, swapped; + unbacked.SetRange(0, kMaxResidencyBits); + residency.SetUnbackedAndSwappedBitmaps(pa.p.start_addr(), unbacked, + swapped); + pageflags.SetStaleBitmap(pa.p.start_addr(), {}); + } + + TreatHugepageTrackers(EnableCollapse::kDisabled, + EnableUnfilteredCollapse::kDisabled, + ReleaseStalePages::kDisabled, &pageflags, &residency); + + EXPECT_EQ(ReleasePages(Length(0)), Length(1)); + EXPECT_EQ(filler_.subrelease_stats().num_pages_subreleased, Length(1)); + EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(0)); + + PAlloc extra = Allocate(Length(1)); + EXPECT_EQ(filler_.subrelease_stats().num_pages_subreleased, Length(0)); + EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(1)); + + Delete(extra); + DeleteVector(p1); +} + // This test confirms that when enable_subrelease_unbacked is set to false, // no unbacked pages are subreleased from non-hugepage-backed trackers. TEST_F(FillerTest, SubreleaseUnbackedPagesDisabled) { @@ -2099,7 +2172,7 @@ TEST_F(FillerTestWithSubreleaseUnbacked, SubreleaseUnbackedAndSwapped) { TreatHugepageTrackers(EnableCollapse::kDisabled, EnableUnfilteredCollapse::kDisabled, ReleaseStalePages::kDisabled, &pageflags, &residency); - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(2)); + EXPECT_EQ(ReleasePages(Length(0)), Length(2)); DeleteVector(p1); } @@ -2128,7 +2201,7 @@ TEST_F(FillerTestWithSubreleaseUnbacked, SubreleaseUnbackedRecovery) { EnableUnfilteredCollapse::kDisabled, ReleaseStalePages::kDisabled, &pageflags, &residency); - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(10)); + EXPECT_EQ(ReleasePages(Length(0)), Length(10)); std::vector p2 = AllocateVector(Length(10)); ASSERT_TRUE(!p2.empty()); @@ -2190,7 +2263,7 @@ TEST_F(FillerTestWithSubreleaseUnbacked, SubreleaseUnbackedDonated) { TreatHugepageTrackers(EnableCollapse::kDisabled, EnableUnfilteredCollapse::kDisabled, ReleaseStalePages::kDisabled, &pageflags, &residency); - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, Length(1)); + EXPECT_EQ(ReleasePages(Length(0)), Length(1)); EXPECT_EQ(GetHugePageTreatmentStats().treated_pages_unbacked_subreleased, 1); DeleteVector(p1); } @@ -2751,7 +2824,7 @@ TEST_F(FillerTestWithSubreleaseUnbacked, GardenReleasedTrackers) { // well. We had 241 released pages. Now we should have 241 + 5 = 246 released // pages. EXPECT_EQ(pa.pt->released_pages(), N - Length(10)); - EXPECT_EQ(filler_.subrelease_stats().total_pages_subreleased, N - Length(5)); + EXPECT_EQ(ReleasePages(Length(0)), Length(5)); EXPECT_EQ(GetHugePageTreatmentStats().treated_pages_unbacked_subreleased, 5); // Clean up. diff --git a/tcmalloc/span.h b/tcmalloc/span.h index 7674a21d2..676b47800 100644 --- a/tcmalloc/span.h +++ b/tcmalloc/span.h @@ -254,10 +254,6 @@ class ABSL_CACHELINE_ALIGNED Span final : public SpanList::Elem { [[nodiscard]] ObjIdx BitmapPtrToIdx(void* ptr, size_t size, uint32_t reciprocal) const; [[nodiscard]] void* BitmapIdxToPtr(ObjIdx idx, size_t size) const; -#ifndef TCMALLOC_INTERNAL_LEGACY_LOCKING - [[nodiscard]] void* BitmapIdxToPtr(ObjIdx idx, size_t size, - uintptr_t start) const; -#endif #ifdef TCMALLOC_INTERNAL_LEGACY_LOCKING static constexpr size_t kNonemptyIndexBits = 5; @@ -610,15 +606,9 @@ inline Span::ObjIdx Span::OffsetToIdx(uintptr_t offset, uint32_t reciprocal) { } #ifndef TCMALLOC_INTERNAL_LEGACY_LOCKING -inline void* Span::BitmapIdxToPtr(ObjIdx idx, size_t size, - uintptr_t start) const { - TC_ASSERT_EQ(start, first_page().start_uintptr()); - uintptr_t off = start + idx * size; - return reinterpret_cast(off); -} - inline void* Span::BitmapIdxToPtr(ObjIdx idx, size_t size) const { - return BitmapIdxToPtr(idx, size, first_page().start_uintptr()); + uintptr_t off = first_page().start_uintptr() + idx * size; + return reinterpret_cast(off); } #endif @@ -780,10 +770,9 @@ inline size_t Span::BitmapPopBatch(absl::Span batch, return count; #else void** ptrs = batch.data(); - const uintptr_t span_start = first_page().start_uintptr(); size_t popped = bitmap_.PopBatch( [&](size_t offset) { - *ptrs++ = BitmapIdxToPtr(static_cast(offset), size, span_start); + *ptrs++ = BitmapIdxToPtr(static_cast(offset), size); }, batch.size()); allocated_.store(allocated_.load(std::memory_order_relaxed) + popped,