From 2566948a32e85212cd3346ea8b4f1429576b078b Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 15 Nov 2024 16:37:42 +0000 Subject: [PATCH] Implement hash table for child process results --- src/threads/thread.c | 50 ++++++++++++++++--- src/threads/thread.h | 6 +-- src/userprog/process.c | 111 ++++++++++++++++------------------------- 3 files changed, 90 insertions(+), 77 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 1fcbc01..b7c6c42 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -84,6 +84,10 @@ void thread_schedule_tail (struct thread *prev); static tid_t allocate_tid (void); static bool donor_priority_less (const struct list_elem *a_, const struct list_elem *b_, void *aux UNUSED); +static unsigned process_result_hash (const struct hash_elem *e, + void *aux UNUSED); +static bool process_result_less (const struct hash_elem *a, + const struct hash_elem *b, void *aux UNUSED); /* Initializes the threading system by transforming the code that's currently running into a thread. This can't work in @@ -122,6 +126,13 @@ thread_init (void) void thread_start (void) { + /* Malloc has been initalised, we can allocate the child results table + for the main thread. */ + struct thread *t = thread_current (); + if (!hash_init (&t->child_results, process_result_hash, process_result_less, + t)) + PANIC ("Failed to initialise child results table for main thread."); + /* Create the idle thread. */ struct semaphore idle_started; sema_init (&idle_started, 0); @@ -244,8 +255,15 @@ thread_create (const char *name, int priority, init_process_result (t); #ifdef USERPROG - hash_init (&t->open_files, fd_hash, fd_less, NULL); - #endif + if (!hash_init (&t->open_files, fd_hash, fd_less, NULL) + || !hash_init (&t->child_results, process_result_hash, + process_result_less, t)) + { + palloc_free_page (t); + free (t->result); + return TID_ERROR; + } +#endif /* Prepare thread for first run by initializing its stack. Do this atomically so intermediate values for the 'stack' @@ -269,9 +287,7 @@ thread_create (const char *name, int priority, intr_set_level (old_level); - /* No need to synchronise child_results since it is only ever accessed by one - thread. By the nature of increasing TIDs, this list is ordered. */ - list_push_back (&parent_thread->child_results, &t->result->elem); + hash_insert (&parent_thread->child_results, &t->result->elem); /* Add to run queue. */ thread_unblock (t); @@ -690,7 +706,6 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->fd_counter = MINIMUM_USER_FD; t->exit_status = -1; - list_init (&t->child_results); old_level = intr_disable (); list_push_back (&all_list, &t->allelem); @@ -822,6 +837,29 @@ allocate_tid (void) return tid; } +/* Hashing function needed for child_results table. + Returns hash of process_result's TID. */ +static unsigned +process_result_hash (const struct hash_elem *e, void *aux UNUSED) +{ + const struct process_result *result + = hash_entry (e, struct process_result, elem); + return hash_int (result->tid); +} + +/* Comparator function needed for child_results table. + Returns less than comparison on process_results' TIDs. */ +static bool +process_result_less (const struct hash_elem *a_, const struct hash_elem *b_, + void *aux UNUSED) +{ + const struct process_result *a + = hash_entry (a_, struct process_result, elem); + const struct process_result *b + = hash_entry (b_, struct process_result, elem); + return a->tid < b->tid; +} + /* Offset of `stack' member within `struct thread'. Used by switch.S, which can't figure it out on its own. */ uint32_t thread_stack_ofs = offsetof (struct thread, stack); diff --git a/src/threads/thread.h b/src/threads/thread.h index c18500b..dfd6f0d 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -44,7 +44,7 @@ struct process_result struct lock lock; /* Lock the exit_status and sema. */ struct semaphore sema; /* Semaphore to signal the parent that the exit_status has been set. */ - struct list_elem elem; /* List element for the parent's children list. */ + struct hash_elem elem; /* Hash element for the parent's children map. */ }; /* A kernel thread or user process. @@ -128,8 +128,8 @@ struct thread /* Process wait properties. */ struct process_result *result; /* Result of the process. */ - struct list child_results; /* List of children's of this thread - process results. */ + struct hash child_results; /* Map of children's of this thread + TID to process result. */ struct file *exec_file; /* Thread's currently running file */ /* Shared between thread.c and synch.c. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index e712e22..0e062b0 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -1,5 +1,6 @@ #include "userprog/process.h" #include +#include #include #include #include @@ -55,6 +56,7 @@ struct process_start_data }; static thread_func start_process NO_RETURN; +static void destruct_process_result (struct hash_elem *e, void *aux); static bool load (const char *cmdline, void (**eip) (void), void **esp); /* Starts a new thread running a user program executed via @@ -315,32 +317,15 @@ push_to_stack (void **esp, void *data, size_t data_size) int process_wait (tid_t child_tid) { - struct process_result *child_result = NULL; - struct list_elem *e; - struct thread *cur = thread_current (); - - for (e = list_begin (&cur->child_results); - e != list_end (&cur->child_results); e = list_next (e)) - { - struct process_result *result - = list_entry (e, struct process_result, elem); - if (result->tid == child_tid) - { - /* Found the child process. */ - child_result = result; - break; - } - - /* List is ordered, allowing us to break early if the child_tid is - greater than the current result's tid. */ - else if (result->tid > child_tid) - break; - } - - /* If the child process was not found, return -1. */ - if (child_result == NULL) + struct thread *t = thread_current (); + struct process_result fake_result; + fake_result.tid = child_tid; + struct hash_elem *e = hash_find (&t->child_results, &fake_result.elem); + if (e == NULL) return -1; + struct process_result *child_result + = hash_entry (e, struct process_result, elem); /* Wait for child to die. */ sema_down (&child_result->sema); @@ -348,18 +333,17 @@ process_wait (tid_t child_tid) wait) for it here to ensure we don't free the lock memory before it is released in process_exit. */ lock_acquire (&child_result->lock); - - /* To prevent waiting for child twice, remove it from the list. + /* To prevent waiting for child twice, remove it from the table. No need to use lock since this is the only thread with access to the struct process_result now. */ - list_remove (&child_result->elem); + hash_delete (&t->child_results, &child_result->elem); /* Get the exit status of the child */ int exit_status = child_result->exit_status; /* Release the lock */ lock_release (&child_result->lock); - + /* Result no-longer used by parent, nor child. Deallocate it. */ free (child_result); return exit_status; } @@ -385,49 +369,12 @@ process_exit (void) lock_release (&filesys_lock); } - /* Update process result. */ + /* Update own process result. */ if (cur->result != NULL) - { - lock_acquire (&cur->result->lock); - cur->result->exit_status = cur->exit_status; - /* Parent has died, child has to free the struct process_result * */ - if (sema_try_down (&cur->result->sema)) - { - lock_release (&cur->result->lock); - free (cur->result); - } - /* Parent is still alive and will be the one to free the - struct process_result *, and may be waiting so call sema_up */ - else - { - sema_up (&cur->result->sema); - lock_release (&cur->result->lock); - } - } + destruct_process_result (&cur->result->elem, cur); /* Free child process results or signal parent's death. */ - struct list_elem *e; - for (e = list_begin (&cur->child_results); - e != list_end (&cur->child_results);) - { - struct process_result *result - = list_entry (e, struct process_result, elem); - struct list_elem *next = list_next (e); - lock_acquire (&result->lock); - /* Child has died (and was not waited for). Free the result. */ - if (sema_try_down (&result->sema)) - { - lock_release (&result->lock); - free (result); - } - /* Child is still alive, signal via sema that parent has died. */ - else - { - sema_up (&result->sema); - lock_release (&result->lock); - } - e = next; - } + hash_destroy (&cur->child_results, destruct_process_result); /* Destroy the current process's page directory and switch back to the kernel-only page directory. */ @@ -447,6 +394,34 @@ process_exit (void) } } +/* Destruct a process_result, with multi-thread awareness. Takes the + process_result->elem and current thread. + If the other thread is running, simply signals death. Otherwise + frees the result. + Also set's process_result->exit_status if the process result is FOR + the current thread. */ +static void +destruct_process_result (struct hash_elem *e, void *cur_) +{ + struct thread *cur = cur_; + struct process_result *result = hash_entry (e, struct process_result, elem); + lock_acquire (&result->lock); + /* Other thread has died (and was not waited for). Free the result. */ + if (sema_try_down (&result->sema)) + { + lock_release (&result->lock); + free (result); + } + /* Other thread is still alive, signal via sema that parent has died. */ + else + { + if (cur->tid == result->tid) + result->exit_status = cur->exit_status; + sema_up (&result->sema); + lock_release (&result->lock); + } +} + /* Sets up the CPU for running user code in the current thread. This function is called on every context switch. */