kthread_exit(9) unexpectedness

[Lawrence Stewart]

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