From 1a2ff35231eb01a745342e24fdd12be632f985a5 Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Thu, 14 Nov 2024 14:32:48 +0000 Subject: [PATCH 1/4] Refactor process_execute to pass process start data as a local reference rather than perform memory allocation on the heap --- src/userprog/process.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index e1200bc..ddd1997 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -65,13 +65,10 @@ process_execute (const char *cmd) char *cmd_copy; tid_t tid; - struct process_start_data *data = malloc (sizeof (struct process_start_data)); - if (data == NULL) - { - return TID_ERROR; - } - sema_init (&data->loaded, 0); - data->success = false; + struct process_start_data data; + + sema_init (&data.loaded, 0); + data.success = false; /* Make a copy of command. Otherwise there's a race between the caller and load(). */ @@ -85,7 +82,7 @@ process_execute (const char *cmd) /* Retrieve first argument of command, which is the file name of the process. */ - char *file_name = strtok_r (cmd_copy, " ", &data->cmd_saveptr); + char *file_name = strtok_r (cmd_copy, " ", &data.cmd_saveptr); /* NOTE: Currently, the file being executed is closed in load () and then reopened here. Because load is an exported public function, this @@ -100,19 +97,18 @@ process_execute (const char *cmd) /* Create a new thread to execute the command, by initializing it running the function 'start_process' with the appropriate arguments. For details of arguments, see 'start_process'. */ - data->cmd = cmd_copy; - strlcpy (data->file_name, file_name, FNAME_MAX_LEN + 1); + data.cmd = cmd_copy; + strlcpy (data.file_name, file_name, FNAME_MAX_LEN + 1); - tid = thread_create (file_name, PRI_DEFAULT, start_process, data); + tid = thread_create (file_name, PRI_DEFAULT, start_process, &data); if (tid == TID_ERROR) palloc_free_page (cmd_copy); else { - sema_down (&data->loaded); - if (!data->success) + sema_down (&data.loaded); + if (!data.success) tid = TID_ERROR; } - free (data); return tid; } From 882185145944e90e5a637c2c4023850208e80a8a Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Thu, 14 Nov 2024 14:42:26 +0000 Subject: [PATCH 2/4] Refactor process_execute to remove use of 'goto' --- src/userprog/process.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index ddd1997..9f404c7 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -67,9 +67,6 @@ process_execute (const char *cmd) struct process_start_data data; - sema_init (&data.loaded, 0); - data.success = false; - /* Make a copy of command. Otherwise there's a race between the caller and load(). */ cmd_copy = palloc_get_page (0); @@ -99,6 +96,8 @@ process_execute (const char *cmd) arguments. For details of arguments, see 'start_process'. */ data.cmd = cmd_copy; strlcpy (data.file_name, file_name, FNAME_MAX_LEN + 1); + sema_init (&data.loaded, 0); + data.success = false; tid = thread_create (file_name, PRI_DEFAULT, start_process, &data); if (tid == TID_ERROR) @@ -126,7 +125,6 @@ static void start_process (void *proc_start_data) { struct intr_frame if_; - bool success; struct process_start_data *data = proc_start_data; @@ -143,34 +141,36 @@ start_process (void *proc_start_data) if (exec_file == NULL) { lock_release (&filesys_lock); - goto fail; + sema_up (&data->loaded); + thread_exit (); } thread_current ()->exec_file = exec_file; file_deny_write (exec_file); lock_release (&filesys_lock); - success = load (data->file_name, &if_.eip, &if_.esp); + data->success = load (data->file_name, &if_.eip, &if_.esp); /* If load failed, quit. */ - if (!success) + if (!data->success) { palloc_free_page (data->cmd); - goto fail; + sema_up (&data->loaded); + thread_exit (); } /* Initialize user process stack and free page used to store the command that executed the process. */ - success = process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name); + data->success = process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name); + bool success = data->success; palloc_free_page (data->cmd); + sema_up (&data->loaded); /* If stack initialization failed, free resources and quit. */ if (!success) { - goto fail; + thread_exit (); } - data->success = true; - sema_up (&data->loaded); /* Start the user process by simulating a return from an interrupt, implemented by intr_exit (in threads/intr-stubs.S). Because intr_exit takes all of its @@ -179,12 +179,6 @@ start_process (void *proc_start_data) and jump to it. */ asm volatile ("movl %0, %%esp; jmp intr_exit" : : "g" (&if_) : "memory"); NOT_REACHED (); - - /* If starting the process failed, exit. */ -fail: - data->success = false; - sema_up (&data->loaded); - thread_exit (); } /* Helper function that initializes the stack of a newly created From 3a46e0f73aed30f949b25402bddf0dacffc0138c Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Thu, 14 Nov 2024 15:54:34 +0000 Subject: [PATCH 3/4] Update user proc stack initialization comments to be more helpful --- src/userprog/process.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 9f404c7..65223c1 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -49,7 +49,7 @@ struct process_start_data be started. */ bool success; /* Indicates whether the process was successfully loaded. */ struct semaphore loaded; /* Semaphore used to signal that the process has - been loaded. */ + finished attempting to load. */ }; static thread_func start_process NO_RETURN; @@ -64,7 +64,6 @@ process_execute (const char *cmd) { char *cmd_copy; tid_t tid; - struct process_start_data data; /* Make a copy of command. @@ -81,11 +80,8 @@ process_execute (const char *cmd) of the process. */ char *file_name = strtok_r (cmd_copy, " ", &data.cmd_saveptr); - /* NOTE: Currently, the file being executed is closed in load () and then - reopened here. Because load is an exported public function, this - might be necessary. */ + /* Validates that the current file to be executed can be opened/exists. */ lock_acquire (&filesys_lock); - /* Validates that the current file to be executed is a valid file */ bool valid_file = filesys_open (file_name) != NULL; lock_release (&filesys_lock); if (!valid_file) @@ -100,14 +96,20 @@ process_execute (const char *cmd) data.success = false; tid = thread_create (file_name, PRI_DEFAULT, start_process, &data); + + /* Return TID_ERROR and free resources if starting execution went wrong. */ if (tid == TID_ERROR) palloc_free_page (cmd_copy); else { + + /* Wait until process file has finished attempting to load via the child + thread before reporting success of starting execution. */ sema_down (&data.loaded); if (!data.success) tid = TID_ERROR; } + return tid; } @@ -119,8 +121,11 @@ static void *push_to_stack (void **esp, void *data, size_t data_size); /* Make the current thread execute 'cmd', passing in a copy of the command string used for processing, the saveptr used by strtok_r (in order to further tokenize the same command and retrieve its - arguments), as well as the name of the file being executed. This - involves loading the specified file and starting it running. */ + arguments), the name of the file being executed, and a semaphore that + calls sema_up to indicate that the 'success' variable passed to it + has been updated to indicate whether the process file loading succeeded. + This involves loading the specified file and calling its main () function + with the specified command arguments. */ static void start_process (void *proc_start_data) { From e4036c715fade8e60addad08499fe076ac7dcdeb Mon Sep 17 00:00:00 2001 From: Themis Demetriades Date: Thu, 14 Nov 2024 16:05:15 +0000 Subject: [PATCH 4/4] Refactor start_process to hold file system lock for less time --- src/userprog/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 65223c1..5c0b356 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -149,9 +149,9 @@ start_process (void *proc_start_data) sema_up (&data->loaded); thread_exit (); } - thread_current ()->exec_file = exec_file; file_deny_write (exec_file); lock_release (&filesys_lock); + thread_current ()->exec_file = exec_file; data->success = load (data->file_name, &if_.eip, &if_.esp);