Date: Sat, 23 Jan 2010 01:38:37 +0000
From: Rui Paulo <rpaulo@freebsd.org>
To: Giovanni Trematerra <giovanni.trematerra@gmail.com>
Cc: Attilio Rao <attilio@freebsd.org>, Jilles Tjoelker <jilles@stack.nl>, freebsd-arch@freebsd.org
Subject: Re: [PATCH] kthread_{suspend, resume, suspend_check} locking bugs
Message-ID: <98A68030-9AD6-4F80-8CCE-29FF29355675@freebsd.org>
In-Reply-To: <20100123000508.GA1428@matrix64.localdomain>
References: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com> <20100116125451.GA99364@stack.nl> <3bbf2fe11001220840o42a59d9cu476fe24063cd55b8@mail.gmail.com> <20100123000508.GA1428@matrix64.localdomain>
next in thread | previous in thread | raw e-mail | index | archive | help
On 23 Jan 2010, at 00:05, Giovanni Trematerra wrote: > On Fri, Jan 22, 2010 at 05:40:41PM +0100, Attilio Rao wrote: >> 2010/1/16 Jilles Tjoelker <jilles@stack.nl>: >>> 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. >> >> After have digged more with jhb I made a new patch: >> http://www.freebsd.org/~attilio/kthread_races.diff > > In kthread_suspend_check you might have written > panic("%s: curthread is not a valid kthread", __func__); > >> >> Gianni, may you test this patch with the modules you made? >> > > With your last patch, no deadlock happens now. > It seems ok. > > Hope this help > > Just in case someone would make a review of the kernel test module > here it is the code: This is a great candidate for a regression add-on. -- Rui Paulo
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?98A68030-9AD6-4F80-8CCE-29FF29355675>