From ce89d3577fea531bb6a9bbe701c8aba6f493580b Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 6 Dec 2024 18:22:00 +0000 Subject: [PATCH] fix: synchronise pagedir which now may be accessed by other threads, with a lock --- src/threads/thread.c | 1 + src/threads/thread.h | 1 + src/userprog/exception.c | 25 ++++++++++++++----------- src/userprog/process.c | 11 ++++++++++- src/userprog/syscall.c | 25 ++++++++++++++++++++----- src/vm/frame.c | 12 ++++++++---- src/vm/page.c | 26 ++++++++++++++++++++++---- src/vm/page.h | 3 ++- 8 files changed, 78 insertions(+), 26 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 92bb940..717dfc7 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -737,6 +737,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->priority = t->base_priority; lock_init (&t->pages_lock); + lock_init (&t->pagedir_lock); old_level = intr_disable (); list_push_back (&all_list, &t->allelem); diff --git a/src/threads/thread.h b/src/threads/thread.h index ca461cd..64234fd 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -145,6 +145,7 @@ struct thread #ifdef USERPROG /* Owned by userprog/process.c. */ uint32_t *pagedir; /* Page directory. */ + struct lock pagedir_lock; /* Lock for the page directory. */ unsigned int fd_counter; /* File descriptor counter for thread's open files. */ struct hash open_files; /* Hash Table of FD -> Struct File. */ diff --git a/src/userprog/exception.c b/src/userprog/exception.c index 84de825..2989598 100644 --- a/src/userprog/exception.c +++ b/src/userprog/exception.c @@ -233,12 +233,19 @@ static bool grow_stack (void *upage) { /* Allocate new page for stack */ + struct thread *t = thread_current (); + lock_acquire (&t->pagedir_lock); void *new_page = frame_alloc (PAL_ZERO, upage, thread_current ()); if (new_page == NULL) - return false; + { + lock_release (&t->pagedir_lock); + return false; + } /* Install the page into user page table */ - if (!pagedir_set_page (thread_current ()->pagedir, upage, new_page, true)) + bool result = pagedir_set_page (t->pagedir, upage, new_page, true); + lock_release (&t->pagedir_lock); + if (!result) { frame_free (new_page); return false; @@ -261,6 +268,7 @@ fetch_page (void *upage, bool write) the faulted virtual address to point to the newly allocated frame. */ struct thread *t = thread_current (); + lock_acquire (&t->pagedir_lock); if (page_in_swap (t, upage)) { /* NOTE: This code should be refactored and moved into helper functions @@ -274,12 +282,11 @@ fetch_page (void *upage, bool write) lock_release (&page->lock); bool writeable = pagedir_is_writable (t->pagedir, upage); - - /* TODO: When this returns false we should quit the page fault, - but currently we continue and check the stack conditions in the - page fault handler. */ - return pagedir_set_page (t->pagedir, upage, kpage, writeable); + bool result = pagedir_set_page (t->pagedir, upage, kpage, writeable); + lock_release (&t->pagedir_lock); + return result; } + lock_release (&t->pagedir_lock); /* An attempt to write to a non-writeable should fail. */ if (write && !page->writable) @@ -297,9 +304,5 @@ fetch_page (void *upage, bool write) return false; } - if (success && page->writable && - !pagedir_is_writable(thread_current()->pagedir, upage)) - pagedir_set_writable(thread_current()->pagedir, upage, true); - return success; } diff --git a/src/userprog/process.c b/src/userprog/process.c index 8fa2d27..1037503 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -396,6 +396,7 @@ process_exit (void) /* Destroy the current process's page directory and switch back to the kernel-only page directory. */ + lock_acquire (&cur->pagedir_lock); pd = cur->pagedir; if (pd != NULL) { @@ -410,6 +411,7 @@ process_exit (void) pagedir_activate (NULL); pagedir_destroy (pd); } + lock_release (&cur->pagedir_lock); } /* Destruct a process_result, with multi-thread awareness. @@ -535,7 +537,9 @@ load (const char *file_name, void (**eip) (void), void **esp) lock_acquire (&filesys_lock); /* Allocate and activate page directory. */ + lock_acquire (&t->pagedir_lock); t->pagedir = pagedir_create (); + lock_release (&t->pagedir_lock); if (t->pagedir == NULL) goto done; process_activate (); @@ -761,11 +765,16 @@ get_usr_kpage (enum palloc_flags flags, void *upage) void *page; #ifdef VM struct thread *t = thread_current (); + lock_acquire (&t->pagedir_lock); if (pagedir_get_page (t->pagedir, upage) != NULL) - return NULL; + { + lock_release (&t->pagedir_lock); + return NULL; + } else page = frame_alloc (flags, upage, t); pagedir_set_accessed (t->pagedir, upage, true); + lock_release (&t->pagedir_lock); #else page = palloc_get_page (flags | PAL_USER); #endif diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 20e66fc..fb854b2 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -575,6 +575,7 @@ validate_user_ptr_helper (const void *start, size_t size, bool write, bool pin) if (!is_user_vaddr (end)) syscall_exit (EXIT_FAILURE); + struct thread *t = thread_current (); for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE) { int result; @@ -590,7 +591,9 @@ validate_user_ptr_helper (const void *start, size_t size, bool write, bool pin) /* If pin is set, pin the frame to prevent eviction. */ if (pin) { - void *kpage = pagedir_get_page(thread_current()->pagedir, ptr); + lock_acquire (&t->pagedir_lock); + void *kpage = pagedir_get_page (t->pagedir, ptr); + lock_release (&t->pagedir_lock); if (kpage == NULL) { // If it was evicted, try to load it back in. @@ -640,18 +643,21 @@ validate_and_pin_user_ptr (const void *start, size_t size, bool write) static void unpin_user_ptr (const void *start, size_t size) { + struct thread *t = thread_current (); void *end = start + size - 1; /* We don't need to do any checks as this function is always called after validate_and_pin_user_ptr. */ /* Go through all pages in the block range, unpinning the frames. */ + lock_acquire (&t->pagedir_lock); for (void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE) { - void *kpage = pagedir_get_page (thread_current ()->pagedir, ptr); + void *kpage = pagedir_get_page (t->pagedir, ptr); ASSERT (kpage != NULL); frame_unpin (kpage); } + lock_release (&t->pagedir_lock); } /* Validates of a C-string starting at ptr is fully contained within valid @@ -659,6 +665,7 @@ unpin_user_ptr (const void *start, size_t size) static void validate_and_pin_user_str (const char *ptr) { + struct thread *t = thread_current (); size_t offset = (uintptr_t) ptr % PGSIZE; for (;;) @@ -671,7 +678,9 @@ validate_and_pin_user_str (const char *ptr) /* Pin the frame to prevent eviction. */ void *page = pg_round_down (ptr); - void *kpage = pagedir_get_page (thread_current ()->pagedir, page); + lock_acquire (&t->pagedir_lock); + void *kpage = pagedir_get_page (t->pagedir, page); + lock_release (&t->pagedir_lock); if (kpage == NULL) { // If it was evicted, attempt to reload. @@ -701,13 +710,15 @@ validate_and_pin_user_str (const char *ptr) static void unpin_user_str (const char *ptr) { + struct thread *t = thread_current (); size_t offset = (uintptr_t)ptr % PGSIZE; const char *str_ptr = ptr; + lock_acquire (&t->pagedir_lock); for (;;) { void *page = pg_round_down(str_ptr); - void *kpage = pagedir_get_page(thread_current()->pagedir, page); + void *kpage = pagedir_get_page (t->pagedir, page); ASSERT(kpage != NULL); frame_unpin (kpage); @@ -715,7 +726,11 @@ unpin_user_str (const char *ptr) while (offset < PGSIZE) { if (*str_ptr == '\0') - return; /* Found end of string */ + { + /* Found end of string */ + lock_release (&t->pagedir_lock); + return; + } str_ptr++; offset++; } diff --git a/src/vm/frame.c b/src/vm/frame.c index 1a6c9a1..c2d1003 100644 --- a/src/vm/frame.c +++ b/src/vm/frame.c @@ -50,7 +50,7 @@ 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 *frame_metadata_get (void *frame); -static struct frame_metadata *get_victim (void); +static struct frame_metadata *get_victim (struct thread *cur); static void free_owners (struct list *owners); static struct frame_metadata *frame_metadata_find (void *frame); @@ -91,7 +91,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner) if (next_victim == NULL) PANIC ("Couldn't allocate a single page to main memory!\n"); - struct frame_metadata *victim = get_victim (); + struct frame_metadata *victim = get_victim (owner); ASSERT (victim != NULL); /* get_victim () should never return null. */ /* 2. Handle victim page writing based on its type. */ @@ -111,7 +111,7 @@ frame_alloc (enum palloc_flags flags, void *upage, struct thread *owner) else { /* Otherwise, insert the page into swap. */ - page_insert_swapped (victim->upage, victim->frame, &victim->owners); + page_insert_swapped (victim->upage, victim->frame, &victim->owners, owner); } /* Free victim's owners. */ @@ -290,7 +290,7 @@ frame_metadata_find (void *frame) /* A pre-condition for calling this function is that the calling thread owns ftable_lock and that lru_list is non-empty. */ static struct frame_metadata * -get_victim (void) +get_victim (struct thread *cur) { struct list_elem *ve = next_victim; struct frame_metadata *frame_metadata; @@ -312,6 +312,8 @@ get_victim (void) { struct frame_owner *frame_owner = list_entry (oe, struct frame_owner, elem); + if (frame_owner->owner != cur) + lock_acquire (&frame_owner->owner->pagedir_lock); uint32_t *pd = frame_owner->owner->pagedir; void *upage = frame_metadata->upage; @@ -320,6 +322,8 @@ get_victim (void) found = false; pagedir_set_accessed (pd, upage, false); } + if (frame_owner->owner != cur) + lock_release (&frame_owner->owner->pagedir_lock); } } diff --git a/src/vm/page.c b/src/vm/page.c index 487103d..6a1ad4a 100644 --- a/src/vm/page.c +++ b/src/vm/page.c @@ -70,13 +70,16 @@ static void page_set_swap (struct thread *owner, uint32_t *pte, insert a new page entry into the user process thread's SPT representing this swapped out page. */ bool -page_insert_swapped (void *upage, void *kpage, struct list *owners) +page_insert_swapped (void *upage, void *kpage, struct list *owners, + struct thread *cur) { struct file *exec_file = NULL; struct list_elem *e; for (e = list_begin (owners); e != list_end (owners); e = list_next (e)) { struct thread *owner = list_entry (e, struct frame_owner, elem)->owner; + if (owner != cur) + lock_acquire (&owner->pagedir_lock); uint32_t *pte = lookup_page (owner->pagedir, upage, false); if (exec_file != NULL || page_is_shared_pte (pte)) { @@ -84,6 +87,8 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners) pagedir_clear_page (owner->pagedir, upage); exec_file = owner->exec_file; ASSERT (exec_file != NULL); + if (owner != cur) + lock_release (&owner->pagedir_lock); continue; } ASSERT (list_size (owners) == 1); @@ -95,7 +100,11 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners) { page = malloc (sizeof (struct page_entry)); if (page == NULL) - return NULL; + { + if (owner != cur) + lock_release (&owner->pagedir_lock); + return false; + } page->upage = upage; lock_init (&page->lock); hash_insert (&owner->pages, &page->elem); @@ -115,6 +124,8 @@ page_insert_swapped (void *upage, void *kpage, struct list *owners) lock_release (&page->lock); lock_release (&owner->pages_lock); + if (owner != cur) + lock_release (&owner->pagedir_lock); } if (exec_file != NULL) { @@ -190,6 +201,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); + lock_acquire (&t->pagedir_lock); if (shareable) { lock_acquire (&shared_file_pages_lock); @@ -203,6 +215,7 @@ page_load_file (struct page_entry *page) if (!install_page (page->upage, sfp->frame, page->writable)) { lock_release (&shared_file_pages_lock); + lock_release (&t->pagedir_lock); return false; } frame_owner_insert (sfp->frame, t); @@ -212,14 +225,14 @@ page_load_file (struct page_entry *page) { void *frame = frame_alloc (PAL_USER, page->upage, t); if (frame == NULL) - PANIC ( - "Could not allocate a frame to load page into memory."); + PANIC ("Could not allocate a frame to load page into memory."); swap_in (frame, sfp->swap_slot); if (!install_page (page->upage, frame, false)) { frame_free (frame); lock_release (&shared_file_pages_lock); + lock_release (&t->pagedir_lock); return false; } } @@ -230,6 +243,7 @@ page_load_file (struct page_entry *page) page->type = PAGE_SHARED; } lock_release (&shared_file_pages_lock); + lock_release (&t->pagedir_lock); return true; } } @@ -245,6 +259,7 @@ page_load_file (struct page_entry *page) if (shareable) lock_release (&shared_file_pages_lock); frame_free (frame); + lock_release (&t->pagedir_lock); return false; } @@ -256,6 +271,7 @@ page_load_file (struct page_entry *page) { if (shareable) lock_release (&shared_file_pages_lock); + lock_release (&t->pagedir_lock); frame_free (frame); return false; } @@ -270,6 +286,7 @@ page_load_file (struct page_entry *page) if (sfp == NULL) { lock_release (&shared_file_pages_lock); + lock_release (&t->pagedir_lock); frame_free (frame); return false; } @@ -284,6 +301,7 @@ page_load_file (struct page_entry *page) lock_release (&shared_file_pages_lock); } + lock_release (&t->pagedir_lock); /* Mark the page as loaded successfully. */ return true; } diff --git a/src/vm/page.h b/src/vm/page.h index 994da72..709cc44 100644 --- a/src/vm/page.h +++ b/src/vm/page.h @@ -49,7 +49,8 @@ struct shared_file_page }; bool init_pages (struct hash *pages); -bool page_insert_swapped (void *upage, void *kpage, struct list *owners); +bool page_insert_swapped (void *upage, void *kpage, struct list *owners, + struct thread *cur); struct page_entry *page_insert_file (struct file *file, off_t ofs, void *upage, uint32_t read_bytes, uint32_t zero_bytes, bool writable, enum page_type);