From owner-freebsd-current@FreeBSD.ORG Thu Sep 4 23:38:39 2008 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B7603106567B for ; Thu, 4 Sep 2008 23:38:39 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from ik-out-1112.google.com (ik-out-1112.google.com [66.249.90.183]) by mx1.freebsd.org (Postfix) with ESMTP id 3D4CE8FC34 for ; Thu, 4 Sep 2008 23:38:38 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: by ik-out-1112.google.com with SMTP id c30so150450ika.3 for ; Thu, 04 Sep 2008 16:38:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:sender :to:subject:cc:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references :x-google-sender-auth; bh=/T6XhfJ9+4e8bOT7+NK/XCUqys+UmWEvyf3lg9AB8V0=; b=xSpqW46pjPldZrw8h0j8xO8rhfNl/ZabEP9jQ7qv8OjVBSdNRzOCu5QAjzxGXzV9vL KEdyW5p63Zf3Pu+mluN2TFv+oxif6NxbR25HfxVs0pg0/5BliXr7ZhL2bPtKcPw0h69u FqQPITFeuCZwCeEuT3KcmnXL8mllPaO9i1l1k= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references:x-google-sender-auth; b=FgpTFFyet4+HK0B5b6FODyD30VD3o5ClMybkT3bwc0Wx+2tUR+15vvW6ycNDJabCp0 L5qs4eS1RyO7o6hB4BfOo3JZQRh8GS1nkCUMcu358bvLQecT5zc/pHLlxAVcWTBQy3iV QU4JASRDnF2SjyMyV9F7LRBd1xPa9yhDboaz4= Received: by 10.102.253.13 with SMTP id a13mr7405969mui.74.1220571517597; Thu, 04 Sep 2008 16:38:37 -0700 (PDT) Received: by 10.103.239.14 with HTTP; Thu, 4 Sep 2008 16:38:37 -0700 (PDT) Message-ID: <3bbf2fe10809041638l52e73516r2cd11ce3eb8366c@mail.gmail.com> Date: Fri, 5 Sep 2008 01:38:37 +0200 From: "Attilio Rao" Sender: asmrookie@gmail.com To: "John Baldwin" In-Reply-To: <200809021608.57542.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200808230003.44081.jhb@freebsd.org> <48B6BC81.5060300@clearchain.com> <20080901.013117.74700691.Tor.Egge@cvsup.no.freebsd.org> <200809021608.57542.jhb@freebsd.org> X-Google-Sender-Auth: 3861be1d62fddda3 Cc: kevinxlinuz@163.com, freebsd-current@freebsd.org, Benjamin.Close@clearchain.com, kib@freebsd.org Subject: Re: [BUG] I think sleepqueue need to be protected in sleepq_broadcast X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Sep 2008 23:38:39 -0000 2008/9/2, John Baldwin : > On Sunday 31 August 2008 09:31:17 pm Tor Egge wrote: > > > > > sleepq_resume_thread() contains an ownership handover of sq if the resumed > > thread is the last one blocked on the wait channel. After the handover, sq > is > > no longer protected by the sleep queue chain lock and should no longer be > > accessed by sleepq_broadcast(). > > > > Normally, when sleepq_broadcast() incorrectly accesses sq after the > handover, > > it will find the sq->sq_blocked queue to be empty, and the code appears to > > work. > > > > If the last correctly woken thread manages to go to sleep again very quickly > on > > another wait channel, sleepq_broadcast() might incorrectly determine that > the > > sq->sq_blocked queue isn't empty, and start doing the wrong thing. > > > So disregard my earlier e-mail. Here is a simple fix for the sleepq case: > > Index: subr_sleepqueue.c > =================================================================== > --- subr_sleepqueue.c (revision 182679) > +++ subr_sleepqueue.c (working copy) > @@ -779,7 +779,7 @@ > > sleepq_broadcast(void *wchan, int flags, int pri, int queue) > { > struct sleepqueue *sq; > - struct thread *td; > > + struct thread *td, *tdn; > > int wakeup_swapper; > > CTR2(KTR_PROC, "sleepq_broadcast(%p, %d)", wchan, flags); > > @@ -793,8 +793,7 @@ > > > /* Resume all blocked threads on the sleep queue. */ > wakeup_swapper = 0; > - while (!TAILQ_EMPTY(&sq->sq_blocked[queue])) { > - td = TAILQ_FIRST(&sq->sq_blocked[queue]); > > + TAILQ_FOREACH_SAFE(td, &sq->sq_blocked[queue], td_slpq, tdn) { > thread_lock(td); > > if (sleepq_resume_thread(sq, td, pri)) > wakeup_swapper = 1; > > > This only uses 'sq' to fetch the head of the queue once up front. It won't > use it again once it has started waking up threads. > > > > A similar (but probably much more difficult to trigger) issue is present > with > > regards to thread_lock() and turnstiles. > > > > The caller of thread_lock() might have performed sufficient locking to > ensure > > that the thread to be locked doesn't go away, but any turnstile spin lock > > pointed to by td->td_lock isn't protected. Making turnstiles type stable > > (setting UMA_ZONE_NOFREE flag for turnstile_zone) should fix that issue. > > > Note that unlike the sleepq case, turnstiles are not made runnable until all > of them are dequeued from the turnstile and assigned a new turnstile. Only > after all that is settled are the threads made runnable in turnstile_unpend(). > > However, that doesn't fix this specific race (though it means the turnstile > code is not subject to the same exact race as the sleepq code above). Making > turnstiles type-stable is indeed probably the only fix for this. :-/ What about setting a flag meaning "turnstile recycled but still used" and allow turnstile_wait() to spin until it gets unset before to access to td_turnstile? Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein