diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 3b7785c..8be1c71 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -187,15 +187,11 @@ try_fetch_page (void *upage, bool write) bool success = false; switch (page->type) { case PAGE_EXECUTABLE: - success = page_load (page, write); + success = page_load (page); break; default: return false; - } - - if (success && write && - !pagedir_is_writable(thread_current()->pagedir, upage)) - pagedir_set_writable(thread_current()->pagedir, upage, true); + } return success; } diff --git a/src/vm/page.c b/src/vm/page.c index 16a9d8e..0470e37 100644 --- a/src/vm/page.c +++ b/src/vm/page.c @@ -5,6 +5,7 @@ #include "threads/malloc.h" #include "threads/palloc.h" #include "threads/vaddr.h" +#include "userprog/pagedir.h" #include "userprog/process.h" #include "vm/frame.h" @@ -41,18 +42,28 @@ struct page_entry * page_insert (struct file *file, off_t ofs, void *upage, uint32_t read_bytes, uint32_t zero_bytes, bool writable, enum page_type type) { + /* If page exists, just update it. */ + struct page_entry *existing = page_get (upage); + if (existing != NULL) + { + existing->read_bytes = read_bytes; + existing->zero_bytes = zero_bytes; + existing->writable = writable; + return existing; + } + + /* Otherwise allocate a new one. */ struct page_entry *page = malloc(sizeof (struct page_entry)); if (page == NULL) return NULL; - page->file = file; page->offset = ofs; page->upage = upage; page->read_bytes = read_bytes; page->zero_bytes = zero_bytes; page->writable = writable; + page->shared = false; page->type = type; - hash_insert (&thread_current ()->pages, &page->elem); return page; } @@ -75,20 +86,23 @@ page_get (void *upage) } bool -page_load (struct page_entry *page, bool writable) +page_load (struct page_entry *page) { /* If the page is read-only, we want to check if it is a shared page already loaded into memory. If it is, we can just map the page to the frame. */ - if (!page->writable && !writable) + if (!page->writable) { + lock_acquire (&shared_files_lock); struct shared_page_entry *shared_page = shared_page_get (page->file, page->upage); + /* Mark page as shared and install the shared frame. */ if (shared_page != NULL) { - /* Map the page to the shared frame for this read-only portion. */ + lock_release (&shared_files_lock); if (!install_page (page->upage, shared_page->frame, false)) return false; + page->shared = true; return true; } } @@ -100,22 +114,20 @@ page_load (struct page_entry *page, bool writable) if (frame == NULL) PANIC ("Could not allocate a frame to load page into memory."); + /* Ensure page is not marked as shared while it doesn't exist in the + shared_files table, to avoid memory leaks. */ + page->shared = false; + /* Map the page to the frame. */ if (!install_page (page->upage, frame, page->writable)) - { - frame_free (frame); - return false; - } + goto fail; /* Move the file pointer to the correct location in the file. Then, read the data from the file into the frame. Checks that we were able to read the expected number of bytes. */ file_seek (page->file, page->offset); if (file_read (page->file, frame, page->read_bytes) != (int) page->read_bytes) - { - frame_free (frame); - return false; - } + goto fail; /* Zero out the remaining bytes in the frame. */ memset (frame + page->read_bytes, 0, page->zero_bytes); @@ -123,15 +135,22 @@ page_load (struct page_entry *page, bool writable) /* If the page is read-only, we need to add it to the shared pages table. */ if (!page->writable) { - if (shared_page_insert (page->file, page->upage, frame) == NULL) - { - frame_free (frame); - return false; - } + struct shared_page_entry *shared_page + = shared_page_insert (page->file, page->upage, frame); + if (shared_page == NULL) + goto fail; + lock_release (&shared_files_lock); + page->shared = true; } /* Mark the page as loaded successfully. */ return true; + +fail: + if (!page->writable) + lock_release (&shared_files_lock); + frame_free (frame); + return false; } /* Function to clean up a page_entry. Given the elem of that page_entry, frees @@ -139,7 +158,13 @@ page_load (struct page_entry *page, bool writable) void page_cleanup (struct hash_elem *e, void *aux UNUSED) { - free (hash_entry (e, struct page_entry, elem)); + struct page_entry *page = hash_entry (e, struct page_entry, elem); + + /* If page is shared then mark it as not present to avoid it being freed. */ + uint32_t *pd = thread_current ()->pagedir; + if (pd != NULL && page->shared) + pagedir_clear_page (pd, page->upage); + free (page); } /* Hashing function needed for the shared_file table. Returns a hash for an diff --git a/src/vm/page.h b/src/vm/page.h index 74344c1..9b7254e 100644 --- a/src/vm/page.h +++ b/src/vm/page.h @@ -23,6 +23,7 @@ struct page_entry { uint32_t read_bytes; /* Number of bytes to read within the page. */ uint32_t zero_bytes; /* Number of bytes to zero within the page. */ bool writable; /* Flag for whether this page is writable or not. */ + bool shared; /* Flag for whether this page is shared or not. */ struct hash_elem elem; /* An elem for the hash table. */ }; @@ -49,7 +50,7 @@ struct page_entry *page_insert (struct file *file, off_t ofs, void *upage, uint32_t read_bytes, uint32_t zero_bytes, bool writable, enum page_type type); struct page_entry *page_get (void *upage); -bool page_load (struct page_entry *page, bool writable); +bool page_load (struct page_entry *page); void page_cleanup (struct hash_elem *e, void *aux); unsigned shared_file_hash (const struct hash_elem *e, void *aux);