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

.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,6 +736,8 @@ init_thread (struct thread *t, const char *name, int nice, int priority,
   t->recent_cpu = recent_cpu;
   t->priority = t->base_priority;
 
+  lock_init (&t->pages_lock);
+
   old_level = intr_disable ();
   list_push_back (&all_list, &t->allelem);
   intr_set_level (old_level);

src/threads/thread.h

@@ -136,6 +136,7 @@ struct thread
     struct list_elem elem;              /* List element. */
 
     struct hash pages;                  /* Table of open user pages. */
+    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. */

src/userprog/exception.c

@@ -2,21 +2,20 @@
 #include <inttypes.h>
 #include <stdio.h>
 #include "stdbool.h"
-#include "threads/synch.h"
 #include "userprog/gdt.h"
-#include "userprog/pagedir.h"
-#include "userprog/process.h"
 #include "threads/interrupt.h"
-#include "threads/palloc.h"
 #include "threads/thread.h"
-#include "threads/vaddr.h"
+#ifdef VM
 #include "vm/frame.h"
 #include "vm/page.h"
 #include "devices/swap.h"
+#include "threads/vaddr.h"
 #include "userprog/pagedir.h"
+#endif
 
 #define MAX_STACK_SIZE (8 * 1024 * 1024) // 8MB
 #define MAX_STACK_OFFSET 32 // 32 bytes offset below stack pointer (ESP)
+
 /* Number of page faults processed. */
 static long long page_fault_cnt;
 
@@ -188,6 +187,7 @@ page_fault (struct intr_frame *f)
       return;
     }
 
+
   /* To implement virtual memory, delete the rest of the function
      body, and replace it with code that brings in the page to
      which fault_addr refers. */
