Remove page refcnts
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 17 Aug 2016 21:22:10 +0000 (17:22 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 29 Nov 2016 16:27:40 +0000 (11:27 -0500)
page_decref() is just page_free(), now.  I'll do a rename in a later
commit.  We still needed to track if it was free or not for the currently
lousy memory allocator.

There might be issues with this, but if you aren't willing to potentially
break compatibility with Linux, then you'll never get anywhere.

There are a few reasons to do reference counts.  Only one we still have is
for devices that want to pin user memory for operations.  Specifically, the
mlx4 OS-bypass stuff does this.  The problem is that the user allocs memory
and gives arbitrary addresses to the device.  Instead, we should have the
device own the memory and let the user mmap the memory.  That gets rid of
any issues with locking the page, since the memory is always 'safe.'

That model doesn't work with traditional scatter-gather.  Worst case, we
can come up with something where we lock the VMR, instead of the page.
Though I'd rather come up with more explicit block data transfer
interfaces.

Note that the mlx4 OS-bypass is extremely dangerous now.  I think it was
always leaking memory before, btw.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/arch/riscv/page_alloc.c
kern/arch/x86/page_alloc.c
kern/drivers/net/bnx2x/bnx2x_cmn.c
kern/drivers/net/udrvr/compat.c
kern/include/page_alloc.h
kern/src/page_alloc.c
scripts/spatch/linux/memory.cocci

index 111e977..bd997b0 100644 (file)
@@ -36,16 +36,12 @@ void page_alloc_init(struct multiboot_info *mbi)
        uintptr_t first_invalid_page = LA2PPN(boot_freelimit);
        assert(first_invalid_page == max_nr_pages);
 
-       // mark kernel pages as in-use
-       for (uintptr_t page = 0; page < first_free_page; page++)
-               page_setref(&pages[page], 1);
-
        // append other pages to the free lists
        for (uintptr_t page = first_free_page; page < first_invalid_page; page++)
        {
-               page_setref(&pages[page], 0);
                BSD_LIST_INSERT_HEAD(&lists[page & (num_colors-1)], &pages[page],
                                     pg_link);
+               &pages[page]->pg_is_free = TRUE;
        }
        nr_free_pages = first_invalid_page - first_free_page;
 
index 2729e49..747cb15 100644 (file)
@@ -32,8 +32,7 @@ static void track_free_page(struct page *page)
                                                                    llc_cache)],
                         page, pg_link);
        nr_free_pages++;
-       /* Page was previous marked as busy, need to set it free explicitly */
-       page_setref(page, 0);
+       page->pg_is_free = TRUE;
 }
 
 static struct page *pa64_to_page(uint64_t paddr)
@@ -93,17 +92,19 @@ static void parse_mboot_region(struct multiboot_mmap_entry *entry, void *data)
        }
 }
 
