fix: properly assign frame owners and deallocate in all required places

src/userprog/pagedir.c

@@ -45,9 +45,9 @@ pagedir_destroy (uint32_t *pd)
           {
             if (page_is_shared_pte (pte))
               continue;
-            if (page_in_swap_pte (pte))
+            else if (page_in_swap_pte (pte))
               swap_drop (page_get_swap_pte (pte));
-            if (*pte & PTE_P)
+            else if (*pte & PTE_P)
               frame_free (pte_get_page (*pte));
           }
         palloc_free_page (pt);

src/vm/frame.c

@@ -50,6 +50,8 @@ 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 *get_victim (void);
+static void free_owners (struct list *owners);
+static struct frame_metadata *frame_metadata_find (void *frame);
 
 /* Initialize the frame system by initializing the frame (hash) table with
    the frame_metadata hashing and comparison functions, as well as initializing
@@ -95,15 +97,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
       page_insert_swapped (victim->upage, victim->frame, &victim->owners);
 
       /* Free victim's owners. */
-      struct list_elem *oe;
+      free_owners (&victim->owners);
-      for (oe = list_begin (&victim->owners);
-           oe != list_end (&victim->owners);)
-        {
-          struct frame_owner *frame_owner
-              = list_entry (oe, struct frame_owner, elem);
-          oe = list_remove (oe);
-          free (list_entry (oe, struct frame_owner, elem));
-        }
 
       /* If zero flag is set, zero out the victim page. */
       if (flags & PAL_ZERO)
@@ -153,7 +147,6 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
   list_push_back (&frame_metadata->owners, &frame_owner->elem);
   frame_metadata->upage = upage;
   lock_release (&lru_lock);
-
   return frame_metadata->frame;
 }
 
@@ -163,18 +156,15 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner)
 void
 frame_free (void *frame)
   {
-    struct frame_metadata key_metadata;
+    struct frame_metadata *frame_metadata = frame_metadata_find (frame);
-    key_metadata.frame = frame;
+    if (frame_metadata == NULL)
+      PANIC ("Attempted to free a frame at kernel address %p, "
+             "but this address is not allocated!\n",
+             frame);
 
-    struct hash_elem *e =
+    free_owners (&frame_metadata->owners);
-      hash_delete (&frame_table, &key_metadata.hash_elem);
-    if (e == NULL) PANIC ("Attempted to free a frame at kernel address %p, "
-                          "but this address is not allocated!\n", frame);
-
-    struct frame_metadata *frame_metadata =
-      hash_entry (e, struct frame_metadata, hash_elem);
-    
     lock_acquire (&lru_lock);
+    hash_delete (&frame_table, &frame_metadata->hash_elem);
     list_remove (&frame_metadata->list_elem);
 
     /* If we're freeing the frame marked as the next victim, update
@@ -193,6 +183,63 @@ frame_free (void *frame)
     palloc_free_page (frame);
   }
 
+/* Add a thread to a frame's frame_metadata owners list. */
+bool
+frame_owner_insert (void *frame, struct thread *owner)
+{
+  struct frame_metadata *frame_metadata = frame_metadata_find (frame);
+  if (frame_metadata == NULL)
+    return false;
+
+  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;
+}
+
+/* Remove and deallocate a frame owner from the frame_metadata owners list.
+  */
+void
+frame_owner_remove (void *frame, struct thread *owner)
+{
+  struct frame_metadata *frame_metadata = frame_metadata_find (frame);
+  if (frame_metadata == NULL)
+    PANIC ("Attempted to remove an owner from a frame at kernel "
+            "address %p, but this address is not allocated!\n",
+            frame);
+
+  struct list_elem *oe;
+  for (oe = list_begin (&frame_metadata->owners);
+        oe != list_end (&frame_metadata->owners);)
+    {
+      struct frame_owner *frame_owner
+          = list_entry (oe, struct frame_owner, elem);
+      oe = list_next (oe);
+      if (frame_owner->owner == owner)
+        {
+          list_remove (&frame_owner->elem);
+          free (frame_owner);
+          return;
+        }
+    }
+  NOT_REACHED ();
+}
+
+/* Find a frame_metadata entry in the frame table. */
+static struct frame_metadata *
+frame_metadata_find (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);
+}
+
 /* 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. */
