Fix two bugs in the base Pintos code:

1. Idle thread doesn't get initialized well: it hangs around on the
   ready list until we have idle time.  Fix by using a semaphore to
   make sure the idle thread gets initialized.

2. After idle thread does get initialized, it can get put back on the
   ready list if you yield from the timer interrupt.  Fix by not ever
   putting the idle thread on the ready list.

Plus fix a bug in the sample solution that tends to mask #2:

3. Doesn't yield from timer interrupt when unblocking a thread.  Fix
   by yielding from timer interrupt when unblocking a thread (of
   higher priority).

Also, fix something that confused students:

4. Rename enable_mlfqs to thread_mlfqs and move it to thread.c (to
   reduce confusion about where it is declared), and heavily comment
   to ensure that students know when it gets initialized.

These problems were drawn to my attention by Godmar Back.  Thanks!

diff --git a/doc/threads.texi b/doc/threads.texi
index f8cd04e704e2ccf1b83ddda41e4e27ab4e2b49fd..cbcda160631b2c918c23d9d81e9ce4277a10be23 100644 (file)
--- a/doc/threads.texi
+++ b/doc/threads.texi
@@ -522,7 +522,7 @@ policy at Pintos startup time.  By default, the priority scheduler
 must be active, but we must be able to choose the 4.4@acronym{BSD}
 scheduler
 with the @option{-mlfqs} kernel option.  Passing this
-option sets @code{enable_mlfqs}, declared in @file{threads/init.h}, to
+option sets @code{thread_mlfqs}, declared in @file{threads/thread.h}, to
 true when the options are parsed by @func{parse_options}, which happens
 midway through @func{main}.
 

diff --git a/src/tests/threads/alarm-priority.c b/src/tests/threads/alarm-priority.c
index 4839ecdfc81491d36c656ea4ef98761cf5206616..2288ff6d6950034db2eedadf3b44c04d60377103 100644 (file)
--- a/src/tests/threads/alarm-priority.c
+++ b/src/tests/threads/alarm-priority.c
@@ -19,7 +19,7 @@ test_alarm_priority (void)
   int i;
   
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   wake_time = timer_ticks () + 5 * TIMER_FREQ;
   sema_init (&wait_sema, 0);

diff --git a/src/tests/threads/alarm-simultaneous.c b/src/tests/threads/alarm-simultaneous.c
index 4ba3651f44cba4a3fca97bd6f7fe7915b286568f..844eea47f9cb2f4e3120fa68a60c54bb4c6c6c4e 100644 (file)
--- a/src/tests/threads/alarm-simultaneous.c
+++ b/src/tests/threads/alarm-simultaneous.c
@@ -37,7 +37,7 @@ test_sleep (int thread_cnt, int iterations)
   int i;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   msg ("Creating %d threads to sleep %d times each.", thread_cnt, iterations);
   msg ("Each thread sleeps 10 ticks each time.");

diff --git a/src/tests/threads/alarm-wait.c b/src/tests/threads/alarm-wait.c
index 458e987affa670f1088451dc4b0386f5145671f9..37d3afcea74243abcce4ccde4cb8a3590985c7c6 100644 (file)
--- a/src/tests/threads/alarm-wait.c
+++ b/src/tests/threads/alarm-wait.c
@@ -57,7 +57,7 @@ test_sleep (int thread_cnt, int iterations)
   int i;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   msg ("Creating %d threads to sleep %d times each.", thread_cnt, iterations);
   msg ("Thread 0 sleeps 10 ticks each time,");

diff --git a/src/tests/threads/mlfqs-block.c b/src/tests/threads/mlfqs-block.c
index 4e3fffa2f6dd6fa65d550b6c91abdd3925230d4b..6d4992da31866946e9d39e427f613d376616dc14 100644 (file)
--- a/src/tests/threads/mlfqs-block.c
+++ b/src/tests/threads/mlfqs-block.c
@@ -25,7 +25,7 @@ test_mlfqs_block (void)
   int64_t start_time;
   struct lock lock;
   
