From: Ben Pfaff Date: Sun, 9 Apr 2006 20:19:35 +0000 (+0000) Subject: The lock functions don't really need to disable interrupts themselves, X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=71b4d5a453498d45ae5929b51247a487c0e3719d;p=pintos-anon 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 258e2fa..8651fb4 100644 --- a/TODO +++ b/TODO @@ -221,16 +221,6 @@ via Godmar Back: * 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. diff --git a/doc/threads.texi b/doc/threads.texi index 432cb26..d1ff666 100644 --- 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. -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 diff --git a/solutions/p1.patch b/solutions/p1.patch index 213e05e..ae8f15b 100644 --- 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); } -@@ -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)); - old_level = intr_disable (); ++ old_level = intr_disable (); + + 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 (); - 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) { - enum intr_level old_level; ++ enum intr_level old_level; + struct thread *t = thread_current (); + struct list_elem *e; - ++ 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); ) @@ -295,7 +301,7 @@ diff -u src/threads/synch.c~ src/threads/synch.c + 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 diff --git a/src/threads/synch.c b/src/threads/synch.c index 5ec14ab..c2764bc 100644 --- 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) { - enum intr_level old_level; - 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 (); - intr_set_level (old_level); } /* 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) { - enum intr_level old_level; 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 (); - intr_set_level (old_level); - return success; } @@ -236,15 +228,11 @@ lock_try_acquire (struct lock *lock) void lock_release (struct lock *lock) { - enum intr_level old_level; - ASSERT (lock != NULL); ASSERT (lock_held_by_current_thread (lock)); - old_level = intr_disable (); lock->holder = NULL; sema_up (&lock->semaphore); - intr_set_level (old_level); } /* Returns true if the current thread holds LOCK, false