@@ -252,7 +252,7 @@ fetch_page (void *upage, bool write)
 {
   /* 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 (upage);
+  struct page_entry *page = page_get (thread_current (), upage);
   if (page == NULL)
     return false;
 
@@ -271,9 +271,7 @@ fetch_page (void *upage, bool write)
       size_t swap_slot = page_get_swap (t, upage);
       swap_in (kpage, swap_slot);
 
-    hash_delete (&thread_current ()->pages, &page->elem);
       lock_release (&page->lock);
-    page_cleanup (&page->elem, NULL);
 
       bool writeable = pagedir_is_writable (t->pagedir, upage);
 
@@ -291,14 +289,9 @@ 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);
-      if (success && page->type == PAGE_FILE)
-        {
-          hash_delete (&thread_current ()->pages, &page->elem);
-          page_cleanup (&page->elem, NULL);
-        }
       break;
     default:
       return false;

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)
@@ -715,7 +717,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. */

src/userprog/syscall.c

@@ -10,6 +10,7 @@
 #include "threads/synch.h"
 #include "userprog/process.h"
 #include "userprog/pagedir.h"
+#include "vm/frame.h"
 #include "vm/page.h"
 #include "vm/mmap.h"
 #include <stdio.h>
@@ -52,9 +53,14 @@ static mapid_t syscall_mmap (int fd, void *addr);
 static void syscall_munmap (mapid_t mapping);
 
 static struct open_file *fd_get_file (int fd);
-static void validate_user_pointer (const void *ptr, size_t size,
-                                   bool check_write);
-static void validate_user_string (const char *str, bool check_write);
+static void validate_user_ptr (const void *start, size_t size,
+                                   bool write);
+static void validate_and_pin_user_ptr (const void *start, size_t size,
+                                       bool write);
+static void validate_and_pin_user_str (const char *ptr);
+static void unpin_user_ptr (const void *start, size_t size);
+
+static void unpin_user_str (const char *ptr);
 static int get_user (const uint8_t *);
 static bool put_user (uint8_t *, uint8_t);
 
@@ -107,7 +113,7 @@ static void
 syscall_handler (struct intr_frame *f)
 {
   /* First, read the system call number from the stack. */
-  validate_user_pointer (f->esp, sizeof (uintptr_t), false);
+  validate_user_ptr (f->esp, sizeof (uintptr_t), false);
   uintptr_t syscall_number = *(int *)f->esp;
   thread_current ()->curr_esp = f->esp;
 
@@ -118,7 +124,7 @@ syscall_handler (struct intr_frame *f)
   struct syscall_arguments syscall = syscall_lookup[syscall_number];
 
   /* Next, read and copy the arguments from the stack pointer. */
-  validate_user_pointer (f->esp + sizeof (uintptr_t),
+  validate_user_ptr (f->esp + sizeof (uintptr_t),
                          syscall.arity * sizeof (uintptr_t), false);
   uintptr_t args[MAX_SYSCALL_ARGS] = { 0 };
   for (int i = 0; i < syscall.arity && i < MAX_SYSCALL_ARGS; i++)
@@ -151,9 +157,11 @@ syscall_exit (int status)
 static pid_t
 syscall_exec (const char *cmd_line)
 {
-  validate_user_string (cmd_line, false);
+  validate_and_pin_user_str (cmd_line);
+  pid_t pid = process_execute (cmd_line);
+  unpin_user_str (cmd_line);
 
-  return process_execute (cmd_line); /* Returns the PID of the new process */
+  return pid;
 }
 
 /* Handles the syscall of wait. Effectively a wrapper for process_wait as the
@@ -170,13 +178,15 @@ syscall_wait (pid_t pid)
 static bool
 syscall_create (const char *file, unsigned initial_size)
 {
-  validate_user_string (file, false);
+  validate_and_pin_user_str (file);
 
   /* Acquire the file system lock to prevent race conditions. */
   lock_acquire (&filesys_lock);
   bool status = filesys_create (file, initial_size);
   lock_release (&filesys_lock);
 
+  unpin_user_str (file);
+
   /* Return the status of the file creation. */
   return status;
 }
@@ -187,13 +197,15 @@ syscall_create (const char *file, unsigned initial_size)
 static bool
 syscall_remove (const char *file)
 {
-  validate_user_string (file, false);
+  validate_and_pin_user_str (file);
 
   /* Acquire the file system lock to prevent race conditions. */
   lock_acquire (&filesys_lock);
   bool status = filesys_remove (file);
   lock_release (&filesys_lock);
 
+  unpin_user_str (file);
+
   /* Return the status of the file removal. */
   return status;
 }
@@ -205,13 +217,15 @@ syscall_remove (const char *file)
 static int
 syscall_open (const char *file)
 {
-  validate_user_string (file, false);
+  validate_and_pin_user_str (file);
 
   /* Acquire the file system lock to prevent race conditions. */
   lock_acquire (&filesys_lock);
   struct file *ptr = filesys_open (file);
   lock_release (&filesys_lock);
 
+  unpin_user_str (file);
+
   /* If the file could not be opened, return failure. */
   if (ptr == NULL)
     return EXIT_FAILURE;
@@ -271,10 +285,11 @@ syscall_read (int fd, void *buffer, unsigned size)
   if (fd < STDIN_FILENO || fd == STDOUT_FILENO)
     return EXIT_FAILURE;
 
-  validate_user_pointer (buffer, size, true);
-
   if (fd == STDIN_FILENO)
     {
+      /* Validate the user buffer. */
+      validate_user_ptr (buffer, size, true);
+
       /* Reading from the console. */
       char *write_buffer = buffer;
       for (unsigned i = 0; i < size; i++)
@@ -292,13 +307,19 @@ syscall_read (int fd, void *buffer, unsigned size)
       if (file_info == NULL)
         return EXIT_FAILURE;
 
+      /* Validate the user buffer, and pin the pages to prevent eviction. */
+      validate_and_pin_user_ptr (buffer, size, true);
+
       /* Acquire the file system lock to prevent race-conditions. */
       lock_acquire (&filesys_lock);
-      int bytes_written = file_read (file_info->file, buffer, size);
+      int bytes_read = file_read (file_info->file, buffer, size);
       lock_release (&filesys_lock);
 
+      /* Unpin the pages to allow eviction. */
+      unpin_user_ptr (buffer, size);
+
       /* Return the number of bytes read. */
-      return bytes_written;
+      return bytes_read;
     }
 }
 
@@ -314,10 +335,11 @@ syscall_write (int fd, const void *buffer, unsigned size)
   if (fd <= 0)
     return 0;
 
-  validate_user_pointer (buffer, size, false);
-
   if (fd == STDOUT_FILENO)
     {
+      /* Validate the user buffer. */
+      validate_user_ptr (buffer, size, false);
+
       /* Writing to the console. */
       putbuf (buffer, size);
 
@@ -333,13 +355,19 @@ syscall_write (int fd, const void *buffer, unsigned size)
       if (file_info == NULL)
         return 0;
 
+      /* Validate the user buffer, and pin the pages to prevent eviction. */
+      validate_and_pin_user_ptr (buffer, size, false);
+
       /* Acquire the file system lock to prevent race conditions. */
       lock_acquire (&filesys_lock);
-      int bytes = file_write (file_info->file, buffer, size);
+      int bytes_written = file_write (file_info->file, buffer, size);
       lock_release (&filesys_lock);
 
+      /* Unpin the pages to allow eviction. */
+      unpin_user_ptr (buffer, size);
+
       /* Return the number of bytes written. */
-      return bytes;
+      return bytes_written;
     }
 }
 
@@ -426,17 +454,16 @@ syscall_mmap (int fd, void *addr)
   if (file_size == 0)
     return MMAP_FAILURE;
 
-  /* Ensure that the mmap page doesn't overlap with the stack. */
-  if (addr >= (thread_current ()->curr_esp) - PGSIZE)
+  /* ensures the page for mmap does not overlap with the stack */
+  if (addr >= (thread_current ()->curr_esp - PGSIZE))
     return MMAP_FAILURE;
 
   /* Check and ensure that there is enough space in the user virtual memory to
      hold the entire file. */
   for (off_t ofs = 0; ofs < file_size; ofs += PGSIZE)
-    {
-      if (page_get (addr + ofs) != NULL)
+      if (page_get (thread_current (), addr + ofs) != NULL)
         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)
@@ -532,55 +559,128 @@ fd_get_file (int fd)
   return hash_entry (e, struct open_file, elem);
 }
 
