Date: Thu, 20 Nov 2008 10:52:30 +1100 From: Lawrence Stewart <lstewart@freebsd.org> To: Julian Elischer <julian@elischer.org> Cc: freebsd-arch@freebsd.org Subject: Re: kthread_exit(9) unexpectedness Message-ID: <4924A6BE.8080308@freebsd.org> In-Reply-To: <49249624.7020108@elischer.org> References: <492412E8.3060700@freebsd.org> <49245D3B.8050607@elischer.org> <49248958.9060308@freebsd.org> <49249624.7020108@elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4924A6BE.8080308>