From 1da0c7d48c329c7960684676962dc1b9d28d5cb4 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 6 Dec 2024 00:29:57 +0000 Subject: [PATCH] fix: properly assign frame owners and deallocate in all required places --- src/userprog/pagedir.c | 4 +- src/vm/frame.c | 100 ++++++++++++++++++++++++++++++++--------- src/vm/frame.h | 3 ++ src/vm/page.c | 39 ++++++++++------ src/vm/page.h | 2 +- 5 files changed, 112 insertions(+), 36 deletions(-) diff --git a/src/userprog/pagedir.c b/src/userprog/pagedir.c index 98bfb17..1d79b38 100644 --- a/src/userprog/pagedir.c +++ b/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); diff --git a/src/vm/frame.c b/src/vm/frame.c index 1daf1f8..6f35606 100644 --- a/src/vm/frame.c +++ b/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; - 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)); - } + free_owners (&victim->owners); /* 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; - key_metadata.frame = frame; + struct frame_metadata *frame_metadata = frame_metadata_find (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 = - 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); - + free_owners (&frame_metadata->owners); 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 diff --git a/src/vm/frame.h b/src/vm/frame.h index 3166c37..47adc24 100644 --- a/src/vm/frame.h +++ b/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 */ diff --git a/src/vm/page.c b/src/vm/page.c index b617994..eb499e1 100644 --- a/src/vm/page.c +++ b/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)) - { - lock_release (&shared_file_pages_lock); - return false; - } + { + if (!install_page (page->upage, sfp->frame, page->writable)) + { + 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->type = PAGE_SHARED; - lock_release (&shared_file_pages_lock); - return true; + page_flag_shared (t, page->upage, true); + if (page->type != PAGE_SHARED) + { + 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' - is flagged to be shared via the owner's page table. */ +/* Returns true iff the page table entry is marked to be shared. */ 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)); } diff --git a/src/vm/page.h b/src/vm/page.h index bb96423..13cce0f 100644 --- a/src/vm/page.h +++ b/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