Date: Sun, 23 Nov 2008 22:25:12 -0800 From: Julian Elischer <julian@elischer.org> To: Lawrence Stewart <lstewart@freebsd.org> Cc: freebsd-arch@freebsd.org Subject: Re: kthread_exit(9) unexpectedness Message-ID: <492A48C8.9080302@elischer.org> In-Reply-To: <4929F90B.1040502@freebsd.org> References: <492412E8.3060700@freebsd.org> <200811201502.23943.jhb@freebsd.org> <4925E30B.8010709@freebsd.org> <200811211348.41536.jhb@freebsd.org> <4929F90B.1040502@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?492A48C8.9080302>