Merge branch 'vm/syscall-mmap/styling' into 'master'

Refactor page to follow pintos styling for indentation and add spacing for readability

See merge request lab2425_autumn/pintos_22!69

src/threads/thread.c

@@ -737,7 +737,6 @@ 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,7 +145,6 @@ 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

@@ -181,11 +181,11 @@ page_fault (struct intr_frame *f)
   /* If the page fault occurred in kernel mode,  then we intentionally indicate
      a fault (for get_user() etc). */
   if (!user)
-  {
-    f->eip = (void *)f->eax;
-    f->eax = 0xffffffff;
-    return;
-  }
+    {
+      f->eip = (void *)f->eax;
+      f->eax = 0xffffffff;
+      return;
+    }
 
 
   /* To implement virtual memory, delete the rest of the function
@@ -233,19 +233,12 @@ 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;
-    }
+    return false;
 
   /* Install the page into user page table */
-  bool result = pagedir_set_page (t->pagedir, upage, new_page, true);
-  lock_release (&t->pagedir_lock);
-  if (!result)
+  if (!pagedir_set_page (thread_current ()->pagedir, upage, new_page, true))
     {
       frame_free (new_page);
       return false;
@@ -268,7 +261,6 @@ 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
@@ -282,11 +274,12 @@ fetch_page (void *upage, bool write)
       lock_release (&page->lock);
 
       bool writeable = pagedir_is_writable (t->pagedir, upage);
-      bool result = pagedir_set_page (t->pagedir, upage, kpage, writeable);
-      lock_release (&t->pagedir_lock);
-      return result;
+
+      /* 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);
     }
-  lock_release (&t->pagedir_lock);
 
   /* An attempt to write to a non-writeable should fail. */
   if (write && !page->writable)
@@ -304,5 +297,9 @@ 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,7 +396,6 @@ 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) 
     {
@@ -411,7 +410,6 @@ process_exit (void)
       pagedir_activate (NULL);
       pagedir_destroy (pd);
     }
-  lock_release (&cur->pagedir_lock);
 }
 
 /* Destruct a process_result, with multi-thread awareness.
@@ -537,9 +535,7 @@ 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 ();
@@ -765,16 +761,11 @@ 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;
-    }
+    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,7 +575,6 @@ 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;
@@ -591,9 +590,7 @@ 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)
     {
-      lock_acquire (&t->pagedir_lock);
-      void *kpage = pagedir_get_page (t->pagedir, ptr);
-      lock_release (&t->pagedir_lock);
+      void *kpage = pagedir_get_page(thread_current()->pagedir, ptr);
       if (kpage == NULL)
       {
         // If it was evicted, try to load it back in.
@@ -643,21 +640,18 @@ 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 (t->pagedir, ptr);
+    void *kpage = pagedir_get_page (thread_current ()->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
@@ -665,7 +659,6 @@ 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 (;;)
@@ -678,9 +671,7 @@ validate_and_pin_user_str (const char *ptr)
 
       /* Pin the frame to prevent eviction. */
       void *page = pg_round_down (ptr);
-      lock_acquire (&t->pagedir_lock);
-      void *kpage = pagedir_get_page (t->pagedir, page);
-      lock_release (&t->pagedir_lock);
+      void *kpage = pagedir_get_page (thread_current ()->pagedir, page);
       if (kpage == NULL)
         {
           // If it was evicted, attempt to reload.
@@ -710,15 +701,13 @@ 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 (t->pagedir, page);
+    void *kpage = pagedir_get_page(thread_current()->pagedir, page);
     ASSERT(kpage != NULL);
     frame_unpin (kpage);
 
@@ -726,11 +715,7 @@ unpin_user_str (const char *ptr)
     while (offset < PGSIZE)
     {
       if (*str_ptr == '\0')
-        {
-          /* Found end of string */
-          lock_release (&t->pagedir_lock);
-          return;
-        }
+        return;  /* Found end of string */
       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 (struct thread *cur);
+static struct frame_metadata *get_victim (void);
 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 (owner);
+      struct frame_metadata *victim = get_victim ();
       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, owner);
+          page_insert_swapped (victim->upage, victim->frame, &victim->owners);
         }
 
       /* Free victim's owners. */
