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

src/threads/thread.c

@@ -737,6 +737,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority,
   t->priority = t->base_priority;
 
   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

@@ -145,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

@@ -233,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)
+    {
+      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;
@@ -261,6 +268,7 @@ fetch_page (void *upage, bool write)
    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
@@ -274,12 +282,11 @@ fetch_page (void *upage, bool write)
       lock_release (&page->lock);
 
       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);
+      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)
@@ -297,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

@@ -396,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) 
     {
@@ -410,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.
@@ -535,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 ();
@@ -761,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)
+    {
+      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

@@ -575,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;
@@ -590,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.
@@ -640,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
@@ -659,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 (;;)
@@ -671,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.
@@ -701,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);
 
@@ -715,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

@@ -50,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);
 
@@ -91,7 +91,7 @@ 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. */
@@ -111,7 +111,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
       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. */
@@ -241,7 +241,6 @@ frame_owner_insert (void *frame, struct thread *owner)
   struct frame_owner *frame_owner = malloc (sizeof (struct frame_owner));
   if (frame_owner == NULL)
     return false;
-
   frame_owner->owner = owner;
   list_push_back (&frame_metadata->owners, &frame_owner->elem);
   return true;
@@ -264,7 +263,6 @@ frame_owner_remove (void *frame, struct thread *owner)
     {
       struct frame_owner *frame_owner
           = list_entry (oe, struct frame_owner, elem);
-
       oe = list_next (oe);
       if (frame_owner->owner == owner)
         {
@@ -286,14 +284,13 @@ frame_metadata_find (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);
 }
 
 /* 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;
@@ -315,6 +312,8 @@ get_victim (void)
           {
             struct frame_owner *frame_owner
                 = list_entry (oe, struct frame_owner, elem);
+            if (frame_owner->owner != cur)
+              lock_acquire (&frame_owner->owner->pagedir_lock);
             uint32_t *pd = frame_owner->owner->pagedir;
             void *upage = frame_metadata->upage;
 
@@ -323,6 +322,8 @@ get_victim (void)
                 found = false;
                 pagedir_set_accessed (pd, upage, false);
               }
+            if (frame_owner->owner != cur)
+              lock_release (&frame_owner->owner->pagedir_lock);
           }
       }
 

src/vm/mmap.c

@@ -87,7 +87,6 @@ mmap_unmap (struct mmap_entry *mmap)
     hash_delete (&thread_current ()->pages, &page->elem);
   }
 
-  /* Close the file and free the mmap entry. */
   file_close (mmap->file);
   free (mmap);
 }

src/vm/page.c

@@ -70,13 +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 (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))
         {
@@ -84,6 +87,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);
+          if (owner != cur)
+            lock_release (&owner->pagedir_lock);
           continue;
         }
       ASSERT (list_size (owners) == 1);
@@ -95,7 +100,11 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners)
         {
           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);
@@ -115,6 +124,8 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners)
 
       lock_release (&page->lock);
       lock_release (&owner->pages_lock);
+      if (owner != cur)
+        lock_release (&owner->pagedir_lock);
     }
   if (exec_file != NULL)
     {
@@ -190,12 +201,12 @@ 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);
+  lock_acquire (&t->pagedir_lock);
   if (shareable)
     {
       lock_acquire (&shared_file_pages_lock);
       struct shared_file_page *sfp
           = shared_file_page_get (page->file, page->upage);
-
       if (sfp != NULL)
         {
           /* Frame exists, just install it. */
@@ -204,28 +215,27 @@ 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;
                 }
               frame_owner_insert (sfp->frame, t);
             }
-
-            /* Otherwise, shared page is in swap. Load it. */
+            /* 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;
                   }
               }
-
             page_flag_shared (t, page->upage, true);
             if (page->type != PAGE_SHARED)
               {
@@ -233,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;
         }
     }
@@ -248,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;
     }
 
@@ -259,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;
   }
@@ -273,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;
         }
@@ -287,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

@@ -49,7 +49,8 @@ struct shared_file_page
 };
 
 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);