kthread_exit(9) unexpectedness

[Lawrence Stewart]

John Baldwin wrote:
> 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 

Ok, I'll start by having a go at rejigging the 7.x and 8.x kthread(9) 
man pages to be more informational and draw out the subtle differences 
we've been discussing in this thread. I'll post man page patches for 
review when they're ready.

> 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.
> 

What are the pros/cons of using mtx_sleep/wakeup vs cv_wait/cv_broadcast?


Cheers,
Lawrence