fix: synchronise pagedir which now may be accessed by other threads, with a lock

fix: re-enable shareable read only executable logic

See merge request lab2425_autumn/pintos_22!65

.gitlab-ci.yml

@@ -38,4 +38,3 @@ test_vm:
   extends: .pintos_tests
   variables:
     DIR: vm
-    IGNORE: (tests/vm/pt-grow-stack|tests/vm/pt-grow-pusha|tests/vm/pt-big-stk-obj|tests/vm/pt-overflowstk|tests/vm/pt-write-code2|tests/vm/pt-grow-stk-sc|tests/vm/page-linear|tests/vm/page-parallel|tests/vm/page-merge-seq|tests/vm/page-merge-par|tests/vm/page-merge-stk|tests/vm/page-merge-mm|tests/vm/mmap-over-stk)

src/threads/thread.c

@@ -736,9 +736,8 @@ 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
+  lock_init (&t->pages_lock);
+  lock_init (&t->pagedir_lock);
 
   old_level = intr_disable ();
   list_push_back (&all_list, &t->allelem);

src/threads/thread.h

@@ -136,9 +136,7 @@ struct thread
     struct list_elem elem;              /* List element. */
 
     struct hash pages;                  /* Table of open user pages. */
-
-    struct lock ptable_lock;           /* Protects access to the process's
-                                           page directory and SPT. */
+    struct lock pages_lock; /* Lock for the supplementary page table. */
 
     /* Memory mapped files for user virtual memory. */
     struct hash mmap_files;             /* List of memory mapped files. */
@@ -147,6 +145,7 @@ struct thread
 #ifdef USERPROG
     /* Owned by userprog/process.c. */
     uint32_t *pagedir;                  /* Page directory. */
+    struct lock pagedir_lock;           /* Lock for the page directory. */
     unsigned int fd_counter;            /* File descriptor counter for thread's
                                            open files. */
     struct hash open_files;             /* Hash Table of FD -> Struct File. */

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))
@@ -237,12 +233,19 @@ static bool
 grow_stack (void *upage)
 {
   /* Allocate new page for stack */
+  struct thread *t = thread_current ();
+  lock_acquire (&t->pagedir_lock);
   void *new_page = frame_alloc (PAL_ZERO, upage, thread_current ());
   if (new_page == NULL)
-    return false;
+    {
+      lock_release (&t->pagedir_lock);
+      return false;
+    }
 
   /* Install the page into user page table */
-  if (!pagedir_set_page (thread_current ()->pagedir, upage, new_page, true))
+  bool result = pagedir_set_page (t->pagedir, upage, new_page, true);
+  lock_release (&t->pagedir_lock);
+  if (!result)
     {
       frame_free (new_page);
       return false;
@@ -254,21 +257,18 @@ 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 (t, upage);
-  lock_release (&t->ptable_lock);
+  struct page_entry *page = page_get (thread_current (), upage);
   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 ();
+  lock_acquire (&t->pagedir_lock);
   if (page_in_swap (t, upage))
     {
       /* NOTE: This code should be refactored and moved into helper functions
@@ -282,9 +282,11 @@ fetch_page (void *upage, bool write)
       lock_release (&page->lock);
 
       bool writeable = pagedir_is_writable (t->pagedir, upage);
-
-      return pagedir_set_page (t->pagedir, upage, kpage, writeable);
+      bool result = pagedir_set_page (t->pagedir, upage, kpage, writeable);
+      lock_release (&t->pagedir_lock);
+      return result;
     }
+  lock_release (&t->pagedir_lock);
 
   /* An attempt to write to a non-writeable should fail. */
   if (write && !page->writable)
@@ -294,7 +296,7 @@ fetch_page (void *upage, bool write)
   bool success = false;
   switch (page->type) {
     case PAGE_MMAP:
-    case PAGE_FILE:
+    case PAGE_EXECUTABLE:
     case PAGE_SHARED:
       success = page_load_file (page);
       break;
@@ -302,9 +304,5 @@ fetch_page (void *upage, bool write)
       return false;
   }
 
-  if (success && page->writable &&
-      !pagedir_is_writable(thread_current()->pagedir, upage))
-    pagedir_set_writable(thread_current()->pagedir, upage, true);
-
   return success;
 }

src/userprog/process.c

@@ -371,7 +371,9 @@ process_exit (void)
   hash_destroy (&cur->open_files, fd_cleanup);
 
   /* Clean up the thread's supplemental page table. */
+  lock_acquire (&cur->pages_lock);
   hash_destroy (&cur->pages, page_cleanup);
+  lock_release (&cur->pages_lock);
 
   /* Close the executable file, implicitly allowing it to be written to. */
   if (cur->exec_file != NULL)
@@ -394,6 +396,7 @@ process_exit (void)
 
   /* Destroy the current process's page directory and switch back
      to the kernel-only page directory. */
+  lock_acquire (&cur->pagedir_lock);
   pd = cur->pagedir;
   if (pd != NULL) 
     {
@@ -408,6 +411,7 @@ process_exit (void)
       pagedir_activate (NULL);
       pagedir_destroy (pd);
     }
+  lock_release (&cur->pagedir_lock);
 }
 
 /* Destruct a process_result, with multi-thread awareness.
@@ -533,7 +537,9 @@ load (const char *file_name, void (**eip) (void), void **esp)
   lock_acquire (&filesys_lock);
 
   /* Allocate and activate page directory. */
+  lock_acquire (&t->pagedir_lock);
   t->pagedir = pagedir_create ();
+  lock_release (&t->pagedir_lock);
   if (t->pagedir == NULL) 
     goto done;
   process_activate ();
@@ -715,7 +721,7 @@ load_segment (struct file *file, off_t ofs, uint8_t *upage,
 
       /* Add the page metadata to the SPT to be lazy loaded later on */
       if (page_insert_file (file, ofs, upage, page_read_bytes, page_zero_bytes,
-                            writable, PAGE_FILE) == NULL)
+                            writable, PAGE_EXECUTABLE) == NULL)
         return false;
 
       /* Advance. */