-  ASSERT (enable_mlfqs);
+  ASSERT (thread_mlfqs);
 
   msg ("Main thread acquiring lock.");
   lock_init (&lock);

diff --git a/src/tests/threads/mlfqs-fair.c b/src/tests/threads/mlfqs-fair.c
index 30282c36e32d15b0da44d074aa99093c69d3815c..f958f816460df7999873523e955ccd6a2d928354 100644 (file)
--- a/src/tests/threads/mlfqs-fair.c
+++ b/src/tests/threads/mlfqs-fair.c
@@ -69,7 +69,7 @@ test_mlfqs_fair (int thread_cnt, int nice_min, int nice_step)
   int nice;
   int i;
 
-  ASSERT (enable_mlfqs);
+  ASSERT (thread_mlfqs);
   ASSERT (thread_cnt <= MAX_THREAD_CNT);
   ASSERT (nice_min >= -10);
   ASSERT (nice_step >= 0);

diff --git a/src/tests/threads/mlfqs-load-1.c b/src/tests/threads/mlfqs-load-1.c
index da9972949112f6c84204899b792922fa8d6ffa7f..6b230aa730c15be5ba708a990639a3910f1a6ffe 100644 (file)
--- a/src/tests/threads/mlfqs-load-1.c
+++ b/src/tests/threads/mlfqs-load-1.c
@@ -21,7 +21,7 @@ test_mlfqs_load_1 (void)
   int elapsed;
   int load_avg;
   
-  ASSERT (enable_mlfqs);
+  ASSERT (thread_mlfqs);
 
   msg ("spinning for up to 45 seconds, please wait...");
 

