From 8ac34063d7b5ecea394d99bb675051a6b09f3662 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Fri, 6 Dec 2024 10:56:38 +0000 Subject: [PATCH 1/8] fix: disable 'shareable' flag to probe race conditions --- src/vm/page.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vm/page.c b/src/vm/page.c index fb94fa0..208e98a 100644 --- a/src/vm/page.c +++ b/src/vm/page.c @@ -183,6 +183,7 @@ page_load_file (struct page_entry *page) panics as this should not happen if eviction is working correctly. */ struct thread *t = thread_current (); bool shareable = !page->writable && file_compare (page->file, t->exec_file); + shareable = false; if (shareable) { lock_acquire (&shared_file_pages_lock); -- 2.49.1 From d389c1582855a334b28be4417b57f97f5dec1aca Mon Sep 17 00:00:00 2001 From: EDiasAlberto Date: Fri, 6 Dec 2024 13:20:43 +0000 Subject: [PATCH 2/8] fix: acquire lru_lock before pinning frames to avoid race condition with eviction --- src/vm/frame.c | 12 +++++++----- src/vm/frame.h | 4 ++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/vm/frame.c b/src/vm/frame.c index 4730558..c1d5ae3 100644 --- a/src/vm/frame.c +++ b/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 @@ -372,7 +375,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); } diff --git a/src/vm/frame.h b/src/vm/frame.h index 76a801a..cb5916e 100644 --- a/src/vm/frame.h +++ b/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); -- 2.49.1 From 4104d2c8525deb07a5f3a6f3388bb26a5f34a625 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 6 Dec 2024 15:23:41 +0000 Subject: [PATCH 3/8] fix: always add to frame owners when installing existing shared page. --- src/vm/page.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/vm/page.c b/src/vm/page.c index 208e98a..8228687 100644 --- a/src/vm/page.c +++ b/src/vm/page.c @@ -199,11 +199,7 @@ 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); - } + frame_owner_insert (sfp->frame, t); } /* Shared page is in swap. Load it. */ else -- 2.49.1 From 3d6e30119b4b94cf592def12b8d29f73c1e4a6c0 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Fri, 6 Dec 2024 15:31:27 +0000 Subject: [PATCH 4/8] refactor: rename lru_lock to ftable_lock for greater clarity, and update comments to reflect this --- src/vm/frame.c | 29 ++++++++++++++--------------- src/vm/frame.h | 5 +++-- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/vm/frame.c b/src/vm/frame.c index c1d5ae3..1a6c9a1 100644 --- a/src/vm/frame.c +++ b/src/vm/frame.c @@ -40,8 +40,8 @@ struct 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; @@ -63,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 @@ -79,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, @@ -122,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. */ @@ -133,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) @@ -165,7 +165,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner) list_push_back (&frame_metadata->owners, &frame_owner->elem); frame_metadata->upage = upage; frame_metadata->pinned = false; - lock_release (&lru_lock); + lock_release (&ftable_lock); return frame_metadata->frame; } @@ -173,28 +173,28 @@ void frame_pin (void *frame) { ASSERT (frame != NULL); - lock_acquire (&lru_lock); + 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 (&lru_lock); + lock_release (&ftable_lock); } void frame_unpin (void *frame) { ASSERT (frame != NULL); - lock_acquire (&lru_lock); + 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 (&lru_lock); + lock_release (&ftable_lock); } /* Attempt to deallocate a frame for a user process by removing it from the @@ -210,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); @@ -224,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); @@ -287,9 +287,8 @@ frame_metadata_find (void *frame) 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) { diff --git a/src/vm/frame.h b/src/vm/frame.h index cb5916e..62a1ec6 100644 --- a/src/vm/frame.h +++ b/src/vm/frame.h @@ -11,8 +11,9 @@ struct frame_owner }; /* Synchronisation variables. */ -/* Protects access to 'lru_list'. */ -struct lock lru_lock; +/* 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 *); -- 2.49.1 From f64b92bbfaafa1b3341aaec732d24df1137dc1d5 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 6 Dec 2024 15:35:23 +0000 Subject: [PATCH 5/8] refactor: document shared_file_page --- src/vm/page.h | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/vm/page.h b/src/vm/page.h index 7b45d9d..9aa98fb 100644 --- a/src/vm/page.h +++ b/src/vm/page.h @@ -9,7 +9,6 @@ enum page_type { PAGE_FILE, 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); -- 2.49.1 From 22f3b0950f37a72427c474770d15eba28a2b0bea Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 6 Dec 2024 15:54:46 +0000 Subject: [PATCH 6/8] Fix: Insert pages in mmap as PAGE_MMAP instead of PAGE_FILE --- src/userprog/syscall.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 8275d51..20e66fc 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -461,10 +461,9 @@ syscall_mmap (int fd, void *addr) /* 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 (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) @@ -473,7 +472,7 @@ syscall_mmap (int fd, void *addr) off_t zero_bytes = PGSIZE - read_bytes; if (page_insert_file (file, ofs, addr + ofs, read_bytes, zero_bytes, true, - PAGE_FILE) == NULL) + PAGE_MMAP) == NULL) return MMAP_FAILURE; } @@ -482,7 +481,6 @@ syscall_mmap (int fd, void *addr) if (mmap == NULL) return MMAP_FAILURE; - return mmap->mapping; } -- 2.49.1 From c1bc70adadcbfb20a8ddd6d9fa47632f0c643001 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 6 Dec 2024 15:55:23 +0000 Subject: [PATCH 7/8] ci: do not ignore an VM tests since it is fully implemented now --- .gitlab-ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a291160..2f9cb09 100644 --- a/.gitlab-ci.yml +++ b/.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) -- 2.49.1 From 7f058ffc9032dc169e2fb8b7cb3022d0daaf024b Mon Sep 17 00:00:00 2001 From: sBubshait Date: Fri, 6 Dec 2024 16:22:26 +0000 Subject: [PATCH 8/8] Refactor page_type to rename PAGE_FILE to PAGE_EXECUTABLE as mmap and executables are now separate --- src/userprog/exception.c | 2 +- src/userprog/process.c | 2 +- src/vm/page.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/userprog/exception.c b/src/userprog/exception.c index db07db9..84de825 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -289,7 +289,7 @@ 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); break; diff --git a/src/userprog/process.c b/src/userprog/process.c index ca99aee..9024540 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -715,7 +715,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. */ diff --git a/src/vm/page.h b/src/vm/page.h index 9aa98fb..994da72 100644 --- a/src/vm/page.h +++ b/src/vm/page.h @@ -7,7 +7,7 @@ enum page_type { - PAGE_FILE, + PAGE_EXECUTABLE, PAGE_MMAP, PAGE_SHARED }; -- 2.49.1