kthread_exit(9) unexpectedness

[Julian Elischer]

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.
> 
> It seems to me that the kthread(9) man page is somewhat unclear with 
> respect to what the KPI actually achieves, making references to thread 
> related activities all over the place when in reality it's manipulating 
> single threaded processes (which for all intensive purposes can be used 
> like threads but are structurally different). I guess this is the cause 
> for the underlying confusion.
> 
> While we're on the topic, I'm also having trouble understanding the 
> reasoning for renaming kthread_create to kthread_add, when in reality 
> 8.x kthread_add is doing a *real* kthread creation and the originally 
> named kthread_create seems to have been a bit of a misnomer (corrected 
> in 8.x by moving the functionality into the kproc_* KPI).

kthread_add was named tha tway to indicate that it is adding a
thread to an existing process in addition to the thread already 
running the code..  I wanted anyone  linking in a binary module
to get a link failure, even if they manageed to sneak past the
other safeguards.

> 
> The changing of the **proc to a **thread argument is enough to ensure 
> code from 7.x won't compile without a tweak on 8.x, so why the rename to 
> add further confusion? With the same line of reasoning, 
> kproc_kthread_add should probably be kproc_kthread_create?

kproc_kthread_add will add a thread to the process if it already
exists, but if it doesn't it will make a new process.

Hey, it's just how I think I guess.

> 
>>>> kthread_add() and kproc_kthread_add() both return thread pointers.  
>>>> Hence 
>> in
>>> Yup.
>>>
>>>> 8.x kthread_exit() is used for exiting kernel threads and wakes up the 
>> thread
>>>> pointer, but in 7.x kthread_exit() is used for exiting kernel processes 
>> and
>>>> wakes up the proc pointer.  I think what is probably needed is to 
>>>> simply 
>>> In the code, yes. Our documented behaviour as far as I can tell is 
>>> different though, unless we equate a "thread handle" to "proc handle" 
>>> prior to 8.x, which I don't think is the case -  they are still 
>>> different.
>>
>> It has always been the case in < 8 that you sleep on the proc handle 
>> (what kthread_create() actually returns in < 8).  And in fact, you 
>> even have to dig around in the proc you get from kthread_create() to 
>> even find the thread pointer as opposed to having the API hand it to you.
>>
>>>> document that arrangement as such.  Note that the sleeping on proc 
>>>> pointer 
>>> I agree that the arrangement needs to be better documented. The 
>>> change in 8.x is subtle enough that reading the kthread man page in 
>>> 7.x and 8.x doesn't immediately make it obvious what's going on.
>>>
>>>> has been the documented way to synchronize with kthread_exit() since 
>>>> 5.0.
>>>>
>>> Could you please point me at this documentation? I've missed it in my 
>>> poking around thus far.
>>
>> It is probably only documented in numerous threads in the mail 
>> archives sadly, but there have been several of them and there have 
>> been several fixes to get this right (the randomdev thread and fdc(4) 
>> thread come to mind).
> 
> If we had no man page, mail archives would be the next best thing. In 
> this instance, we have a misleading man page and I think it would be 
> more beneficial to align the documentation/implementation rather than 
> leaving people confused.
> 
> 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 really haven't got this stuff in my head at the moment so I can't 
comment.

> 
> Cheers,
> Lawrence
> _______________________________________________
> freebsd-arch at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe at freebsd.org"