@@ -759,11 +765,16 @@ get_usr_kpage (enum palloc_flags flags, void *upage)
   void *page;
 #ifdef VM
   struct thread *t = thread_current ();
+  lock_acquire (&t->pagedir_lock);
   if (pagedir_get_page (t->pagedir, upage) != NULL)
-    return NULL;
+    {
+      lock_release (&t->pagedir_lock);
+      return NULL;
+    }
   else
     page = frame_alloc (flags, upage, t);
   pagedir_set_accessed (t->pagedir, upage, true);
+  lock_release (&t->pagedir_lock);
 #else
   page = palloc_get_page (flags | PAL_USER);
 #endif

src/userprog/syscall.c

@@ -460,15 +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)
-        {
-          lock_release (&thread_current ()->ptable_lock);
-          return MMAP_FAILURE;
-        }
-    }
+        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)
@@ -477,20 +472,15 @@ syscall_mmap (int fd, void *addr)
       off_t zero_bytes = PGSIZE - read_bytes;
 
       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;
-        }
+                            PAGE_MMAP) == NULL)
+        return MMAP_FAILURE;
     }
-  lock_release (&thread_current ()->ptable_lock);
 
   /* Create a new mapping for the file. */
   struct mmap_entry *mmap = mmap_insert (file, addr);
   if (mmap == NULL)
       return MMAP_FAILURE;
 
-
   return mmap->mapping;
 }
 
@@ -585,6 +575,7 @@ validate_user_ptr_helper (const void *start, size_t size, bool write, bool pin)
   if (!is_user_vaddr (end))
     syscall_exit (EXIT_FAILURE);
 
