From owner-cvs-src@FreeBSD.ORG Thu Sep 16 22:15:16 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5327C16A4CE; Thu, 16 Sep 2004 22:15:16 +0000 (GMT) Received: from www.cryptography.com (li-22.members.linode.com [64.5.53.22]) by mx1.FreeBSD.org (Postfix) with ESMTP id 0C4EE43D4C; Thu, 16 Sep 2004 22:15:16 +0000 (GMT) (envelope-from nate@root.org) Received: from [10.0.0.34] (adsl-67-127-84-57.dsl.snfc21.pacbell.net [67.127.84.57]) by www.cryptography.com (8.12.8/8.12.8) with ESMTP id i8GMFEDl016884; Thu, 16 Sep 2004 15:15:15 -0700 Message-ID: <414A1073.8010404@root.org> Date: Thu, 16 Sep 2004 15:15:15 -0700 From: Nate Lawson User-Agent: Mozilla Thunderbird 0.7.3 (Windows/20040803) X-Accept-Language: en-us, en MIME-Version: 1.0 To: Pawel Jakub Dawidek References: <20040916185923.2F92316A552@hub.freebsd.org> <4149EC27.9080200@root.org> <20040916204321.GE30151@darkness.comp.waw.pl> In-Reply-To: <20040916204321.GE30151@darkness.comp.waw.pl> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/md md.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Sep 2004 22:15:16 -0000 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