fix: synchronise destruction of page directories with frame table

src/threads/thread.c

@@ -736,10 +736,6 @@ 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->spt_lock);
-  #endif
-
   old_level = intr_disable ();
   list_push_back (&all_list, &t->allelem);
   intr_set_level (old_level);

src/threads/thread.h

@@ -149,8 +149,6 @@ struct thread
     struct hash open_files;             /* Hash Table of FD -> Struct File. */
 #endif
 
-    struct lock spt_lock;
-
     void *curr_esp;
 
     /* Owned by thread.c. */

src/userprog/pagedir.c

@@ -6,6 +6,7 @@
 #include "threads/init.h"
 #include "threads/pte.h"
 #include "threads/palloc.h"
+#include "threads/synch.h"
 #include "vm/frame.h"
 #include "vm/page.h"
 
@@ -35,6 +36,7 @@ pagedir_destroy (uint32_t *pd)
     return;
 
   ASSERT (pd != init_page_dir);
+  lock_acquire (&lru_lock);
   for (pde = pd; pde < pd + pd_no (PHYS_BASE); pde++)
     if (*pde & PTE_P) 
       {
@@ -53,6 +55,7 @@ pagedir_destroy (uint32_t *pd)
         palloc_free_page (pt);
       }
   palloc_free_page (pd);
+  lock_release (&lru_lock);
 }
 
 /* Returns the address of the page table entry for virtual

src/vm/frame.c

@@ -28,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
   {
@@ -175,23 +172,29 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
 void
 frame_pin (void *frame)
 {
+  ASSERT (frame != NULL);
+  lock_acquire (&lru_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 (&lru_lock);
 }
-
+  
 void
 frame_unpin (void *frame)
 {
+  ASSERT (frame != NULL);
+  lock_acquire (&lru_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 (&lru_lock);
 }
 
 /* Attempt to deallocate a frame for a user process by removing it from the
@@ -206,8 +209,13 @@ frame_free (void *frame)
              "but this address is not allocated!\n",
              frame);
 
+    bool lock_held = lock_held_by_current_thread (&lru_lock);
+
     free_owners (&frame_metadata->owners);
-    lock_acquire (&lru_lock);
+
+    if (!lock_held)
+      lock_acquire (&lru_lock);
+
     hash_delete (&frame_table, &frame_metadata->hash_elem);
     list_remove (&frame_metadata->list_elem);
 
@@ -221,7 +229,9 @@ frame_free (void *frame)
         else
           next_victim = lru_next (next_victim);
       }
-    lock_release (&lru_lock);
+
+    if (!lock_held)
+      lock_release (&lru_lock);
 
     free (frame_metadata);
     palloc_free_page (frame);
@@ -372,7 +382,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,11 @@ struct frame_owner
   struct list_elem elem; /* List element for the list of owners. */
 };
 
+/* Synchronisation variables. */
+/* Protects access to 'lru_list'. */
+struct lock lru_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/page.c

@@ -133,8 +133,7 @@ page_insert_file (struct file *file, off_t ofs, void *upage,
                   enum page_type type)
 {
   /* If page exists, just update it. */
-  struct thread *t = thread_current ();
+  struct page_entry *existing = page_get (thread_current (), upage);
-  struct page_entry *existing = page_get (t, upage);
   if (existing != NULL)
     {
       ASSERT (existing->read_bytes == read_bytes);
@@ -142,7 +141,7 @@ page_insert_file (struct file *file, off_t ofs, void *upage,
       existing->writable = existing->writable || writable;
       return existing;
     }
-  
+
   struct page_entry *page = malloc(sizeof (struct page_entry));
   if (page == NULL)
     return NULL;
@@ -165,13 +164,11 @@ page_insert_file (struct file *file, off_t ofs, void *upage,
 struct page_entry *
 page_get (struct thread *thread, void *upage)
 {
-  lock_acquire (&thread->spt_lock);
   struct page_entry fake_page_entry;
   fake_page_entry.upage = upage;
 
   struct hash_elem *e
     = hash_find (&thread->pages, &fake_page_entry.elem);
-  lock_release (&thread->spt_lock);
   if (e == NULL)
       return NULL;
 
@@ -293,7 +290,6 @@ page_load_file (struct page_entry *page)
 void
 page_cleanup (struct hash_elem *e, void *aux UNUSED)
 {
-  lock_acquire (&thread_current ()->spt_lock);
   struct page_entry *page = hash_entry (e, struct page_entry, elem);
   if (page->type == PAGE_SHARED)
     {
@@ -316,7 +312,6 @@ page_cleanup (struct hash_elem *e, void *aux UNUSED)
       lock_release (&shared_file_pages_lock);
     }
   free (page);
-  lock_release (&thread_current ()->spt_lock);
 }
 
 /* Flags the provided page table entry as representing a swapped out page. */
@@ -348,9 +343,7 @@ page_set_swap (struct thread *owner, uint32_t *pte, size_t swap_slot)
 bool
 page_in_swap (struct thread *owner, void *upage)
 {
-   lock_acquire (&owner->spt_lock);
    uint32_t *pte = lookup_page (owner->pagedir, upage, false);
-   lock_release (&owner->spt_lock);
    return page_in_swap_pte (pte);
 }
 
@@ -367,7 +360,6 @@ page_in_swap_pte (uint32_t *pte)
 size_t
 page_get_swap (struct thread *owner, void *upage)
 {
-   lock_acquire (&owner->spt_lock);  
    uint32_t *pte = lookup_page (owner->pagedir, upage, false);
 
    ASSERT (pte != NULL);
@@ -375,7 +367,6 @@ page_get_swap (struct thread *owner, void *upage)
 
    /* Masks the address bits and returns truncated value. */
    page_flag_swap (pte, false);
-   lock_release (&owner->spt_lock);
    return ((*pte & PTE_ADDR) >> ADDR_START_BIT);
 }