@@ -241,6 +241,7 @@ 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;
@@ -263,6 +264,7 @@ 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)
         {
@@ -284,13 +286,14 @@ 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 (struct thread *cur)
+get_victim (void)
   {
     struct list_elem *ve = next_victim;
     struct frame_metadata *frame_metadata;
@@ -312,8 +315,6 @@ get_victim (struct thread *cur)
           {
             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;
 
@@ -322,8 +323,6 @@ get_victim (struct thread *cur)
                 found = false;
                 pagedir_set_accessed (pd, upage, false);
               }
-            if (frame_owner->owner != cur)
-              lock_release (&frame_owner->owner->pagedir_lock);
           }
       }
 

src/vm/mmap.c

@@ -67,26 +67,27 @@ mmap_unmap (struct mmap_entry *mmap)
      if necessary. */
   off_t length = file_length (mmap->file);
   for (off_t ofs = 0; ofs < length; ofs += PGSIZE)
-  {
-    void *upage = mmap->upage + ofs;
-
-    /* Get the SPT page entry for this page. */
-    struct page_entry *page = page_get(thread_current (), upage);
-    if (page == NULL)
-      continue;
-
-    /* Write the page back to the file if it is dirty. */
-    if (pagedir_is_dirty (thread_current ()->pagedir, upage))
     {
-      lock_acquire (&filesys_lock);
-      file_write_at (mmap->file, upage, page->read_bytes, ofs);
-      lock_release (&filesys_lock);
+      void *upage = mmap->upage + ofs;
+
+      /* Get the SPT page entry for this page. */
+      struct page_entry *page = page_get(thread_current (), upage);
+      if (page == NULL)
+        continue;
+
+      /* Write the page back to the file if it is dirty. */
+      if (pagedir_is_dirty (thread_current ()->pagedir, upage))
+        {
+          lock_acquire (&filesys_lock);
+          file_write_at (mmap->file, upage, page->read_bytes, ofs);
+          lock_release (&filesys_lock);
+        }
+
+      /* Remove the page from the supplemental page table. */
+      hash_delete (&thread_current ()->pages, &page->elem);
     }
 
-    /* Remove the page from the supplemental page table. */
-    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,16 +70,13 @@ 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,
-                     struct thread *cur)
+page_insert_swapped (void *upage, void *kpage, struct list *owners)
 {
   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))
         {
@@ -87,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);
-          if (owner != cur)
-            lock_release (&owner->pagedir_lock);
           continue;
         }
       ASSERT (list_size (owners) == 1);
@@ -100,11 +95,7 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners,
         {
           page = malloc (sizeof (struct page_entry));
           if (page == NULL)
-            {
-              if (owner != cur)
-                lock_release (&owner->pagedir_lock);
-              return false;
-            }
+            return NULL;
           page->upage = upage;
           lock_init (&page->lock);
           hash_insert (&owner->pages, &page->elem);
@@ -124,8 +115,6 @@ 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)
     {
@@ -201,12 +190,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. */
@@ -215,27 +204,28 @@ 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);
             }
-            /* Shared page is in swap. Load it. */
+
+            /* Otherwise, 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)
               {
@@ -243,7 +233,6 @@ page_load_file (struct page_entry *page)
                 page->type = PAGE_SHARED;
               }
             lock_release (&shared_file_pages_lock);
-            lock_release (&t->pagedir_lock);
             return true;
         }
     }
@@ -259,7 +248,6 @@ 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;
     }
 
@@ -271,7 +259,6 @@ 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;
   }
@@ -286,7 +273,6 @@ 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;
         }
@@ -301,7 +287,6 @@ 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;
 }
@@ -338,12 +323,12 @@ page_cleanup (struct hash_elem *e, void *aux UNUSED)
 /* Flags the provided page table entry as representing a swapped out page. */
 void
 page_flag_swap (uint32_t *pte, bool set)
-  {
-    if (set)
-      *pte |= (1 << SWAP_FLAG_BIT);
-    else
-      *pte &= ~(1 << SWAP_FLAG_BIT);
-  }
+{
+  if (set)
+    *pte |= (1 << SWAP_FLAG_BIT);
+  else
+    *pte &= ~(1 << SWAP_FLAG_BIT);
+}
 
 /* Sets the address bits of the page table entry to the provided swap slot
    value. To be used for later retrieval of the swap slot when page faulting. */

src/vm/page.h

@@ -49,8 +49,7 @@ struct shared_file_page
 };
 
 bool init_pages (struct hash *pages);
-bool page_insert_swapped (void *upage, void *kpage, struct list *owners,
-                          struct thread *cur);
+bool page_insert_swapped (void *upage, void *kpage, struct list *owners);
 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);