From: Ben Pfaff <blp@cs.stanford.edu>
Date: Mon, 25 Sep 2006 20:26:35 +0000 (+0000)
Subject: Fix two bugs in the base Pintos code:
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=92866b2056d927d32ea6343bd62eed25e4c6b612;p=pintos-anon

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 f8cd04e..cbcda16 100644
--- 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/solutions/p1.patch b/solutions/p1.patch
index 7f95cef..a307916 100644
--- a/solutions/p1.patch
+++ b/solutions/p1.patch
@@ -61,7 +61,7 @@ diff -u src/devices/timer.c~ src/devices/timer.c
  }
  
  /* Suspends execution for approximately MS milliseconds. */
-@@ -132,6 +158,16 @@ timer_interrupt (struct intr_frame *args
+@@ -132,6 +158,17 @@ timer_interrupt (struct intr_frame *args
  {
    ticks++;
    thread_tick ();
@@ -73,6 +73,7 @@ diff -u src/devices/timer.c~ src/devices/timer.c
 +      if (ticks < t->wakeup_time) 
 +        break;
 +      sema_up (&t->timer_sema);
++      thread_yield_to_higher_priority ();
 +      list_pop_front (&wait_list);
 +    }
  }
@@ -417,7 +418,7 @@ diff -u src/threads/thread.c~ src/threads/thread.c
 -  /* Enforce preemption. */
 -  if (++thread_ticks >= TIME_SLICE)
 -    intr_yield_on_return ();
-+  if (enable_mlfqs) 
++  if (thread_mlfqs) 
 +    {
 +      /* Update load average. */
 +      if (timer_ticks () % TIMER_FREQ == 0) 
@@ -455,7 +456,7 @@ diff -u src/threads/thread.c~ src/threads/thread.c
 +  /* Switch threads if time slice has expired. */
 +  if (++thread_ticks >= TIME_SLICE) 
 +    {
-+      if (enable_mlfqs)
++      if (thread_mlfqs)
 +        thread_recompute_priority (thread_current ());
 +      intr_yield_on_return (); 
 +    }
@@ -480,7 +481,7 @@ diff -u src/threads/thread.c~ src/threads/thread.c
  
    /* Initialize thread. */
 -  init_thread (t, name, priority);
-+  init_thread (t, name, enable_mlfqs ? cur->priority : priority);
++  init_thread (t, name, thread_mlfqs ? cur->priority : priority);
    tid = t->tid = allocate_tid ();
 +  t->nice = cur->nice;
 +  t->recent_cpu = cur->recent_cpu;
@@ -551,7 +552,7 @@ diff -u src/threads/thread.c~ src/threads/thread.c
 +thread_set_priority (int priority) 
  {
 -  thread_current ()->priority = new_priority;
-+  if (!enable_mlfqs) 
++  if (!thread_mlfqs) 
 +    {
 +      struct thread *t = thread_current ();
 +
@@ -561,7 +562,7 @@ diff -u src/threads/thread.c~ src/threads/thread.c
  }
  
  /* Returns the current thread's priority. */
-@@ -298,33 +386,93 @@ thread_get_priority (void) 
+@@ -298,33 +386,98 @@ thread_get_priority (void) 
  
  /* Sets the current thread's nice value to NICE. */
  void
@@ -630,7 +631,7 @@ diff -u src/threads/thread.c~ src/threads/thread.c
 +  int old_priority = t->priority;
 +  int default_priority = t->normal_priority;
 +  int donation = PRI_MIN;
-+  if (enable_mlfqs) 
++  if (thread_mlfqs) 
 +    {
 +      default_priority = PRI_MAX - fix_round (t->recent_cpu) / 4 - t->nice * 2;
 +      if (default_priority < PRI_MIN)
@@ -659,7 +660,12 @@ diff -u src/threads/thread.c~ src/threads/thread.c
 +                                                 thread_lower_priority, NULL),
 +                                       struct thread, elem);
 +      if (max->priority > cur->priority)
-+        thread_yield (); 
++        {
++          if (intr_context ())
++            intr_yield_on_return ();
++          else
++            thread_yield (); 
++        }
 +    }
 +  intr_set_level (old_level);
  }
diff --git a/src/tests/threads/alarm-priority.c b/src/tests/threads/alarm-priority.c
index 4839ecd..2288ff6 100644
--- 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 4ba3651..844eea4 100644
--- 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 458e987..37d3afc 100644
--- 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 4e3fffa..6d4992d 100644
--- 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 30282c3..f958f81 100644
--- 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 da99729..6b230aa 100644
--- 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 81caa39..b6a3eb6 100644
--- 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 1462a80..8ea4dd1 100644
--- 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 c217f83..670b00c 100644
--- 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 bb462d4..810b05a 100644
--- 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 3e31081..c1efb1b 100644
--- 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 3f63db8..4965d75 100644
--- 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 e1bccbd..df4689c 100644
--- 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 a9e0dd5..7f65fef 100644
--- 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 71cf8a9..3a3a9a5 100644
--- 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 2a67b52..3189f3a 100644
--- 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 89fd93d..b33cb72 100644
--- 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 2f89196..3af98a3 100644
--- 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 70f8acb..3c3aacb 100644
--- 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 d7eb9cc..2834a88 100644
--- 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 8495826..2931704 100644
--- 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 4455c21..a6fec05 100644
--- 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 8d9f335..ba1cae3 100644
--- 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 fbef34b..b45bdb8 100644
--- 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);