Add comments.

diff --git a/src/threads/palloc.c b/src/threads/palloc.c
index a25c09e0715d3140ff3f122f051aff0539693daa..2b07ad769c139ff3f7f661980936fbde0e6eea5e 100644 (file)
--- a/src/threads/palloc.c
+++ b/src/threads/palloc.c
@@ -58,6 +58,10 @@ palloc_init (void)
   list_init (&free_pages);
 }
 
+/* Obtains and returns a free page.  If PAL_ZERO is set in FLAGS,
+   then the page is filled with zeros.  If no pages are
+   available, returns a null pointer, unless PAL_ASSERT is set in
+   FLAGS, in which case the kernel panics. */
 void *
 palloc_get (enum palloc_flags flags)
 {
@@ -65,6 +69,10 @@ palloc_get (enum palloc_flags flags)
 
   lock_acquire (&lock);
 
+  /* If there's a page in the free list, take it.
+     Otherwise, if there's a page not yet added to the free list,
+     use it.
+     Otherwise, we're out of memory. */
   if (!list_empty (&free_pages))
     page = list_entry (list_pop_front (&free_pages), struct page, free_elem);
   else if (uninit_start < uninit_end) 
@@ -91,6 +99,7 @@ palloc_get (enum palloc_flags flags)
   return page;
 }
 
+/* Frees PAGE. */
 void
 palloc_free (void *page_) 
 {

diff --git a/src/threads/thread.c b/src/threads/thread.c
index 2694a10959ba9ccd9965f498197ec5add8905a5f..52f0f8d4fc73d8f65818163aaca9c7229cf8e97e 100644 (file)
--- a/src/threads/thread.c
+++ b/src/threads/thread.c
@@ -12,6 +12,9 @@
 #include "gdt.h"
 #endif
 
+/* Value for struct thread's `magic' member.
+   Used to detect stack overflow.  See the big comment at the top
+   of thread.h for details. */
 #define THREAD_MAGIC 0x1234abcdu
 
 /* List of processes in THREAD_READY state, that is, processes
@@ -194,7 +197,8 @@ thread_name (struct thread *t)
 }
 
 /* Returns the running thread.
-   This is running_thread() plus a couple of sanity checks. */
+   This is running_thread() plus a couple of sanity checks.
+   See the big comment at the top of thread.h for details. */
 struct thread *
 thread_current (void) 
 {
@@ -218,6 +222,8 @@ thread_exit (void)
 {
   ASSERT (!intr_context ());
 
+  /* Just set our status to dying and schedule another process.
+     We will be destroyed during the call to schedule_tail(). */
   intr_disable ();
   thread_current ()->status = THREAD_DYING;
   schedule ();
@@ -242,7 +248,11 @@ thread_yield (void)
 }
 
 /* Puts the current thread to sleep.  It will not be scheduled
-   again until awoken by thread_unblock(). */
+   again until awoken by thread_unblock().
+
+   This function must be called with interrupts turned off.  It
+   is usually a better idea to use one of the synchronization
+   primitives in synch.h. */
 void
 thread_block (void) 
 {
@@ -319,7 +329,7 @@ new_thread (const char *name)
   return t;
 }
 
-/* Initializes T as a new thread named NAME. */
+/* Initializes T as a new, blocked thread named NAME. */
 static void
 init_thread (struct thread *t, const char *name)
 {
@@ -391,12 +401,17 @@ schedule_tail (struct thread *prev)
   
   ASSERT (intr_get_level () == INTR_OFF);
 
+  /* Mark us as running. */
   cur->status = THREAD_RUNNING;
 
 #ifdef USERPROG
+  /* Activate the new address space. */
   addrspace_activate (cur);
 #endif
 
+  /* If the thread we switched from is dying, destroy it.
+     This must happen late because it's not a good idea to
+     e.g. destroy the page table you're currently using. */
   if (prev != NULL && prev->status == THREAD_DYING) 
     destroy_thread (prev);
 }

diff --git a/src/threads/thread.h b/src/threads/thread.h
index e8927def25ebc99594a541425096ddc6eff8c4e4..a70b48b50cb0305f2147089a0b4db17948e8de46 100644 (file)
--- a/src/threads/thread.h
+++ b/src/threads/thread.h
@@ -9,15 +9,66 @@
 #include "addrspace.h"
 #endif
 
-enum thread_status 
+/* States in a thread's life cycle. */
+enum thread_status
   {
-    THREAD_RUNNING,
-    THREAD_READY,
-    THREAD_BLOCKED,
-    THREAD_DYING
+    THREAD_RUNNING,     /* Running thread. */
+    THREAD_READY,       /* Not running but ready to run. */
+    THREAD_BLOCKED,     /* Waiting for an event to trigger. */
+    THREAD_DYING        /* About to be destroyed. */
   };
 
-struct thread 
+/* A kernel thread or user process.
+
+   Each thread structure is stored in its own 4 kB page.  The
+   thread structure itself sits at the very bottom of the page
+   (at offset 0).  The rest of the page is reserved for the
+   thread's kernel stack, which grows downward from the top of
+   the page (at offset 4 kB).  Here's an illustration:
+
+        4 kB +---------------------------------+
+             |          kernel stack           |
+             |                |                |
+             |                |                |
+             |                V                |
+             |         grows downward          |
+             |                                 |
+             |                                 |
+             |                                 |
+             |                                 |
+             |                                 |
+             |                                 |
+             |                                 |
+             |                                 |
+             +---------------------------------+
+             |             magic               |
+             |               :                 |
+             |               :                 |
+             |              name               |
+             |             status              |
+        0 kB +---------------------------------+
+
+   The upshot of this is twofold:
+
+      1. First, `struct thread' must not be allowed to grow too
+         big.  If it does, then there will not be enough room for
+         the kernel stack.  Our base `struct thread' is only a
+         few bytes in size.  It probably should stay well under 1
+         kB.
+
+      2. Second, kernel stacks must not be allowed to grow too
+         large.  If a stack overflows, it will corrupt the thread
+         state.  Thus, kernel functions should not allocate large
+         structures or arrays as non-static local variables.  Use
+         dynamic allocation with malloc() or palloc_get()
+         instead.
+
+   The first symptom of either of these problems will probably be
+   an assertion failure in thread_current(), which checks that
+   the `magic' member of the running thread's `struct thread' is
+   set to THREAD_MAGIC.  Stack overflow will normally change this
+   value, triggering the assertion. */
+struct thread
   {
     /* These members are owned by the thread_*() functions. */
     enum thread_status status;          /* Thread state. */
@@ -29,7 +80,7 @@ struct thread
     /* These members are owned by the addrspace_*() functions. */
     uint32_t *pagedir;                  /* Page directory. */
 #endif
-    
+
     /* Marker to detect stack overflow. */
     unsigned magic;                     /* Always set to THREAD_MAGIC. */
   };