From ddcd59fdf8a744cc2b19b1d43e78b300e67320b3 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 02:52:38 +0000 Subject: [PATCH 1/8] Add child and own process result information to struct thread --- src/threads/thread.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/threads/thread.h b/src/threads/thread.h index 1c05030..cf35b82 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -4,6 +4,8 @@ #include #include #include +#include +#include "threads/synch.h" #include "threads/fixed-point.h" /* States in a thread's life cycle. */ @@ -29,6 +31,19 @@ typedef int tid_t; #define NICE_DEFAULT 0 /* Default niceness. */ #define NICE_MAX 20 /* Highest niceness. */ +/* A process result, synchronised between parent and child. */ +struct process_result +{ + tid_t tid; /* The tid of the child process. */ + int exit_status; /* The exit status of the child process. Initially set to + -1, then to 0 when parent dies, or to exit_status when + child dies (whichever happens first). */ + struct lock lock; /* Lock to synchronise access to the exit_status. */ + 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. */ +}; + /* A kernel thread or user process. Each thread structure is stored in its own 4 kB page. The @@ -108,6 +123,11 @@ struct thread int nice; /* Nice value for this thread */ fp32_t recent_cpu; /* Amount of time this process received */ + /* Process wait properties. */ + struct process_result *result; /* Result of the process. */ + struct list child_results; /* List of children's of this thread + process results. */ + /* Shared between thread.c and synch.c. */ struct list_elem elem; /* List element. */ From ec8547aec9f145e5ead34b9f55a78d5ff238df47 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 03:31:48 +0000 Subject: [PATCH 2/8] Implement creation of process results --- src/threads/thread.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/threads/thread.c b/src/threads/thread.c index 457f7b9..ee76718 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -9,6 +9,7 @@ #include "threads/flags.h" #include "threads/interrupt.h" #include "threads/intr-stubs.h" +#include "threads/malloc.h" #include "threads/palloc.h" #include "threads/switch.h" #include "threads/synch.h" @@ -68,6 +69,7 @@ static void kernel_thread (thread_func *, void *aux); static void idle (void *aux UNUSED); static struct thread *running_thread (void); static struct thread *next_thread_to_run (void); +static void init_process_result (struct thread *t); static void init_thread (struct thread *, const char *name, int nice, int priority, fp32_t recent_cpu); static bool is_thread (struct thread *) UNUSED; @@ -110,6 +112,7 @@ thread_init (void) initial_thread_recent_cpu); initial_thread->status = THREAD_RUNNING; initial_thread->tid = allocate_tid (); + initial_thread->result = NULL; /* Main thread cannot be waited for. */ } /* Starts preemptive thread scheduling by enabling interrupts. @@ -236,6 +239,7 @@ thread_create (const char *name, int priority, struct thread *parent_thread = thread_current (); init_thread (t, name, parent_thread->nice, priority, parent_thread->recent_cpu); tid = t->tid = allocate_tid (); + init_process_result (t); /* Prepare thread for first run by initializing its stack. Do this atomically so intermediate values for the 'stack' @@ -259,6 +263,10 @@ 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. */ + list_insert (&parent_thread->child_results, &t->result->elem); + /* Add to run queue. */ thread_unblock (t); @@ -632,6 +640,18 @@ is_thread (struct thread *t) return t != NULL && t->magic == THREAD_MAGIC; } +/* Allocate and initialise a process result for given thread. */ +static void +init_process_result (struct thread *t) +{ + struct process_result *result = malloc (sizeof (struct process_result)); + result->tid = t->tid; + result->exit_status = t->exit_status; + lock_init (&result->lock); + sema_init (&result->sema, 0); + t->result = result; +} + /* Does basic initialization of T as a blocked thread named NAME. */ static void @@ -660,6 +680,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority, t->priority = t->base_priority; t->exit_status = -1; + list_init (&t->child_results); old_level = intr_disable (); list_push_back (&all_list, &t->allelem); From 7349b4e66f3a4542d390a975fe41890d5b242d37 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 03:55:34 +0000 Subject: [PATCH 3/8] Fix typo list_insert -> list_push_back for thread.child_results --- src/threads/thread.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index ee76718..379894d 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -264,8 +264,8 @@ 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. */ - list_insert (&parent_thread->child_results, &t->result->elem); + thread. By the nature of increasing TIDs, this list is ordered. */ + list_push_back (&parent_thread->child_results, &t->result->elem); /* Add to run queue. */ thread_unblock (t); From fde70dcf5934aa083c7513e47b316024ef3831d2 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 03:56:05 +0000 Subject: [PATCH 4/8] Implement process_wait. --- src/threads/thread.h | 5 ++- src/userprog/process.c | 76 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/src/threads/thread.h b/src/threads/thread.h index cf35b82..6465d53 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -36,9 +36,8 @@ struct process_result { tid_t tid; /* The tid of the child process. */ int exit_status; /* The exit status of the child process. Initially set to - -1, then to 0 when parent dies, or to exit_status when - child dies (whichever happens first). */ - struct lock lock; /* Lock to synchronise access to the exit_status. */ + -1, then to exit_status when child dies. */ + struct lock lock; /* Lock to synchronise access to the 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. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index 6c8ef2d..5b41a34 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -1,6 +1,7 @@ #include "userprog/process.h" #include #include +#include #include #include #include @@ -202,12 +203,34 @@ start_process (void *file_name_) int process_wait (tid_t child_tid UNUSED) { - /* As a temporary wait, waiting will just put the thread to sleep for one - second (TIMER_FREQ = 100 ticks ~ 1 second). */ - /* TODO: Implement process_wait () correctly. Remove the next line. */ - timer_sleep (TIMER_FREQ); - - return 0; /* TODO: Change this too */ + 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) + { + child_result = result; + break; + } + /* List is ordered, allowing us to break early. */ + else if (result->tid > child_tid) + break; + } + if (child_result == NULL) + return -1; + /* Wait for child to die. */ + sema_down (&child_result->sema); + /* To prevent waiting for child twice, remove it from the list. + No need to use lock since this is the only thread with access to + the struct process_result now. */ + list_remove (&child_result->elem); + int exit_status = child_result->exit_status; + free (child_result); + return exit_status; } /* Free the current process's resources. */ @@ -219,6 +242,47 @@ process_exit (void) printf ("%s: exit(%d)\n", cur->name, cur->exit_status); + /* Update 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); + } + } + + struct list_elem *e; + 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); + 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); + } + } + /* Destroy the current process's page directory and switch back to the kernel-only page directory. */ pd = cur->pagedir; From d95894085b0b0d9305561fde7510df262ea21102 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 09:15:22 +0000 Subject: [PATCH 5/8] Implement syscall_exec via process_execute --- src/userprog/syscall.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/userprog/syscall.c b/src/userprog/syscall.c index 1dbe73b..03e0721 100644 --- a/src/userprog/syscall.c +++ b/src/userprog/syscall.c @@ -110,10 +110,14 @@ syscall_exit (int status) } static pid_t -syscall_exec (const char *cmd_line UNUSED) +syscall_exec (const char *cmd_line) { - //TODO - return 0; + /* To check the end we would need to traverse the null-terminate string, + which is equally unsafe as just leaving process_execute to do it. */ + cmd_line = validate_user_pointer (cmd_line, 1); + if (cmd_line == NULL) + thread_exit (); + process_execute (cmd_line); } static int From 84fe637c7e284dd8dab7e2a7ce036857900e1926 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 09:16:18 +0000 Subject: [PATCH 6/8] Remove process_result lock since it is an invalid solution TODO : synchronise process_result in another way --- src/threads/thread.c | 1 - src/threads/thread.h | 1 - src/userprog/process.c | 22 ++++------------------ 3 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index 379894d..aecafdf 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -647,7 +647,6 @@ init_process_result (struct thread *t) struct process_result *result = malloc (sizeof (struct process_result)); result->tid = t->tid; result->exit_status = t->exit_status; - lock_init (&result->lock); sema_init (&result->sema, 0); t->result = result; } diff --git a/src/threads/thread.h b/src/threads/thread.h index 6465d53..f6227f5 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -37,7 +37,6 @@ struct process_result tid_t tid; /* The tid of the child process. */ int exit_status; /* The exit status of the child process. Initially set to -1, then to exit_status when child dies. */ - struct lock lock; /* Lock to synchronise access to the 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. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index 5b41a34..1db334a 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -245,21 +245,14 @@ process_exit (void) /* Update 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); - } + 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); - } + sema_up (&cur->result->sema); } struct list_elem *e; @@ -268,19 +261,12 @@ process_exit (void) { struct process_result *result = list_entry (e, struct process_result, elem); - 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); - } + free (result); /* Child is still alive, signal via sema that parent has died. */ else - { - sema_up (&result->sema); - lock_release (&result->lock); - } + sema_up (&result->sema); } /* Destroy the current process's page directory and switch back From 6ed1ccd21ea012ffb37d8c8d47e87b5f90979fc8 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Fri, 8 Nov 2024 10:50:10 +0000 Subject: [PATCH 7/8] Fix process_result locking by acquiring in process_wait as well to prevent freeing memory too early --- src/threads/thread.c | 1 + src/threads/thread.h | 7 ++++--- src/userprog/process.c | 27 +++++++++++++++++++++++---- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/threads/thread.c b/src/threads/thread.c index aecafdf..379894d 100644 --- a/src/threads/thread.c +++ b/src/threads/thread.c @@ -647,6 +647,7 @@ init_process_result (struct thread *t) struct process_result *result = malloc (sizeof (struct process_result)); result->tid = t->tid; result->exit_status = t->exit_status; + lock_init (&result->lock); sema_init (&result->sema, 0); t->result = result; } diff --git a/src/threads/thread.h b/src/threads/thread.h index f6227f5..2947256 100644 --- a/src/threads/thread.h +++ b/src/threads/thread.h @@ -34,9 +34,10 @@ typedef int tid_t; /* A process result, synchronised between parent and child. */ struct process_result { - tid_t tid; /* The tid of the child process. */ - int exit_status; /* The exit status of the child process. Initially set to - -1, then to exit_status when child dies. */ + tid_t tid; /* The tid of the child process. */ + int exit_status; /* The exit status of the child process. Initially set + to -1, then to exit_status when child dies. */ + 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. */ diff --git a/src/userprog/process.c b/src/userprog/process.c index 1db334a..370a216 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -224,11 +224,16 @@ process_wait (tid_t child_tid UNUSED) return -1; /* Wait for child to die. */ sema_down (&child_result->sema); + /* We need lock release in process_exit, so we need to acquire (and possibly + 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. No need to use lock since this is the only thread with access to the struct process_result now. */ list_remove (&child_result->elem); int exit_status = child_result->exit_status; + lock_release (&child_result->lock); free (child_result); return exit_status; } @@ -245,14 +250,21 @@ process_exit (void) /* Update 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)) - free (cur->result); + { + 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); + { + sema_up (&cur->result->sema); + lock_release (&cur->result->lock); + } } struct list_elem *e; @@ -261,12 +273,19 @@ process_exit (void) { struct process_result *result = list_entry (e, struct process_result, elem); + lock_acquire (&result->lock); /* Child has died (and was not waited for). Free the result. */ if (sema_try_down (&result->sema)) - free (result); + { + lock_release (&result->lock); + free (result); + } /* Child is still alive, signal via sema that parent has died. */ else - sema_up (&result->sema); + { + sema_up (&result->sema); + lock_release (&result->lock); + } } /* Destroy the current process's page directory and switch back From 7778e05aa42e7fa8c6f0e2aee1924f3c2df64ec8 Mon Sep 17 00:00:00 2001 From: Gleb Koval Date: Sat, 9 Nov 2024 11:06:36 +0000 Subject: [PATCH 8/8] Fix deadlock by release of lock and semaphore in the wrong order --- src/userprog/process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/userprog/process.c b/src/userprog/process.c index 370a216..3943b21 100644 --- a/src/userprog/process.c +++ b/src/userprog/process.c @@ -262,8 +262,8 @@ process_exit (void) struct process_result *, and may be waiting so call sema_up */ else { - sema_up (&cur->result->sema); lock_release (&cur->result->lock); + sema_up (&cur->result->sema); } } @@ -283,8 +283,8 @@ process_exit (void) /* Child is still alive, signal via sema that parent has died. */ else { - sema_up (&result->sema); lock_release (&result->lock); + sema_up (&result->sema); } }