+/* Helper function that validates a block of memory and optionally pins frames.
+   thread_exit() if the memory is invalid. Used only by the two helper functions
+   validate_user_ptr and validate_and_pin_user_ptr. See the comments for those
+    functions for more details on each. */
+static void
+validate_user_ptr_helper (const void *start, size_t size, bool write, bool pin)
+{
+  if (size == 0)
+    return;
+
+  /* ptr < ptr + size - 1, so sufficient to check that (ptr + size -1) is a
+     valid user virtual memory address. */
+  void *end = start + size - 1;
+  if (!is_user_vaddr (end))
+    syscall_exit (EXIT_FAILURE);
+
+  for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE)
+  {
+    int result;
+
+    /* Check read access to pointer. */
+    if ((result = get_user (ptr)) == -1)
+      syscall_exit (EXIT_FAILURE);
+
+    /* Check write access to pointer (if required). */
+    if (write && !put_user ((uint8_t *)ptr, result))
+      syscall_exit (EXIT_FAILURE);
+
+    /* If pin is set, pin the frame to prevent eviction. */
+    if (pin)
+    {
+      void *kpage = pagedir_get_page(thread_current()->pagedir, ptr);
+      if (kpage == NULL)
+      {
+        // If it was evicted, try to load it back in.
+        ptr -= PGSIZE;
+        continue;
+      }
+
+      frame_pin(kpage);
+    }
+  }
+}
+
 /* Validates if a block of memory starting at PTR and of size SIZE bytes is
    fully contained within valid user virtual memory. thread_exit () if the
    memory is invalid.
    If the size is 0, the function does no checks and returns PTR. */
 static void