+/* Expect == 1 -> busy, 0 -> free */
 static void check_range(uint64_t start, uint64_t end, int expect)
 {
-       int ref;
+       int free;
+
        if (PGOFF(start))
                printk("Warning: check_range given unaligned addr 0x%016llx\n", start);
        for (uint64_t i = start; i < end; i += PGSIZE)  {
-               ref = kref_refcnt(&pa64_to_page(i)->pg_kref);
-               if (ref != expect) {
+               free = pa64_to_page(i)->pg_is_free ? 0 : 1;
+               if (free != expect) {
                        printk("Error: while checking range [0x%016llx, 0x%016llx), "
-                              "physaddr 0x%016llx refcnt was %d, expected %d\n", start,
-                              end, i, ref, expect);
+                              "physaddr 0x%016llx free was %d, expected %d\n", start,
+                              end, i, free, expect);
                        panic("");
                }
        }
@@ -185,7 +186,7 @@ static void account_for_pages(physaddr_t boot_freemem_paddr)
 
        printk("Warning: poor memory detection (qemu?).  May lose 1GB of RAM\n");
        for (physaddr_t i = 0; i < top_of_busy; i += PGSIZE)
-               assert(kref_refcnt(&pa64_to_page(i)->pg_kref) == 1);
+               assert(!pa64_to_page(i)->pg_is_free);
        for (physaddr_t i = top_of_busy; i < top_of_free_1; i += PGSIZE)
                track_free_page(pa64_to_page(i));
        /* If max_paddr is less than the start of our potential second free mem
@@ -197,7 +198,7 @@ static void account_for_pages(physaddr_t boot_freemem_paddr)
        if (max_paddr < start_of_free_2)
                return;
        for (physaddr_t i = top_of_free_1; i < start_of_free_2; i += PGSIZE)
-               assert(kref_refcnt(&pa64_to_page(i)->pg_kref) == 1);
+               assert(!pa64_to_page(i)->pg_is_free);
        for (physaddr_t i = start_of_free_2; i < max_paddr; i += PGSIZE)
                track_free_page(pa64_to_page(i));
 }
@@ -206,17 +207,13 @@ static void account_for_pages(physaddr_t boot_freemem_paddr)
 void page_alloc_init(struct multiboot_info *mbi)
 {
        page_alloc_bootstrap();
-       /* First, we need to initialize the pages array such that all memory is busy
-        * by default.
-        *
-        * To init the free list(s), each page that is already allocated/busy will
-        * remain increfed.  All other pages that were reported as 'free' will be
-        * added to a free list.  Their refcnts are set to 0.
+       /* First, all memory is busy / not free by default.
         *
         * To avoid a variety of headaches, any memory below 1MB is considered busy.
         * Likewise, everything in the kernel, up to _end is also busy.  And
         * everything we've already boot_alloc'd is busy.  These chunks of memory
-        * are reported as 'free' by multiboot.
+        * are reported as 'free' by multiboot.  All of this memory is below
+        * boot_freemem_paddr.  We don't treat anything below that as free.
         *
         * We'll also abort the mapping for any addresses over max_paddr, since
         * we'll never use them.  'pages' does not track them either.
@@ -229,8 +226,6 @@ void page_alloc_init(struct multiboot_info *mbi)
         * sections are jumbo-aligned. */
        physaddr_t boot_freemem_paddr = PADDR(ROUNDUP(boot_freemem, PGSIZE));
 
-       for (long i = 0; i < max_nr_pages; i++)
-               page_setref(&pages[i], 1);
        if (mboot_has_mmaps(mbi)) {
                mboot_foreach_mmap(mbi, parse_mboot_region, (void*)boot_freemem_paddr);
                /* Test the page alloc - if this gets slow, we can CONFIG it */
index 6b67b18..4ab1d1e 100644 (file)
@@ -614,6 +614,8 @@ panic("Not implemented");
                                int len = rem > gro_size ? gro_size : rem;
                                skb_fill_page_desc(skb, frag_id++,
                                                   old_rx_pg.page, offset, len);
+                               /* TODO: if this is pinning for I/O, we need to change to a
+                                * device-ownership / mmap model. */
                                if (offset)
                                        page_incref(old_rx_pg.page);
                                offset += len;
index f2cd930..9c258ef 100644 (file)
@@ -141,7 +141,10 @@ int get_user_page(struct proc *p, unsigned long uvastart, int write, int force,
                goto err1;
        }
 
-       page_incref(pp);
+       /* TODO (GUP): change the interface such that devices provide the memory and
+        * the user mmaps it, instead of trying to pin arbitrary user memory. */
+       warn_once("Extremely unsafe, unpinned memory mapped!  If your process dies, you might scribble on RAM!");
+
        plist[0] = pp;
        ret = 1;
 err1:
index 484f62d..65e8060 100644 (file)
@@ -41,7 +41,6 @@ typedef BSD_LIST_ENTRY(page) page_list_entry_t;
  * buffer page (in a page mapping) */
 struct page {
        BSD_LIST_ENTRY(page)            pg_link;        /* membership in various lists */
-       struct kref                                     pg_kref;
        atomic_t                                        pg_flags;
        struct page_map                         *pg_mapping; /* for debugging... */
        unsigned long                           pg_index;
@@ -49,7 +48,8 @@ struct page {
        void                                            *pg_private;    /* type depends on page usage */
        struct semaphore                        pg_sem;         /* for blocking on IO */
        uint64_t                                gpa;            /* physical address in guest */
-                                                               /* pg_private is overloaded. */
+
+       bool                                            pg_is_free;     /* TODO: will remove */
 };
 
 /******** Externally visible global variables ************/
@@ -73,9 +73,7 @@ void *get_cont_pages_node(int node, size_t order, int flags);
 void *get_cont_phys_pages_at(size_t order, physaddr_t at, int flags);
 void free_cont_pages(void *buf, size_t order);
 
-void page_incref(page_t *page);
 void page_decref(page_t *page);
-void page_setref(page_t *page, size_t val);
 
 int page_is_free(size_t ppn);
 void lock_page(struct page *page);
index 9609f8d..0c6ffb2 100644 (file)
@@ -47,8 +47,8 @@ void colored_page_alloc_init()
 static void __page_init(struct page *page)
 {
        memset(page, 0, sizeof(page_t));
-       page_setref(page, 1);
        sem_init(&page->pg_sem, 0);
+       page->pg_is_free = FALSE;
 }
 
 #define __PAGE_ALLOC_FROM_RANGE_GENERIC(page, base_color, range, predicate) \
@@ -324,23 +324,12 @@ error_t kpage_alloc_specific(page_t** page, size_t ppn)
 }
 
 /* Check if a page with the given physical page # is free. */
-int page_is_free(size_t ppn) {
-       page_t* page = ppn2page(ppn);
-       if (kref_refcnt(&page->pg_kref))
-               return FALSE;
-       return TRUE;
-}
-
-/*
- * Increment the reference count on a page
- */
-void page_incref(page_t *page)
+int page_is_free(size_t ppn)
 {
-       kref_get(&page->pg_kref, 1);
+       return ppn2page(ppn)->pg_is_free;
 }
 
-/* Decrement the reference count on a page, freeing it if there are no more
- * refs. */
+/* Frees the page */
 void page_decref(page_t *page)
 {
        spin_lock_irqsave(&colored_page_free_list_lock);
@@ -348,18 +337,9 @@ void page_decref(page_t *page)
        spin_unlock_irqsave(&colored_page_free_list_lock);
 }
 
-/* Decrement the reference count on a page, freeing it if there are no more
- * refs.  Don't call this without holding the lock already. */
+/* Frees the page.  Don't call this without holding the lock already. */
 static void __page_decref(page_t *page)
 {
-       kref_put(&page->pg_kref);
-}
-
-/* Kref release function. */
-static void page_release(struct kref *kref)
-{
-       struct page *page = container_of(kref, struct page, pg_kref);
-
        if (atomic_read(&page->pg_flags) & PG_BUFFER)
                free_bhs(page);
        /* Give our page back to the free list.  The protections for this are that
@@ -369,14 +349,7 @@ static void page_release(struct kref *kref)
           page,
           pg_link
        );
-}
-
-/* Helper when initializing a page - just to prevent the proliferation of
- * page_release references (and because this function is sitting around in the
- * code).  Sets the reference count on a page to a specific value, usually 1. */
-void page_setref(page_t *page, size_t val)
-{
-       kref_init(&page->pg_kref, page_release, val);
+       page->pg_is_free = TRUE;
 }
 
 /* Attempts to get a lock on the page for IO operations.  If it is already
@@ -405,9 +378,9 @@ void print_pageinfo(struct page *page)
                printk("Null page\n");
                return;
        }
-       printk("Page %d (%p), Flags: 0x%08x Refcnt: %d\n", page2ppn(page),
+       printk("Page %d (%p), Flags: 0x%08x Is Free: %d\n", page2ppn(page),
               page2kva(page), atomic_read(&page->pg_flags),
-              kref_refcnt(&page->pg_kref));
+              page->pg_is_free);
        if (page->pg_mapping) {
                printk("\tMapped into object %p at index %d\n",
                       page->pg_mapping->pm_host, page->pg_index);
index 6536d09..54808f4 100644 (file)
@@ -59,7 +59,7 @@ expression FLAGS;
 expression PG;
 @@
 -get_page(PG)
-+page_incref(PG)
++get_page_wont_compile_use_mmap(PG)
 
 @@
 expression PG;