Date: Sat, 16 Jan 2010 13:54:51 +0100
From: Jilles Tjoelker <jilles@stack.nl>
To: Attilio Rao <attilio@freebsd.org>
Cc: freebsd-arch@freebsd.org
Subject: Re: [PATCH] kthread_{suspend, resume, suspend_check} locking bugs
Message-ID: <20100116125451.GA99364@stack.nl>
In-Reply-To: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com>
References: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100116125451.GA99364>