Messages in this thread | | | Date | Wed, 15 Apr 2009 16:27:12 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] slow_work_thread() should do the exclusive wait |
| |
On Tue, 14 Apr 2009 00:24:51 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/13, Trond Myklebust wrote: > > > > On Mon, 2009-04-13 at 23:48 +0200, Oleg Nesterov wrote: > > > On 04/13, David Howells wrote: > > > > > > > > Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > > > > > > > > > Should that really be TASK_INTERRUPTIBLE? I don't see anything obvious > > > > > in the enclosing for(;;) loop that checks for or handles signals... > > > > > > > > If it were TASK_UNINTERRUPTIBLE, it would sit there in the D-state when not > > > > doing anything. I must admit, I thought I was calling daemonize(), but that > > > > seems to have got lost somewhere. > > > > > > daemonize() is not needed, kthread_create() creates the kernel thread which > > > ignores all signals. So it doesn't matter which state we use to sleep, > > > TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE. > > > > Yes, but that is precisely why it is cleaner to use > > TASK_UNINTERRUPTIBLE. It documents the fact that signal handling isn't > > needed (whether or not the thread is blocking them). > > Agreed. But TASK_UNINTERRUPTIBLE can confuse a user which does > "cat /proc/loadavg" on the idle machine... > > Note that, for example, worker_thread() uses TASK_INTERRUPTIBLE too, and I > think for the same reason. >
Yup. It's a very common pattern for kernel threads to sleep in state TASK_INTERRUPTIBLE. It is "well known" (lol) that kernel threads don't accept signals, and having a kernel thread sleep in state TASK_UNINTERRUPTIBLE will indeed contribute to load average and we get distressed emails quite promptly when we do that.
The patch itself is a little worrisome. The wake-all semantics are very good at covering up little race bugs. And switching to wake-once is a great way of exposing hitherto-unsuspected races.
<looks for races>
Nothing immediately leaps out, but you know how these things are.
I wonder if slow_work_cull_timeout() should have some sort of barrier, so the write is suitably visible to the woken thread. Bearing in mind that the thread might _already_ have been woken by someone else?
off-topic: afacit the code will cull a maximum of one thread per five seconds. But the rate of thread _creation_ is, afacit, unbound. Are there scenarios in which we can get a runaway thread count?
| |