-validate_user_pointer (const void *ptr, size_t size, bool check_write)
+validate_user_ptr (const void *start, size_t size, bool write)
 {
-  if (size == 0)
-    return;
-  /* ptr < ptr + size - 1, so sufficient to check that (ptr + size -1) is a
-     valid user virtual memory address. */
-  void *last = ptr + size - 1;
-  if (!is_user_vaddr (last))
-    syscall_exit (EXIT_FAILURE);
-  ptr = pg_round_down (ptr);
-  while (ptr <= last)
+  validate_user_ptr_helper (start, size, write, false);
+}
+
+/* Validates if a block of memory starting at PTR and of size SIZE bytes is
+   fully contained within valid user virtual memory. thread_exit () if the
+   memory is invalid. The function also checks if the memory is writable if
+   WRITE flag is set.
+
+   The function attempts to preload the pages in case they are not in memory
+   yet (e.g., in a swap, lazy loading). If this is successful, the frame pages
+   are pinned to prevent eviction prior to access.
+
+   As such, a call to this function MUST be followed by a call to
+   unpin_user_ptr (START, SIZE) to unpin the pages and allow eviction.
+
+   If the size is 0, the function does no checks and returns PTR. */
+static void
+validate_and_pin_user_ptr (const void *start, size_t size, bool write)
 {
-      int result;
-      /* Check read access to pointer. */
-      if ((result = get_user (ptr)) == -1)
-        syscall_exit (EXIT_FAILURE);
-      /* Check write access to pointer (if required). */
-      if (check_write && !put_user (ptr, result))
-        syscall_exit (EXIT_FAILURE);
-      ptr += PGSIZE;
+  validate_user_ptr_helper (start, size, write, true);
+}
+
+/* Unpins all the pages containing a block of memory starting at START and of
+   size SIZE bytes.
+
+   Pre: The pages were previously pinned by validate_and_pin_user_ptr (START,
+        SIZE). */
+static void
+unpin_user_ptr (const void *start, size_t size)
+{
+  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. */
+  for (void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE)
+  {
+    void *kpage = pagedir_get_page (thread_current ()->pagedir, ptr);
+    ASSERT (kpage != NULL);
+
+    frame_unpin (kpage);
   }
 }
 
 /* Validates of a C-string starting at ptr is fully contained within valid
    user virtual memory. thread_exit () if the memory is invalid. */
 static void
-validate_user_string (const char *ptr, bool check_write)
+validate_and_pin_user_str (const char *ptr)
 {
   size_t offset = (uintptr_t) ptr % PGSIZE;
 
   for (;;)
     {
-      void *page = pg_round_down (ptr);
-
-      if (!is_user_vaddr (page))
-        syscall_exit (EXIT_FAILURE);
       if (!is_user_vaddr (ptr))
         syscall_exit (EXIT_FAILURE);
-      int result;
-      if ((result = get_user ((const uint8_t *)ptr)) == -1)
-        syscall_exit (EXIT_FAILURE);
-      if (check_write && !put_user ((uint8_t *)ptr, result))
+
+      if (get_user ((const uint8_t *)ptr) == -1)
         syscall_exit (EXIT_FAILURE);
 
+      /* Pin the frame to prevent eviction. */
+      void *page = pg_round_down (ptr);
+      void *kpage = pagedir_get_page (thread_current ()->pagedir, page);
+      if (kpage == NULL)
+        {
+          // If it was evicted, attempt to reload.
+          ptr -= PGSIZE;
+          continue;
+        }
+
+      frame_pin (kpage);
+
       while (offset < PGSIZE)
         {
           if (*ptr == '\0')
@@ -591,7 +691,36 @@ validate_user_string (const char *ptr, bool check_write)
         }
 
       offset = 0;
+    }
+}
 
+/* Unpins all the pages containing a C-string starting at PTR.
+
+   Pre: The pages were previously pinned by validate_and_pin_user_str (PTR).
+        PTR points to a valid C string that ends with '\0'. */
+static void
+unpin_user_str (const char *ptr)
+{
+  size_t offset = (uintptr_t)ptr % PGSIZE;
+  const char *str_ptr = ptr;
+
+  for (;;)
+  {
+    void *page = pg_round_down(str_ptr);
+    void *kpage = pagedir_get_page(thread_current()->pagedir, page);
+    ASSERT(kpage != NULL);
+    frame_unpin (kpage);
+
+    /* Scan until end of string or page */
+    while (offset < PGSIZE)
+    {
+      if (*str_ptr == '\0')
+        return;  /* Found end of string */
+      str_ptr++;
+      offset++;
+    }
+
+    offset = 0;
   }
 }
 

src/vm/frame.c

@@ -2,12 +2,13 @@
 #include <hash.h>
 #include <list.h>
 #include <string.h>
-
 #include "frame.h"
 #include "page.h"
+#include "filesys/file.h"
 #include "threads/malloc.h"
 #include "threads/vaddr.h"
 #include "userprog/pagedir.h"
