@@ -189,8 +189,27 @@ fn SemAndSignalRelease<'r>(sem: &'r Sem<~[Waitqueue]>)
189189 }
190190}
191191
192+ // FIXME(#3598): Want to use an Option down below, but we need a custom enum
193+ // that's not polymorphic to get around the fact that lifetimes are invariant
194+ // inside of type parameters.
195+ enum ReacquireOrderLock < ' self > {
196+ Nothing , // c.c
197+ Just ( & ' self Semaphore ) ,
198+ }
199+
192200/// A mechanism for atomic-unlock-and-deschedule blocking and signalling.
193- pub struct Condvar < ' self > { priv sem: & ' self Sem < ~[ Waitqueue ] > }
201+ pub struct Condvar < ' self > {
202+ // The 'Sem' object associated with this condvar. This is the one that's
203+ // atomically-unlocked-and-descheduled upon and reacquired during wakeup.
204+ priv sem: & ' self Sem < ~[ Waitqueue ] > ,
205+ // This is (can be) an extra semaphore which is held around the reacquire
206+ // operation on the first one. This is only used in cvars associated with
207+ // rwlocks, and is needed to ensure that, when a downgrader is trying to
208+ // hand off the access lock (which would be the first field, here), a 2nd
209+ // writer waking up from a cvar wait can't race with a reader to steal it,
210+ // See the comment in write_cond for more detail.
211+ priv order : ReacquireOrderLock < ' self > ,
212+ }
194213
195214#[ unsafe_destructor]
196215impl < ' self > Drop for Condvar < ' self > { fn finalize ( & self ) { } }
@@ -247,7 +266,8 @@ impl<'self> Condvar<'self> {
247266 // unkillably reacquire the lock needs to happen atomically
248267 // wrt enqueuing.
249268 if out_of_bounds. is_none ( ) {
250- reacquire = Some ( SemAndSignalReacquire ( self . sem ) ) ;
269+ reacquire = Some ( CondvarReacquire { sem : self . sem ,
270+ order : self . order } ) ;
251271 }
252272 }
253273 }
@@ -261,28 +281,29 @@ impl<'self> Condvar<'self> {
261281 // This is needed for a failing condition variable to reacquire the
262282 // mutex during unwinding. As long as the wrapper (mutex, etc) is
263283 // bounded in when it gets released, this shouldn't hang forever.
264- struct SemAndSignalReacquire < ' self > {
284+ struct CondvarReacquire < ' self > {
265285 sem : & ' self Sem < ~[ Waitqueue ] > ,
286+ order : ReacquireOrderLock < ' self > ,
266287 }
267288
268289 #[ unsafe_destructor]
269- impl < ' self > Drop for SemAndSignalReacquire < ' self > {
290+ impl < ' self > Drop for CondvarReacquire < ' self > {
270291 fn finalize ( & self ) {
271292 unsafe {
272293 // Needs to succeed, instead of itself dying.
273294 do task:: unkillable {
274- self. sem . acquire ( ) ;
295+ match self. order {
296+ Just ( lock) => do lock. access {
297+ self . sem . acquire ( ) ;
298+ } ,
299+ Nothing => {
300+ self . sem . acquire ( ) ;
301+ } ,
302+ }
275303 }
276304 }
277305 }
278306 }
279-
280- fn SemAndSignalReacquire < ' r > ( sem : & ' r Sem < ~[ Waitqueue ] > )
281- -> SemAndSignalReacquire < ' r > {
282- SemAndSignalReacquire {
283- sem : sem
284- }
285- }
286307 }
287308
288309 /// Wake up a blocked task. Returns false if there was no blocked task.
@@ -350,9 +371,12 @@ fn check_cvar_bounds<U>(out_of_bounds: Option<uint>, id: uint, act: &str,
350371
351372#[ doc( hidden) ]
352373impl Sem < ~[ Waitqueue ] > {
353- // The only other place that condvars get built is rwlock_write_mode.
374+ // The only other places that condvars get built are rwlock.write_cond()
375+ // and rwlock_write_mode.
354376 pub fn access_cond < U > ( & self , blk : & fn ( c : & Condvar ) -> U ) -> U {
355- do self . access { blk ( & Condvar { sem : self } ) }
377+ do self . access {
378+ blk ( & Condvar { sem : self , order : Nothing } )
379+ }
356380 }
357381}
358382
@@ -534,17 +558,39 @@ impl RWlock {
534558 * was signalled might reacquire the lock before other waiting writers.)
535559 */
536560 pub fn write_cond < U > ( & self , blk : & fn ( c : & Condvar ) -> U ) -> U {
537- // NB: You might think I should thread the order_lock into the cond
538- // wait call, so that it gets waited on before access_lock gets
539- // reacquired upon being woken up. However, (a) this would be not
540- // pleasant to implement (and would mandate a new 'rw_cond' type) and
541- // (b) I think violating no-starvation in that case is appropriate.
561+ // It's important to thread our order lock into the condvar, so that
562+ // when a cond.wait() wakes up, it uses it while reacquiring the
563+ // access lock. If we permitted a waking-up writer to "cut in line",
564+ // there could arise a subtle race when a downgrader attempts to hand
565+ // off the reader cloud lock to a waiting reader. This race is tested
566+ // in arc.rs (test_rw_write_cond_downgrade_read_race) and looks like:
567+ // T1 (writer) T2 (downgrader) T3 (reader)
568+ // [in cond.wait()]
569+ // [locks for writing]
570+ // [holds access_lock]
571+ // [is signalled, perhaps by
572+ // downgrader or a 4th thread]
573+ // tries to lock access(!)
574+ // lock order_lock
575+ // xadd read_count[0->1]
576+ // tries to lock access
577+ // [downgrade]
578+ // xadd read_count[1->2]
579+ // unlock access
580+ // Since T1 contended on the access lock before T3 did, it will steal
581+ // the lock handoff. Adding order_lock in the condvar reacquire path
582+ // solves this because T1 will hold order_lock while waiting on access,
583+ // which will cause T3 to have to wait until T1 finishes its write,
584+ // which can't happen until T2 finishes the downgrade-read entirely.
542585 unsafe {
543586 do task:: unkillable {
544587 ( & self . order_lock ) . acquire ( ) ;
545588 do ( & self . access_lock ) . access_cond |cond| {
546589 ( & self . order_lock ) . release ( ) ;
547- do task:: rekillable { blk( cond) }
590+ do task:: rekillable {
591+ let opt_lock = Just ( & self . order_lock ) ;
592+ blk ( & Condvar { order : opt_lock, ..* cond } )
593+ }
548594 }
549595 }
550596 }
@@ -605,7 +651,8 @@ impl RWlock {
605651 // Guaranteed not to let another writer in, because
606652 // another reader was holding the order_lock. Hence they
607653 // must be the one to get the access_lock (because all
608- // access_locks are acquired with order_lock held).
654+ // access_locks are acquired with order_lock held). See
655+ // the comment in write_cond for more justification.
609656 ( & self . access_lock ) . release ( ) ;
610657 }
611658 }
@@ -709,7 +756,10 @@ impl<'self> RWlockWriteMode<'self> {
709756pub fn write < U > ( & self , blk : & fn ( ) -> U ) -> U { blk ( ) }
710757 /// Access the pre-downgrade rwlock in write mode with a condvar.
711758pub fn write_cond < U > ( & self , blk : & fn ( c : & Condvar ) -> U ) -> U {
712- blk ( & Condvar { sem : & self . lock . access_lock } )
759+ // Need to make the condvar use the order lock when reacquiring the
760+ // access lock. See comment in RWlock::write_cond for why.
761+ blk ( & Condvar { sem : & self . lock . access_lock ,
762+ order : Just ( & self . lock . order_lock ) , } )
713763 }
714764}
715765
0 commit comments