From owner-freebsd-arch@FreeBSD.ORG Mon Nov 17 11:06:49 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 7260A1065677 for ; Mon, 17 Nov 2008 11:06:49 +0000 (UTC) (envelope-from owner-bugmaster@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 178E88FC1A for ; Mon, 17 Nov 2008 11:06:47 +0000 (UTC) (envelope-from owner-bugmaster@FreeBSD.org) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id mAHB6kjt082458 for ; Mon, 17 Nov 2008 11:06:46 GMT (envelope-from owner-bugmaster@FreeBSD.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id mAHB6kqV082454 for freebsd-arch@FreeBSD.org; Mon, 17 Nov 2008 11:06:46 GMT (envelope-from owner-bugmaster@FreeBSD.org) Date: Mon, 17 Nov 2008 11:06:46 GMT Message-Id: <200811171106.mAHB6kqV082454@freefall.freebsd.org> X-Authentication-Warning: freefall.freebsd.org: gnats set sender to owner-bugmaster@FreeBSD.org using -f From: FreeBSD bugmaster To: freebsd-arch@FreeBSD.org Cc: Subject: Current problem reports assigned to freebsd-arch@FreeBSD.org 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: Mon, 17 Nov 2008 11:06:49 -0000 Note: to view an individual PR, use: http://www.freebsd.org/cgi/query-pr.cgi?pr=(number). The following is a listing of current problems submitted by FreeBSD users. These represent problem reports covering all versions including experimental development code and obsolete releases. S Tracker Resp. Description -------------------------------------------------------------------------------- o kern/120749 arch [request] Suggest upping the default kern.ps_arg_cache 1 problem total. From owner-freebsd-arch@FreeBSD.ORG Wed Nov 19 13:51:32 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 0FEA91065677 for ; Wed, 19 Nov 2008 13:51:32 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from outbound.icp-qv1-irony-out2.iinet.net.au (outbound.icp-qv1-irony-out2.iinet.net.au [203.59.1.107]) by mx1.freebsd.org (Postfix) with ESMTP id 853098FC16 for ; Wed, 19 Nov 2008 13:51:31 +0000 (UTC) (envelope-from lstewart@freebsd.org) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApoEAA2iI0l8qEpL/2dsb2JhbADSeIJ5 X-IronPort-AV: E=Sophos;i="4.33,631,1220198400"; d="scan'208";a="402785684" Received: from unknown (HELO lawrence1.loshell.room52.net) ([124.168.74.75]) by outbound.icp-qv1-irony-out2.iinet.net.au with ESMTP; 19 Nov 2008 22:21:44 +0900 Message-ID: <492412E8.3060700@freebsd.org> Date: Thu, 20 Nov 2008 00:21:44 +1100 From: Lawrence Stewart User-Agent: Thunderbird 2.0.0.17 (X11/20081109) MIME-Version: 1.0 To: freebsd-arch@freebsd.org Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: 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: Wed, 19 Nov 2008 13:51:32 -0000 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. From sys/kern/kern_kthread.c ----------------------begin 8.x kthread_exit()-------------------------- void kthread_exit(void) { struct proc *p; /* A module may be waiting for us to exit. */ wakeup(curthread); /* * We could rely on thread_exit to call exit1() but * there is extra work that needs to be done */ if (curthread->td_proc->p_numthreads == 1) kproc_exit(0); /* never returns */ p = curthread->td_proc; PROC_LOCK(p); PROC_SLOCK(p); thread_exit(); } ----------------------end 8.x kthread_exit()---------------------------- ---------------------begin 7.x kthread_exit()--------------------------- void kthread_exit(int ecode) { struct thread *td; struct proc *p; td = curthread; p = td->td_proc; /* * Reparent curthread from proc0 to init so that the zombie * is harvested. */ sx_xlock(&proctree_lock); PROC_LOCK(p); proc_reparent(p, initproc); PROC_UNLOCK(p); sx_xunlock(&proctree_lock); /* * Wakeup anyone waiting for us to exit. */ wakeup(p); /* Buh-bye! */ exit1(td, W_EXITCODE(ecode, 0)); } ----------------------end 7.x kthread_exit()---------------------------- From the 7.x kthread(9) manpage: "While exiting, the function exit1(9) will initiate a call to wakeup(9) on the thread handle." The 8.x kthread manpage has no mention of the wakeup behaviour whatsoever. So from the code above, we can see that the 7.x kthread_exit() calls wakeup() on the *proc instead of the *thread. In 8.x, kthread_exit() calls wakeup() on the *thread and the newly added kproc_exit() function will wakeup() anyone waiting on the *proc. Looking at: http://svn.freebsd.org/viewvc/base/head/sys/kern/kern_kthread.c?view=log the confusion seems to have crept in around r173004 during the KPI refactoring to support true kernel threads. Historically it seems that kthread_exit() called wakeup on the *proc (which to my mind seems counter intuitive, but whatever). Then in r173052 we switch to the 8.x style of calling wakeup on the *thread, which matches the function naming convention and 7.x man page comment. At a minimum we need a better discussion of the differences in the man page, but the behaviour change seems unnecessarily intrusive to me and has nasty side effects i.e. deadlock. Keeping consistent wakeup behaviour between 7.x and 8.x would I suspect be desirable and avoid this issue biting others. Thoughts? Cheers, Lawrence From owner-freebsd-arch@FreeBSD.ORG Wed Nov 19 19:07:16 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 6F36E106567D; Wed, 19 Nov 2008 19:07:16 +0000 (UTC) (envelope-from prvs=julian=202b42db1@elischer.org) Received: from smtp-outbound.ironport.com (smtp-outbound.ironport.com [63.251.108.112]) by mx1.freebsd.org (Postfix) with ESMTP id 5A9B58FC17; Wed, 19 Nov 2008 19:07:16 +0000 (UTC) (envelope-from prvs=julian=202b42db1@elischer.org) Received: from unknown (HELO julian-mac.elischer.org) ([10.251.60.177]) by smtp-outbound.ironport.com with ESMTP; 19 Nov 2008 10:38:53 -0800 Message-ID: <49245D3B.8050607@elischer.org> Date: Wed, 19 Nov 2008 10:38:51 -0800 From: Julian Elischer User-Agent: Thunderbird 2.0.0.17 (Macintosh/20080914) MIME-Version: 1.0 To: Lawrence Stewart References: <492412E8.3060700@freebsd.org> In-Reply-To: <492412E8.3060700@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Wed, 19 Nov 2008 19:07:16 -0000 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. > > From sys/kern/kern_kthread.c > > ----------------------begin 8.x kthread_exit()-------------------------- > void > kthread_exit(void) > { > struct proc *p; > > /* A module may be waiting for us to exit. */ > wakeup(curthread); > > /* > * We could rely on thread_exit to call exit1() but > * there is extra work that needs to be done > */ > if (curthread->td_proc->p_numthreads == 1) > kproc_exit(0); /* never returns */ > > p = curthread->td_proc; > PROC_LOCK(p); > PROC_SLOCK(p); > thread_exit(); > } > ----------------------end 8.x kthread_exit()---------------------------- > > ---------------------begin 7.x kthread_exit()--------------------------- > void > kthread_exit(int ecode) > { > struct thread *td; > struct proc *p; > > td = curthread; > p = td->td_proc; > > /* > * Reparent curthread from proc0 to init so that the zombie > * is harvested. > */ > sx_xlock(&proctree_lock); > PROC_LOCK(p); > proc_reparent(p, initproc); > PROC_UNLOCK(p); > sx_xunlock(&proctree_lock); > > /* > * Wakeup anyone waiting for us to exit. > */ > wakeup(p); > > /* Buh-bye! */ > exit1(td, W_EXITCODE(ecode, 0)); > } > ----------------------end 7.x kthread_exit()---------------------------- > > From the 7.x kthread(9) manpage: > > "While exiting, the function exit1(9) will initiate a call to wakeup(9) > on the thread handle." > > The 8.x kthread manpage has no mention of the wakeup behaviour whatsoever. > > So from the code above, we can see that the 7.x kthread_exit() calls > wakeup() on the *proc instead of the *thread. That was a bug. > In 8.x, kthread_exit() > calls wakeup() on the *thread and the newly added kproc_exit() function > will wakeup() anyone waiting on the *proc. more intuitive, no? That is what what was supposed to happen but we can't change a Kernel API in mid series. > > Looking at: > http://svn.freebsd.org/viewvc/base/head/sys/kern/kern_kthread.c?view=log > the confusion seems to have crept in around r173004 during the KPI > refactoring to support true kernel threads. > > Historically it seems that kthread_exit() called wakeup on the *proc > (which to my mind seems counter intuitive, but whatever). Then in > r173052 we switch to the 8.x style of calling wakeup on the *thread, > which matches the function naming convention and 7.x man page comment. this is because historically kthread_xxx actes on actual processes. so the proc was unique to each. the kthread man page became the kproc man page so that is where the info on wakeup might have gone. A new kthread man page was made... > > At a minimum we need a better discussion of the differences in the man > page, but the behaviour change seems unnecessarily intrusive to me and > has nasty side effects i.e. deadlock. Keeping consistent wakeup > behaviour between 7.x and 8.x would I suspect be desirable and avoid > this issue biting others. which one would you change? > > Thoughts? > > Cheers, > Lawrence > _______________________________________________ > freebsd-arch@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" From owner-freebsd-arch@FreeBSD.ORG Wed Nov 19 22:16:26 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 6DFA91065670 for ; Wed, 19 Nov 2008 22:16:26 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from outbound.icp-qv1-irony-out3.iinet.net.au (outbound.icp-qv1-irony-out3.iinet.net.au [203.59.1.148]) by mx1.freebsd.org (Postfix) with ESMTP id BE9C68FC18 for ; Wed, 19 Nov 2008 22:16:25 +0000 (UTC) (envelope-from lstewart@freebsd.org) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApoEAGwYJEl8qEpL/2dsb2JhbADTMYJ5 X-IronPort-AV: E=Sophos;i="4.33,634,1220198400"; d="scan'208";a="352848008" Received: from unknown (HELO lawrence1.loshell.room52.net) ([124.168.74.75]) by outbound.icp-qv1-irony-out3.iinet.net.au with ESMTP; 20 Nov 2008 06:47:04 +0900 Message-ID: <49248958.9060308@freebsd.org> Date: Thu, 20 Nov 2008 08:47:04 +1100 From: Lawrence Stewart User-Agent: Thunderbird 2.0.0.17 (X11/20081109) MIME-Version: 1.0 To: Julian Elischer References: <492412E8.3060700@freebsd.org> <49245D3B.8050607@elischer.org> In-Reply-To: <49245D3B.8050607@elischer.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Wed, 19 Nov 2008 22:16:26 -0000 Julian Elischer wrote: > 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. >> >> From sys/kern/kern_kthread.c >> >> ----------------------begin 8.x kthread_exit()-------------------------- >> void >> kthread_exit(void) >> { >> struct proc *p; >> >> /* A module may be waiting for us to exit. */ >> wakeup(curthread); >> >> /* >> * We could rely on thread_exit to call exit1() but >> * there is extra work that needs to be done >> */ >> if (curthread->td_proc->p_numthreads == 1) >> kproc_exit(0); /* never returns */ >> >> p = curthread->td_proc; >> PROC_LOCK(p); >> PROC_SLOCK(p); >> thread_exit(); >> } >> ----------------------end 8.x kthread_exit()---------------------------- >> >> ---------------------begin 7.x kthread_exit()--------------------------- >> void >> kthread_exit(int ecode) >> { >> struct thread *td; >> struct proc *p; >> >> td = curthread; >> p = td->td_proc; >> >> /* >> * Reparent curthread from proc0 to init so that the zombie >> * is harvested. >> */ >> sx_xlock(&proctree_lock); >> PROC_LOCK(p); >> proc_reparent(p, initproc); >> PROC_UNLOCK(p); >> sx_xunlock(&proctree_lock); >> >> /* >> * Wakeup anyone waiting for us to exit. >> */ >> wakeup(p); >> >> /* Buh-bye! */ >> exit1(td, W_EXITCODE(ecode, 0)); >> } >> ----------------------end 7.x kthread_exit()---------------------------- >> >> From the 7.x kthread(9) manpage: >> >> "While exiting, the function exit1(9) will initiate a call to >> wakeup(9) on the thread handle." >> >> The 8.x kthread manpage has no mention of the wakeup behaviour >> whatsoever. >> >> So from the code above, we can see that the 7.x kthread_exit() calls >> wakeup() on the *proc instead of the *thread. > > > That was a bug. > >> In 8.x, kthread_exit() calls wakeup() on the *thread and the newly >> added kproc_exit() function will wakeup() anyone waiting on the *proc. > > more intuitive, no? That is what what was supposed to happen > but we can't change a Kernel API in mid series. Yes I agree the 8.x behaviour is more intuitive. I'm not sure I'm clear on why the wakeup behaviour is part of the KPI though. The documented behaviour in the 7.x kthread(9) man page is that it calls wakeup() on the "thread handle". So the documented behaviour is the intuitively correct one. The actual behaviour is "wrong", although historically consistent. > > >> >> Looking at: >> http://svn.freebsd.org/viewvc/base/head/sys/kern/kern_kthread.c?view=log >> the confusion seems to have crept in around r173004 during the KPI >> refactoring to support true kernel threads. >> >> Historically it seems that kthread_exit() called wakeup on the *proc >> (which to my mind seems counter intuitive, but whatever). Then in >> r173052 we switch to the 8.x style of calling wakeup on the *thread, >> which matches the function naming convention and 7.x man page comment. > > this is because historically kthread_xxx actes on actual processes. > so the proc was unique to each. Yep I understood that looking through the history. Even still I don't see why we didn't just call wakeup() on the *thread anyway (or if *thread wasn't meaningful previously, equate the *thread to the *proc). > > the kthread man page became the kproc man page so that is where the > info on wakeup might have gone. A new kthread man page was made... Ah ok, I had missed the rename of the man page. You are correct, kproc(9) mentions calling wakeup() on the *proc. > >> >> At a minimum we need a better discussion of the differences in the man >> page, but the behaviour change seems unnecessarily intrusive to me and >> has nasty side effects i.e. deadlock. Keeping consistent wakeup >> behaviour between 7.x and 8.x would I suspect be desirable and avoid >> this issue biting others. > > which one would you change? heh, good question. On the one hand we have the intuitively correct behaviour in 8.x, although the 8.x kthread_exit() behaviour with respect to wakeup() is not documented at all in the kthread(9) man page. On the other, we have the 7.x documented behaviour which is correct, but the actual behaviour of the code (which is historically consistent) is incorrect and at odds with the 8.x behaviour. I'm playing devil's advocate here as now I'm curious whether this issue is really considered part of the KPI or not. If the actual behaviour is what's important, then we obviously can't make the change in 7.x. If the documented behaviour is what we are supposed to be honoring, then technically the change could be made, no? Devil's advocate musings aside, my personal feelings are that we should be aiming for intuitive correctness in the KPI i.e. leaving the 8.x code as it is makes sense. Even though I feel the wakeup() behaviour is not technically part of the KPI in 7.x, I don't think we should change the code. Therefore I would propose some improvements to both the 7.x and 8.x kthread(9) man pages which clearly document the actual behaviour and subtle differences between 7.x and 8.x. I also suspect an entry in UPDATING should be added close to the existing 20071020 entry that retrospectively discusses the switch and the subtle difference in kthread_exit() behaviour. Finally, mentioning that the value of __FreeBSD_version can be checked against 800002 using an #ifdef test to conditionally detect which behaviour should be used would also be a good idea. The above changes should equip developers with all the info needed to maintain code that crosses the 7.x/8.x gap with minimal loss in hair. Cheers, Lawrence From owner-freebsd-arch@FreeBSD.ORG Wed Nov 19 22:41:41 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 E06021065670; Wed, 19 Nov 2008 22:41:41 +0000 (UTC) (envelope-from prvs=julian=202b42db1@elischer.org) Received: from smtp-outbound.ironport.com (smtp-outbound.ironport.com [63.251.108.112]) by mx1.freebsd.org (Postfix) with ESMTP id CC4FF8FC08; Wed, 19 Nov 2008 22:41:41 +0000 (UTC) (envelope-from prvs=julian=202b42db1@elischer.org) Received: from unknown (HELO julian-mac.elischer.org) ([10.251.60.94]) by smtp-outbound.ironport.com with ESMTP; 19 Nov 2008 14:41:42 -0800 Message-ID: <49249624.7020108@elischer.org> Date: Wed, 19 Nov 2008 14:41:40 -0800 From: Julian Elischer User-Agent: Thunderbird 2.0.0.17 (Macintosh/20080914) MIME-Version: 1.0 To: Lawrence Stewart References: <492412E8.3060700@freebsd.org> <49245D3B.8050607@elischer.org> <49248958.9060308@freebsd.org> In-Reply-To: <49248958.9060308@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Wed, 19 Nov 2008 22:41:42 -0000 Lawrence Stewart wrote: > I'm not sure I'm clear on why the wakeup behaviour is part of the KPI > though. The documented behaviour in the 7.x kthread(9) man page is that > it calls wakeup() on the "thread handle". So the documented behaviour is > the intuitively correct one. The actual behaviour is "wrong", although > historically consistent. AH I thought you said the documentation for kthread didn't mention teh wakeup.. so, we should probably MFC the fix [...] >> >> which one would you change? > > heh, good question. > > On the one hand we have the intuitively correct behaviour in 8.x, > although the 8.x kthread_exit() behaviour with respect to wakeup() is > not documented at all in the kthread(9) man page. patch suggested :-) > > On the other, we have the 7.x documented behaviour which is correct, but > the actual behaviour of the code (which is historically consistent) is > incorrect and at odds with the 8.x behaviour. in 7.x nearly everything uses kproc... so we could probably safely change it now. > > I'm playing devil's advocate here as now I'm curious whether this issue > is really considered part of the KPI or not. If the actual behaviour is > what's important, then we obviously can't make the change in 7.x. If the > documented behaviour is what we are supposed to be honoring, then > technically the change could be made, no? > > Devil's advocate musings aside, my personal feelings are that we should > be aiming for intuitive correctness in the KPI i.e. leaving the 8.x code > as it is makes sense. Even though I feel the wakeup() behaviour is not > technically part of the KPI in 7.x, I don't think we should change the > code. > > Therefore I would propose some improvements to both the 7.x and 8.x > kthread(9) man pages which clearly document the actual behaviour and > subtle differences between 7.x and 8.x. > > I also suspect an entry in UPDATING should be added close to the > existing 20071020 entry that retrospectively discusses the switch and > the subtle difference in kthread_exit() behaviour. > > Finally, mentioning that the value of __FreeBSD_version can be checked > against 800002 using an #ifdef test to conditionally detect which > behaviour should be used would also be a good idea. > > The above changes should equip developers with all the info needed to > maintain code that crosses the 7.x/8.x gap with minimal loss in hair. from MEMORY the wakeup and sleep are both done inside our own functions and the user is not expected to do them himself. so as long as we fix it on both sides.... (I may be wrong on that, I haven't looked at the code but just memories..) > > Cheers, > Lawrence From owner-freebsd-arch@FreeBSD.ORG Thu Nov 20 00:04:48 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 8190F106564A for ; Thu, 20 Nov 2008 00:04:48 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from outbound.icp-qv1-irony-out4.iinet.net.au (outbound.icp-qv1-irony-out4.iinet.net.au [203.59.1.150]) by mx1.freebsd.org (Postfix) with ESMTP id EC5B18FC12 for ; Thu, 20 Nov 2008 00:04:47 +0000 (UTC) (envelope-from lstewart@freebsd.org) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApoEACM2JEl8qEpL/2dsb2JhbADTLYJ5 X-IronPort-AV: E=Sophos;i="4.33,635,1220198400"; d="scan'208";a="278606325" Received: from unknown (HELO lawrence1.loshell.room52.net) ([124.168.74.75]) by outbound.icp-qv1-irony-out4.iinet.net.au with ESMTP; 20 Nov 2008 08:52:31 +0900 Message-ID: <4924A6BE.8080308@freebsd.org> Date: Thu, 20 Nov 2008 10:52:30 +1100 From: Lawrence Stewart User-Agent: Thunderbird 2.0.0.17 (X11/20081109) MIME-Version: 1.0 To: Julian Elischer References: <492412E8.3060700@freebsd.org> <49245D3B.8050607@elischer.org> <49248958.9060308@freebsd.org> <49249624.7020108@elischer.org> In-Reply-To: <49249624.7020108@elischer.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Thu, 20 Nov 2008 00:04:48 -0000 Julian Elischer wrote: > Lawrence Stewart wrote: > >> I'm not sure I'm clear on why the wakeup behaviour is part of the KPI >> though. The documented behaviour in the 7.x kthread(9) man page is >> that it calls wakeup() on the "thread handle". So the documented >> behaviour is the intuitively correct one. The actual behaviour is >> "wrong", although historically consistent. > AH > > I thought you said the documentation for kthread didn't mention teh > wakeup.. > so, we should probably MFC the fix > I said the 7.x kthread(9) man page mentioned the wakeup(), the 8.x kthread(9) man page does not. This definitely needs to be corrected. > > [...] > >>> >>> which one would you change? >> >> heh, good question. >> >> On the one hand we have the intuitively correct behaviour in 8.x, >> although the 8.x kthread_exit() behaviour with respect to wakeup() is >> not documented at all in the kthread(9) man page. > > patch suggested :-) Can do. > >> >> On the other, we have the 7.x documented behaviour which is correct, >> but the actual behaviour of the code (which is historically >> consistent) is incorrect and at odds with the 8.x behaviour. > > in 7.x nearly everything uses kproc... so we could probably safely > change it now. We could definitely change it for all in tree cases easily enough. My concern is out of tree code. This change would make any out of tree code that relies on the actually implemented wakeup() mechanism potentially deadlock, which is not nice. We could argue that the documented behaviour is correct and that we're correcting a bug with the fix... still, tis cold comfort for anyone who's working code now deadlocks. I do like the maintenance simplicity the change would bring moving forward. I'm still not sure the code change is the best idea. Does anyone else have thoughts on the matter? > >> >> I'm playing devil's advocate here as now I'm curious whether this >> issue is really considered part of the KPI or not. If the actual >> behaviour is what's important, then we obviously can't make the change >> in 7.x. If the documented behaviour is what we are supposed to be >> honoring, then technically the change could be made, no? >> >> Devil's advocate musings aside, my personal feelings are that we >> should be aiming for intuitive correctness in the KPI i.e. leaving the >> 8.x code as it is makes sense. Even though I feel the wakeup() >> behaviour is not technically part of the KPI in 7.x, I don't think we >> should change the code. >> >> Therefore I would propose some improvements to both the 7.x and 8.x >> kthread(9) man pages which clearly document the actual behaviour and >> subtle differences between 7.x and 8.x. >> >> I also suspect an entry in UPDATING should be added close to the >> existing 20071020 entry that retrospectively discusses the switch and >> the subtle difference in kthread_exit() behaviour. >> >> Finally, mentioning that the value of __FreeBSD_version can be checked >> against 800002 using an #ifdef test to conditionally detect which >> behaviour should be used would also be a good idea. >> >> The above changes should equip developers with all the info needed to >> maintain code that crosses the 7.x/8.x gap with minimal loss in hair. > > from MEMORY the wakeup and sleep are both done inside our own > functions and the user is not expected to do them himself. > so as long as we fix it on both sides.... > (I may be wrong on that, I haven't looked at the code but just memories..) Not sure what sleep you're referring to, but yes we say we don't require the user to do their own wakeup() as their thread dies. Though consumers of the API in 7.x and 8.x might have worked around the difference in behaviour by adding their own wakeup() (it's how I did it initially before digging a bit deeper). Cheers, Lawrence From owner-freebsd-arch@FreeBSD.ORG Thu Nov 20 00:38:36 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 CE4F61065670 for ; Thu, 20 Nov 2008 00:38:36 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from outbound.icp-qv1-irony-out4.iinet.net.au (outbound.icp-qv1-irony-out4.iinet.net.au [203.59.1.150]) by mx1.freebsd.org (Postfix) with ESMTP id 52B6F8FC13 for ; Thu, 20 Nov 2008 00:38:36 +0000 (UTC) (envelope-from lstewart@freebsd.org) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApoEALBAJEl8qEpL/2dsb2JhbADTD4ItTA X-IronPort-AV: E=Sophos;i="4.33,636,1220198400"; d="scan'208";a="278628727" Received: from unknown (HELO lawrence1.loshell.room52.net) ([124.168.74.75]) by outbound.icp-qv1-irony-out4.iinet.net.au with ESMTP; 20 Nov 2008 09:38:34 +0900 Message-ID: <4924B18A.4060100@freebsd.org> Date: Thu, 20 Nov 2008 11:38:34 +1100 From: Lawrence Stewart User-Agent: Thunderbird 2.0.0.17 (X11/20081109) MIME-Version: 1.0 To: Julian Elischer References: <492412E8.3060700@freebsd.org> <49245D3B.8050607@elischer.org> <49248958.9060308@freebsd.org> <49249624.7020108@elischer.org> <4924A6BE.8080308@freebsd.org> In-Reply-To: <4924A6BE.8080308@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Thu, 20 Nov 2008 00:38:36 -0000 Lawrence Stewart wrote: > Julian Elischer wrote: >> Lawrence Stewart wrote: >> [snip] > >> >>> >>> On the other, we have the 7.x documented behaviour which is correct, >>> but the actual behaviour of the code (which is historically >>> consistent) is incorrect and at odds with the 8.x behaviour. >> >> in 7.x nearly everything uses kproc... so we could probably safely >> change it now. > > We could definitely change it for all in tree cases easily enough. My > concern is out of tree code. This change would make any out of tree code > that relies on the actually implemented wakeup() mechanism potentially > deadlock, which is not nice. We could argue that the documented > behaviour is correct and that we're correcting a bug with the fix... > still, tis cold comfort for anyone who's working code now deadlocks. > > I do like the maintenance simplicity the change would bring moving forward. > > I'm still not sure the code change is the best idea. Does anyone else > have thoughts on the matter? > [snip] *slaps forehead* So, it just occurred to me through my mid-morning mental haze after a chat with attilio@ that we should just call wakeup() on both the *proc _and_ *thread in 7.x and be done with it. It doesn't hurt anyone, maintains the current behaviour, ensures we're living up to our documented KPI of delivering a wakeup on the thread handle and resolves the compatibility issues between 7.x and 8.x in this respect - no potential deadlocks is good++. I'll have a go at a patch for code + man pages shortly. Cheers, Lawrence From owner-freebsd-arch@FreeBSD.ORG Thu Nov 20 20:37:05 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 544A81065674; Thu, 20 Nov 2008 20:37:05 +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 A0F328FC1C; Thu, 20 Nov 2008 20:37:04 +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 mAKKaq1G068073; Thu, 20 Nov 2008 15:36:58 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: freebsd-arch@freebsd.org Date: Thu, 20 Nov 2008 15:02:23 -0500 User-Agent: KMail/1.9.7 References: <492412E8.3060700@freebsd.org> In-Reply-To: <492412E8.3060700@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200811201502.23943.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [IPv6:::1]); Thu, 20 Nov 2008 15:36:58 -0500 (EST) X-Virus-Scanned: ClamAV 0.93.1/8653/Thu Nov 20 04:04:07 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: Lawrence Stewart 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: Thu, 20 Nov 2008 20:37:05 -0000 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 kthread_add() and kproc_kthread_add() both return thread pointers. Hence in 8.x kthread_exit() is used for exiting kernel threads and wakes up the thread pointer, but in 7.x kthread_exit() is used for exiting kernel processes and wakes up the proc pointer. I think what is probably needed is to simply document that arrangement as such. Note that the sleeping on proc pointer has been the documented way to synchronize with kthread_exit() since 5.0. -- John Baldwin From owner-freebsd-arch@FreeBSD.ORG Thu Nov 20 22:22:06 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 4EC8F1065673 for ; Thu, 20 Nov 2008 22:22:06 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from outbound.icp-qv1-irony-out2.iinet.net.au (outbound.icp-qv1-irony-out2.iinet.net.au [203.59.1.107]) by mx1.freebsd.org (Postfix) with ESMTP id B82A18FC1F for ; Thu, 20 Nov 2008 22:22:05 +0000 (UTC) (envelope-from lstewart@freebsd.org) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApoEAB5yJUl8qEpL/2dsb2JhbADRYYJ8 X-IronPort-AV: E=Sophos;i="4.33,639,1220198400"; d="scan'208";a="403343341" Received: from unknown (HELO lawrence1.loshell.room52.net) ([124.168.74.75]) by outbound.icp-qv1-irony-out2.iinet.net.au with ESMTP; 21 Nov 2008 07:22:03 +0900 Message-ID: <4925E30B.8010709@freebsd.org> Date: Fri, 21 Nov 2008 09:22:03 +1100 From: Lawrence Stewart User-Agent: Thunderbird 2.0.0.17 (X11/20081109) MIME-Version: 1.0 To: John Baldwin References: <492412E8.3060700@freebsd.org> <200811201502.23943.jhb@freebsd.org> In-Reply-To: <200811201502.23943.jhb@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Thu, 20 Nov 2008 22:22:06 -0000 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. > kthread_add() and kproc_kthread_add() both return thread pointers. Hence in Yup. > 8.x kthread_exit() is used for exiting kernel threads and wakes up the thread > pointer, but in 7.x kthread_exit() is used for exiting kernel processes and > wakes up the proc pointer. I think what is probably needed is to simply In the code, yes. Our documented behaviour as far as I can tell is different though, unless we equate a "thread handle" to "proc handle" prior to 8.x, which I don't think is the case - they are still different. > document that arrangement as such. Note that the sleeping on proc pointer I agree that the arrangement needs to be better documented. The change in 8.x is subtle enough that reading the kthread man page in 7.x and 8.x doesn't immediately make it obvious what's going on. > has been the documented way to synchronize with kthread_exit() since 5.0. > Could you please point me at this documentation? I've missed it in my poking around thus far. Cheers, Lawrence From owner-freebsd-arch@FreeBSD.ORG Fri Nov 21 09:25:08 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 A6B0D106564A; Fri, 21 Nov 2008 09:25:08 +0000 (UTC) (envelope-from prvs=julian=204d9a09f@elischer.org) Received: from smtp-outbound.ironport.com (smtp-outbound.ironport.com [63.251.108.112]) by mx1.freebsd.org (Postfix) with ESMTP id 7F8158FC16; Fri, 21 Nov 2008 09:25:08 +0000 (UTC) (envelope-from prvs=julian=204d9a09f@elischer.org) Received: from unknown (HELO julian-mac.elischer.org) ([10.251.60.108]) by smtp-outbound.ironport.com with ESMTP; 21 Nov 2008 00:56:15 -0800 Message-ID: <492677AE.7060607@elischer.org> Date: Fri, 21 Nov 2008 00:56:14 -0800 From: Julian Elischer User-Agent: Thunderbird 2.0.0.18 (Macintosh/20081105) MIME-Version: 1.0 To: Lawrence Stewart References: <492412E8.3060700@freebsd.org> <200811201502.23943.jhb@freebsd.org> <4925E30B.8010709@freebsd.org> In-Reply-To: <4925E30B.8010709@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Fri, 21 Nov 2008 09:25:08 -0000 Lawrence Stewart wrote: > > I agree that the arrangement needs to be better documented. The change > in 8.x is subtle enough that reading the kthread man page in 7.x and 8.x > doesn't immediately make it obvious what's going on. > I tried to walk the fine line between being completely incompatible and letting people fall into a trap. The arguments and prototypes are different so code should fail to compile unless people have made changes to their code.. From owner-freebsd-arch@FreeBSD.ORG Fri Nov 21 19:59:09 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 4B6781065677; Fri, 21 Nov 2008 19:59:09 +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 D795E8FC14; Fri, 21 Nov 2008 19:59:08 +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 mALJwuri081978; Fri, 21 Nov 2008 14:59:02 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: Lawrence Stewart Date: Fri, 21 Nov 2008 13:48:41 -0500 User-Agent: KMail/1.9.7 References: <492412E8.3060700@freebsd.org> <200811201502.23943.jhb@freebsd.org> <4925E30B.8010709@freebsd.org> In-Reply-To: <4925E30B.8010709@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200811211348.41536.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [IPv6:::1]); Fri, 21 Nov 2008 14:59:03 -0500 (EST) X-Virus-Scanned: ClamAV 0.93.1/8661/Fri Nov 21 10:39:30 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: Fri, 21 Nov 2008 19:59:09 -0000 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, ...); } > > kthread_add() and kproc_kthread_add() both return thread pointers. Hence in > > Yup. > > > 8.x kthread_exit() is used for exiting kernel threads and wakes up the thread > > pointer, but in 7.x kthread_exit() is used for exiting kernel processes and > > wakes up the proc pointer. I think what is probably needed is to simply > > In the code, yes. Our documented behaviour as far as I can tell is > different though, unless we equate a "thread handle" to "proc handle" > prior to 8.x, which I don't think is the case - they are still different. It has always been the case in < 8 that you sleep on the proc handle (what kthread_create() actually returns in < 8). And in fact, you even have to dig around in the proc you get from kthread_create() to even find the thread pointer as opposed to having the API hand it to you. > > document that arrangement as such. Note that the sleeping on proc pointer > > I agree that the arrangement needs to be better documented. The change > in 8.x is subtle enough that reading the kthread man page in 7.x and 8.x > doesn't immediately make it obvious what's going on. > > > has been the documented way to synchronize with kthread_exit() since 5.0. > > > > Could you please point me at this documentation? I've missed it in my > poking around thus far. It is probably only documented in numerous threads in the mail archives sadly, but there have been several of them and there have been several fixes to get this right (the randomdev thread and fdc(4) thread come to mind). -- John Baldwin