The lock functions don't really need to disable interrupts themselves,

since they're protected by the semaphore.
Thanks to Godmar Back for reporting this.

diff --git a/TODO b/TODO
index 258e2fa8764ad80ceab3ef35c9a75998ab474970..8651fb419f1c97bbd423c692ca1b7c19674da016 100644 (file)
--- a/TODO
+++ b/TODO
@@ -221,16 +221,6 @@ via Godmar Back:
 
 * Add FS persistence test(s).
 
 
 * Add FS persistence test(s).
 

-* lock_acquire(), lock_release() don't need additional intr_dis/enable
-  calls, because the semaphore protects lock->holder.
-  [ Think this over: is this really true when priority donation is 
-    implemented?  intr_dis/enable prevents the race with thread_set_priority. 
-    Leaving it there could help the students getting the correct synchronization
-    right.
-  ]
-
-
-

 * process_death test needs improvement
 
 * Internal tests.
 * process_death test needs improvement
 
 * Internal tests.

diff --git a/doc/threads.texi b/doc/threads.texi
index 432cb2668224dab21a7be7164d476324cf2ad63d..d1ff66640cd05f9d428375bf6bf0bd91958357ea 100644 (file)
--- a/doc/threads.texi
+++ b/doc/threads.texi
@@ -328,9 +328,10 @@ timer ticks or input events.  Turning off interrupts also increases the
 interrupt handling latency, which can make a machine feel sluggish if
 taken too far.
 
 interrupt handling latency, which can make a machine feel sluggish if
 taken too far.
 

-You may need to add or modify code where interrupts are already
-disabled, such as in @func{sema_up} or @func{sema_down}.  You should
-still try to keep this code as short as you can.
+The synchronization primitives themselves in @file{synch.c} are
+implemented by disabling interrupts.  You may need to increase the
+amount of code that runs with interrupts disabled here, but you should
+still try to keep it to a minimum.

 
 Disabling interrupts can be useful for debugging, if you want to make
 sure that a section of code is not interrupted.  You should remove
 
 Disabling interrupts can be useful for debugging, if you want to make
 sure that a section of code is not interrupted.  You should remove

diff --git a/solutions/p1.patch b/solutions/p1.patch
index 213e05ef29e2f749ab24ad6f5a38e02b7d0ca630..ae8f15b9fec458b0b01e3cc47d13d7532c56237c 100644 (file)
--- a/solutions/p1.patch
+++ b/solutions/p1.patch
@@ -236,10 +236,16 @@ diff -u src/threads/synch.c~ src/threads/synch.c
    intr_set_level (old_level);
  }
  
    intr_set_level (old_level);
  }
  

