mmap() merges, fixed 0-page allocation bug
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 8 Jul 2010 21:04:12 +0000 (14:04 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:48 +0000 (17:35 -0700)
do_mmap() will attempt to merge its new VMR, which will help a bit
(although it might not be the best way to do things).

There was also a bug with VMR requests for the 0th page, which is now
fixed.  You can create that vmr, but it's not recommended.  mmap() and
friends will check to make sure userspace doesn't affect the 0th page.
Also note that glibc did not like getting the 0 page, for whatever
reason.

kern/include/mm.h
kern/src/mm.c
kern/src/testing.c

index 34a9fa7..69da2ea 100644 (file)
@@ -50,6 +50,7 @@ void vmr_init(void);
 struct vm_region *create_vmr(struct proc *p, uintptr_t va, size_t len);
 struct vm_region *split_vmr(struct vm_region *vmr, uintptr_t va);
 int merge_vmr(struct vm_region *first, struct vm_region *second);
+struct vm_region *merge_me(struct vm_region *vmr);
 int grow_vmr(struct vm_region *vmr, uintptr_t va);
 int shrink_vmr(struct vm_region *vmr, uintptr_t va);
 void destroy_vmr(struct vm_region *vmr);
@@ -60,7 +61,9 @@ void duplicate_vmrs(struct proc *p, struct proc *new_p);
 void print_vmrs(struct proc *p);
 
 /* mmap() related functions.  These manipulate VMRs and change the hardware page
- * tables. */
+ * tables.  Any requests below the LOWEST_VA will silently be upped.  This may
+ * be a dynamic proc-specific variable later. */
+#define MMAP_LOWEST_VA 0x00001000
 void *mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
            int fd, size_t offset);
 void *do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
index ca25509..8734c38 100644 (file)
@@ -38,13 +38,9 @@ void vmr_init(void)
  * tree of some sort for easier lookups. */
 struct vm_region *create_vmr(struct proc *p, uintptr_t va, size_t len)
 {
-       struct vm_region *vmr = 0, *vm_i, *vm_link;
+       struct vm_region *vmr = 0, *vm_i, *vm_next;
        uintptr_t gap_end;
 
-       /* Don't allow a vm region into the 0'th page (null ptr issues) */
-       if (va == 0)
-               va = 1 * PGSIZE;
-
        assert(!PGOFF(va));
        assert(!PGOFF(len));
        assert(va + len <= UMAPTOP);
@@ -59,17 +55,17 @@ struct vm_region *create_vmr(struct proc *p, uintptr_t va, size_t len)
                TAILQ_INSERT_HEAD(&p->vm_regions, vmr, vm_link);
        } else {
                TAILQ_FOREACH(vm_i, &p->vm_regions, vm_link) {
-                       vm_link = TAILQ_NEXT(vm_i, vm_link);
-                       gap_end = vm_link ? vm_link->vm_base : UMAPTOP;
+                       vm_next = TAILQ_NEXT(vm_i, vm_link);
+                       gap_end = vm_next ? vm_next->vm_base : UMAPTOP;
                        /* skip til we get past the 'hint' va */
                        if (va >= gap_end)
                                continue;
-                       /* Found a gap that is big enough */
+                       /* Find a gap that is big enough */
                        if (gap_end - vm_i->vm_end >= len) {
                                vmr = kmem_cache_alloc(vmr_kcache, 0);
                                /* if we can put it at va, let's do that.  o/w, put it so it
                                 * fits */
-                               if (gap_end >= va + len)
+                               if ((gap_end >= va + len) && (va >= vm_i->vm_end))
                                        vmr->vm_base = va;
                                else
                                        vmr->vm_base = vm_i->vm_end;
@@ -137,6 +133,24 @@ int merge_vmr(struct vm_region *first, struct vm_region *second)
        return 0;
 }
 
+/* Attempts to merge vmr with adjacent VMRs, returning a ptr to be used for vmr.
+ * It could be the same struct vmr, or possibly another one (usually lower in
+ * the address space. */
+struct vm_region *merge_me(struct vm_region *vmr)
+{
+       struct vm_region *vmr_temp;
+       /* Merge will fail if it cannot do it.  If it succeeds, the second VMR is
+        * destroyed, so we need to be a bit careful. */
+       vmr_temp = TAILQ_PREV(vmr, vmr_tailq, vm_link);
+       if (vmr_temp)
+               if (!merge_vmr(vmr_temp, vmr))
+                       vmr = vmr_temp;
+       vmr_temp = TAILQ_NEXT(vmr, vm_link);
+       if (vmr_temp)
+               merge_vmr(vmr, vmr_temp);
+       return vmr;
+}
+
 /* Grows the vm region up to (and not including) va.  Fails if another is in the
  * way, etc. */
 int grow_vmr(struct vm_region *vmr, uintptr_t va)
@@ -244,7 +258,9 @@ void print_vmrs(struct proc *p)
        struct vm_region *vmr;
        printk("VM Regions for proc %d\n", p->pid);
        TAILQ_FOREACH(vmr, &p->vm_regions, vm_link)
-               printk("%02d: (0x%08x - 0x%08x)\n", count++, vmr->vm_base, vmr->vm_end);
+               printk("%02d: (0x%08x - 0x%08x): %08p, %08p, %08p\n", count++,
+                      vmr->vm_base, vmr->vm_end, vmr->vm_prot, vmr->vm_flags,
+                      vmr->vm_file);
 }
 
 
@@ -277,6 +293,7 @@ void *mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
                if (!file)
                        return MAP_FAILED;
        }