+#include "userprog/syscall.h"
 #include "threads/synch.h"
 
 /* Hash table that maps every active frame's kernel virtual address
@@ -27,21 +28,20 @@ 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
   {
      void *frame;  /* The kernel virtual address holding the frame. */
      void *upage; /* The user virtual address pointing to the frame. */
      struct list owners;          /* List of threads that own the frame. */
+     bool pinned;                 /* Indicates wheter the frame should be
+                                     considered as an eviction candidate.*/
      struct hash_elem hash_elem;  /* Tracks the position of the 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;
@@ -49,6 +49,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 void free_owners (struct list *owners);
 static struct frame_metadata *frame_metadata_find (void *frame);
@@ -62,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
@@ -78,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,
@@ -93,8 +94,25 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
       struct frame_metadata *victim = get_victim ();
       ASSERT (victim != NULL); /* get_victim () should never return null. */
 
-      /* 2. Swap out victim into disk. */
+      /* 2. Handle victim page writing based on its type. */
+      struct page_entry *victim_page = page_get (thread_current (), victim->upage);
+      if (victim_page != NULL && victim_page->type == PAGE_MMAP)
+        {
+          /* If it was a memory-mapped file page, we just write it back
+             to the file if it was dirty. */
+          if (pagedir_is_dirty(owner->pagedir, victim->upage))
+            {
+              lock_acquire (&filesys_lock);
+              file_write_at (victim_page->file, victim->upage,
+                             victim_page->read_bytes, victim_page->offset);
+              lock_release (&filesys_lock);
+            }
+        }
+      else
+        {
+          /* Otherwise, insert the page into swap. */
           page_insert_swapped (victim->upage, victim->frame, &victim->owners);
+        }
 
       /* Free victim's owners. */
       free_owners (&victim->owners);
@@ -104,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. */
@@ -115,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)
@@ -146,10 +164,39 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
   list_init (&frame_metadata->owners);
   list_push_back (&frame_metadata->owners, &frame_owner->elem);
   frame_metadata->upage = upage;
-  lock_release (&lru_lock);
+  frame_metadata->pinned = false;
+  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
    frame table as well as lru_list, and freeing the underlying page
    memory & metadata struct. Panics if the frame isn't active in memory. */
@@ -163,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);
 
@@ -177,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);
@@ -194,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;
@@ -216,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)
         {
@@ -237,12 +286,12 @@ 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);
 }
 
-/* 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. */
+   owns ftable_lock and that lru_list is non-empty. */
 static struct frame_metadata *
 get_victim (void)
   {
@@ -255,6 +304,10 @@ get_victim (void)
         ve = lru_next (ve);
         struct list_elem *oe;
 
+        /* Skip pinned frames */
+        if (frame_metadata->pinned)
+          continue;
+
         /* Returns once a frame that was not accessed by any owner is found. */
         found = true;
         for (oe = list_begin (&frame_metadata->owners);
@@ -316,6 +369,17 @@ frame_metadata_less (const struct hash_elem *a_, const struct hash_elem *b_,
   return a->frame < b->frame;
 }
 
+static struct frame_metadata *
+frame_metadata_get (void *frame)
+{
+  struct frame_metadata key_metadata;
+  key_metadata.frame = 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);
+}
+
 /* Returns the next recently used element after the one provided, which
    is achieved by iterating through lru_list like a circular queue
    (wrapping around the list at the tail). */

src/vm/frame.h

@@ -10,8 +10,15 @@ 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);
 void frame_free (void *frame);
 
 bool frame_owner_insert (void *frame, struct thread *owner);

src/vm/mmap.c

@@ -1,5 +1,6 @@
 #include "mmap.h"
 #include "page.h"
+#include "threads/thread.h"
 #include "threads/vaddr.h"
 #include "threads/malloc.h"
 #include "userprog/syscall.h"
@@ -70,7 +71,7 @@ mmap_unmap (struct mmap_entry *mmap)
       void *upage = mmap->upage + ofs;
 
       /* Get the SPT page entry for this page. */
-    struct page_entry *page = page_get(upage);
+      struct page_entry *page = page_get(thread_current (), upage);
       if (page == NULL)
         continue;
 
@@ -86,6 +87,7 @@ 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