+  struct thread *t = thread_current ();
   for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE)
   {
     int result;
@@ -600,7 +591,9 @@ validate_user_ptr_helper (const void *start, size_t size, bool write, bool pin)
     /* If pin is set, pin the frame to prevent eviction. */
     if (pin)
     {
-      void *kpage = pagedir_get_page(thread_current()->pagedir, ptr);
+      lock_acquire (&t->pagedir_lock);
+      void *kpage = pagedir_get_page (t->pagedir, ptr);
+      lock_release (&t->pagedir_lock);
       if (kpage == NULL)
       {
         // If it was evicted, try to load it back in.
@@ -650,18 +643,21 @@ validate_and_pin_user_ptr (const void *start, size_t size, bool write)
 static void
 unpin_user_ptr (const void *start, size_t size)
 {
+  struct thread *t = thread_current ();
   void *end = start + size - 1;
 
   /* We don't need to do any checks as this function is always called after
      validate_and_pin_user_ptr. */
   /* Go through all pages in the block range, unpinning the frames. */
+  lock_acquire (&t->pagedir_lock);
   for (void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE)
   {
-    void *kpage = pagedir_get_page (thread_current ()->pagedir, ptr);
+    void *kpage = pagedir_get_page (t->pagedir, ptr);
     ASSERT (kpage != NULL);
 
     frame_unpin (kpage);
   }
+  lock_release (&t->pagedir_lock);
 }
 
 /* Validates of a C-string starting at ptr is fully contained within valid
@@ -669,6 +665,7 @@ unpin_user_ptr (const void *start, size_t size)
 static void
 validate_and_pin_user_str (const char *ptr)
 {
+  struct thread *t = thread_current ();
   size_t offset = (uintptr_t) ptr % PGSIZE;
 
   for (;;)
@@ -681,7 +678,9 @@ validate_and_pin_user_str (const char *ptr)
 
       /* Pin the frame to prevent eviction. */
       void *page = pg_round_down (ptr);
-      void *kpage = pagedir_get_page (thread_current ()->pagedir, page);
+      lock_acquire (&t->pagedir_lock);
+      void *kpage = pagedir_get_page (t->pagedir, page);
+      lock_release (&t->pagedir_lock);
       if (kpage == NULL)
         {
           // If it was evicted, attempt to reload.
@@ -711,13 +710,15 @@ validate_and_pin_user_str (const char *ptr)
 static void
 unpin_user_str (const char *ptr)
 {
+  struct thread *t = thread_current ();
   size_t offset = (uintptr_t)ptr % PGSIZE;
   const char *str_ptr = ptr;
 
+  lock_acquire (&t->pagedir_lock);
   for (;;)
   {
     void *page = pg_round_down(str_ptr);
-    void *kpage = pagedir_get_page(thread_current()->pagedir, page);
+    void *kpage = pagedir_get_page (t->pagedir, page);
     ASSERT(kpage != NULL);
     frame_unpin (kpage);
 
@@ -725,7 +726,11 @@ unpin_user_str (const char *ptr)
     while (offset < PGSIZE)
     {
       if (*str_ptr == '\0')
-        return;  /* Found end of string */
+        {
+          /* Found end of string */
+          lock_release (&t->pagedir_lock);
+          return;
+        }
       str_ptr++;
       offset++;
     }

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
   {
@@ -44,8 +40,8 @@ struct frame_metadata
                                      within 'frame_table', whose key is the 
                                      kernel virtual address of the frame. */
      struct list_elem list_elem;  /* Tracks the position of the frame metadata
-                                     in either the 'active' or 'inactive' list,
-                                     so a victim can be chosen for eviction. */
+                                     within 'lru_list', so a victim can be
+                                     chosen for eviction. */
   };
 
 hash_hash_func frame_metadata_hash;
@@ -54,7 +50,7 @@ hash_less_func frame_metadata_less;
 static struct list_elem *lru_next (struct list_elem *e);
 static struct list_elem *lru_prev (struct list_elem *e);
 static struct frame_metadata *frame_metadata_get (void *frame);
-static struct frame_metadata *get_victim (void);
+static struct frame_metadata *get_victim (struct thread *cur);
 static void free_owners (struct list *owners);
 static struct frame_metadata *frame_metadata_find (void *frame);
 
@@ -67,7 +63,7 @@ frame_init (void)
   hash_init (&frame_table, frame_metadata_hash, frame_metadata_less, NULL);
 
   list_init (&lru_list);
-  lock_init (&lru_lock);
+  lock_init (&ftable_lock);
 }
 
 /* TODO: Consider synchronisation more closely (i.e. just for hash
@@ -83,7 +79,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
   struct frame_metadata *frame_metadata;
   flags |= PAL_USER;
 
-  lock_acquire (&lru_lock);
+  lock_acquire (&ftable_lock);
   void *frame = palloc_get_page (flags);
 
   /* If a frame couldn't be allocated we must be out of main memory. Thus,
@@ -95,11 +91,10 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
       if (next_victim == NULL)
         PANIC ("Couldn't allocate a single page to main memory!\n");
 
-      struct frame_metadata *victim = get_victim ();
+      struct frame_metadata *victim = get_victim (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,13 +106,12 @@ 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
         {
           /* Otherwise, insert the page into swap. */
-          page_insert_swapped (victim->upage, victim->frame, &victim->owners);
+          page_insert_swapped (victim->upage, victim->frame, &victim->owners, owner);
         }
 
       /* Free victim's owners. */
@@ -128,7 +122,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
         memset (victim->frame, 0, PGSIZE);
 
       /* 3. Indicate that the new frame's metadata will be stored
-            inside the same structure that stored the victim's metadata.
+            inside the same structure that stored the victim's metadata.frame.c
             As both the new frame and the victim frame share the same kernel
             virtual address, the hash map need not be updated, and neither
             the list_elem value as both share the same lru_list position. */
@@ -139,7 +133,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
      we must update the frame table with a new entry, and grow lru_list. */
   else
     {
-      /* Must own lru_lock here, as otherwise there is a race condition
+      /* Must own ftable_lock here, as otherwise there is a race condition
          with next_victim either being NULL or uninitialized. */
       frame_metadata = malloc (sizeof (struct frame_metadata));
       if (frame_metadata == NULL)
@@ -171,30 +165,36 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
   list_push_back (&frame_metadata->owners, &frame_owner->elem);
   frame_metadata->upage = upage;
   frame_metadata->pinned = false;
-  lock_release (&lru_lock);
+  lock_release (&ftable_lock);
   return frame_metadata->frame;
 }
 
 void
 frame_pin (void *frame)
 {
+  ASSERT (frame != NULL);
+  lock_acquire (&ftable_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 (&ftable_lock);
 }
-
+  
 void
 frame_unpin (void *frame)
 {
+  ASSERT (frame != NULL);
+  lock_acquire (&ftable_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 (&ftable_lock);
 }
 
 /* Attempt to deallocate a frame for a user process by removing it from the
@@ -210,7 +210,7 @@ frame_free (void *frame)
              frame);
 
     free_owners (&frame_metadata->owners);
-    lock_acquire (&lru_lock);
+    lock_acquire (&ftable_lock);
     hash_delete (&frame_table, &frame_metadata->hash_elem);
     list_remove (&frame_metadata->list_elem);
 
@@ -224,7 +224,7 @@ frame_free (void *frame)
         else
           next_victim = lru_next (next_victim);
       }
-    lock_release (&lru_lock);
+    lock_release (&ftable_lock);
 
     free (frame_metadata);
     palloc_free_page (frame);
@@ -287,12 +287,10 @@ 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
-   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. */
+/* A pre-condition for calling this function is that the calling thread
+   owns ftable_lock and that lru_list is non-empty. */
 static struct frame_metadata *
-get_victim (void)
+get_victim (struct thread *cur)
   {
     struct list_elem *ve = next_victim;
     struct frame_metadata *frame_metadata;
@@ -300,7 +298,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 +312,8 @@ 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! */
-
+            if (frame_owner->owner != cur)
+              lock_acquire (&frame_owner->owner->pagedir_lock);
             uint32_t *pd = frame_owner->owner->pagedir;
             void *upage = frame_metadata->upage;
 
@@ -327,8 +322,8 @@ get_victim (void)
                 found = false;
                 pagedir_set_accessed (pd, upage, false);
               }
-
-            lock_release (&frame_owner->owner->ptable_lock);
+            if (frame_owner->owner != cur)
+              lock_release (&frame_owner->owner->pagedir_lock);
           }
       }
 
@@ -383,7 +378,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,12 @@ struct frame_owner
   struct list_elem elem; /* List element for the list of owners. */
 };
 
+/* Synchronisation variables. */
+/* Protects access to the frame table and its related components. */
+struct lock ftable_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"
@@ -71,18 +70,16 @@ static void page_set_swap (struct thread *owner, uint32_t *pte,
    insert a new page entry into the user process thread's SPT representing
    this swapped out page. */
 bool
-page_insert_swapped (void *upage, void *kpage, struct list *owners)
+page_insert_swapped (void *upage, void *kpage, struct list *owners,
+                     struct thread *cur)
 {
   struct file *exec_file = NULL;
   struct list_elem *e;
   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);
-        }
-
+      if (owner != cur)
+        lock_acquire (&owner->pagedir_lock);
       uint32_t *pte = lookup_page (owner->pagedir, upage, false);
       if (exec_file != NULL || page_is_shared_pte (pte))
         {
@@ -90,24 +87,28 @@ 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);
+          if (owner != cur)
+            lock_release (&owner->pagedir_lock);
           continue;
         }
       ASSERT (list_size (owners) == 1);
 
       /* 1. Initialize swapped page entry. */
       struct page_entry *page = page_get (owner, upage);
+      lock_acquire (&owner->pages_lock);
       if (page == NULL)
         {
           page = malloc (sizeof (struct page_entry));
           if (page == NULL)
-            return NULL;
+            {
+              if (owner != cur)
+                lock_release (&owner->pagedir_lock);
+              return false;
+            }
           page->upage = upage;
           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
@@ -122,6 +123,9 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners)
       page_set_swap (owner, pte, swap_slot);
 
       lock_release (&page->lock);
+      lock_release (&owner->pages_lock);
+      if (owner != cur)
+        lock_release (&owner->pagedir_lock);
     }
   if (exec_file != NULL)
     {
@@ -141,24 +145,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 +168,29 @@ 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);
-
+  struct thread *t = thread_current ();
+  lock_acquire (&t->pages_lock);
+  hash_insert (&t->pages, &page->elem);
+  lock_release (&t->pages_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
-   whose ptable_lock you own. */
+   such page_entry exists in the hash map.*/
 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;
 
+  lock_acquire (&thread->pages_lock);
   struct hash_elem *e
     = hash_find (&thread->pages, &fake_page_entry.elem);
+  lock_release (&thread->pages_lock);
   if (e == NULL)
-    {
       return NULL;
-    }
 
-  struct page_entry *pe = hash_entry (e, struct page_entry, elem);
-
-  return pe;
+  return hash_entry (e, struct page_entry, elem);
 }
 
 bool
@@ -213,7 +201,7 @@ 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;
+  lock_acquire (&t->pagedir_lock);
   if (shareable)
     {
       lock_acquire (&shared_file_pages_lock);
@@ -227,27 +215,24 @@ page_load_file (struct page_entry *page)
               if (!install_page (page->upage, sfp->frame, page->writable))
                 {
                   lock_release (&shared_file_pages_lock);
+                  lock_release (&t->pagedir_lock);
                   return false;
                 }
-              /* First time adding the shared page, so add thread as owner. */
-              if (page->type != PAGE_SHARED)
-                {
-                  frame_owner_insert (sfp->frame, t);
-                }
+              frame_owner_insert (sfp->frame, t);
             }
             /* Shared page is in swap. Load it. */
             else
               {
                 void *frame = frame_alloc (PAL_USER, page->upage, t);
                 if (frame == NULL)
-                  PANIC (
-                      "Could not allocate a frame to load page into memory.");
+                  PANIC ("Could not allocate a frame to load page into memory.");
                 swap_in (frame, sfp->swap_slot);
 
                 if (!install_page (page->upage, frame, false))
                   {
                     frame_free (frame);
                     lock_release (&shared_file_pages_lock);
+                    lock_release (&t->pagedir_lock);
                     return false;
                   }
               }
@@ -258,6 +243,7 @@ page_load_file (struct page_entry *page)
                 page->type = PAGE_SHARED;
               }
             lock_release (&shared_file_pages_lock);
