From owner-freebsd-arch@FreeBSD.ORG Tue Dec 2 22:32:57 2008 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 0AB70106564A; Tue, 2 Dec 2008 22:32:57 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 8E5D78FC13; Tue, 2 Dec 2008 22:32:56 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [IPv6:::1]) (authenticated bits=0) by server.baldwin.cx (8.14.3/8.14.3) with ESMTP id mB2MWoEF011992; Tue, 2 Dec 2008 17:32:50 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: Lawrence Stewart Date: Tue, 2 Dec 2008 17:07:41 -0500 User-Agent: KMail/1.9.7 References: <492412E8.3060700@freebsd.org> <200811211348.41536.jhb@freebsd.org> <4929F90B.1040502@freebsd.org> In-Reply-To: <4929F90B.1040502@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200812021707.41545.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [IPv6:::1]); Tue, 02 Dec 2008 17:32:50 -0500 (EST) X-Virus-Scanned: ClamAV 0.93.1/8713/Tue Dec 2 14:59:31 2008 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.6 required=4.2 tests=AWL,BAYES_00,NO_RELAYS autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: freebsd-arch@freebsd.org Subject: Re: kthread_exit(9) unexpectedness 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: Tue, 02 Dec 2008 22:32:57 -0000 On Sunday 23 November 2008 07:44:59 pm Lawrence Stewart wrote: > John Baldwin wrote: > > On Thursday 20 November 2008 05:22:03 pm Lawrence Stewart wrote: > >> John Baldwin wrote: > >>> On Wednesday 19 November 2008 08:21:44 am Lawrence Stewart wrote: > >>>> Hi all, > >>>> > >>>> I tracked down a deadlock in some of my code today to some weird > >>>> behaviour in the kthread(9) KPI. The executive summary is that > >>>> kthread_exit() thread termination notification using wakeup() behaves as > >>>> expected intuitively in 8.x, but not in 7.x. > >>> In 5.x/6.x/7.x kthreads are still processes and it has always been a > > wakeup on > >>> the proc pointer. kthread_create() in 7.x returns a proc pointer, not a > >>> thread pointer for example. In 8.x kthreads are actual threads and > >> Yep, but the processes have a *thread in them right? The API naming is > >> obviously slightly misleading, but it essentially creates a new single > >> threaded process prior to 8.x. > > > > Yes, but you have to go explicitly use FIRST_THREAD_IN_PROC(). Most of the > > kernel modules I've written that use kthread's in < 8 do this: > > > > static struct proc *foo_thread; > > > > /* Called for MOD_LOAD. */ > > static void > > load(...) > > { > > > > error = kthread_create(..., &foo_thread); > > } > > > > static void > > unload(...) > > { > > > > /* set flag */ > > msleep(foo_thread, ...); > > } > > > > And never actually use the thread at all. However, if you write the code for > > 8.x, now you _do_ get a kthread and sleep on the thread so it becomes: > > > > static struct thread *foo_thread; > > > > static void > > load(...) > > { > > > > error = kproc_kthread_add(..., proc0, &foo_thread); > > } > > > > static void > > unload(...) > > { > > > > /* set flag */ > > msleep(foo_thread, ...); > > } > > > > > Sure, but to write the code in this way means you are exercising > undocumented knowledge of the KPI. I suspect the average developer > completely unfamiliar with the KPI would (and should!) use the man page > to learn about the functionality it provides. > > With that basis in mind, it seems unreasonable to expect the developer > to come to the conclusion that "...will initiate a call to wakeup(9) on > the thread handle." refers to sleeping on the *proc passed in to > kthread_create. Perhaps I'm not as switched on as the average developer, > but when I read it I certainly did not understand that the KPI created > processes and that the man page used the term thread to really mean a > single threaded process. I also did no equate "thread handle" with the > *proc returned by kthread_create. This is why the API in 8 is better, it is far less confusing. > Apart from the discussion thus far, you haven't actually commented yet > on my proposed single line change to kthread_exit() in 7.x to call > wakeup on the *thread as well as the *proc. Do you have any specific > thoughts on or objection to that idea? I would rather fix the docs first and not encourage folks to use FIRST_THREAD_IN_PROC (a). My only worry about the additional wakeup is other places that may be sleeping on the thread pointer for another reason. It might even be better to add a dedicated condvar to 'struct thread' in 8.x that is used for the wakeup and do the wakeup on that rather than the thread pointer to be honest. -- John Baldwin