Date: Thu, 16 Sep 2004 15:15:15 -0700 From: Nate Lawson <nate@root.org> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/md md.c Message-ID: <414A1073.8010404@root.org> In-Reply-To: <20040916204321.GE30151@darkness.comp.waw.pl> References: <20040916185923.2F92316A552@hub.freebsd.org> <4149EC27.9080200@root.org> <20040916204321.GE30151@darkness.comp.waw.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
Pawel Jakub Dawidek wrote: > On Thu, Sep 16, 2004 at 12:40:23PM -0700, Nate Lawson wrote: > +> >@@ -379,9 +379,8 @@ > +> > bp->bio_bcount = bp->bio_length; > +> > mtx_lock(&sc->queue_mtx); > +> > bioq_disksort(&sc->bio_queue, bp); > +> >- mtx_unlock(&sc->queue_mtx); > +> >- > +> > wakeup(sc); > +> >+ mtx_unlock(&sc->queue_mtx); > +> > } > +> > +> I think the original order is correct since you can occur 2 switches if > +> you wakeup first and then unlock. > > Nope, this order was wrong: > > thread1 thread2 > ----------------------- > mtx_lock(mtx) > ... > mtx_unlock(mtx) > mtx_lock(mtx) > wakeup(ptr) > msleep(ptr, mtx) <- Race, it will be never woken up. You still have a race, like this: thread1 thread2 ----------------------------- mtx_lock(mtx) wakeup(ptr) mtx_unlock(mtx) mtx_lock(mtx) msleep(ptr, mtx) You should be checking the work condition in thread 2 while holding the mutex but before going to sleep. Adding work to the queue happens in thread 1 where you write "..." and that is done with the mutex held so there is no race. The full diagram with this detail included is: thread1 thread2 ----------------------------- mtx_lock(mtx) add work to queue mtx_unlock(mtx) mtx_lock(mtx) wakeup(ptr) check queue for work item if (!work item) msleep(ptr, mtx) else dequeue work item and loop Since the work item is added in thread1 with the mutex held, the check for it in thread2 is safe and race-free. A wakeup is only there to kickstart thread2 if it's asleep. If it's running, it needs to check atomically that there is no work before sleeping. If it doesn't do this, it's a bug. -- Nate
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?414A1073.8010404>