Clean up code and update design doc

All tests passing on qemu.
multi-oom hangs on bochs and syn-read fails.

src/threads/thread.c

@@ -364,19 +364,17 @@ void thread_exit(void) {
        and schedule another process.  That process will destroy us
        when it calls thread_schedule_tail(). */
     struct thread *cur = thread_current();
+    struct list_elem *e;
 
     /* Tell blocking lock we are no longer waiting for it. */
     if (cur->blocking_lock != NULL) {
         list_remove(&cur->lock_elem);
     }
 
     /* Free all locks. */
-    struct list_elem *e;
-    for (e = list_begin(&cur->locks_acquired);
-         e != list_end(&cur->locks_acquired);
-         /* increment in loop */) {
+    while (!list_empty(&cur->locks_acquired)) {
+        e = list_begin(&cur->locks_acquired);
         struct lock *lock = list_entry(e, struct lock, elem);
-        e = list_next(e);
         lock_release(lock);
     }
 
@@ -397,7 +395,7 @@ void thread_exit(void) {
     process_exit();
 #endif
 
-    if ((cur->parent != NULL) && (cur->parent != initial_thread)) {
+    if (cur->parent != NULL) {
         list_remove(&cur->kid_elem);
     }
 
@@ -610,7 +608,8 @@ int add_open_file(struct thread *cur, struct file *file, int fd) {
     struct sys_file *new_file = palloc_get_page(PAL_ZERO);
     /* Not enough memory. */
     if (new_file == NULL) {
-        return -1;
+        file_close(file);
+        return ERR;
     }
     memset(new_file, 0, sizeof *new_file);
     new_file->file = file;
@@ -622,7 +621,7 @@ int add_open_file(struct thread *cur, struct file *file, int fd) {
     }
 
     /* Couldn't find file. */
-    return -1;
+    return ERR;
 }
 
 /*! Get file with file descriptor *fd*. */

src/userprog/DESIGNDOC

@@ -12,20 +12,23 @@ Justin Leong <jleong@caltech.edu>
 John Li <jzli@caltech.edu>
 Christina Lin <cylin@caltech.edu>
 
->> Specify how many late tokens you are using on this assignment: 2
+>> Specify how many late tokens you are using on this assignment: 4
 
 >> What is the Git repository and commit hash for your submission?
    (You only need to include the commit-hash in the file you submit
    on Moodle.)
 
    Repository URL: https://github.com/omegaphoenix/cheetOS
-   commit: aa0dfcc3c89b57c3faf9934ee469aa02a013e5ff
+   commit: 
+   tag: project4-3
 
 ---- PRELIMINARIES ----
 
 >> If you have any preliminary comments on your submission, notes for the
 >> TAs, or extra credit, please give them here.
 
+All tests pass on qemu.
+
 >> Please cite any offline or online sources you consulted while
 >> preparing your submission, other than the Pintos documentation, course
 >> text, lecture notes, and course instructors.
@@ -57,8 +60,7 @@ which parts.
 >> L1: How many hours did each team member spend on this assignment?
    Make sure that each member's total time is listed.
 
-  Justin Leong: Approximately 31:22:50 (hh/mm/ss) (45 hours total including
-  time after deadline)
+  Justin Leong: At least 51:22:55 (hh/mm/ss)
   John Li: Approximately 8 hours. Was busy planning and executing several events this week.
   Christina Lin: Approximately 26-27 hours.
 
@@ -133,15 +135,16 @@ can have two levels of validation.
 We added this variable to process.c to synchronize filesystem calls.
     struct lock filesys_lock;          /*! Lock that belongs to process. */
 
-In our thread struct, we included these attributes and their following descriptions:
-
+We added this struct to thread.h for the list of open files.
     /* Open file. This is for a linked list of open files in each thread. */
     struct sys_file {
         struct file *file;
         int fd;
         struct list_elem file_elem;
     };
 
+In our thread struct, we included these attributes and their following descriptions:
+
     /*! Shared between thread.c and userprog/syscall.c. */
     /**@{*/
     int num_files;                      /*!< Number of open files (counting closed ones). */
@@ -154,7 +157,9 @@ In our thread struct, we included these attributes and their following descripti
     struct list kids;                   /*!< List of children processes. */
     struct list_elem kid_elem;          /*!< List element for parent's kids list. */
     struct semaphore wait_sema;         /*!< Sempahore for process_wait. */
+    struct semaphore done_sema;         /*!< Sempahore for process_exit. */
     bool waited_on;                     /*!< True if process_wait has been called. */
+    struct lock wait_lock;              /*!< Lock for checking waited_on. */
     /**@}*/
 
     /*! Shared between by userprog/process.c and userprog.syscall.c and
@@ -171,18 +176,18 @@ In our thread struct, we included these attributes and their following descripti
 >> Are file descriptors unique within the entire OS or just within a
 >> single process?
 
-File descriptors are only unique within a single process. The thread contains
-an array of pointers to open files and finds an empty (null) index to place
-an open file in. When an open file is closed, then it's spot in the array
-is set to NULL.
+File descriptors are only unique within a single thread. The thread contains
+a list of open files using sys_file. Every time a new file is opened num_files
+is incremented and then the new file gets a fd of num_files + 2 before it is
+incremented since 0 and 1 are reserved for stdin and stdout file descriptors.
 
 ---- ALGORITHMS ----
 
 >> B3: Describe your code for reading and writing user data from the
 >> kernel.
 
 We use valid_read_addr to check the pointer to the buffer and the buffer. If
-we are reading/writing from/to a file, we get the file form the thread and
+we are reading/writing from/to a file, we get the file from the thread and
 make the filesys call. If we are reading/writing from/to the terminal, we
 read one byte at a time and we output 300 bytes at a time.
 
@@ -229,7 +234,9 @@ functions.
 
 When an error is detected, we free allocated resources by calling sys_exit()
 which calls thread_exit() which iterates through all of its open files and
-locks and frees them. The executable is also freed here.
+locks and frees them. thread_exit() then calls process_exit() and the
+executable is freed here. If we know no files have been allocated we can call
+thread_exit() directly.
 
 Example: If an argument is not a valid read address, we call sys_exit(-1)
 which sets the exit status to -1 and then calls thread_exit(). thread_exit()
@@ -243,7 +250,7 @@ that it is waiting on that it is no longer waiting.
 >> loading.  How does your code ensure this?  How is the load
 >> success/failure status passed back to the thread that calls "exec"?
 
-We add a semaphore which is blocked initially and so sys_exec() waits
+We add a wait_sema semaphore which is blocked initially and so sys_exec() waits
 on this sempahore before returning. The semaphore is up'ed when the
 executable has completed loading and the thread has a boolean "loaded"
 which is true if successful. Once sys_exec() resumes, it can check
@@ -258,20 +265,27 @@ possibly free the thread.
 >> terminates without waiting, before C exits?  After C exits?  Are
 >> there any special cases?
 
-When P calls wait(C) before C exits, the kid's status is not set to
-THREAD_DYING yet and so P waits on a semaphore that is initialized to 0
-and which is incremented when the kid is ready to die. Then it gets the
-exit status and frees the kid.
+When P calls wait(C) before C exits, C is not finished yet so it has not
+up'ed C->wait_sema. So P is blocked on C->wait_sema. When C calls
+process_exit() when it is done, it up's C->wait_sema and tries to down
+C->done_sema which is set to 0 when C is created. Then P retrieves
+C->exit_status and up's C->done_sema which tells C that it can now free
+it's memory.
 
-After C exits, if it hasn't been freed by a previous wait call,
-P just gets the exit status and frees the kid. If it has been freed, P
-returns -1;
+After C exits, if it hasn't been freed by a previous wait call, it waits
+on C->done_sema which is 0 and thus blocking. When P makes the wait call,
+P just gets the exit status and frees the kid by up'ing C->done_sema.
+If wait has already been called on C, P returns -1;
 
 When P terminates without waiting, it tells each one of it's kids that they
-don't have a parent anymore by setting their struct thread *parent to NULL.
-If this is before C exits, then when C exits, thread_schedule_tail() will
-free the kid. If this is after C exits, then the kid is freed right away
-before the parent dies.
+don't have a parent anymore by setting their struct thread *parent to NULL
+and up'ing their C->done_sema. Then when C calls process_exit(), it will down
+the C->done_sema semaphore and since it was 1, it will be able to continue
+and free it's page.
+
+The special case is the initial thread where we need to initialize the
+done_sema to 1 so that when it exits, it is not waiting on a parent to
+up it's done_sema.
 
 ---- RATIONALE ----
 
@@ -288,6 +302,7 @@ distinguishes between read and write for clarity.
 >> B10: What advantages or disadvantages can you see to your design
 >> for file descriptors?
 
+Disadvantages of current design:
 The first design we used is to use an array of pointers to files. The advantage
 of this approach is that it is quick and easy to access open_files from the array
 since fd maps directly to an index (by subtracting 2 for stdin and stout).
@@ -297,6 +312,7 @@ processes. Using a linked list would reduce the space requirement on the
 thread struct but would increase the access time and complexity of getting the
 file associated with a file descriptor.
 
+Advantages of current design:
 We reimplemented to use a linked list instead which solved some of our memory
 management issues and allowed us to handle as many files as necessary. We
 created a struct to make this implementation easier which kept track of the

src/userprog/process.c

@@ -38,14 +38,14 @@ tid_t process_execute(const char *cmdline) {
 
     /* Make a copy of CMDLINE.
        Otherwise there's a race between the caller and load(). */
-    cmdline_copy = palloc_get_page(0);
+    cmdline_copy = palloc_get_page(PAL_ZERO);
     if (cmdline_copy == NULL) {
         return TID_ERROR;
     }
     strlcpy(cmdline_copy, cmdline, PGSIZE);
 
     /* Get FILE_NAME from CMDLINE */
-    cmdline_copy2 = palloc_get_page(0);
+    cmdline_copy2 = palloc_get_page(PAL_ZERO);
     if (cmdline_copy2 == NULL) {
         palloc_free_page(cmdline_copy);
         return TID_ERROR;
@@ -154,14 +154,14 @@ int process_wait(tid_t child_tid UNUSED) {
 
     /* Check if no kid with child_tid. */
     if (kid == NULL || kid->tid != child_tid || kid->parent != cur) {
-        return -1;
+        return ERR;
     }
 
     /* Lock in case of interrupt. */
     lock_acquire(&kid->wait_lock);
     if (kid->waited_on) {
         lock_release(&kid->wait_lock);
-        return -1;
+        return ERR;
     }
     kid->waited_on = true;
     lock_release(&kid->wait_lock);
@@ -191,9 +191,6 @@ void process_exit(void) {
 
     /* Let parent know it is done. */
     sema_up(&cur->wait_sema);
-    while(!list_empty(&cur->wait_sema.waiters)) {
-        sema_up(&cur->wait_sema);
-    }
     /* Wait for parent to retrieve exit status. */
     ASSERT(cur->parent != NULL || cur->done_sema.value > 0);
     sema_down(&cur->done_sema);

src/userprog/syscall.c

@@ -263,11 +263,7 @@ int sys_open(const char *file) {
     fd = add_open_file(cur, open_file, fd);
     release_file_lock();
 
-    if (fd == -1) {
-        acquire_file_lock();
-        file_close(open_file);
-        release_file_lock();
-    }
+    ASSERT(fd >= CONSOLE_FD || fd == ERR);
     return fd;
 }
 
@@ -419,7 +415,7 @@ void sys_close(int fd) {
 static bool valid_read_addr(const void *addr) {
     /* Check that address is below PHYS_BASE
        and then attempt to read a byte at the address */
-    return addr != NULL && is_user_vaddr(addr) && (get_user(addr) != -1);
+    return addr != NULL && is_user_vaddr(addr) && (get_user(addr) != ERR);
 }
 
 /* Returns true if addr is valid for writing */
@@ -446,5 +442,5 @@ static bool put_user (uint8_t *udst, uint8_t byte) {
     int error_code;
     asm ("movl $1f, %0; movb %b2, %1; 1:"
          : "=&a" (error_code), "=m" (*udst) : "q" (byte));
-    return error_code != -1;
+    return error_code != ERR;
 }