Make page_insert() consume the caller's refcnt
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 16 Aug 2016 17:48:49 +0000 (13:48 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 29 Nov 2016 16:27:40 +0000 (11:27 -0500)
The page refcounting needs to go.  The refcnt was from a time when a page
could have multiple objects tracking it independently.  Nowadays that is
handled higher up, such as in the page cache.

For the most part, the freeing/allocating of the memory is handled higher
up in the stack.  We were already doing this with e.g. procinfo, where we
would free it twice, doing double the work necessary.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/src/mm.c
kern/src/pmap.c
kern/src/process.c
kern/src/syscall.c

index f05e33e..b53a6cf 100644 (file)
@@ -297,12 +297,11 @@ static int copy_pages(struct proc *p, struct proc *new_p, uintptr_t va_start,
                        /* TODO: check for jumbos */
                        if (upage_alloc(new_p, &pp, 0))
                                return -ENOMEM;
                        /* TODO: check for jumbos */
                        if (upage_alloc(new_p, &pp, 0))
                                return -ENOMEM;
+                       memcpy(page2kva(pp), KADDR(pte_get_paddr(pte)), PGSIZE);
                        if (page_insert(new_p->env_pgdir, pp, va, pte_get_settings(pte))) {
                                page_decref(pp);
                                return -ENOMEM;
                        }
                        if (page_insert(new_p->env_pgdir, pp, va, pte_get_settings(pte))) {
                                page_decref(pp);
                                return -ENOMEM;
                        }
-                       memcpy(page2kva(pp), KADDR(pte_get_paddr(pte)), PGSIZE);
-                       page_decref(pp);
                } else if (pte_is_paged_out(pte)) {
                        /* TODO: (SWAP) will need to either make a copy or CoW/refcnt the
                         * backend store.  For now, this PTE will be the same as the
                } else if (pte_is_paged_out(pte)) {
                        /* TODO: (SWAP) will need to either make a copy or CoW/refcnt the
                         * backend store.  For now, this PTE will be the same as the
index 749f245..557738a 100644 (file)
@@ -187,7 +187,7 @@ void *boot_zalloc(size_t amt, size_t align)
  *   - If there is already a page mapped at 'va', it is page_remove()d.
  *   - If necessary, on demand, allocates a page table and inserts it into
  *     'pgdir'.
  *   - If there is already a page mapped at 'va', it is page_remove()d.
  *   - If necessary, on demand, allocates a page table and inserts it into
  *     'pgdir'.
- *   - page_incref() should be called if the insertion succeeds.
+ *   - This saves your refcnt in the pgdir (refcnts going away soon).
  *   - The TLB must be invalidated if a page was formerly present at 'va'.
  *     (this is handled in page_remove)
  *
  *   - The TLB must be invalidated if a page was formerly present at 'va'.
  *     (this is handled in page_remove)
  *
@@ -214,15 +214,8 @@ int page_insert(pgdir_t pgdir, struct page *page, void *va, int perm)
        pte_t pte = pgdir_walk(pgdir, va, 1);
        if (!pte_walk_okay(pte))
                return -ENOMEM;
        pte_t pte = pgdir_walk(pgdir, va, 1);
        if (!pte_walk_okay(pte))
                return -ENOMEM;
-       /* Two things here:  First, we need to up the ref count of the page we want
-        * to insert in case it is already mapped at va.  In that case we don't want
-        * page_remove to ultimately free it, and then for us to continue as if pp
-        * wasn't freed. (moral = up the ref asap) */
-       kref_get(&page->pg_kref, 1);
-       /* Careful, page remove handles the cases where the page is PAGED_OUT and
-        * any other state. (TODO: review all other states, maybe rm only for P) */
-       if (pte_is_mapped(pte))
-               page_remove(pgdir, va);
+       /* Leftover from older times, but we no longer suppor this: */
+       assert(!pte_is_mapped(pte));
        pte_write(pte, page2pa(page), perm);
        return 0;
 }
        pte_write(pte, page2pa(page), perm);
        return 0;
 }
index af23582..47840d7 100644 (file)
@@ -515,11 +515,12 @@ static void __proc_free(struct kref *kref)
        else
                printd("[kernel] pid %d not in the PID hash in %s\n", p->pid,
                       __FUNCTION__);
        else
                printd("[kernel] pid %d not in the PID hash in %s\n", p->pid,
                       __FUNCTION__);
-       /* all memory below UMAPTOP should have been freed via the VMRs.  the stuff
-        * above is the global page and procinfo/procdata */
-       env_user_mem_free(p, (void*)UMAPTOP, UVPT - UMAPTOP); /* 3rd arg = len... */
+       /* All memory below UMAPTOP should have been freed via the VMRs.  The stuff
+        * above is the global info/page and procinfo/procdata.  We free procinfo
+        * and procdata, but not the global memory - that's system wide.  We could
+        * clear the PTEs of the upper stuff (UMAPTOP to UVPT), but we shouldn't
+        * need to. */
        env_user_mem_walk(p, 0, UMAPTOP, __cb_assert_no_pg, 0);
        env_user_mem_walk(p, 0, UMAPTOP, __cb_assert_no_pg, 0);
-       /* These need to be freed again, since they were allocated with a refcnt. */
        free_cont_pages(p->procinfo, LOG2_UP(PROCINFO_NUM_PAGES));
        free_cont_pages(p->procdata, LOG2_UP(PROCDATA_NUM_PAGES));
 
        free_cont_pages(p->procinfo, LOG2_UP(PROCINFO_NUM_PAGES));
        free_cont_pages(p->procdata, LOG2_UP(PROCDATA_NUM_PAGES));
 
index fbad9ef..8758f2c 100644 (file)
@@ -570,7 +570,6 @@ static int sys_cache_buster(struct proc *p, uint32_t num_writes,
                        upage_alloc(p, &a_page[i],1);
                        page_insert(p->env_pgdir, a_page[i], (void*)INSERT_ADDR + PGSIZE*i,
                                    PTE_USER_RW);
                        upage_alloc(p, &a_page[i],1);
                        page_insert(p->env_pgdir, a_page[i], (void*)INSERT_ADDR + PGSIZE*i,
                                    PTE_USER_RW);
-                       page_decref(a_page[i]);
                }
                spin_unlock(&buster_lock);
        }
                }
                spin_unlock(&buster_lock);
        }