pm: Remove the venerable pm_remove_contig()
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 3 Apr 2018 19:42:44 +0000 (15:42 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 30 Apr 2018 18:36:28 +0000 (14:36 -0400)
It did a lot, but was more complicated than we needed.  It also might have
been doing too much.

It was responsible for attempting to remove pages even if they were being
used by an active mmaping - shootdown, collect dirty bits, writeback, unmap
for real and catch any intermediate users, etc.  I'm not sure we need all
of that, and if we do, maybe we'll fix the PM / MM code before that (more
scalable structures, huge pages, etc).

It also was marking PTEs not present before checking their dirty bits.
That might not be necessary for all uses.  It looks like we wanted to force
any users to fault, which would trigger a pm_load_page, which would clear
the removal flag.  Not 100% on that.  Be careful if you use that old code.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/include/fs_file.h
kern/include/pagemap.h
kern/src/pagemap.c

index 58d55ff..9b4f06a 100644 (file)
  *
  * The PM code is pretty rough.  For now, we're using the old VFS PM code, but
  * it needs a few changes:
- * - Hole punching / truncate:  pm_remove_contig() leaves behind pages if they
- *   were concurrently opened.  We can't detach from the tree since the pm_page
- *   assumes it is always hooked in.  This means a truncate during a read or
- *   mmap could leave data behind in the PM, which will be accessible when len
- *   gets increased again (e.g. seek and write).  Truncate could zero that page,
- *   though we'd need a history counter so reads don't see a partial value.
- *   Further, pinned VMRs get skipped.  During trunc, they should get removed
- *   and generate a SIGBUS on access.
  * - Heavily integrated with struct page.  Maybe some sort of 'struct page' on
  *   demand, that is built for an IO mapping.  There might be some huge-page
  *   stuff here too.
index 68aadf4..f2ad2f0 100644 (file)
@@ -58,8 +58,6 @@ int pm_load_page_nowait(struct page_map *pm, unsigned long index,
 void pm_put_page(struct page *page);
 void pm_add_vmr(struct page_map *pm, struct vm_region *vmr);
 void pm_remove_vmr(struct page_map *pm, struct vm_region *vmr);
-int pm_remove_contig(struct page_map *pm, unsigned long index,
-                     unsigned long nr_pgs);
 void pm_remove_or_zero_pages(struct page_map *pm, unsigned long start_idx,
                              unsigned long nr_pgs);
 void pm_writeback_pages(struct page_map *pm);
index f7f1290..5adbb8f 100644 (file)
@@ -48,22 +48,7 @@ void pm_remove_vmr(struct page_map *pm, struct vm_region *vmr)
 #endif
 #define PM_REFCNT_SHIFT (PM_FLAGS_SHIFT + 1)
 
-#define PM_REMOVAL (1UL << PM_FLAGS_SHIFT)
-
-static bool pm_slot_check_removal(void *slot_val)
-{
-       return (unsigned long)slot_val & PM_REMOVAL ? TRUE : FALSE;
-}
-
-static void *pm_slot_set_removal(void *slot_val)
-{
-       return (void*)((unsigned long)slot_val | PM_REMOVAL);
-}
-
-static void *pm_slot_clear_removal(void *slot_val)
-{
-       return (void*)((unsigned long)slot_val & ~PM_REMOVAL);
-}
+#define PM_UNUSED_FLAG (1UL << PM_FLAGS_SHIFT)
 
 static int pm_slot_check_refcnt(void *slot_val)
 {
@@ -140,7 +125,6 @@ static struct page *pm_find_page(struct page_map *pm, unsigned long index)
                page = pm_slot_get_page(slot_val);
                if (!page)
                        goto out;
-               slot_val = pm_slot_clear_removal(slot_val);
                slot_val = pm_slot_inc_refcnt(slot_val);        /* not a page kref */
        } while (!atomic_cas_ptr(tree_slot, old_slot_val, slot_val));
        assert(page->pg_tree_slot == tree_slot);
@@ -290,11 +274,6 @@ static bool vmr_has_page_idx(struct vm_region *vmr, unsigned long pg_idx)
        return ((start_pg <= pg_idx) && (pg_idx < start_pg + nr_pgs));
 }
 
