[kernel][vm] Remove ScanForZeroPages

The full scanning of all pages in all VMOs for zero pages is now just
used by an informational query, with the actual zero page deduper using
different mechanisms through the page queues.

ScanForZeroPages and related methods represent unneeded complexity to
maintain and test.

Change-Id: I628b6dc854a3fe24562ef2a3a3fa8ae32de10e54
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/742426
Fuchsia-Auto-Submit: Adrian Danis <[email protected]>
Reviewed-by: Rasha Eqbal <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>

Author: AdrianDanis

zircon/kernel/vm/include/vm/vm_cow_pages.h

@@ -483,10 +483,6 @@ class VmCowPages final
   using AttributionCounts = VmObject::AttributionCounts;
   AttributionCounts AttributedPagesInRangeLocked(uint64_t offset, uint64_t len) const TA_REQ(lock_);
 
-  // Scans this cow pages range for zero pages and frees them if |reclaim| is set to true. Returns
-  // the number of pages freed or scanned.
-  uint32_t ScanForZeroPagesLocked(bool reclaim) TA_REQ(lock_);
-
   enum class EvictionHintAction : uint8_t {
     Follow,
     Ignore,

zircon/kernel/vm/include/vm/vm_object.h

@@ -712,19 +712,6 @@ class VmObject : public VmHierarchyBase,
   // Detaches the underlying page source, if present. Can be called multiple times.
   virtual void DetachSource() {}
 
-  // Walks through every VMO, calls ScanForZeroPages on them, and returns the sum. This function is
-  // very expensive and will hold the AllVmosLock mutex for the entire duration. Should not be
-  // called casually or when it is not suitable to block operations against all other VMOs for an
-  // extended period of time.
-  static uint32_t ScanAllForZeroPages(bool reclaim);
-
-  // Scans for pages that could validly be de-duped/decommitted back to the zero page. If `reclaim`
-  // is true the pages will actually be de-duped. In either case the number of found pages is
-  // returned. It is expected that this would hold the VMOs lock for an extended period of time
-  // and should only be called when it is suitable for block all VMO operations for an extended
-  // period of time.
-  virtual uint32_t ScanForZeroPages(bool reclaim) { return 0; }
-
  protected:
   explicit VmObject(fbl::RefPtr<VmHierarchyState> hierarchy_state_ptr);
 

zircon/kernel/vm/include/vm/vm_object_paged.h

@@ -221,8 +221,6 @@ class VmObjectPaged final : public VmObject {
   zx_status_t CreateChildSlice(uint64_t offset, uint64_t size, bool copy_name,
                                fbl::RefPtr<VmObject>* child_vmo) override;
 
-  uint32_t ScanForZeroPages(bool reclaim) override;
-
   // Returns whether or not zero pages can be safely deduped from this VMO. Zero pages cannot be
   // deduped if the VMO is in use for kernel mappings, or if the pages cannot be accessed from the
   // physmap due to not being cached.

zircon/kernel/vm/scanner.cc

@@ -77,8 +77,6 @@ KCOUNTER(zero_scan_pages_scanned, "vm.scanner.zero_scan.total_pages_considered")
 KCOUNTER(zero_scan_pages_deduped, "vm.scanner.zero_scan.pages_deduped")
 
 void scanner_print_stats() {
-  uint64_t zero_pages = VmObject::ScanAllForZeroPages(false);
-  printf("[SCAN]: Found %lu zero pages across all of memory\n", zero_pages);
   PageQueues::Counts queue_counts = pmm_page_queues()->QueueCounts();
   for (size_t i = 0; i < PageQueues::kNumReclaim; i++) {
     printf("[SCAN]: Found %lu reclaimable pages in queue %zu\n", queue_counts.reclaim[i], i);

zircon/kernel/vm/unittests/vmo_unittest.cc

@@ -1313,82 +1313,6 @@ static bool vmo_clone_removes_write_test() {
   END_TEST;
 }
 
-static bool vmo_zero_scan_test() {
-  BEGIN_TEST;
-
-  AutoVmScannerDisable scanner_disable;
-
-  auto mem = testing::UserMemory::Create(PAGE_SIZE);
-  ASSERT_NONNULL(mem);
-
-  const auto& user_aspace = mem->aspace();
-  ASSERT_NONNULL(user_aspace);
-  ASSERT_TRUE(user_aspace->is_user());
-
-  // Initially uncommitted, which should not count as having zero pages.
-  EXPECT_EQ(0u, mem->vmo()->ScanForZeroPages(false));
-
-  // Validate that this mapping reads as zeros
-  EXPECT_EQ(ZX_OK, user_aspace->SoftFault(mem->base(), 0u));
-  EXPECT_EQ(0, mem->get<int32_t>());
-
-  // Reading from the page should not have committed anything, zero or otherwise.
-  EXPECT_EQ(0u, mem->vmo()->ScanForZeroPages(false));
-
-  // IF we write to the page, this should make it committed.
-  EXPECT_EQ(ZX_OK, user_aspace->SoftFault(mem->base(), VMM_PF_FLAG_WRITE));
-  mem->put<int32_t>(0);
-  EXPECT_EQ(1u, mem->vmo()->ScanForZeroPages(false));
-
-  // Check that changing the contents effects the zero page count.
-  EXPECT_EQ(ZX_OK, user_aspace->SoftFault(mem->base(), VMM_PF_FLAG_WRITE));
-  mem->put<int32_t>(42);
-  EXPECT_EQ(0u, mem->vmo()->ScanForZeroPages(false));
-  EXPECT_EQ(ZX_OK, user_aspace->SoftFault(mem->base(), VMM_PF_FLAG_WRITE));
-  mem->put<int32_t>(0);
-  EXPECT_EQ(1u, mem->vmo()->ScanForZeroPages(false));
-
-  // Scanning should drop permissions in the hardware page table from write to read-only.
-  paddr_t paddr_readable;
-  uint mmu_flags;
-  EXPECT_EQ(ZX_OK, user_aspace->SoftFault(mem->base(), VMM_PF_FLAG_WRITE));
-  mem->put<int32_t>(0);
-  zx_status_t status = user_aspace->arch_aspace().Query(mem->base(), &paddr_readable, &mmu_flags);
-  EXPECT_EQ(ZX_OK, status);
-  EXPECT_TRUE(mmu_flags & ARCH_MMU_FLAG_PERM_WRITE);
-  mem->vmo()->ScanForZeroPages(false);
-  status = user_aspace->arch_aspace().Query(mem->base(), &paddr_readable, &mmu_flags);
-  EXPECT_EQ(ZX_OK, status);
-  EXPECT_FALSE(mmu_flags & ARCH_MMU_FLAG_PERM_WRITE);
-
-  // Pinning the page should prevent it from being counted.
-  EXPECT_EQ(1u, mem->vmo()->ScanForZeroPages(false));
-  EXPECT_EQ(ZX_OK, mem->vmo()->CommitRangePinned(0, PAGE_SIZE, false));
-  EXPECT_EQ(0u, mem->vmo()->ScanForZeroPages(false));
-  mem->vmo()->Unpin(0, PAGE_SIZE);
-  EXPECT_EQ(1u, mem->vmo()->ScanForZeroPages(false));
-
-  // Creating a kernel mapping should prevent any counting from occurring.
-  VmAspace* kernel_aspace = VmAspace::kernel_aspace();
-  void* ptr;
-  status = kernel_aspace->MapObjectInternal(mem->vmo(), "test", 0, PAGE_SIZE, &ptr, 0,
-                                            VmAspace::VMM_FLAG_COMMIT, kArchRwFlags);
-  EXPECT_EQ(ZX_OK, status);
-  EXPECT_EQ(0u, mem->vmo()->ScanForZeroPages(false));
-  kernel_aspace->FreeRegion(reinterpret_cast<vaddr_t>(ptr));
-  EXPECT_EQ(1u, mem->vmo()->ScanForZeroPages(false));
-
-  // Actually evict the page now and check that the attribution count goes down and the eviction
-  // event count goes up.
-  EXPECT_EQ(1u, mem->vmo()->AttributedPages().uncompressed);
-  EXPECT_EQ(0u, mem->vmo()->EvictionEventCount());
-  EXPECT_EQ(1u, mem->vmo()->ScanForZeroPages(true));
-  EXPECT_EQ(0u, mem->vmo()->AttributedPages().uncompressed);
-  EXPECT_EQ(1u, mem->vmo()->EvictionEventCount());
-
-  END_TEST;
-}
-
 static bool vmo_move_pages_on_access_test() {
   BEGIN_TEST;
 
@@ -2388,18 +2312,6 @@ static bool vmo_attribution_dedup_test() {
   EXPECT_EQ(true, verify_object_page_attribution(vmo.get(), expected_gen_count,
                                                  AttributionCounts{2u, 0u}));
 
-  // Scan for zero pages, returning only the count (without triggering any reclamation). This should
-  // *not* change the generation count.
-  ASSERT_EQ(2u, vmo->ScanForZeroPages(false));
-  EXPECT_EQ(true, verify_object_page_attribution(vmo.get(), expected_gen_count,
-                                                 AttributionCounts{2u, 0u}));
-
-  // Scan for zero pages and reclaim them. This should change the generation count.
-  ASSERT_EQ(2u, vmo->ScanForZeroPages(true));
-  ++expected_gen_count;
-  EXPECT_EQ(true,
-            verify_object_page_attribution(vmo.get(), expected_gen_count, AttributionCounts{}));
-
   END_TEST;
 }
 
@@ -3710,19 +3622,6 @@ static bool vmo_dedup_dirty_test() {
   // No committed pages remaining.
   EXPECT_EQ(0u, vmo->AttributedPages().uncompressed);
 
-  // Commit the page again.
-  vmo->CommitRange(0, PAGE_SIZE);
-  page = vmo->DebugGetPage(0);
-
-  // The page is still clean.
-  EXPECT_TRUE(pmm_page_queues()->DebugPageIsReclaim(page));
-
-  // Zero scan should be able to dedup a clean page.
-  EXPECT_TRUE(vmo->ScanForZeroPages(true));
-
-  // No committed pages remaining.
-  EXPECT_EQ(0u, vmo->AttributedPages().uncompressed);
-
   // Write to the page making it dirty.
   uint8_t data = 0xff;
   status = vmo->Write(&data, 0, sizeof(data));
@@ -3734,7 +3633,6 @@ static bool vmo_dedup_dirty_test() {
 
   // We should not be able to dedup the page.
   EXPECT_FALSE(vmo->DebugGetCowPages()->DedupZeroPage(page, 0));
-  EXPECT_FALSE(vmo->ScanForZeroPages(true));
   EXPECT_EQ(1u, vmo->AttributedPages().uncompressed);
 
   END_TEST;
@@ -3766,7 +3664,6 @@ VM_UNITTEST(vmo_lookup_test)
 VM_UNITTEST(vmo_lookup_slice_test)
 VM_UNITTEST(vmo_lookup_clone_test)
 VM_UNITTEST(vmo_clone_removes_write_test)
-VM_UNITTEST(vmo_zero_scan_test)
 VM_UNITTEST(vmo_move_pages_on_access_test)
 VM_UNITTEST(vmo_eviction_hints_test)
 VM_UNITTEST(vmo_always_need_evicts_loaned_test)

zircon/kernel/vm/vm_cow_pages.cc

@@ -470,75 +470,6 @@ bool VmCowPages::DedupZeroPage(vm_page_t* page, uint64_t offset) {
   return false;
 }
 
-uint32_t VmCowPages::ScanForZeroPagesLocked(bool reclaim) {
-  canary_.Assert();
-
-  if (!can_decommit_zero_pages_locked()) {
-    // Even if !reclaim, we don't count zero pages in contiguous VMOs because we can't reclaim them
-    // anyway (at least not just due to them being zero; user mode can decommit).  We also don't
-    // add contiguous VMO pages to the ZeroFork queue ever, so counting these would only create
-    // false hope that we could potentially loan the pages despite explicitly not wanting to
-    // auto-decommit zero pages as expressed by !can_decommit_zero_pages_locked().  In future we
-    // may relax this restriction on contiguous VMOs at which point it'd be fine to remove zero
-    // pages (assuming other criteria are met, like not being pinned).
-    return 0;
-  }
-
-  // Check if we have any slice children. Slice children may have writable mappings to our pages,
-  // and so we need to also remove any mappings from them. Non-slice children could only have
-  // read-only mappings, which is the state we already want, and so we don't need to touch them.
-  for (auto& child : children_list_) {
-    AssertHeld(child.lock_);
-    if (child.is_slice_locked()) {
-      // Slices are strict subsets of their parents so we don't need to bother looking at parent
-      // limits etc and can just operate on the entire range.
-      child.RangeChangeUpdateLocked(0, child.size_, RangeChangeOp::RemoveWrite);
-    }
-  }
-
-  // We stack-own loaned pages from when they're removed until they're freed.
-  __UNINITIALIZED StackOwnedLoanedPagesInterval raii_interval;
-
-  list_node_t freed_list;
-  list_initialize(&freed_list);
-
-  uint32_t count = 0;
-  page_list_.RemovePages(
-      [&count, &freed_list, reclaim, this](VmPageOrMarker* p, uint64_t off) {
-        // Pinned pages cannot be decommitted so do not consider them, and also skip references as
-        // they they are presumably already deduped and would need to be decompressed to scan them.
-        // If the page supports dirty tracking, it should be Clean.
-        if (p->IsPage() && p->Page()->object.pin_count == 0 &&
-            (!is_page_dirty_tracked(p->Page()) || is_page_clean(p->Page())) &&
-            IsZeroPage(p->Page())) {
-          count++;
-          if (reclaim) {
-            // Need to remove all mappings (include read) ones to this range before we remove the
-            // page.
-            AssertHeld(this->lock_);
-            RangeChangeUpdateLocked(off, PAGE_SIZE, RangeChangeOp::Unmap);
-            vm_page_t* page = p->ReleasePage();
-            pmm_page_queues()->Remove(page);
-            DEBUG_ASSERT(!list_in_list(&page->queue_node));
-            list_add_tail(&freed_list, &page->queue_node);
-            *p = VmPageOrMarker::Marker();
-          }
-        }
-        return ZX_ERR_NEXT;
-      },
-      0, VmPageList::MAX_SIZE);
-
-  FreePagesLocked(&freed_list, /*freeing_owned_pages=*/true);
-
-  if (reclaim && count > 0) {
-    IncrementHierarchyGenerationCountLocked();
-    // A batch free is counted as a single eviction event.
-    eviction_event_count_++;
-  }
-
-  return count;
-}
-
 zx_status_t VmCowPages::Create(fbl::RefPtr<VmHierarchyState> root_lock, VmCowPagesOptions options,
                                uint32_t pmm_alloc_flags, uint64_t size,
                                fbl::RefPtr<VmCowPages>* cow_pages) {

zircon/kernel/vm/vm_object.cc

@@ -50,16 +50,6 @@ VmObject::~VmObject() {
   DEBUG_ASSERT(children_list_.is_empty());
 }
 
-uint32_t VmObject::ScanAllForZeroPages(bool reclaim) {
-  uint32_t count = 0;
-  Guard<CriticalMutex> guard{AllVmosLock::Get()};
-
-  for (auto& vmo : all_vmos_) {
-    count += vmo.ScanForZeroPages(reclaim);
-  }
-  return count;
-}
-
 void VmObject::AddToGlobalList() {
   Guard<CriticalMutex> guard{AllVmosLock::Get()};
   all_vmos_.push_back(this);

zircon/kernel/vm/vm_object_paged.cc

@@ -166,31 +166,6 @@ bool VmObjectPaged::CanDedupZeroPagesLocked() {
   return true;
 }
 
-uint32_t VmObjectPaged::ScanForZeroPages(bool reclaim) {
-  canary_.Assert();
-
-  Guard<CriticalMutex> guard{lock()};
-
-  // Skip uncached VMOs as we cannot efficiently scan them.
-  if ((cache_policy_ & ZX_CACHE_POLICY_MASK) != ZX_CACHE_POLICY_CACHED) {
-    return 0;
-  }
-
-  // Skip any VMOs that have non user mappings as we cannot safely remove write permissions from
-  // them and indicates this VMO is actually in use by the kernel and we probably would not want to
-  // perform zero page de-duplication on it even if we could.
-  for (auto& m : mapping_list_) {
-    if (!m.aspace()->is_user()) {
-      return 0;
-    }
-    // Remove write from the mapping to ensure it's not being concurrently modified.
-    m.assert_object_lock();
-    m.AspaceRemoveWriteVmoRangeLocked(0, size_locked());
-  }
-
-  return cow_pages_locked()->ScanForZeroPagesLocked(reclaim);
-}
-
 zx_status_t VmObjectPaged::CreateCommon(uint32_t pmm_alloc_flags, uint32_t options, uint64_t size,
                                         fbl::RefPtr<VmObjectPaged>* obj) {
   DEBUG_ASSERT(!(options & (kContiguous | kCanBlockOnPageRequests)));