Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 24 Nov 2008 11:44:59 +1100
From:      Lawrence Stewart <lstewart@freebsd.org>
To:        John Baldwin <jhb@freebsd.org>
Cc:        freebsd-arch@freebsd.org
Subject:   Re: kthread_exit(9) unexpectedness
Message-ID:  <4929F90B.1040502@freebsd.org>
In-Reply-To: <200811211348.41536.jhb@freebsd.org>
References:  <492412E8.3060700@freebsd.org> <200811201502.23943.jhb@freebsd.org> <4925E30B.8010709@freebsd.org> <200811211348.41536.jhb@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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).

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?

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

Cheers,
Lawrence



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4929F90B.1040502>