fix: synchronise destruction of page directories with frame table

src/threads/thread.c

@@ -736,10 +736,6 @@ init_thread (struct thread *t, const char *name, int nice, int priority,
   t->recent_cpu = recent_cpu;
   t->priority = t->base_priority;
 
-#ifdef VM
-  lock_init (&t->ptable_lock);
-#endif
-
   old_level = intr_disable ();
   list_push_back (&all_list, &t->allelem);
   intr_set_level (old_level);

src/threads/thread.h

@@ -137,9 +137,6 @@ struct thread
 
     struct hash pages;                  /* Table of open user pages. */
 
-    struct lock ptable_lock;           /* Protects access to the process's
-                                           page directory and SPT. */
-
     /* Memory mapped files for user virtual memory. */
     struct hash mmap_files;             /* List of memory mapped files. */
     unsigned int mmap_counter;          /* Counter for memory mapped files. */

src/userprog/exception.c

@@ -2,7 +2,6 @@
 #include <inttypes.h>
 #include <stdio.h>
 #include "stdbool.h"
-#include "threads/synch.h"
 #include "userprog/gdt.h"
 #include "threads/interrupt.h"
 #include "threads/thread.h"
@@ -169,9 +168,6 @@ page_fault (struct intr_frame *f)
    So we attempt to grow the stack. If this does not work, we check our SPT to
    see if the page is expected to have data loaded in memory. */
   void *upage = pg_round_down (fault_addr);
-
-  printf ("FATHER, I FAULT AT %p!\n", fault_addr);
-
   if (not_present && is_user_vaddr (upage) && upage != NULL)
     {
       if (fetch_page (upage, write))
@@ -254,21 +250,17 @@ grow_stack (void *upage)
 bool
 fetch_page (void *upage, bool write)
 {
-  struct thread *t = thread_current ();
   /* Check if the page is in the supplemental page table. That is, it is a page
      that is expected to be in memory. */
-  lock_acquire (&t->ptable_lock);
+  struct page_entry *page = page_get (thread_current (), upage);
-  struct page_entry *page = page_get (t, upage);
-  lock_release (&t->ptable_lock);
   if (page == NULL)
-    {
+    return false;
-      return false;
-    }
 
   /* Check if the non-present user page is in the swap partition.
    If so, swap it back into main memory, updating the PTE for
    the faulted virtual address to point to the newly allocated
    frame. */
+  struct thread *t = thread_current ();
   if (page_in_swap (t, upage))
     {
       /* NOTE: This code should be refactored and moved into helper functions
@@ -283,6 +275,9 @@ fetch_page (void *upage, bool write)
 
       bool writeable = pagedir_is_writable (t->pagedir, upage);
 
+      /* TODO: When this returns false we should quit the page fault,
+        but currently we continue and check the stack conditions in the
+        page fault handler. */
       return pagedir_set_page (t->pagedir, upage, kpage, writeable);
     }
 

src/userprog/pagedir.c

@@ -6,6 +6,7 @@
 #include "threads/init.h"
 #include "threads/pte.h"
 #include "threads/palloc.h"
+#include "threads/synch.h"
 #include "vm/frame.h"
 #include "vm/page.h"
 
@@ -35,6 +36,7 @@ pagedir_destroy (uint32_t *pd)
     return;
 
   ASSERT (pd != init_page_dir);
+  lock_acquire (&lru_lock);
   for (pde = pd; pde < pd + pd_no (PHYS_BASE); pde++)
     if (*pde & PTE_P) 
       {
@@ -53,6 +55,7 @@ pagedir_destroy (uint32_t *pd)
         palloc_free_page (pt);
       }
   palloc_free_page (pd);
+  lock_release (&lru_lock);
 }
 
 /* Returns the address of the page table entry for virtual

src/userprog/syscall.c

@@ -460,14 +460,10 @@ syscall_mmap (int fd, void *addr)
 
   /* Check and ensure that there is enough space in the user virtual memory to
      hold the entire file. */
-  lock_acquire (&thread_current ()->ptable_lock);
   for (off_t ofs = 0; ofs < file_size; ofs += PGSIZE)
     {
       if (page_get (thread_current (), addr + ofs) != NULL)
-        {
+        return MMAP_FAILURE;
-          lock_release (&thread_current ()->ptable_lock);
-          return MMAP_FAILURE;
-        }
     }
 
   /* Map the file data into the user virtual memory starting from addr. */
@@ -478,12 +474,8 @@ syscall_mmap (int fd, void *addr)
 
       if (page_insert_file (file, ofs, addr + ofs, read_bytes, zero_bytes, true,
                        PAGE_FILE) == NULL)
-        {
+        return MMAP_FAILURE;
-          lock_release (&thread_current ()->ptable_lock);
-          return MMAP_FAILURE;
-        }
     }
-  lock_release (&thread_current ()->ptable_lock);
 
   /* Create a new mapping for the file. */
   struct mmap_entry *mmap = mmap_insert (file, addr);

src/vm/frame.c

@@ -6,7 +6,6 @@
 #include "page.h"
 #include "filesys/file.h"
 #include "threads/malloc.h"
-#include "threads/thread.h"
 #include "threads/vaddr.h"
 #include "userprog/pagedir.h"
 #include "userprog/syscall.h"
@@ -29,9 +28,6 @@ struct list lru_list;
    victim. Otherwise, the next element in the queue is similarly considered. */
 struct list_elem *next_victim = NULL;
 
-/* Synchronisation variables. */
-/* Protects access to 'lru_list'. */
-struct lock lru_lock;
 
 struct frame_metadata
   {
@@ -99,7 +95,6 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
       ASSERT (victim != NULL); /* get_victim () should never return null. */
 
       /* 2. Handle victim page writing based on its type. */
-      lock_acquire (&owner->ptable_lock);
       struct page_entry *victim_page = page_get (thread_current (), victim->upage);
       if (victim_page != NULL && victim_page->type == PAGE_MMAP)
         {
@@ -111,7 +106,6 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
               file_write_at (victim_page->file, victim->upage,
                              victim_page->read_bytes, victim_page->offset);
               lock_release (&filesys_lock);
-              lock_release (&owner->ptable_lock);
             }
         }
       else
@@ -178,23 +172,29 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
 void
 frame_pin (void *frame)
 {
+  ASSERT (frame != NULL);
+  lock_acquire (&lru_lock);
   struct frame_metadata *frame_metadata = frame_metadata_get (frame);
   if (frame_metadata == NULL)
     PANIC ("Attempted to pin a frame at an unallocated kernel address '%p'\n",
            frame);
 
   frame_metadata->pinned = true;
+  lock_release (&lru_lock);
 }
-
+  
 void
 frame_unpin (void *frame)
 {
+  ASSERT (frame != NULL);
+  lock_acquire (&lru_lock);
   struct frame_metadata *frame_metadata = frame_metadata_get (frame);
   if (frame_metadata == NULL)
     PANIC ("Attempted to unpin a frame at an unallocated kernel address '%p'\n",
            frame);
 
   frame_metadata->pinned = false;
+  lock_release (&lru_lock);
 }
 
 /* Attempt to deallocate a frame for a user process by removing it from the
@@ -209,8 +209,13 @@ frame_free (void *frame)
              "but this address is not allocated!\n",
              frame);
 
+    bool lock_held = lock_held_by_current_thread (&lru_lock);
+
     free_owners (&frame_metadata->owners);
-    lock_acquire (&lru_lock);
+
+    if (!lock_held)
+      lock_acquire (&lru_lock);
+
     hash_delete (&frame_table, &frame_metadata->hash_elem);
     list_remove (&frame_metadata->list_elem);
 
@@ -224,7 +229,9 @@ frame_free (void *frame)
         else
           next_victim = lru_next (next_victim);
       }
-    lock_release (&lru_lock);
+
+    if (!lock_held)
+      lock_release (&lru_lock);
 
     free (frame_metadata);
     palloc_free_page (frame);
@@ -287,10 +294,9 @@ frame_metadata_find (void *frame)
   return hash_entry (e, struct frame_metadata, hash_elem);
 }
 
-/* Obtain the next frame that should be evicted following the clock (second
+/* TODO: Account for page aliases when checking accessed bit. */
-   chance) algorithm, ignoring pinned frames. A pre-condition for calling this 
+/* A pre-condition for calling this function is that the calling thread
-   function is that the calling thread owns lru_lock and that lru_list is 
+   owns lru_lock and that lru_list is non-empty. */
-   non-empty. */
 static struct frame_metadata *
 get_victim (void)
   {
@@ -300,7 +306,6 @@ get_victim (void)
     while (!found)
       {
         frame_metadata = list_entry (ve, struct frame_metadata, list_elem);
-
         ve = lru_next (ve);
         struct list_elem *oe;
 
@@ -315,10 +320,6 @@ get_victim (void)
           {
             struct frame_owner *frame_owner
                 = list_entry (oe, struct frame_owner, elem);
-
-            lock_acquire (&frame_owner->owner->ptable_lock);
-            /* TODO: Account for death of frame_owner here! */
-
             uint32_t *pd = frame_owner->owner->pagedir;
             void *upage = frame_metadata->upage;
 
@@ -327,8 +328,6 @@ get_victim (void)
                 found = false;
                 pagedir_set_accessed (pd, upage, false);
               }
-
-            lock_release (&frame_owner->owner->ptable_lock);
           }
       }
 