@@ -65,6 +65,7 @@ static void page_flag_swap (uint32_t *pte, bool set);
 static void page_set_swap (struct thread *owner, uint32_t *pte,
                            size_t swap_slot);
 
+// TODO: Deal with NULL malloc returns
 /* Swap out 'owner' process's 'upage' stored at 'kpage'. Then, allocate and
    insert a new page entry into the user process thread's SPT representing
    this swapped out page. */
@@ -88,31 +89,32 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners)
       ASSERT (list_size (owners) == 1);
 
       /* 1. Initialize swapped page entry. */
-      struct page_entry *page = malloc (sizeof (struct page_entry));
+      struct page_entry *page = page_get (owner, upage);
+      lock_acquire (&owner->pages_lock);
       if (page == NULL)
-        return false;
-
+        {
+          page = malloc (sizeof (struct page_entry));
+          if (page == NULL)
+            return NULL;
           page->upage = upage;
           lock_init (&page->lock);
+          hash_insert (&owner->pages, &page->elem);
+        }
 
       /* Mark page as 'swapped' and flag the page directory as having
         been modified *before* eviction begins to prevent the owner of the
         victim page from accessing/modifying it mid-eviction. */
-      /* TODO: We need to stop the process from destroying pagedir
-        mid-eviction, as this could render the page table entry invalid. */
-      ASSERT (pte != NULL);
-
+      /* TODO: We need to stop the process from destroying pagedir mid-eviction,
+        as this could render the page table entry invalid. */
       page_flag_swap (pte, true);
       lock_acquire (&page->lock);
-      struct hash_elem *elem = hash_insert (&owner->pages, &page->elem);
-      ASSERT (elem == NULL);
-
       pagedir_clear_page (owner->pagedir, upage);
 
       size_t swap_slot = swap_out (kpage);
       page_set_swap (owner, pte, swap_slot);
 
       lock_release (&page->lock);
+      lock_release (&owner->pages_lock);
     }
   if (exec_file != NULL)
     {
@@ -133,7 +135,7 @@ page_insert_file (struct file *file, off_t ofs, void *upage,
                   enum page_type type)
 {
   /* If page exists, just update it. */
-  struct page_entry *existing = page_get (upage);
+  struct page_entry *existing = page_get (thread_current (), upage);
   if (existing != NULL)
     {
       ASSERT (existing->read_bytes == read_bytes);
@@ -153,21 +155,27 @@ page_insert_file (struct file *file, off_t ofs, void *upage,
   page->read_bytes = read_bytes;
   page->zero_bytes = zero_bytes;
   page->writable = writable;
+  lock_init (&page->lock);
 
-  hash_insert (&thread_current ()->pages, &page->elem);
+  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.*/
 struct page_entry *
-page_get (void *upage)
+page_get (struct thread *thread, void *upage)
 {
   struct page_entry fake_page_entry;
   fake_page_entry.upage = upage;
 
+  lock_acquire (&thread->pages_lock);
   struct hash_elem *e
-    = hash_find (&thread_current ()->pages, &fake_page_entry.elem);
+    = hash_find (&thread->pages, &fake_page_entry.elem);
+  lock_release (&thread->pages_lock);
   if (e == NULL)
       return NULL;
 
@@ -187,6 +195,7 @@ page_load_file (struct page_entry *page)
       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. */
@@ -197,13 +206,10 @@ page_load_file (struct page_entry *page)
                   lock_release (&shared_file_pages_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);
             }
-            }
-            /* 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);
@@ -219,6 +225,7 @@ page_load_file (struct page_entry *page)
                     return false;
                   }
               }
+
             page_flag_shared (t, page->upage, true);
             if (page->type != PAGE_SHARED)
               {

src/vm/page.h

@@ -7,9 +7,8 @@
 
 enum page_type
 {
-  PAGE_FILE,
+  PAGE_EXECUTABLE,
   PAGE_MMAP,
-  PAGE_EMPTY,
   PAGE_SHARED
 };
 
@@ -35,13 +34,18 @@ 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);
@@ -49,7 +53,7 @@ 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);
-struct page_entry *page_get (void *upage);
+struct page_entry *page_get (struct thread *thread, void *upage);
 bool page_load_file (struct page_entry *page);
 void page_cleanup (struct hash_elem *e, void *aux);