-@@ -200,6 +218,23 @@ lock_acquire (struct lock *lock)
+@@ -200,8 +218,29 @@ lock_acquire (struct lock *lock)
+ lock_acquire (struct lock *lock)
+ {
++  enum intr_level old_level;
++
+   ASSERT (lock != NULL);
+   ASSERT (!intr_context ());

    ASSERT (!lock_held_by_current_thread (lock));
  
    ASSERT (!lock_held_by_current_thread (lock));
  

-   old_level = intr_disable ();
++  old_level = intr_disable ();

 +
 +  if (lock->holder != NULL) 
 +    {
 +
 +  if (lock->holder != NULL) 
 +    {


@@ -259,18 +265,18 @@ diff -u src/threads/synch.c~ src/threads/synch.c
 +
    sema_down (&lock->semaphore);
    lock->holder = thread_current ();
 +
    sema_down (&lock->semaphore);
    lock->holder = thread_current ();

-   intr_set_level (old_level);
-@@ -238,13 +273,37 @@ void
++  intr_set_level (old_level);
+@@ -238,9 +273,37 @@ void

  lock_release (struct lock *lock) 
  {
  lock_release (struct lock *lock) 
  {

-   enum intr_level old_level;
++  enum intr_level old_level;

 +  struct thread *t = thread_current ();
 +  struct list_elem *e;
 +  struct thread *t = thread_current ();
 +  struct list_elem *e;

- 
++

    ASSERT (lock != NULL);
    ASSERT (lock_held_by_current_thread (lock));
  
    ASSERT (lock != NULL);
    ASSERT (lock_held_by_current_thread (lock));
  

-   old_level = intr_disable ();
++  old_level = intr_disable ();

 +
 +  /* Return donations to threads that want this lock. */
 +  for (e = list_begin (&t->donors); e != list_end (&t->donors); ) 
 +
 +  /* Return donations to threads that want this lock. */
 +  for (e = list_begin (&t->donors); e != list_end (&t->donors); ) 


@@ -295,7 +301,7 @@ diff -u src/threads/synch.c~ src/threads/synch.c
 +  thread_recompute_priority (t);
 +  thread_yield_to_higher_priority ();
 +
 +  thread_recompute_priority (t);
 +  thread_yield_to_higher_priority ();
 +

-   intr_set_level (old_level);
++  intr_set_level (old_level);

  }
  
 @@ -264,6 +323,7 @@ struct semaphore_elem 
  }
  
 @@ -264,6 +323,7 @@ struct semaphore_elem 

diff --git a/src/threads/synch.c b/src/threads/synch.c
index 5ec14abf8d4dfa11851e02917a1d030540a560f0..c2764bc38d1cdb3b47efb0f0a43f8d694d493947 100644 (file)
--- a/src/threads/synch.c
+++ b/src/threads/synch.c
@@ -192,16 +192,12 @@ lock_init (struct lock *lock)
 void
 lock_acquire (struct lock *lock)
 {
 void
 lock_acquire (struct lock *lock)
 {

-  enum intr_level old_level;
-

   ASSERT (lock != NULL);
   ASSERT (!intr_context ());
   ASSERT (!lock_held_by_current_thread (lock));
 
   ASSERT (lock != NULL);
   ASSERT (!intr_context ());
   ASSERT (!lock_held_by_current_thread (lock));
 

-  old_level = intr_disable ();

   sema_down (&lock->semaphore);
   lock->holder = thread_current ();
   sema_down (&lock->semaphore);
   lock->holder = thread_current ();

-  intr_set_level (old_level);

 }
 
 /* Tries to acquires LOCK and returns true if successful or false
 }
 
 /* Tries to acquires LOCK and returns true if successful or false


@@ -213,18 +209,14 @@ lock_acquire (struct lock *lock)
 bool
 lock_try_acquire (struct lock *lock)
 {
 bool
 lock_try_acquire (struct lock *lock)
 {

-  enum intr_level old_level;

   bool success;
 
   ASSERT (lock != NULL);
   ASSERT (!lock_held_by_current_thread (lock));
 
   bool success;
 
   ASSERT (lock != NULL);
   ASSERT (!lock_held_by_current_thread (lock));
 

-  old_level = intr_disable ();

   success = sema_try_down (&lock->semaphore);
   if (success)
     lock->holder = thread_current ();
   success = sema_try_down (&lock->semaphore);
   if (success)
     lock->holder = thread_current ();

-  intr_set_level (old_level);
-

   return success;
 }
 
   return success;
 }
 


@@ -236,15 +228,11 @@ lock_try_acquire (struct lock *lock)
 void
 lock_release (struct lock *lock) 
 {
 void
 lock_release (struct lock *lock) 
 {

-  enum intr_level old_level;
-

   ASSERT (lock != NULL);
   ASSERT (lock_held_by_current_thread (lock));
 
   ASSERT (lock != NULL);
   ASSERT (lock_held_by_current_thread (lock));
 

-  old_level = intr_disable ();

   lock->holder = NULL;
   sema_up (&lock->semaphore);
   lock->holder = NULL;
   sema_up (&lock->semaphore);

-  intr_set_level (old_level);

 }
 
 /* Returns true if the current thread holds LOCK, false
 }
 
 /* Returns true if the current thread holds LOCK, false