kthread_exit(9) unexpectedness

[John Baldwin]

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