diff --git a/tcmalloc/huge_page_filler.h b/tcmalloc/huge_page_filler.h index ce44a1bd3..0cdfe6cc5 100644 --- a/tcmalloc/huge_page_filler.h +++ b/tcmalloc/huge_page_filler.h @@ -2721,14 +2721,10 @@ class HugePageUnbackedTrackerTreatment final : public HugePageTreatment { bitmaps.unbacked, pages_per_huge_page, ReductionOp::kAll); state.swapped = Scale( bitmaps.swapped, pages_per_huge_page, ReductionOp::kAny); - if (pf) { - // TODO(b/525422238): Return by value rather than use an output - // parameter. - ResidencyBitmap b; - pf->GetSinglePageBitmaps(tracker->location().start_addr(), b); - state.stale = Scale( - b, pages_per_huge_page, ReductionOp::kAny); - } + auto single_page_bitmaps = + pf->GetSinglePageBitmaps(tracker->location().start_addr()); + state.stale = Scale( + single_page_bitmaps.stale, pages_per_huge_page, ReductionOp::kAny); const bool backoff = treatment_stats_.collapse_time_max_cycles > max_collapse_cycles; diff --git a/tcmalloc/huge_page_filler_fuzz.cc b/tcmalloc/huge_page_filler_fuzz.cc index 290fd608a..e10fad487 100644 --- a/tcmalloc/huge_page_filler_fuzz.cc +++ b/tcmalloc/huge_page_filler_fuzz.cc @@ -124,9 +124,8 @@ class FakePageFlags : public PageFlagsBase { return PageStats{}; } - absl::StatusCode GetSinglePageBitmaps(const void* addr, - ResidencyBitmap& stale) override { - return absl::StatusCode::kUnimplemented; + PageFlagsBitmaps GetSinglePageBitmaps(const void* addr) override { + return {.status = absl::StatusCode::kUnimplemented}; } std::optional IsHugepageBacked(const void* addr) override { diff --git a/tcmalloc/huge_page_filler_test.cc b/tcmalloc/huge_page_filler_test.cc index d778a3b64..a776056d5 100644 --- a/tcmalloc/huge_page_filler_test.cc +++ b/tcmalloc/huge_page_filler_test.cc @@ -289,10 +289,11 @@ class FakePageFlags : public PageFlagsBase { return PageStats{}; } - absl::StatusCode GetSinglePageBitmaps(const void* addr, - ResidencyBitmap& stale) override { - stale.SetBit(0); - return absl::StatusCode::kOk; + PageFlagsBitmaps GetSinglePageBitmaps(const void* addr) override { + PageFlagsBitmaps ret; + ret.stale.SetBit(0); + ret.status = absl::StatusCode::kOk; + return ret; } void MarkHugePageBacked(void* addr, bool is_hugepage_backed) { diff --git a/tcmalloc/internal/BUILD b/tcmalloc/internal/BUILD index 409ce35b2..991f77a5b 100644 --- a/tcmalloc/internal/BUILD +++ b/tcmalloc/internal/BUILD @@ -1014,6 +1014,7 @@ cc_test( "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/log:check", + "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", diff --git a/tcmalloc/internal/pageflags.cc b/tcmalloc/internal/pageflags.cc index eb8ff0885..4e074b7e1 100644 --- a/tcmalloc/internal/pageflags.cc +++ b/tcmalloc/internal/pageflags.cc @@ -339,21 +339,25 @@ std::optional PageFlags::Get(const void* const addr, } return ret; } -absl::StatusCode PageFlags::GetSinglePageBitmaps(const void* addr, - ResidencyBitmap& stale) { +PageFlagsBase::PageFlagsBitmaps PageFlags::GetSinglePageBitmaps( + const void* addr) { + PageFlagsBitmaps ret; uintptr_t currPage = reinterpret_cast(addr); if ((currPage & (kHugePageSize - 1)) != 0) { TC_LOG("Address is not hugepage aligned"); - return absl::StatusCode::kFailedPrecondition; + ret.status = absl::StatusCode::kFailedPrecondition; + return ret; } if (fd_ < 0) { - return absl::StatusCode::kUnavailable; + ret.status = absl::StatusCode::kUnavailable; + return ret; } auto res = Seek(currPage); if (res != absl::StatusCode::kOk) { - return res; + ret.status = res; + return ret; } const size_t kHardwarePagesInHugePage = kHugePageSize / GetPageSize(); @@ -365,7 +369,8 @@ absl::StatusCode PageFlags::GetSinglePageBitmaps(const void* addr, kSizeOfHugepageInPagemap, nullptr); if (status != kSizeOfHugepageInPagemap) { TC_LOG("Could not read from pageflags file"); - return absl::StatusCode::kUnavailable; + ret.status = absl::StatusCode::kUnavailable; + return ret; } last_head_read_ = -1; @@ -379,7 +384,8 @@ absl::StatusCode PageFlags::GetSinglePageBitmaps(const void* addr, } if (PageTail(flags)) { if (ABSL_PREDICT_FALSE(last_head_read_ == -1)) { - return absl::StatusCode::kFailedPrecondition; + ret.status = absl::StatusCode::kFailedPrecondition; + return ret; } flags = last_head_read_; } @@ -390,16 +396,17 @@ absl::StatusCode PageFlags::GetSinglePageBitmaps(const void* addr, } } else { if (stale_start != -1) { - stale.SetRange(stale_start, i - stale_start); + ret.stale.SetRange(stale_start, i - stale_start); stale_start = -1; } } } if (stale_start != -1) { - stale.SetRange(stale_start, kHardwarePagesInHugePage - stale_start); + ret.stale.SetRange(stale_start, kHardwarePagesInHugePage - stale_start); } - return absl::StatusCode::kOk; + ret.status = absl::StatusCode::kOk; + return ret; } uint64_t PageFlags::MaybeReadStaleScanSeconds(const char* filename) { diff --git a/tcmalloc/internal/pageflags.h b/tcmalloc/internal/pageflags.h index 6b18c41e7..d59fd99fe 100644 --- a/tcmalloc/internal/pageflags.h +++ b/tcmalloc/internal/pageflags.h @@ -68,8 +68,13 @@ class PageFlagsBase { PageFlagsBase& operator=(PageFlagsBase&&) = delete; virtual std::optional IsHugepageBacked(const void* addr) = 0; virtual std::optional Get(const void* addr, size_t size) = 0; - virtual absl::StatusCode GetSinglePageBitmaps(const void* addr, - ResidencyBitmap& stale) = 0; + + struct PageFlagsBitmaps { + ResidencyBitmap stale; + absl::StatusCode status; + }; + + virtual PageFlagsBitmaps GetSinglePageBitmaps(const void* addr) = 0; }; // PageFlags offers a look at kernel page flags to identify pieces of memory as @@ -101,8 +106,7 @@ class PageFlags final : public PageFlagsBase { // dynamically allocate memory when needed. Using std::optional allows us to // use the function in places where memory allocation is prohibited. std::optional Get(const void* addr, size_t size) override; - absl::StatusCode GetSinglePageBitmaps(const void* addr, - ResidencyBitmap& stale) override; + PageFlagsBitmaps GetSinglePageBitmaps(const void* addr) override; std::optional IsHugepageBacked(const void* addr) override; private: diff --git a/tcmalloc/internal/pageflags_test.cc b/tcmalloc/internal/pageflags_test.cc index 6869afb6d..15aeae84b 100644 --- a/tcmalloc/internal/pageflags_test.cc +++ b/tcmalloc/internal/pageflags_test.cc @@ -81,9 +81,8 @@ class PageFlagsFriend { return r_.IsHugepageBacked(addr); } - decltype(auto) GetSinglePageBitmaps(const void* addr, - ResidencyBitmap& stale) { - return r_.GetSinglePageBitmaps(addr, stale); + decltype(auto) GetSinglePageBitmaps(const void* addr) { + return r_.GetSinglePageBitmaps(addr); } void SetCachedScanSeconds( @@ -608,14 +607,12 @@ TEST(StaleSeconds, TextOverflow) { TEST(PageFlagsTest, GetSinglePageBitmapsErrorCases) { { PageFlagsFriend s; - ResidencyBitmap stale; - EXPECT_EQ(s.GetSinglePageBitmaps(reinterpret_cast(1), stale), + EXPECT_EQ(s.GetSinglePageBitmaps(reinterpret_cast(1)).status, absl::StatusCode::kFailedPrecondition); } { PageFlagsFriend s("/dev/null/impossible"); - ResidencyBitmap stale; - EXPECT_EQ(s.GetSinglePageBitmaps(nullptr, stale), + EXPECT_EQ(s.GetSinglePageBitmaps(nullptr).status, absl::StatusCode::kUnavailable); } { @@ -623,8 +620,7 @@ TEST(PageFlagsTest, GetSinglePageBitmapsErrorCases) { absl::StrCat(testing::TempDir(), "/fake_pageflags_short"); SetContents(fake_pageflags, "x"); PageFlagsFriend s(fake_pageflags); - ResidencyBitmap stale; - EXPECT_EQ(s.GetSinglePageBitmaps(nullptr, stale), + EXPECT_EQ(s.GetSinglePageBitmaps(nullptr).status, absl::StatusCode::kUnavailable); } { @@ -639,8 +635,7 @@ TEST(PageFlagsTest, GetSinglePageBitmapsErrorCases) { SetContents(fake_pageflags, content); PageFlagsFriend s(fake_pageflags); - ResidencyBitmap stale; - EXPECT_EQ(s.GetSinglePageBitmaps(nullptr, stale), + EXPECT_EQ(s.GetSinglePageBitmaps(nullptr).status, absl::StatusCode::kFailedPrecondition); } } @@ -661,11 +656,10 @@ TEST(PageFlagsTest, GetSinglePageBitmapsSuccess) { SetContents(fake_pageflags, content); PageFlagsFriend s(fake_pageflags); - ResidencyBitmap stale; - stale.Clear(); - EXPECT_EQ(s.GetSinglePageBitmaps(nullptr, stale), absl::StatusCode::kOk); - EXPECT_EQ(stale.CountBits(), kMaxResidencyBits / 2); + auto ret = s.GetSinglePageBitmaps(nullptr); + EXPECT_EQ(ret.status, absl::StatusCode::kOk); + EXPECT_EQ(ret.stale.CountBits(), kMaxResidencyBits / 2); } } // namespace diff --git a/tcmalloc/internal/profile_builder_test.cc b/tcmalloc/internal/profile_builder_test.cc index 46785da91..80790f400 100644 --- a/tcmalloc/internal/profile_builder_test.cc +++ b/tcmalloc/internal/profile_builder_test.cc @@ -40,6 +40,7 @@ #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/log/check.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" @@ -71,9 +72,8 @@ class StubPageFlags final : public PageFlagsBase { public: StubPageFlags() = default; ~StubPageFlags() override = default; - absl::StatusCode GetSinglePageBitmaps(const void* addr, - ResidencyBitmap& stale) override { - return absl::StatusCode::kUnimplemented; + PageFlagsBitmaps GetSinglePageBitmaps(const void* addr) override { + return {.status = absl::StatusCode::kUnimplemented}; } std::optional Get(const void* addr, size_t size) override { PageStats ret;