Hi Neil,
There are several problems with this patch. Ignoring the fact
that you use __module_get. Firstly, you bump the module count
permentantly while the thread is running: how does it ever get
unloaded? Secondly, modprobe becomes your parent.
There have been ambitious attempts to do a nice "thread
creation and stopping" interface before. Given the delicate logic
involved in shutting threads down, I think this makes sense. Maybe
something like:
/* Struct which identifies a kernel thread, handed to creator and
thread. */
struct kthread
{
int pid;
int should_die; /* Thread should exit when this is set. */
/* User supplied arg... */
void *arg;
};
struct kthread *create_thread(int (*fn)(struct kthread*), void *arg,
unsigned long flags,
const char *namefmt, ...);
void cleanup_thread(struct kthread *);
create_thread would use keventd to start the thread, and stop_thread
would tell keventd to set should_die, wmb(), wake it up, and
sys_wait() for it.
Thoughts?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
On Mon, 16 Jun 2003, Rusty Russell wrote:
> There have been ambitious attempts to do a nice "thread
> creation and stopping" interface before. Given the delicate logic
> involved in shutting threads down, I think this makes sense. Maybe
> something like:
>
> /* Struct which identifies a kernel thread, handed to creator and
> thread. */
> struct kthread
> {
> int pid;
> int should_die; /* Thread should exit when this is set. */
>
> /* User supplied arg... */
> void *arg;
> };
>
> struct kthread *create_thread(int (*fn)(struct kthread*), void *arg,
> unsigned long flags,
> const char *namefmt, ...);
> void cleanup_thread(struct kthread *);
>
> create_thread would use keventd to start the thread, and stop_thread
> would tell keventd to set should_die, wmb(), wake it up, and
> sys_wait() for it.
>
> Thoughts?
> Rusty.
Why using keventd? Personally I'd prefer a synchronous thread start/stop,
particularly with the thread living in a module.
Maybe some generalisation of:
static DECLARE_WAIT_QUEUE_HEAD(wq_kthread);
static struct completion kthread_died;
static int should_die;
static int my_kthread(void *started)
{
daemonize("my_kthread");
complete((struct completion *)started);
while (!should_die) {
/* sleep for wq_kthread and do requested stuff */
}
complete_and_exit(&kthread_died, 0);
/* never reached */
return 0;
}
int my_kthread_create(...)
{
DECLARE_COMPLETION(started);
int pid;
should_die = 0;
init_completion(kthread_died);
pid = kernel_thread(my_kthread, &startup, CLONE_FS|CLONE_FILES);
if (pid <= 0)
return -EAGAIN;
wait_for_completion(&started);
return pid;
}
void my_kthread_join(...)
{
should_die = 1;
wake_up(&wq_kthread);
wait_for_completion(&kthread_died);
}
Assuming the create/join things are called from module init/exit path this
eliminates the need to bump the module refcnt.
To make this more generic I think it would be sufficient to move the
start/exit completions into your struct kthread.
Martin
Martin Diehl <[email protected]> wrote:
>
> > create_thread would use keventd to start the thread, and stop_thread
> > would tell keventd to set should_die, wmb(), wake it up, and
> > sys_wait() for it.
> >
> > Thoughts?
> > Rusty.
>
> Why using keventd?
keventd knows how to clean up children, handle SIGCHLD, etc. That code was
hard-won.
And kernel threads which are parented by userspace processes tend to
accidentally inherit things we'd rather they didn't. daemonize() and
reparent_to_init() try to fix things up, but I'm still not sure we got it
all.
Using keventd will tend to prevent mistakes.
In message <[email protected]> you write:
> Why using keventd? Personally I'd prefer a synchronous thread start/stop,
> particularly with the thread living in a module.
> Maybe some generalisation of:
It would be syncronous: but doing kernel_thread yourself means trying
to clean up using daemonize et al, which is incomplete and always
makes me nervous.
An implementation detail to users, but IMHO an important one.
Also, this replaces complete_and_exit: the thread can just exit. This
simplifies things for the users, too...
Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
On Mon, 16 Jun 2003, Rusty Russell wrote:
> In message <[email protected]> you write:
> > Why using keventd? Personally I'd prefer a synchronous thread start/stop,
> > particularly with the thread living in a module.
> > Maybe some generalisation of:
>
> It would be syncronous:
You mean your cleanup_thread would block for completion of the keventd
stuff? Ok, this would work. But then, when calling cleanup_thread, f.e. we
must not hold any semaphore which might be acquired by _any_ other work
scheduled for keventd or we might end in deadlock (like the rtnl+hotplug
issue we had seen recently).
> but doing kernel_thread yourself means trying
> to clean up using daemonize et al, which is incomplete and always
> makes me nervous.
I thought this was fixed in 2.5 for some time now, but seems I shouldn't
rely on that ;-)
> An implementation detail to users, but IMHO an important one.
>
> Also, this replaces complete_and_exit: the thread can just exit. This
> simplifies things for the users, too...
Personally I do like the complete_and_exit thing as a simple and clear
finalisation point. And if I didn't miss something above wrt. your
cleanup_kthread being synchronous I'm not sure whether the locking
implication do really make things easier - YMMV of course.
Thanks.
Martin
In message <[email protected]> you write:
> On Mon, 16 Jun 2003, Rusty Russell wrote:
> > It would be syncronous:
>
> You mean your cleanup_thread would block for completion of the keventd
> stuff? Ok, this would work. But then, when calling cleanup_thread, f.e. we
> must not hold any semaphore which might be acquired by _any_ other work
> scheduled for keventd or we might end in deadlock (like the rtnl+hotplug
> issue we had seen recently).
I think we're talking across each other: take a look at the existing
kernel/kmod.c __call_usermodehelper to see how we wait at the moment.
> > Also, this replaces complete_and_exit: the thread can just exit. This
> > simplifies things for the users, too...
>
> Personally I do like the complete_and_exit thing as a simple and clear
> finalisation point.
Not as clean as "wait until the thread has exited", surely!
Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
On Mon, 16 Jun 2003, Rusty Russell wrote:
> > You mean your cleanup_thread would block for completion of the keventd
> > stuff? Ok, this would work. But then, when calling cleanup_thread, f.e. we
> > must not hold any semaphore which might be acquired by _any_ other work
> > scheduled for keventd or we might end in deadlock (like the rtnl+hotplug
> > issue we had seen recently).
>
> I think we're talking across each other: take a look at the existing
> kernel/kmod.c __call_usermodehelper to see how we wait at the moment.
Maybe talking across each other... what I meant is some deadlock like
this (IMHO possible both on UP and SMP):
rmmod (f.e.) keventd somewhere else
down(&some_sem)
cleanup_thread()
.
. schedule_work(w1)
.
. w1 (queued, or maybe running):
. down(&some_sem);
. ...
. up(&some_sem);
.
schedule_work(w2)
.
. w2 (queued behind w1)
. should_die = 1
. sys_wait4()
. complete(thread_exit)
.
/* some_sem still hold */
wait_for_completion(thread_exit)
Next time we schedule keventd w1 will be executed first which wants to
acquire some_sem which is still hold by the rmmod-thread - which in turn
blocks for completion of w2 which is queued behind w1 -> deadlock.
The point is the queueing in keventd combined with stuff waiting for
keventd-completion could create some possibilities for lock order
violation which are at least not very obvious.
IMHO cleanup_thread would be something one MUST NOT call with any lock
hold, not even a semaphore if it might get acquired anywhere else in
keventd-context.
Thanks.
Martin
In message <[email protected]> you write:
> IMHO cleanup_thread would be something one MUST NOT call with any lock
> hold, not even a semaphore if it might get acquired anywhere else in
> keventd-context.
Err, yes. I thought this was fairly clear. cf. del_timer_sync and a
timer function.
Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.