+            lock_release (&t->pagedir_lock);
             return true;
         }
     }
@@ -273,6 +259,7 @@ page_load_file (struct page_entry *page)
       if (shareable)
         lock_release (&shared_file_pages_lock);
       frame_free (frame);
+      lock_release (&t->pagedir_lock);
       return false;
     }
 
@@ -284,6 +271,7 @@ page_load_file (struct page_entry *page)
   {
     if (shareable)
       lock_release (&shared_file_pages_lock);
+    lock_release (&t->pagedir_lock);
     frame_free (frame);
     return false;
   }
@@ -298,6 +286,7 @@ page_load_file (struct page_entry *page)
       if (sfp == NULL)
         {
           lock_release (&shared_file_pages_lock);
+          lock_release (&t->pagedir_lock);
           frame_free (frame);
           return false;
         }
@@ -312,6 +301,7 @@ page_load_file (struct page_entry *page)
       lock_release (&shared_file_pages_lock);
     }
 
+  lock_release (&t->pagedir_lock);
   /* Mark the page as loaded successfully. */
   return true;
 }

src/vm/page.h

@@ -7,9 +7,8 @@
 
 enum page_type
 {
-  PAGE_FILE,
+  PAGE_EXECUTABLE,
   PAGE_MMAP,
-  PAGE_EMPTY,
   PAGE_SHARED
 };
 
@@ -35,17 +34,23 @@ struct page_entry
 
 struct shared_file_page
 {
-  struct file *file;
-  void *upage;
-  void *frame;
-  size_t swap_slot;
-  int ref_count;
+  struct file *file; /* The shared file page's source file, used for indexing
+                        the table. */
+  void *upage; /* The shared page's upage which is the same across all process
+                  using it. Used for indexing the table. */
+  void *frame; /* Set to the frame address of the page when it is in memory.
+                  Set to NULL when the page is in swap. */
+  size_t swap_slot; /* Set to the swap_slot of the shared paged if it is
+                      currently in swap. Should not be used when frame is not
+                      NULL.*/
+  int ref_count; /* Number of processes that are using this shared page. */
 
-  struct hash_elem elem;
+  struct hash_elem elem; /* AN elem for the hash table. */
 };
 
 bool init_pages (struct hash *pages);
-bool page_insert_swapped (void *upage, void *kpage, struct list *owners);
+bool page_insert_swapped (void *upage, void *kpage, struct list *owners,
+                          struct thread *cur);
 struct page_entry *page_insert_file (struct file *file, off_t ofs, void *upage,
                                      uint32_t read_bytes, uint32_t zero_bytes,
                                      bool writable, enum page_type);