+       addr = MAX(addr, MMAP_LOWEST_VA);
        void *result = do_mmap(p, addr, len, prot, flags, file, offset);
        if (file)
                file_decref(file);
@@ -299,7 +316,7 @@ void *__do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
        len = ROUNDUP(len, PGSIZE);
        int num_pages = len / PGSIZE;
 
-       struct vm_region *vmr;
+       struct vm_region *vmr, *vmr_temp;
 
 #ifndef __CONFIG_DEMAND_PAGING__
        flags |= MAP_POPULATE;
@@ -318,19 +335,19 @@ void *__do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
        vmr->vm_flags = flags;
        vmr->vm_file = file;
        vmr->vm_foff = offset;
-       /* TODO: consider checking to see if we can merge vmrs */
-
+       addr = vmr->vm_base;            /* so we know which pages to populate later */
+       vmr = merge_me(vmr);            /* attempts to merge with neighbors */
        /* Fault in pages now if MAP_POPULATE - die on failure.  We want to populate
-        * the region requested, but we ought to be careful and only populate the
-        * requested length and not any merged regions.  doing this by page for now,
-        * though some form of a helper would be nice. */
+        * the region requested, but we need to be careful and only populate the
+        * requested length and not any merged regions, which is why we set addr
+        * above and use it here. */
        if (flags & MAP_POPULATE)
                for (int i = 0; i < num_pages; i++)
-                       if (__handle_page_fault(p, vmr->vm_base + i*PGSIZE, vmr->vm_prot)) {
+                       if (__handle_page_fault(p, addr + i*PGSIZE, vmr->vm_prot)) {
                                spin_unlock(&p->proc_lock);
                                proc_destroy(p);
                        }
-       return (void*SAFE)TC(vmr->vm_base);
+       return (void*SAFE)TC(addr);
 }
 
 int mprotect(struct proc *p, uintptr_t addr, size_t len, int prot)
@@ -338,7 +355,7 @@ int mprotect(struct proc *p, uintptr_t addr, size_t len, int prot)
        printd("mprotect(addr %x, len %x, prot %x)\n", addr, len, prot);
        if (!len)
                return 0;
-       if (addr % PGSIZE) {
+       if ((addr % PGSIZE) || (addr < MMAP_LOWEST_VA)) {
                set_errno(current_tf, EINVAL);
                return -1;
        }
@@ -402,7 +419,7 @@ int munmap(struct proc *p, uintptr_t addr, size_t len)
        printd("munmap(addr %x, len %x, prot %x)\n", addr, len, prot);
        if (!len)
                return 0;
-       if (addr % PGSIZE) {
+       if ((addr % PGSIZE) || (addr < MMAP_LOWEST_VA)) {
                set_errno(current_tf, EINVAL);
                return -1;
        }
index f26162b..1903168 100644 (file)
@@ -1022,13 +1022,13 @@ void test_vm_regions(void)
        shrink_vmr(vmrs[2], 0x9000);
        results[1].base = 0x8000;
        results[1].end = 0x9000;
-       check_vmrs(p, results, 2, n++);
+       check_vmrs(p, results, 2, n++); /* 10 */
        if (vmrs[2] != find_vmr(p, 0x8500))
-               printk("Failed to find the right vmr!");
+               printk("Failed to find the right vmr!\n");
        if (vmrs[2] != find_first_vmr(p, 0x8500))
-               printk("Failed to find the right vmr!");
+               printk("Failed to find the right vmr!\n");
        if (vmrs[2] != find_first_vmr(p, 0x7500))
-               printk("Failed to find the right vmr!");
+               printk("Failed to find the right vmr!\n");
        if (find_first_vmr(p, 0x9500))
                printk("Found a vmr when we shouldn't!\n");
        /* grow up to another */
@@ -1046,6 +1046,34 @@ void test_vm_regions(void)
        merge_vmr(vmrs[0], vmrs[2]);
        results[0].end = 0x9000;
        check_vmrs(p, results, 1, n++);
+       destroy_vmr(vmrs[0]);
+       check_vmrs(p, results, 0, n++);
+       /* Check the automerge function */
+       vmrs[0] = create_vmr(p, 0x2000, 0x1000);
+       vmrs[1] = create_vmr(p, 0x3000, 0x1000);
+       vmrs[2] = create_vmr(p, 0x4000, 0x1000);
+       for (int i = 0; i < 3; i++) {
+               vmrs[i]->vm_prot = PROT_READ;
+               vmrs[i]->vm_flags = 0;
+               vmrs[i]->vm_file = 0; /* would like to test this, it's a pain for now */
+       }
+       vmrs[0] = merge_me(vmrs[1]);
+       results[0].base = 0x2000;
+       results[0].end = 0x5000;
+       check_vmrs(p, results, 1, n++);
+       destroy_vmr(vmrs[0]);
+       check_vmrs(p, results, 0, n++);
+       /* Check unfixed creation requests */
+       vmrs[0] = create_vmr(p, 0x0000, 0x1000);
+       vmrs[1] = create_vmr(p, 0x0000, 0x1000);
+       vmrs[2] = create_vmr(p, 0x0000, 0x1000);
+       results[0].base = 0x0000;
+       results[0].end  = 0x1000;
+       results[1].base = 0x1000;
+       results[1].end  = 0x2000;
+       results[2].base = 0x2000;
+       results[2].end  = 0x3000;
+       check_vmrs(p, results, 3, n++);
 
        printk("Finished vm_regions test!\n");
 }