@@ -383,7 +382,6 @@ frame_metadata_get (void *frame)
 
   struct hash_elem *e = hash_find (&frame_table, &key_metadata.hash_elem);
   if (e == NULL) return NULL;
-
   return hash_entry (e, struct frame_metadata, hash_elem);
 }
 

src/vm/frame.h

@@ -10,7 +10,11 @@ struct frame_owner
   struct list_elem elem; /* List element for the list of owners. */
 };
 
+/* Synchronisation variables. */
+/* Protects access to 'lru_list'. */
+struct lock lru_lock;
 void frame_init (void);
+
 void *frame_alloc (enum palloc_flags, void *, struct thread *);
 void frame_pin (void *frame);
 void frame_unpin (void *frame);

src/vm/mmap.c

@@ -66,7 +66,6 @@ mmap_unmap (struct mmap_entry *mmap)
   /* Free all the pages associated with the mapping, writing back to the file
      if necessary. */
   off_t length = file_length (mmap->file);
-  lock_acquire (&thread_current ()->ptable_lock);
   for (off_t ofs = 0; ofs < length; ofs += PGSIZE)
   {
     void *upage = mmap->upage + ofs;
@@ -87,7 +86,6 @@ mmap_unmap (struct mmap_entry *mmap)
     /* Remove the page from the supplemental page table. */
     hash_delete (&thread_current ()->pages, &page->elem);
   }
