Fixes page reference counting wrt to upage_alloc()
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 23 Sep 2010 23:44:06 +0000 (16:44 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:54 +0000 (17:35 -0700)
upage_alloc() returns a reference counted page.  page_insert() and
friends will incref it when it stores the reference.  When you are done
using the struct page * that upage_alloc() returned, you must decref.
Just like other pointers and objects.

kern/arch/i686/pmap.c
kern/include/page_alloc.h
kern/src/env.c
kern/src/mm.c
kern/src/page_alloc.c
kern/src/pmap.c
kern/src/syscall.c

index 97a13f0..f64065c 100644 (file)
@@ -614,8 +614,8 @@ pgdir_walk(pde_t *pgdir, const void *SNT va, int create)
        }
        if (kpage_alloc(&new_table))
                return NULL;
-       page_setref(new_table,1);
        memset(page2kva(new_table), 0, PGSIZE);
+       /* storing our ref to new_table in the PTE */
        *the_pde = (pde_t)page2pa(new_table) | PTE_P | PTE_W | PTE_U;
        return &((pde_t*COUNT(NPTENTRIES))KADDR(PTE_ADDR(*the_pde)))[PTX(va)];
 }
@@ -760,7 +760,7 @@ page_check(void)
        assert(check_va2pa(boot_pgdir, PGSIZE) == ~0);
        assert(kref_refcnt(&pp1->pg_kref) == 1);
        assert(kref_refcnt(&pp2->pg_kref) == 1);
-       kref_put(&pp1->pg_kref);
+       page_decref(pp1);
 
        // so it should be returned by page_alloc
        assert(kpage_alloc(&pp) == 0 && pp == pp1);
index d82309d..dea207d 100644 (file)
@@ -55,7 +55,6 @@ error_t upage_alloc(struct proc* p, page_t *SAFE *page, int zero);
 error_t kpage_alloc(page_t *SAFE *page);
 error_t upage_alloc_specific(struct proc* p, page_t *SAFE *page, size_t ppn);
 error_t kpage_alloc_specific(page_t *SAFE *page, size_t ppn);
-error_t colored_upage_alloc(uint8_t* map, page_t *SAFE *page, size_t color);
 
 void *CT(1 << order) get_cont_pages(size_t order, int flags);
 void free_cont_pages(void *buf, size_t order);
index f59e175..39f1b53 100644 (file)
@@ -44,12 +44,10 @@ WRITES(e->env_pgdir, e->env_cr3, e->procinfo, e->procdata)
        page_t *pgdir = NULL;
        static page_t * RO shared_page = 0;
 
-       /*
-        * First, allocate a page for the pgdir of this process and up
-        * its reference count since this will never be done elsewhere
-        */
+       /* Get a page for the pgdir.  Storing the ref in pgdir/env_pgdir */
        r = kpage_alloc(&pgdir);
-       if(r < 0) return r;
+       if (r < 0)
+               return r;
 
        /*
         * Next, set up the e->env_pgdir and e->env_cr3 pointers to point
@@ -97,22 +95,17 @@ WRITES(e->env_pgdir, e->env_cr3, e->procinfo, e->procdata)
        memset(e->procinfo, 0, sizeof(struct procinfo));
        memset(e->procdata, 0, sizeof(struct procdata));
 
-       /* Finally, set up the Global Shared Data page for all processes.
-        * Can't be trusted, but still very useful at this stage for us.
-        * Consider removing when we have real processes.
-        * (TODO).  Note the page is alloced only the first time through
-        */
+       /* Finally, set up the Global Shared Data page for all processes.  Can't be
+        * trusted, but still very useful at this stage for us.  Consider removing
+        * when we have real processes (TODO). 
+        *
+        * Note the page is alloced only the first time through, and its ref is
+        * stored in shared_page. */
        if (!shared_page) {
-               if(upage_alloc(e, &shared_page,1) < 0)
+               if (upage_alloc(e, &shared_page, 1) < 0)
                        goto env_setup_vm_error;
-               // Up it, so it never goes away.  One per user, plus one from page_alloc
-               // This is necessary, since it's in the per-process range of memory that
-               // gets freed during page_free.
-               __kref_get(&shared_page->pg_kref, 1);
        }
-
-       // Inserted into every process's address space at UGDATA
-       if(page_insert(e->env_pgdir, shared_page, (void*SNT)UGDATA, PTE_USER_RW) < 0)
+       if (page_insert(e->env_pgdir, shared_page, (void*)UGDATA, PTE_USER_RW) < 0)
                goto env_setup_vm_error;
 
        return 0;
index d072a6e..5ac0f0d 100644 (file)
@@ -590,9 +590,12 @@ int __handle_page_fault(struct proc *p, uintptr_t va, int prot)
                 * you a new page.  In the future, we want to CoW this. */
                if ((vmr->vm_flags |= MAP_PRIVATE) && (vmr->vm_prot |= PROT_WRITE)) {
                        struct page *cache_page = a_page;
-                       if (upage_alloc(p, &a_page, FALSE))
+                       if (upage_alloc(p, &a_page, FALSE)) {
+                               page_decref(cache_page);        /* was the original a_page */
                                return -ENOMEM;
+                       }
                        memcpy(page2kva(a_page), page2kva(cache_page), PGSIZE);
+                       page_decref(cache_page);                /* was the original a_page */
                }
                /* if this is an executable page, we might have to flush the instruction
                 * cache if our HW requires it. */