@@ -230,6 +277,19 @@ get_victim (void)
     return frame_metadata;
   }
 
+static void
+free_owners (struct list *owners)
+{
+  struct list_elem *oe;
+  for (oe = list_begin (owners); oe != list_end (owners);)
+    {
+      struct frame_owner *frame_owner
+          = list_entry (oe, struct frame_owner, elem);
+      oe = list_remove (oe);
+      free (frame_owner);
+    }
+}
+
 /* Hash function for frame metadata, used for storing entries in the
    frame table. */
 unsigned

src/vm/frame.h

@@ -14,4 +14,7 @@ void frame_init (void);
 void *frame_alloc (enum palloc_flags, void *, struct thread *);
 void frame_free (void *frame);
 
+bool frame_owner_insert (void *frame, struct thread *owner);
+void frame_owner_remove (void *frame, struct thread *owner);
+
 #endif /* vm/frame.h */

src/vm/page.c

@@ -191,11 +191,18 @@ page_load_file (struct page_entry *page)
         {
           /* Frame exists, just install it. */
           if (sfp->frame != NULL)
-            if (!install_page (page->upage, sfp->frame, page->writable))
+            {
-              {
+              if (!install_page (page->upage, sfp->frame, page->writable))
-                lock_release (&shared_file_pages_lock);
+                {
-                return false;
+                  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. */
             else
               {
@@ -211,12 +218,15 @@ page_load_file (struct page_entry *page)
                     lock_release (&shared_file_pages_lock);
                     return false;
                   }
-                page_flag_shared (t, page->upage, true);
               }
-          sfp->ref_count++;
+            page_flag_shared (t, page->upage, true);
-          page->type = PAGE_SHARED;
+            if (page->type != PAGE_SHARED)
-          lock_release (&shared_file_pages_lock);
+              {
-          return true;
+                sfp->ref_count++;
+                page->type = PAGE_SHARED;
+              }
+            lock_release (&shared_file_pages_lock);
+            return true;
         }
     }
 
@@ -286,6 +296,8 @@ page_cleanup (struct hash_elem *e, void *aux UNUSED)
       struct shared_file_page *sfp
           = shared_file_page_get (page->file, page->upage);
       ASSERT (sfp != NULL);
+      if (sfp->frame != NULL)
+        frame_owner_remove (sfp->frame, thread_current ());
       sfp->ref_count--;
       if (sfp->ref_count == 0)
         {
@@ -379,8 +391,7 @@ page_flag_shared (struct thread *owner, void *upage, bool shared)
     *pte &= ~(1 << SHARED_FLAG_BIT);
 }
 
-/* Returns true iff the page with user address 'upage' owned by 'owner'
+/* Returns true iff the page table entry is marked to be shared. */
-   is flagged to be shared via the owner's page table. */
 bool
 page_is_shared_pte (uint32_t *pte)
 {
@@ -403,7 +414,9 @@ static unsigned
 shared_file_page_hash (const struct hash_elem *e, void *aux UNUSED)
 {
   struct shared_file_page *sfp = hash_entry (e, struct shared_file_page, elem);
-  void *bytes[2] = { sfp->upage, sfp->file };
+  void *inode = file_get_inode (sfp->file);
+  void *upage = sfp->upage;
+  void *bytes[2] = { inode, upage };
   return hash_bytes (bytes, sizeof (bytes));
 }
 

src/vm/page.h

@@ -59,6 +59,6 @@ size_t page_get_swap (struct thread *owner, void *upage);
 size_t page_get_swap_pte (uint32_t *pte);
 
 bool page_is_shared_pte (uint32_t *pte);
-void shared_file_pages_init ();
+void shared_file_pages_init (void);
 
 #endif