From owner-freebsd-arch@FreeBSD.ORG Sat Jan 23 00:33:30 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 B14891065679; Sat, 23 Jan 2010 00:33:30 +0000 (UTC) (envelope-from giovanni.trematerra@gmail.com) Received: from smtp.net.vodafone.it (smtp.net.vodafone.it [83.224.65.24]) by mx1.freebsd.org (Postfix) with ESMTP id 3194D8FC08; Sat, 23 Jan 2010 00:33:29 +0000 (UTC) Received: from matrix64.localdomain ([109.115.25.212]) by smtp.net.vodafone.it with ESMTP id o0N05DsA024036; Sat, 23 Jan 2010 01:05:15 +0100 Received: from matrix64.localdomain (localhost [127.0.0.1]) by matrix64.localdomain (8.14.3/8.14.3) with ESMTP id o0N05EwZ001504; Sat, 23 Jan 2010 01:05:15 +0100 (CET) (envelope-from giovanni.trematerra@gmail.com) Received: (from gianni@localhost) by matrix64.localdomain (8.14.3/8.14.3/Submit) id o0N059a6001503; Sat, 23 Jan 2010 01:05:09 +0100 (CET) (envelope-from giovanni.trematerra@gmail.com) X-Authentication-Warning: matrix64.localdomain: gianni set sender to giovanni.trematerra@gmail.com using -f Date: Sat, 23 Jan 2010 01:05:08 +0100 From: Giovanni Trematerra To: Attilio Rao Message-ID: <20100123000508.GA1428@matrix64.localdomain> References: <3bbf2fe11001091709t228c48cft1c17af686e9e9c46@mail.gmail.com> <20100116125451.GA99364@stack.nl> <3bbf2fe11001220840o42a59d9cu476fe24063cd55b8@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3bbf2fe11001220840o42a59d9cu476fe24063cd55b8@mail.gmail.com> User-Agent: Mutt/1.4.2.3i Cc: Jilles Tjoelker , 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, 23 Jan 2010 00:33:30 -0000 On Fri, Jan 22, 2010 at 05:40:41PM +0100, Attilio Rao wrote: > 2010/1/16 Jilles Tjoelker : > > 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: /*- * Copyright (c) 2010 * Giovanni Trematerra * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above * copyright notice and this permission notice appear in all copies. * * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ #include #include #include #include #include #include #include #include #include #ifdef TESTPAUSE_DEBUG #define DPRINTF(x) do { printf x; } while (0) #else #define DPRINTF(x) #endif static struct mtx test_global_lock; static int global_condvar; volatile int QUIT; int test_thrcnt = 3; static void thr_suspender(void *arg) { struct thread *td = (struct thread *) arg; int error; for (;;) { if (QUIT == 1) break; error = kthread_suspend(td, 10*hz); if (error != 0) { if (error == EWOULDBLOCK) panic("Ooops: kthread deadlock\n"); else panic("kthread_suspend error: %d\n", error); break; } } mtx_lock(&test_global_lock); test_thrcnt--; wakeup(&global_condvar); mtx_unlock(&test_global_lock); kthread_exit(); } static void thr_resumer(void *arg) { struct thread *td = (struct thread *) arg; int error; for (;;) { if (QUIT == 1) break; error = kthread_resume(td); if (error != 0) panic("%s: error on kthread_resume. error: %d\n", __func__, error); if (QUIT == 1) break; } mtx_lock(&test_global_lock); test_thrcnt--; wakeup(&global_condvar); mtx_unlock(&test_global_lock); kthread_exit(); } static void thr_getsuspended(void *arg) { for (;;) { if (QUIT == 1) break; kthread_suspend_check(curthread); } mtx_lock(&test_global_lock); test_thrcnt--; wakeup(&global_condvar); mtx_unlock(&test_global_lock); kthread_exit(); } static void kthrdlk_init(void) { struct proc *testproc; struct thread *newthr; int error; QUIT = 0; mtx_init(&test_global_lock, "thrdlk_lock", NULL, MTX_DEF); testproc = NULL; error = kproc_kthread_add(thr_getsuspended, NULL, &testproc, &newthr, 0, 0, "kthrdlk", "thr_getsuspended"); if (error != 0) uprintf("cannot start thr_getsuspended error: %d\n", error); error = kproc_kthread_add(thr_resumer, newthr, &testproc, NULL, 0, 0, "testproc", "thr_resumer"); if (error != 0) uprintf("cannot start thr_resumer error: %d\n", error); error = kproc_kthread_add(thr_suspender, newthr, &testproc, NULL, 0, 0, "testproc", "thr_suspender"); if (error != 0) uprintf("cannot start thr_suspender error: %d\n", error); } static void kthrdlk_done(void) { int ret; /* wait kernel threads end */ mtx_lock(&test_global_lock); QUIT = 1; while (test_thrcnt != 0) { ret = mtx_sleep(&global_condvar, &test_global_lock, 0, "waiting thrs end", 30 * hz); if (ret == EWOULDBLOCK) { uprintf("thrpause not die! remaing: %d", test_thrcnt); break; } } if (test_thrcnt == 0) DPRINTF(("All threads stopped \n")); mtx_destroy(&test_global_lock); } static int kthrdlk_handler(module_t mod, int /*modeventtype_t*/ what, void *arg) { switch (what) { case MOD_LOAD: kthrdlk_init(); uprintf("kthrdlk loaded!\n"); return (0); case MOD_UNLOAD: kthrdlk_done(); uprintf("Bye Bye! kthrdlk unloaded!\n"); return (0); } return (EOPNOTSUPP); } static moduledata_t mod_data= { "kthrdlk", kthrdlk_handler, 0 }; MODULE_VERSION(kthrdlk, 1); DECLARE_MODULE(kthrdlk, mod_data, SI_SUB_EXEC, SI_ORDER_ANY); -- Gianni