-static unsigned long vmr_get_end_idx(struct vm_region *vmr)
-{
-       return ((vmr->vm_end - vmr->vm_base) + vmr->vm_foff) >> PGSHIFT;
-}
-
 /* Runs CB on every PTE in the VMR that corresponds to the file's pg_idx, for up
  * to max_nr_pgs. */
 static void vmr_for_each(struct vm_region *vmr, unsigned long pg_idx,
@@ -324,301 +303,6 @@ static void vmr_for_each(struct vm_region *vmr, unsigned long pg_idx,
        env_user_mem_walk(vmr->vm_proc, (void*)start_va, len, callback, vmr);
 }
 
-/* These next two helpers are called on a VMR's range of VAs corresponding to a
- * pages in a PM undergoing removal.
- *
- * In general, it is safe to mark !P or 0 a PTE so long as the page the PTE
- * points to belongs to a PM.  We'll refault, find the page, and rebuild the
- * PTE.  This allows us to handle races like: {pm marks !p, {fault, find page,
- * abort removal, write new pte}, pm clears pte}.
- *
- * In that race, HPF is writing the PTE, which removal code subsequently looks
- * at to determine if the underlying page is dirty.  We need to make sure no one
- * clears dirty bits unless they handle the WB (or discard).  HPF preserves the
- * dirty bit for this reason. */
-static int __pm_mark_not_present(struct proc *p, pte_t pte, void *va, void *arg)
-{
-       struct page *page;
-       /* mapped includes present.  Any PTE pointing to a page (mapped) will get
-        * flagged for removal and have its access prots revoked.  We need to deal
-        * with mapped-but-maybe-not-present in case of a dirtied file that was
-        * mprotected to PROT_NONE (which is not present) */
-       if (pte_is_unmapped(pte))
-               return 0;
-       page = pa2page(pte_get_paddr(pte));
-       if (atomic_read(&page->pg_flags) & PG_REMOVAL)
-               pte_clear_present(pte);
-       return 0;
-}
-
-static int __pm_mark_dirty_pgs_unmap(struct proc *p, pte_t pte, void *va,
-                                     void *arg)
-{
-       struct page *page;
-       /* we're not checking for 'present' or not, since we marked them !P earlier.
-        * but the CB is still called on everything in the range.  we can tell the
-        * formerly-valid PTEs from the completely unmapped, since the latter are
-        * unmapped, while the former have other things in them, but just are !P. */
-       if (pte_is_unmapped(pte))
-               return 0;
-       page = pa2page(pte_get_paddr(pte));
-       /* need to check for removal again, just like in mark_not_present */
-       if (atomic_read(&page->pg_flags) & PG_REMOVAL) {
-               if (pte_is_dirty(pte))
-                       atomic_or(&page->pg_flags, PG_DIRTY);
-               pte_clear(pte);
-       }
-       return 0;
-}
-
-static int __pm_mark_unmap(struct proc *p, pte_t pte, void *va, void *arg)
-{
-       struct page *page;
-       if (pte_is_unmapped(pte))
-               return 0;
-       page = pa2page(pte_get_paddr(pte));
-       if (atomic_read(&page->pg_flags) & PG_REMOVAL)
-               pte_clear(pte);
-       return 0;
-}
-
-static void shootdown_and_reset_ptrstore(void *proc_ptrs[], int *arr_idx)
-{
-       for (int i = 0; i < *arr_idx; i++)
-               proc_tlbshootdown((struct proc*)proc_ptrs[i], 0, 0);
-       *arr_idx = 0;
-}
-
-/* Attempts to remove pages from the pm, from [index, index + nr_pgs).  Returns
- * the number of pages removed.  There can only be one remover at a time per PM
- * - others will return 0. */
-int pm_remove_contig(struct page_map *pm, unsigned long index,
-                     unsigned long nr_pgs)
-{
-       unsigned long i;
-       int nr_removed = 0;
-       void **tree_slot;
-       void *old_slot_val, *slot_val;
-       struct vm_region *vmr_i;
-       bool pm_has_pinned_vmrs = FALSE;
-       /* using this for both procs and later WBs */
-       #define PTR_ARR_LEN 10
-       void *ptr_store[PTR_ARR_LEN];
-       int ptr_free_idx = 0;
-       struct page *page;
-
-       /* could also call a simpler remove if nr_pgs == 1 */
-       if (!nr_pgs)
-               return 0;
-
-       /* This is a mess.  Qlock due to the radix_delete later.  spinlock for the
-        * VMR lists. */
-       qlock(&pm->pm_qlock);
-       spin_lock(&pm->pm_lock);
-       assert(index + nr_pgs > index); /* til we figure out who validates */
-       /* check for any pinned VMRs.  if we have none, then we can skip some loops
-        * later */
-       TAILQ_FOREACH(vmr_i, &pm->pm_vmrs, vm_pm_link) {
-               if (vmr_i->vm_flags & MAP_LOCKED)
-                       pm_has_pinned_vmrs = TRUE;
-       }
-       /* this pass, we mark pages for removal */
-       for (i = index; i < index + nr_pgs; i++) {
-               if (pm_has_pinned_vmrs) {
-                       /* for pinned pages, we don't even want to attempt to remove them */
-                       TAILQ_FOREACH(vmr_i, &pm->pm_vmrs, vm_pm_link) {
-                               /* once we've found a pinned page, we can skip over the rest of
-                                * the range of pages mapped by this vmr - even if the vmr
-                                * hasn't actually faulted them in yet. */
-                               if ((vmr_i->vm_flags & MAP_LOCKED) &&
-                                   (vmr_has_page_idx(vmr_i, i))) {
-                                       i = vmr_get_end_idx(vmr_i) - 1; /* loop will +1 */
-                                       goto next_loop_mark_rm;
-                               }
-                       }
-               }
-               /* TODO: would like a radix_next_slot() iterator (careful with skipping
-                * chunks of the loop) */
-               tree_slot = radix_lookup_slot(&pm->pm_tree, i);
-               if (!tree_slot)
-                       continue;
-               old_slot_val = ACCESS_ONCE(*tree_slot);
-               slot_val = old_slot_val;
-               page = pm_slot_get_page(slot_val);
-               if (!page)
-                       continue;
-               /* syncing with lookups, writebacks, etc.  only one remover per pm in
-                * general.  any new ref-getter (WB, lookup, etc) will clear removal,
-                * causing us to abort later. */
-               if (pm_slot_check_refcnt(slot_val))
-                       continue;
-               /* it's possible that removal is already set, if we happened to repeat a
-                * loop (due to running out of space in the proc arr) */
-               slot_val = pm_slot_set_removal(slot_val);
-               if (!atomic_cas_ptr(tree_slot, old_slot_val, slot_val))
-                       continue;
-               /* mark the page itself.  this isn't used for syncing - just out of
-                * convenience for ourselves (memwalk callbacks are easier).  need the
-                * atomic in case a new user comes in and tries mucking with the flags*/
-               atomic_or(&page->pg_flags, PG_REMOVAL);
-next_loop_mark_rm:
-               ;
-       }
-       /* second pass, over VMRs instead of pages.  we remove the marked pages from
-        * all VMRs, collecting the procs for batch shootdowns.  not sure how often
-        * we'll have more than one VMR (for a PM) per proc.  shared libs tend to
-        * have a couple, so we'll still batch things up */
-       TAILQ_FOREACH(vmr_i, &pm->pm_vmrs, vm_pm_link) {
-               /* might have some pinned VMRs that only map part of the file we aren't
-                * messing with (so they didn't trigger earlier). */
-               if (vmr_i->vm_flags & MAP_LOCKED)
-                       continue;
-               /* Private mappings: for each page, either PMs have a separate copy
-                * hanging off their PTE (in which case they aren't using the PM page,
-                * and the actual page in use won't have PG_REMOVAL set, and the CB will
-                * ignore it), or they are still using the shared version.  In which
-                * case they haven't written it yet, and we can remove it.  If they
-                * concurrently are performing a write fault to CoW the page, they will
-                * incref and clear REMOVAL, thereby aborting the remove anyways.
-                *
-                * Though if the entire mapping is unique-copies of private pages, we
-                * won't need a shootdown.  mem_walk can't handle this yet though. */
-               if (!vmr_has_page_idx(vmr_i, index))
-                       continue;
-               spin_lock(&vmr_i->vm_proc->pte_lock);
-               /* all PTEs for pages marked for removal are marked !P for the entire
-                * range.  it's possible we'll remove some extra PTEs (races with
-                * loaders, etc), but those pages will remain in the PM and should get
-                * soft-faulted back in. */
-               vmr_for_each(vmr_i, index, nr_pgs, __pm_mark_not_present);
-               spin_unlock(&vmr_i->vm_proc->pte_lock);
-               /* batching TLB shootdowns for a given proc (continue if found).
-                * the proc stays alive while we hold a read lock on the PM tree,
-                * since the VMR can't get yanked out yet. */
-               for (i = 0; i < ptr_free_idx; i++) {
-                       if (ptr_store[i] == vmr_i->vm_proc)
-                               break;
-               }
-               if (i != ptr_free_idx)
-                       continue;
-               if (ptr_free_idx == PTR_ARR_LEN)
-                       shootdown_and_reset_ptrstore(ptr_store, &ptr_free_idx);
-               ptr_store[ptr_free_idx++] = vmr_i->vm_proc;
-       }
-       /* Need to shootdown so that all TLBs have the page marked absent.  Then we
-        * can check the dirty bit, now that concurrent accesses will fault.  btw,
-        * we have a lock ordering: pm (RCU) -> proc lock (state, vcmap, etc) */
-       shootdown_and_reset_ptrstore(ptr_store, &ptr_free_idx);
-       /* Now that we've shotdown, we can check for dirtiness.  One downside to
-        * this approach is we check every VMR for a page, even once we know the
-        * page is dirty.  We also need to unmap the pages (set ptes to 0) for any
-        * that we previously marked not present (complete the unmap).  We're racing
-        * with munmap here, which treats the PTE as a weak ref on a page. */
-       TAILQ_FOREACH(vmr_i, &pm->pm_vmrs, vm_pm_link) {
-               if (vmr_i->vm_flags & MAP_LOCKED)
-                       continue;
-               if (!vmr_has_page_idx(vmr_i, index))
-                       continue;
-               spin_lock(&vmr_i->vm_proc->pte_lock);
-               if (vmr_i->vm_prot & PROT_WRITE)
-                       vmr_for_each(vmr_i, index, nr_pgs, __pm_mark_dirty_pgs_unmap);
-               else
-                       vmr_for_each(vmr_i, index, nr_pgs, __pm_mark_unmap);
-               spin_unlock(&vmr_i->vm_proc->pte_lock);
-       }
-       /* Now we'll go through from the PM again and deal with pages are dirty. */
-       i = index;
-handle_dirty:
-       for (/* i set already */; i < index + nr_pgs; i++) {
-               /* TODO: consider putting in the pinned check & advance again.  Careful,
-                * since we could unlock on a handle_dirty loop, and skipping could skip
-                * over a new VMR, but those pages would still be marked for removal.
-                * It's not wrong, currently, to have spurious REMOVALs. */
-               tree_slot = radix_lookup_slot(&pm->pm_tree, i);
-               if (!tree_slot)
-                       continue;
-               page = pm_slot_get_page(*tree_slot);
-               if (!page)
-                       continue;
-               /* only operate on pages we marked earlier */
-               if (!(atomic_read(&page->pg_flags) & PG_REMOVAL))
-                       continue;
-               /* if someone has used it since we grabbed it, we lost the race and
-                * won't remove it later.  no sense writing it back now either. */
-               if (!pm_slot_check_removal(*tree_slot)) {
-                       /* since we set PG_REMOVAL, we're the ones to clear it */
-                       atomic_and(&page->pg_flags, ~PG_REMOVAL);
-                       continue;
-               }
-               /* this dirty flag could also be set by write()s, not just VMRs */
-               if (atomic_read(&page->pg_flags) & PG_DIRTY) {
-                       /* need to bail out.  after we WB, we'll restart this big loop where
-                        * we left off ('i' is still set) */
-                       if (ptr_free_idx == PTR_ARR_LEN)
-                               break;
-                       ptr_store[ptr_free_idx++] = page;
-                       /* once we've decided to WB, we can clear the dirty flag.  might
-                        * have an extra WB later, but we won't miss new data */
-                       atomic_and(&page->pg_flags, ~PG_DIRTY);
-               }
-       }
-       /* we're unlocking, meaning VMRs and the radix tree can be changed, but we
-        * are still the only remover. still can have new refs that clear REMOVAL */
-       spin_unlock(&pm->pm_lock);
-       /* could batch these up, etc. */
-       for (int j = 0; j < ptr_free_idx; j++)
-               pm->pm_op->writepage(pm, (struct page*)ptr_store[j]);
-       ptr_free_idx = 0;
-       spin_lock(&pm->pm_lock);
-       /* bailed out of the dirty check loop earlier, need to finish and WB.  i is
-        * still set to where we failed and left off in the big loop. */
-       if (i < index + nr_pgs)
-               goto handle_dirty;
-       /* TODO: RCU - we need a write lock here (the current spinlock is fine) */
-       /* All dirty pages were WB, anything left as REMOVAL can be removed */
-       for (i = index; i < index + nr_pgs; i++) {
-               /* TODO: consider putting in the pinned check & advance again */
-               tree_slot = radix_lookup_slot(&pm->pm_tree, i);
-               if (!tree_slot)
-                       continue;
-               old_slot_val = ACCESS_ONCE(*tree_slot);
-               slot_val = old_slot_val;
-               page = pm_slot_get_page(*tree_slot);
-               if (!page)
-                       continue;
-               if (!(atomic_read(&page->pg_flags) & PG_REMOVAL))
-                       continue;
-               /* syncing with lookups, writebacks, etc.  if someone has used it since
-                * we started removing, they would have cleared the slot's REMOVAL (but
-                * not PG_REMOVAL), though the refcnt could be back down to 0 again. */
-               if (!pm_slot_check_removal(slot_val)) {
-                       /* since we set PG_REMOVAL, we're the ones to clear it */
-                       atomic_and(&page->pg_flags, ~PG_REMOVAL);
-                       continue;
-               }
-               if (pm_slot_check_refcnt(slot_val))
-                       warn("Unexpected refcnt in PM remove!");
-               /* Note that we keep slot REMOVAL set, so the radix tree thinks it's
-                * still an item (artifact of that implementation). */
-               slot_val = pm_slot_set_page(slot_val, 0);
-               if (!atomic_cas_ptr(tree_slot, old_slot_val, slot_val)) {
-                       atomic_and(&page->pg_flags, ~PG_REMOVAL);
-                       continue;
-               }
-               /* at this point, we're free at last!  When we update the radix tree, it
-                * still thinks it has an item.  This is fine.  Lookups will now fail
-                * (since the page is 0), and insertions will block on the write lock.*/
-               atomic_set(&page->pg_flags, 0); /* cause/catch bugs */
-               page_decref(page);
-               nr_removed++;
-               radix_delete(&pm->pm_tree, i);
-       }
-       pm->pm_num_pages -= nr_removed;
-       spin_unlock(&pm->pm_lock);
-       qunlock(&pm->pm_qlock);
-       return nr_removed;
-}
-
 static bool pm_has_vmr_with_page(struct page_map *pm, unsigned long pg_idx)
 {
        struct vm_region *vmr_i;