From owner-freebsd-arch@FreeBSD.ORG Sat Jan 16 12:54:54 2010 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 210F21065698; Sat, 16 Jan 2010 12:54:54 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id D8DD98FC23; Sat, 16 Jan 2010 12:54:53 +0000 (UTC) Received: from toad.stack.nl (toad.stack.nl [IPv6:2001:610:1108:5010::135]) by mx1.stack.nl (Postfix) with ESMTP id E3EA735A825; Sat, 16 Jan 2010 13:54:51 +0100 (CET) Received: by toad.stack.nl (Postfix, from userid 1677) id CDC7073F9D; Sat, 16 Jan 2010 13:54:51 +0100 (CET) Date: Sat, 16 Jan 2010 13:54:51 +0100 From: Jilles Tjoelker To: Attilio Rao Message-ID: <20100116125451.GA99364@stack.nl> References: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com> User-Agent: Mutt/1.5.18 (2008-05-17) Cc: freebsd-arch@freebsd.org Subject: Re: [PATCH] kthread_{suspend, resume, suspend_check} locking bugs X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 16 Jan 2010 12:54:54 -0000 On Sun, Jan 10, 2010 at 02:09:43AM +0100, Attilio Rao wrote: > I think that routines kthread_suspend(), kthread_resume() and > kthread_suspend_check() are not adeguately SMP protected. > That is because, in particular, the critical path doesn't protect, > together, TDF_KTH_SUSP and sleeping activity. The right pattern should > be to use the thread lock spinlock as an interlock and use msleep. > Such bugs have not been revealed probabilly because there has been a > lack of testing of such primitives and there are not, currently, > consumers within our stock kernel. > Additively, kthread_suspend_check() seems to require to always pass > curthread, which is silly (as we don't have to conform to any > particular KPI), thus I think it is appropriate for the prototype to > change. > The following patch should fix the issue: > http://www.freebsd.org/~attilio/kthread.diff The analysis and patch look sensible. > That patch would have benefit from the priority argument (for PDROP) > for msleep_spin(9), and I don't understand why we don't support it > (the log message doesn't seem much clearer). > If nobody objects, after this patch goes in I would add the priority > argument to msleep_spin() too. No objection here. -- Jilles Tjoelker