diff --git a/src/tests/threads/mlfqs-load-60.c b/src/tests/threads/mlfqs-load-60.c
index 81caa399e106235a5dc990cbb87cc91b8921a3cb..b6a3eb68cb4f2e761a081b6cdbf61704e9ba5bd5 100644 (file)
--- a/src/tests/threads/mlfqs-load-60.c
+++ b/src/tests/threads/mlfqs-load-60.c
@@ -116,7 +116,7 @@ test_mlfqs_load_60 (void)
 {
   int i;
   
-  ASSERT (enable_mlfqs);
+  ASSERT (thread_mlfqs);
 
   start_time = timer_ticks ();
   msg ("Starting %d niced load threads...", THREAD_CNT);

diff --git a/src/tests/threads/mlfqs-load-avg.c b/src/tests/threads/mlfqs-load-avg.c
index 1462a80eb3c9594d269f239e8e46f18d24281aaf..8ea4dd14a9243638f16dec6ffefde2b753a1f6a1 100644 (file)
--- a/src/tests/threads/mlfqs-load-avg.c
+++ b/src/tests/threads/mlfqs-load-avg.c
@@ -117,7 +117,7 @@ test_mlfqs_load_avg (void)
 {
   int i;
   
-  ASSERT (enable_mlfqs);
+  ASSERT (thread_mlfqs);
 
   start_time = timer_ticks ();
   msg ("Starting %d load threads...", THREAD_CNT);

diff --git a/src/tests/threads/mlfqs-recent-1.c b/src/tests/threads/mlfqs-recent-1.c
index c217f837f4e3f573429b9d1870c1d4c2fb992e6b..670b00cb05ba0180ad1729b8d441c9c952477944 100644 (file)
--- a/src/tests/threads/mlfqs-recent-1.c
+++ b/src/tests/threads/mlfqs-recent-1.c
@@ -112,7 +112,7 @@ test_mlfqs_recent_1 (void)
   int64_t start_time;
   int last_elapsed = 0;
   
-  ASSERT (enable_mlfqs);
+  ASSERT (thread_mlfqs);
 
   msg ("Sleeping 10 seconds to allow recent_cpu to decay, please wait...");
   start_time = timer_ticks ();

diff --git a/src/tests/threads/priority-change.c b/src/tests/threads/priority-change.c
index bb462d462d85fe02c1a4ca22a132883ec881c0b4..810b05a1d76973ebdf28f914d03c91318c021e85 100644 (file)
--- a/src/tests/threads/priority-change.c
+++ b/src/tests/threads/priority-change.c
@@ -13,7 +13,7 @@ void
 test_priority_change (void) 
 {
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   msg ("Creating a high-priority thread 2.");
   thread_create ("thread 2", PRI_DEFAULT + 1, changing_thread, NULL);

diff --git a/src/tests/threads/priority-condvar.c b/src/tests/threads/priority-condvar.c
index 3e31081da6a9b673c2d0a933189e46cb0205e990..c1efb1bdee871759fb05b3ca5267bf24f7cd0a98 100644 (file)
--- a/src/tests/threads/priority-condvar.c
+++ b/src/tests/threads/priority-condvar.c
@@ -19,7 +19,7 @@ test_priority_condvar (void)
   int i;
   
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   lock_init (&lock);
   cond_init (&condition);

diff --git a/src/tests/threads/priority-donate-lower.c b/src/tests/threads/priority-donate-lower.c
index 3f63db8d00bac93e4f4b9d14fa0727d7cc6aa20f..4965d756c47700a82663e1140862f66486c47c3f 100644 (file)
--- a/src/tests/threads/priority-donate-lower.c
+++ b/src/tests/threads/priority-donate-lower.c
@@ -18,7 +18,7 @@ test_priority_donate_lower (void)
   struct lock lock;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-donate-multiple.c b/src/tests/threads/priority-donate-multiple.c
index e1bccbdb4b567baef5c1767173ff9271aeda74c9..df4689cecc602a17bc6dfbe183dd23504a8970b1 100644 (file)
--- a/src/tests/threads/priority-donate-multiple.c
+++ b/src/tests/threads/priority-donate-multiple.c
@@ -24,7 +24,7 @@ test_priority_donate_multiple (void)
   struct lock a, b;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-donate-multiple2.c b/src/tests/threads/priority-donate-multiple2.c
index a9e0dd546cf25f93148b67d91e47a0a532b43998..7f65fefc8de03dd9d219da356df8aad44cc85216 100644 (file)
--- a/src/tests/threads/priority-donate-multiple2.c
+++ b/src/tests/threads/priority-donate-multiple2.c
@@ -30,7 +30,7 @@ test_priority_donate_multiple2 (void)
   struct lock a, b;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-donate-nest.c b/src/tests/threads/priority-donate-nest.c
index 71cf8a9186db979a14ae224ccd809cf2f21754bf..3a3a9a590a2b677031524ad351f6e20e88216b8d 100644 (file)
--- a/src/tests/threads/priority-donate-nest.c
+++ b/src/tests/threads/priority-donate-nest.c
@@ -31,7 +31,7 @@ test_priority_donate_nest (void)
   struct locks locks;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-donate-one.c b/src/tests/threads/priority-donate-one.c
index 2a67b52e0960f99bbfba53ba757f8c2967b17cb4..3189f3a0b94a030290b286ea4a78623ddd5ea2e3 100644 (file)
--- a/src/tests/threads/priority-donate-one.c
+++ b/src/tests/threads/priority-donate-one.c
@@ -24,7 +24,7 @@ test_priority_donate_one (void)
   struct lock lock;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-donate-sema.c b/src/tests/threads/priority-donate-sema.c
index 89fd93d5c7d6c6bf15ae3e9fd85273d8b55b9c94..b33cb721ed77daa5c88df8f8995d22044a1a8f41 100644 (file)
--- a/src/tests/threads/priority-donate-sema.c
+++ b/src/tests/threads/priority-donate-sema.c
@@ -32,7 +32,7 @@ test_priority_donate_sema (void)
   struct lock_and_sema ls;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-fifo.c b/src/tests/threads/priority-fifo.c
index 2f891962ad4e0a796bdc1851da970c27c93af7dd..3af98a30ac217e7e7978c87ce1327771c42b10b7 100644 (file)
--- a/src/tests/threads/priority-fifo.c
+++ b/src/tests/threads/priority-fifo.c
@@ -37,7 +37,7 @@ test_priority_fifo (void)
   int i, cnt;
 
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-preempt.c b/src/tests/threads/priority-preempt.c
index 70f8acbee8ee67bd14349f73106a10dfbb031565..3c3aacbd0336f41fb69d386c9576cb18f07c00ae 100644 (file)
--- a/src/tests/threads/priority-preempt.c
+++ b/src/tests/threads/priority-preempt.c
@@ -18,7 +18,7 @@ void
 test_priority_preempt (void) 
 {
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   /* Make sure our priority is the default. */
   ASSERT (thread_get_priority () == PRI_DEFAULT);

diff --git a/src/tests/threads/priority-sema.c b/src/tests/threads/priority-sema.c
index d7eb9cc076cfbaf26f9a174ff1503dcdb24d6d1a..2834a880be25c92aa9989c6e3e828c13fb0fc107 100644 (file)
--- a/src/tests/threads/priority-sema.c
+++ b/src/tests/threads/priority-sema.c
@@ -18,7 +18,7 @@ test_priority_sema (void)
   int i;
   
   /* This test does not work with the MLFQS. */
-  ASSERT (!enable_mlfqs);
+  ASSERT (!thread_mlfqs);
 
   sema_init (&sema, 0);
   thread_set_priority (PRI_MIN);

diff --git a/src/threads/init.c b/src/threads/init.c
index 8495826da379a3624e061d3a53cc8432aa04764c..2931704120c67bf39441ce56aa469a128b556e39 100644 (file)
--- a/src/threads/init.c
+++ b/src/threads/init.c
@@ -41,11 +41,6 @@ size_t ram_pages;
 /* Page directory with kernel mappings only. */
 uint32_t *base_page_dir;
 
-/* -mlfqs:
-   If false (default), use round-robin scheduler.
-   If true, use multi-level feedback queue scheduler. */
-bool enable_mlfqs;
-
 #ifdef FILESYS
 /* -f: Format the file system? */
 static bool format_filesys;
@@ -254,7 +249,7 @@ parse_options (char **argv)
       else if (!strcmp (name, "-rs"))
         random_init (atoi (value));
       else if (!strcmp (name, "-mlfqs"))
-        enable_mlfqs = true;
+        thread_mlfqs = true;
 #ifdef USERPROG
       else if (!strcmp (name, "-ul"))
         user_page_limit = atoi (value);

diff --git a/src/threads/init.h b/src/threads/init.h
index 4455c212d04dec6d07ea201862c07d83cebcb4a0..a6fec055d0b66067fd590441b06c2d2a141492ea 100644 (file)
--- a/src/threads/init.h
+++ b/src/threads/init.h
@@ -12,11 +12,6 @@ extern size_t ram_pages;
 /* Page directory with kernel mappings only. */
 extern uint32_t *base_page_dir;
 
-/* -o mlfqs:
-   If false (default), use round-robin scheduler.
-   If true, use multi-level feedback queue scheduler. */
-extern bool enable_mlfqs;
-
 /* -q: Power off when kernel tasks complete? */
 extern bool power_off_when_done;
 

diff --git a/src/threads/thread.c b/src/threads/thread.c
index 8d9f33558bb5313b1e137250479b5103a88db689..ba1cae365f711f44474d6206bffb4e4ca25ccddc 100644 (file)
--- a/src/threads/thread.c
+++ b/src/threads/thread.c
@@ -50,6 +50,13 @@ static long long user_ticks;    /* # of timer ticks in user programs. */
 #define TIME_SLICE 4            /* # of timer ticks to give each thread. */
 static unsigned thread_ticks;   /* # of timer ticks since last yield. */
 
+/* If false (default), use round-robin scheduler.
+   If true, use multi-level feedback queue scheduler.
+   Controlled by kernel command-line options "-o mlfqs".
+   Note that the command line is not parsed until well after
+   thread_init() is called. */
+bool thread_mlfqs;
+
 static void kernel_thread (thread_func *, void *aux);
 
 static void idle (void *aux UNUSED);
@@ -71,7 +78,14 @@ static tid_t allocate_tid (void);
 
    After calling this function, be sure to initialize the page
    allocator before trying to create any threads with
-   thread_create(). */
+   thread_create().
+
+   The kernel command line is not parsed until *after* this
+   function returns, so that when this function runs,
+   thread_mlfqs is always false.
+
+   It is not safe to call thread_current() until this function
+   finishes. */
 void
 thread_init (void) 
 {
@@ -88,12 +102,28 @@ thread_init (void)
 }
 
 /* Starts preemptive thread scheduling by enabling interrupts.
-   Also creates the idle thread. */
+   Also creates the idle thread.
+
+   By the time this function runs, thread_mlfqs has been properly
+   initialized to its final value. */
 void
 thread_start (void) 
 {
-  thread_create ("idle", PRI_MIN, idle, NULL);
+  /* Create the idle thread with maximum priority.  This ensures
+     that it will be scheduled soon after interrupts are enabled.
+     The idle thread will block almost immediately upon
+     scheduling, and subsequently it will never appear on the
+     ready list, so the priority here is otherwise
+     unimportant. */
+  struct semaphore idle_started;
+  sema_init (&idle_started, 0);
+  thread_create ("idle", PRI_MAX, idle, &idle_started);
+
+  /* Start preemptive thread scheduling. */
   intr_enable ();
+
+  /* Wait for the idle thread to initialize idle_thread. */
+  sema_down (&idle_started);
 }
 
 /* Called by the timer interrupt handler at each timer tick.
@@ -278,7 +308,8 @@ thread_yield (void)
   ASSERT (!intr_context ());
 
   old_level = intr_disable ();
-  list_push_back (&ready_list, &cur->elem);
+  if (cur != idle_thread) 
+    list_push_back (&ready_list, &cur->elem);
   cur->status = THREAD_READY;
   schedule ();
   intr_set_level (old_level);
@@ -333,21 +364,17 @@ thread_get_recent_cpu (void)
 
    The idle thread is initially put on the ready list by
    thread_start().  It will be scheduled once initially, at which
-   point it initializes idle_thread and immediately blocks.
-   After that, the idle thread never appears in the ready list.
-   It is returned by next_thread_to_run() as a special case when
-   the ready list is empty. */
+   point it initializes idle_thread, "up"s the semaphore passed
+   to it to enable thread_start() to continue, and immediately
+   blocks.  After that, the idle thread never appears in the
+   ready list.  It is returned by next_thread_to_run() as a
+   special case when the ready list is empty. */
 static void
-idle (void *aux UNUSED) 
+idle (void *idle_started_ UNUSED) 
 {
-  /* Initialize idle_thread.
-
-     Until we run for the first time, idle_thread remains a null
-     pointer.  That's okay because we know that, at that point,
-     the ready list has at least one element (the idle thread),
-     so next_thread_to_run() will not attempt to return the idle
-     thread. */
+  struct semaphore *idle_started = idle_started_;
   idle_thread = thread_current ();
+  sema_up (idle_started);
 
   for (;;) 
     {

diff --git a/src/threads/thread.h b/src/threads/thread.h
index fbef34b4673720b2f62bf3a80d1e08eadb441aa3..b45bdb8616f02b66c72a3c455215233de1b4f761 100644 (file)
--- a/src/threads/thread.h
+++ b/src/threads/thread.h
@@ -101,6 +101,13 @@ struct thread
     unsigned magic;                     /* Detects stack overflow. */
   };
 
+/* If false (default), use round-robin scheduler.
+   If true, use multi-level feedback queue scheduler.
+   Controlled by kernel command-line options "-o mlfqs".
+   Note that the command line is not parsed until well after
+   thread_init() is called. */
+extern bool thread_mlfqs;
+
 void thread_init (void);
 void thread_start (void);