fix: implement ptable_lock for synchronising the SPT structure for insertions/accesses

src/threads/thread.c

@@ -736,6 +736,10 @@ 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,6 +137,9 @@ 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,6 +2,7 @@
 #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"
@@ -168,6 +169,9 @@ 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))
@@ -250,17 +254,21 @@ 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. */
-  struct page_entry *page = page_get (thread_current (), upage);
+  lock_acquire (&t->ptable_lock);
+  struct page_entry *page = page_get (t, upage);
+  lock_release (&t->ptable_lock);
   if (page == NULL)
+    {
       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
@@ -275,9 +283,6 @@ 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/syscall.c

@@ -460,11 +460,15 @@ 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)
+        {
+          lock_release (&thread_current ()->ptable_lock);
           return MMAP_FAILURE;
         }
+    }
 
   /* Map the file data into the user virtual memory starting from addr. */
   for (off_t ofs = 0; ofs < file_size; ofs += PGSIZE)
@@ -474,8 +478,12 @@ syscall_mmap (int fd, void *addr)
 
       if (page_insert_file (file, ofs, addr + ofs, read_bytes, zero_bytes, true,
                        PAGE_FILE) == NULL)
+        {
+          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,6 +6,7 @@
 #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"
@@ -98,6 +99,7 @@ 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)
         {
@@ -109,6 +111,7 @@ 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
@@ -284,9 +287,10 @@ frame_metadata_find (void *frame)
   return hash_entry (e, struct frame_metadata, hash_elem);
 }
 
-/* TODO: Account for page aliases when checking accessed bit. */
-/* A pre-condition for calling this function is that the calling thread
-   owns lru_lock and that lru_list is non-empty. */
+/* Obtain the next frame that should be evicted following the clock (second
+   chance) algorithm, ignoring pinned frames. A pre-condition for calling this 
+   function is that the calling thread owns lru_lock and that lru_list is 
+   non-empty. */
 static struct frame_metadata *
 get_victim (void)
   {
@@ -296,6 +300,7 @@ get_victim (void)
     while (!found)
       {
         frame_metadata = list_entry (ve, struct frame_metadata, list_elem);
+
         ve = lru_next (ve);
         struct list_elem *oe;
 
@@ -310,6 +315,10 @@ 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;
 
@@ -318,6 +327,8 @@ get_victim (void)
                 found = false;
                 pagedir_set_accessed (pd, upage, false);
               }
+
+            lock_release (&frame_owner->owner->ptable_lock);
           }
       }
 

src/vm/mmap.c

@@ -66,6 +66,7 @@ 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;
@@ -86,6 +87,7 @@ 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,6 +8,7 @@
 #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"
@@ -77,6 +78,11 @@ 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))
         {
@@ -84,6 +90,8 @@ 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);
@@ -99,6 +107,7 @@ 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
@@ -132,15 +141,24 @@ 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)
@@ -155,24 +173,36 @@ 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);
+  
+  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.*/
+   such page_entry exists in the hash map. Must only be called on a thread
+   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;
+    }
 
-  return hash_entry (e, struct page_entry, elem);
+  struct page_entry *pe = hash_entry (e, struct page_entry, elem);
+
+  return pe;
 }
 
 bool