From b413e78bacc4a0331191e581cd060281ba47c54a Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 2 Sep 2004 21:40:16 +0000 Subject: [PATCH] Add comments. --- src/threads/palloc.c | 9 ++++++ src/threads/thread.c | 21 ++++++++++++-- src/threads/thread.h | 65 +++++++++++++++++++++++++++++++++++++++----- 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/src/threads/palloc.c b/src/threads/palloc.c index a25c09e..2b07ad7 100644 --- 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 2694a10..52f0f8d 100644 --- 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 e8927de..a70b48b 100644 --- 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. */ }; -- 2.30.2