@@ -603,9 +606,7 @@ int __handle_page_fault(struct proc *p, uintptr_t va, int prot)
         * separately (file, no file) */
        int pte_prot = (vmr->vm_prot & PROT_WRITE) ? PTE_USER_RW :
                       (vmr->vm_prot & (PROT_READ|PROT_EXEC)) ? PTE_USER_RO : 0;
-       /* Need to incref, since we manually insert in the pgdir.  Internal version,
-        * to go along with the upage_alloc from above. */
-       __kref_get(&a_page->pg_kref, 1);
+       /* We have a ref to a_page, which we are storing in the PTE */
        *pte = PTE(page2ppn(a_page), PTE_P | pte_prot);
        return 0;
 }
index 5262bae..657f0c7 100644 (file)
@@ -21,7 +21,6 @@
 #define l3 (available_caches.l3)
 
 static void __page_decref(page_t *CT(1) page);
-static void __page_incref(page_t *CT(1) page);
 static error_t __page_alloc_specific(page_t** page, size_t ppn);
 
 #ifdef __CONFIG_PAGE_COLORING__
@@ -68,8 +67,7 @@ static void __page_clear(page_t *SAFE page)
                *page = LIST_FIRST(&colored_page_free_list[i]);                     \
                LIST_REMOVE(*page, pg_link);                                        \
                __page_clear(*page);                                                \
-               /* Note the 0 initial value, due to how user pages are refcnt'd */  \
-               page_setref((*page), 0);                                            \
+               page_setref((*page), 1);                                            \
                return i;                                                           \
        }                                                                       \
        return -ENOMEM;
@@ -108,14 +106,13 @@ static error_t __page_alloc_specific(page_t** page, size_t ppn)
        *page = sp_page;
        LIST_REMOVE(*page, pg_link);
        __page_clear(*page);
-       /* Note the 0 initial value, due to how user pages are refcnt'd.  If you
-        * have a page, you need to kref_get it before you *use* it. */
-       page_setref(*page, 0);
+       page_setref(*page, 1);
        return 0;
 }
 
 /**
  * @brief Allocates a physical page from a pool of unused physical memory.
+ * Note, the page IS reference counted.
  *
  * Zeroes the page.
  *
@@ -141,6 +138,7 @@ error_t upage_alloc(struct proc* p, page_t** page, int zero)
        return ret;
 }
 
+/* Allocates a refcounted page of memory for the kernel's use */
 error_t kpage_alloc(page_t** page) 
 {
        ssize_t ret;
@@ -151,8 +149,6 @@ error_t kpage_alloc(page_t** page)
 
        if (ret >= 0) {
                global_next_color = ret;        
-               /* this is the "it's okay if it was 0" kref */
-               __kref_get(&(*page)->pg_kref, 1);
                ret = ESUCCESS;
        }
        spin_unlock_irqsave(&colored_page_free_list_lock);
@@ -198,7 +194,6 @@ void *get_cont_pages(size_t order, int flags)
        for(int i=0; i<npages; i++) {
                page_t* page;
                __page_alloc_specific(&page, first+i);
-               __kref_get(&page->pg_kref, 1);
        }
        spin_unlock_irqsave(&colored_page_free_list_lock);
        return ppn2kva(first);
@@ -241,7 +236,6 @@ error_t kpage_alloc_specific(page_t** page, size_t ppn)
 {
        spin_lock_irqsave(&colored_page_free_list_lock);
        __page_alloc_specific(page, ppn);
-       page_incref(*page);
        spin_unlock_irqsave(&colored_page_free_list_lock);
        return 0;
 }
@@ -259,11 +253,6 @@ int page_is_free(size_t ppn) {
  */
 void page_incref(page_t *page)
 {
-       __page_incref(page);
-}
-
-void __page_incref(page_t *page)
-{
        kref_get(&page->pg_kref, 1);
 }
 
index fdebcc7..8aa72a4 100644 (file)
@@ -107,11 +107,8 @@ int page_insert(pde_t *pgdir, struct page *page, void *va, int perm)
        /* 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)
-        * Secondly, we need to use the __kref_get() since it is possible that pp
-        * has a refcnt of 0 - which is what happens when you get a fresh page from
-        * the free list (for now). */
-       __kref_get(&page->pg_kref, 1);
+        * 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. */
        if (!PAGE_UNMAPPED(*pte))
                page_remove(pgdir, va);
index 37ee3c8..a4f596f 100644 (file)
@@ -111,6 +111,7 @@ 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);
+                       page_decref(a_page[i]);
                }
                spin_unlock(&buster_lock);
        }
@@ -350,8 +351,8 @@ static ssize_t sys_fork(env_t* e)
                                page_decref(pp);
                                return -1;
                        }
-
                        pagecopy(page2kva(pp),ppn2kva(PTE2PPN(*pte)));
+                       page_decref(pp);
                } else {
                        assert(PAGE_PAGED_OUT(*pte));
                        /* TODO: (SWAP) will need to either make a copy or CoW/refcnt the