Date: Tue, 2 Dec 2008 17:07:41 -0500 From: John Baldwin <jhb@freebsd.org> To: Lawrence Stewart <lstewart@freebsd.org> Cc: freebsd-arch@freebsd.org Subject: Re: kthread_exit(9) unexpectedness Message-ID: <200812021707.41545.jhb@freebsd.org> In-Reply-To: <4929F90B.1040502@freebsd.org> References: <492412E8.3060700@freebsd.org> <200811211348.41536.jhb@freebsd.org> <4929F90B.1040502@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200812021707.41545.jhb>