-  lock_release (&thread_current ()->ptable_lock);
 
   file_close (mmap->file);
   free (mmap);

src/vm/page.c

@@ -8,7 +8,6 @@
 #include "threads/palloc.h"
 #include "threads/synch.h"
 #include "devices/swap.h"
-#include "threads/thread.h"
 #include "userprog/process.h"
 #include "userprog/pagedir.h"
 #include "vm/frame.h"
@@ -78,11 +77,6 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners)
   for (e = list_begin (owners); e != list_end (owners); e = list_next (e))
     {
       struct thread *owner = list_entry (e, struct frame_owner, elem)->owner;
-      if (!lock_held_by_current_thread (&owner->ptable_lock))
-        {
-          lock_acquire (&owner->ptable_lock);
-        }
-
       uint32_t *pte = lookup_page (owner->pagedir, upage, false);
       if (exec_file != NULL || page_is_shared_pte (pte))
         {
@@ -90,8 +84,6 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners)
           pagedir_clear_page (owner->pagedir, upage);
           exec_file = owner->exec_file;
           ASSERT (exec_file != NULL);
-
-          lock_release (&owner->ptable_lock);
           continue;
         }
       ASSERT (list_size (owners) == 1);
@@ -107,7 +99,6 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners)
           lock_init (&page->lock);
           hash_insert (&owner->pages, &page->elem);
         }
-      lock_release (&owner->ptable_lock);
 
       /* Mark page as 'swapped' and flag the page directory as having
         been modified *before* eviction begins to prevent the owner of the
@@ -141,24 +132,15 @@ page_insert_file (struct file *file, off_t ofs, void *upage,
                   uint32_t read_bytes, uint32_t zero_bytes, bool writable,
                   enum page_type type)
 {
-  bool ptlock_held =
-    lock_held_by_current_thread(&thread_current ()->ptable_lock);
-
   /* If page exists, just update it. */
-  if (!ptlock_held)
-    lock_acquire (&thread_current ()->ptable_lock);
-
   struct page_entry *existing = page_get (thread_current (), upage);
-
   if (existing != NULL)
     {
       ASSERT (existing->read_bytes == read_bytes);
       ASSERT (existing->zero_bytes == zero_bytes);
       existing->writable = existing->writable || writable;
-      lock_release (&thread_current ()->ptable_lock);
       return existing;
     }
-  lock_release (&thread_current ()->ptable_lock);
 
   struct page_entry *page = malloc(sizeof (struct page_entry));
   if (page == NULL)
@@ -173,36 +155,24 @@ page_insert_file (struct file *file, off_t ofs, void *upage,
   page->writable = writable;
   lock_init (&page->lock);
 
-  lock_acquire (&thread_current ()->ptable_lock);
+  hash_insert (&thread_current ()->pages, &page->elem);
-  hash_insert  (&thread_current ()->pages, &page->elem);
-  
-  if (!ptlock_held)
-    lock_release (&thread_current ()->ptable_lock);
-
   return page;
 }
 
 /* Gets a page_entry from the starting address of the page. Returns NULL if no
-   such page_entry exists in the hash map. Must only be called on a thread
+   such page_entry exists in the hash map.*/
-   whose ptable_lock you own. */
 struct page_entry *
 page_get (struct thread *thread, void *upage)
 {
-  ASSERT (lock_held_by_current_thread (&thread->ptable_lock));
-
   struct page_entry fake_page_entry;
   fake_page_entry.upage = upage;
 
   struct hash_elem *e
     = hash_find (&thread->pages, &fake_page_entry.elem);
   if (e == NULL)
-    {
       return NULL;
-    }
 
-  struct page_entry *pe = hash_entry (e, struct page_entry, elem);
+  return hash_entry (e, struct page_entry, elem);
-
-  return pe;
 }
 
 bool
@@ -213,7 +183,6 @@ page_load_file (struct page_entry *page)
      panics as this should not happen if eviction is working correctly. */
   struct thread *t = thread_current ();
   bool shareable = !page->writable && file_compare (page->file, t->exec_file);
-  shareable = false;
   if (shareable)
     {
       lock_acquire (&shared_file_pages_lock);