[PATCH] Add module_kernel_thread for threads that live in modules.

[PATCH] Add module_kernel_thread for threads that live in modules.

[NeilBrown]

Nfsd (and lockd) run kernel threads that currently use
MOD_{INC,DEC}_USE_COUNT to manage references to the module on
behalf of those threads.  This is deprecated and unsafe.

This patch introduces module_kernel_thread which ensures that
references counts are taken and dropped as appropriate for kernel
threads, and uses it for nfsd and lockd.

NeilBrown

### Comments for ChangeSet

If a kernel thread runs code that is in a module, it can be started
with module_kernel_thread, and this will safely managed the reference
counts on the module implied by the existance of the thread.

This is then used for nfsd and lockd.

 ----------- Diffstat output ------------
 ./fs/lockd/svc.c             |    9 ++------
 ./fs/nfsd/nfssvc.c           |   11 +++-------
 ./include/linux/module.h     |   15 +++++++++++++
 ./include/linux/sunrpc/svc.h |    4 +--
 ./kernel/module.c            |   47 +++++++++++++++++++++++++++++++++++++++++++
 ./net/sunrpc/svc.c           |    7 ++++--
 6 files changed, 76 insertions(+), 17 deletions(-)

diff ./fs/lockd/svc.c~current~ ./fs/lockd/svc.c
--- ./fs/lockd/svc.c~current~	2003-06-05 12:04:57.000000000 +1000
+++ ./fs/lockd/svc.c	2003-06-05 12:04:51.000000000 +1000
@@ -80,7 +80,7 @@ static inline void clear_grace_period(vo
 /*
  * This is the lockd kernel thread
  */
-static void
+static int
 lockd(struct svc_rqst *rqstp)
 {
 	struct svc_serv	*serv = rqstp->rq_server;
@@ -88,7 +88,6 @@ lockd(struct svc_rqst *rqstp)
 	unsigned long grace_period_expire;
 
 	/* Lock module and set up kernel thread */
-	MOD_INC_USE_COUNT;
 	lock_kernel();
 
 	/*
@@ -181,9 +180,7 @@ lockd(struct svc_rqst *rqstp)
 	/* release rpciod */
 	rpciod_down();
 
-	/* Release module */
-	unlock_kernel();
-	MOD_DEC_USE_COUNT;
+	return 0;
 }
 
 /*
@@ -238,7 +235,7 @@ lockd_up(void)
 	/*
 	 * Create the kernel thread and wait for it to start.
 	 */
-	error = svc_create_thread(lockd, serv);
+	error = svc_create_thread(lockd, serv, THIS_MODULE);
 	if (error) {
 		printk(KERN_WARNING
 			"lockd_up: create thread failed, error=%d\n", error);

diff ./fs/nfsd/nfssvc.c~current~ ./fs/nfsd/nfssvc.c
--- ./fs/nfsd/nfssvc.c~current~	2003-06-05 12:04:57.000000000 +1000
+++ ./fs/nfsd/nfssvc.c	2003-06-05 12:04:51.000000000 +1000
@@ -48,7 +48,7 @@
 #define	SIG_NOCLEAN	SIGHUP
 
 extern struct svc_program	nfsd_program;
-static void			nfsd(struct svc_rqst *rqstp);
+static int			nfsd(struct svc_rqst *rqstp);
 struct timeval			nfssvc_boot;
 static struct svc_serv 		*nfsd_serv;
 static atomic_t			nfsd_busy;
@@ -114,7 +114,7 @@ nfsd_svc(unsigned short port, int nrserv
 	nrservs -= (nfsd_serv->sv_nrthreads-1);
 	while (nrservs > 0) {
 		nrservs--;
-		error = svc_create_thread(nfsd, nfsd_serv);
+		error = svc_create_thread(nfsd, nfsd_serv, THIS_MODULE);
 		if (error < 0)
 			break;
 	}
@@ -163,7 +163,7 @@ update_thread_usage(int busy_threads)
 /*
  * This is the NFS server kernel thread
  */
-static void
+static int
 nfsd(struct svc_rqst *rqstp)
 {
 	struct svc_serv	*serv = rqstp->rq_server;
@@ -172,7 +172,6 @@ nfsd(struct svc_rqst *rqstp)
 	sigset_t shutdown_mask, allowed_mask;
 
 	/* Lock module and set up kernel thread */
-	MOD_INC_USE_COUNT;
 	lock_kernel();
 	daemonize("nfsd");
 	current->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
@@ -255,9 +254,7 @@ nfsd(struct svc_rqst *rqstp)
 
 	/* Release the thread */
 	svc_exit_thread(rqstp);
-
-	/* Release module */
-	MOD_DEC_USE_COUNT;
+	return 0;
 }
 
 int

diff ./include/linux/module.h~current~ ./include/linux/module.h
--- ./include/linux/module.h~current~	2003-06-05 12:04:57.000000000 +1000
+++ ./include/linux/module.h	2003-06-05 11:34:24.000000000 +1000
@@ -314,6 +314,9 @@ static inline void module_put(struct mod
 	}
 }
 
+extern int module_kernel_thread(int (*fn)(void*), void *arg, 
+				unsigned long flags, struct module *owner);
+
 #else /*!CONFIG_MODULE_UNLOAD*/
 static inline int try_module_get(struct module *module)
 {
@@ -328,6 +331,12 @@ static inline void __module_get(struct m
 #define symbol_put(x) do { } while(0)
 #define symbol_put_addr(p) do { } while(0)
 
+static inline int module_kernel_thread(int (*fn)(void*), void *arg, 
+				       unsigned long flags, struct module *owner)
+{
+	return kernel_thread(fn, arg, flags);
+}
+	       
 #endif /* CONFIG_MODULE_UNLOAD */
 
 /* This is a #define so the string doesn't get put in every .o file */
@@ -419,6 +428,12 @@ static inline int unregister_module_noti
 	return 0;
 }
 
+static inline int module_kernel_thread(int (*fn)(void*), void *arg,
+				       unsigned long flags, struct module *owner)
+{
+	return kernel_thread(fn, arg, flags);
+}
+
 #endif /* CONFIG_MODULES */
 
 #ifdef MODULE

diff ./include/linux/sunrpc/svc.h~current~ ./include/linux/sunrpc/svc.h
--- ./include/linux/sunrpc/svc.h~current~	2003-06-05 12:04:57.000000000 +1000
+++ ./include/linux/sunrpc/svc.h	2003-06-05 12:04:51.000000000 +1000
@@ -276,13 +276,13 @@ struct svc_procedure {
 /*
  * This is the RPC server thread function prototype
  */
-typedef void		(*svc_thread_fn)(struct svc_rqst *);
+typedef int		(*svc_thread_fn)(struct svc_rqst *);
 
 /*
  * Function prototypes.
  */
 struct svc_serv *  svc_create(struct svc_program *, unsigned int);
-int		   svc_create_thread(svc_thread_fn, struct svc_serv *);
+int		   svc_create_thread(svc_thread_fn, struct svc_serv *, struct module *);
 void		   svc_exit_thread(struct svc_rqst *);
 void		   svc_destroy(struct svc_serv *);
 int		   svc_process(struct svc_serv *, struct svc_rqst *);

diff ./kernel/module.c~current~ ./kernel/module.c
--- ./kernel/module.c~current~	2003-06-05 12:04:57.000000000 +1000
+++ ./kernel/module.c	2003-06-05 11:34:24.000000000 +1000
@@ -616,6 +616,53 @@ void symbol_put_addr(void *addr)
 }
 EXPORT_SYMBOL_GPL(symbol_put_addr);
 
+
+/*
+ * If a kernel_thread runs in a module, we might want to
+ * module to be refcounted by the threads.  This code
+ * allows that to happen.
+ * When module_kernel_thread completes, the module will
+ * have been ref-counted iif the thread started.
+ */
+struct kern_thread_info {
+	int (*fn)(void*);
+	void *arg;
+	struct module *owner;
+};
+static int module_kernel_thread_helper(void *arg)
+{
+	struct kern_thread_info *kti = arg;
+	int rv;
+
+	rv = kti->fn(kti->arg);
+	module_put(kti->owner);
+	kfree(kti);
+	return rv;
+}
+int module_kernel_thread(int (*fn)(void*), void *arg, 
+			 unsigned long flags, struct module *owner)
+{
+	struct kern_thread_info *kti;
+	int err;
+
+	if (!owner)
+		return kernel_thread(fn, arg, flags);
+	kti = kmalloc(sizeof(*kti), GFP_KERNEL);
+	if (!kti)
+		return -ENOMEM;
+	kti->fn = fn;
+	kti->arg = arg;
+	kti->owner = owner;
+	__module_get(owner);
+	err = kernel_thread(module_kernel_thread_helper, kti, flags);
+	if (err < 0) {
+		module_put(owner);
+		kfree(kti);
+	}
+	return err;
+}
+EXPORT_SYMBOL(module_kernel_thread);
+
 #else /* !CONFIG_MODULE_UNLOAD */
 static void print_unload_info(struct seq_file *m, struct module *mod)
 {

diff ./net/sunrpc/svc.c~current~ ./net/sunrpc/svc.c
--- ./net/sunrpc/svc.c~current~	2003-06-05 12:04:57.000000000 +1000
+++ ./net/sunrpc/svc.c	2003-06-05 12:06:13.000000000 +1000
@@ -14,6 +14,7 @@
 #include <linux/in.h>
 #include <linux/unistd.h>
 #include <linux/mm.h>
+#include <linux/module.h>
 
 #include <linux/sunrpc/types.h>
 #include <linux/sunrpc/xdr.h>
@@ -150,7 +151,8 @@ svc_release_buffer(struct svc_rqst *rqst
  * Create a server thread
  */
 int
-svc_create_thread(svc_thread_fn func, struct svc_serv *serv)
+svc_create_thread(svc_thread_fn func, struct svc_serv *serv,
+		  struct module *owner)
 {
 	struct svc_rqst	*rqstp;
 	int		error = -ENOMEM;
@@ -169,7 +171,8 @@ svc_create_thread(svc_thread_fn func, st
 
 	serv->sv_nrthreads++;
 	rqstp->rq_server = serv;
-	error = kernel_thread((int (*)(void *)) func, rqstp, 0);
+	error = module_kernel_thread((int(*)(void*))func, rqstp,
+				     0, owner);
 	if (error < 0)
 		goto out_thread;
 	svc_sock_update_bufs(serv);

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Andrey Klochko]

Neil,

On Thu, Jun 05, 2003 at 12:30:32PM +1000, NeilBrown wrote:

>  	struct svc_serv	*serv = rqstp->rq_server;
> @@ -88,7 +88,6 @@ lockd(struct svc_rqst *rqstp)
>  	unsigned long grace_period_expire;
>  
>  	/* Lock module and set up kernel thread */
> -	MOD_INC_USE_COUNT;
>  	lock_kernel();
>  
>  	/*
> @@ -181,9 +180,7 @@ lockd(struct svc_rqst *rqstp)
>  	/* release rpciod */
>  	rpciod_down();
>  
> -	/* Release module */
> -	unlock_kernel();

You've locked the kernel and didn't unlock it.
 
> -	MOD_DEC_USE_COUNT;
> +	return 0;
> 
  
Andrey

-- 
-------------------------------------------------------------
Andrey Klochko
System Administrator
Sibley School of Mechanical and Aerospace Engineering
288 Grumman Hall
Cornell University
Ithaca, NY 14853

e-mail: andrey@mae.cornell.edu
phone: 607-255-0360

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Neil Brown]

On Thursday June 5, andrey@morgon.mae.cornell.edu wrote:
> >  
> > -	/* Release module */
> > -	unlock_kernel();
> 
> You've locked the kernel and didn't unlock it.
>  

This was just before the thread exited.  When a thread exits it
automatically drops the kernel lock anyway.  It seemed un-necessary to
explicitly unlock it aswell.

NeilBrown

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Andrey Klochko]

Thanks for expalining that.

Andrey

On Sat, Jun 07, 2003 at 03:33:26PM +1000, Neil Brown wrote:
> On Thursday June 5, andrey@morgon.mae.cornell.edu wrote:
> > >  
> > > -	/* Release module */
> > > -	unlock_kernel();
> > 
> > You've locked the kernel and didn't unlock it.
> >  
> 
> This was just before the thread exited.  When a thread exits it
> automatically drops the kernel lock anyway.  It seemed un-necessary to
> explicitly unlock it aswell.
> 
> NeilBrown

-- 
-------------------------------------------------------------
Andrey Klochko
System Administrator
Sibley School of Mechanical and Aerospace Engineering
288 Grumman Hall
Cornell University
Ithaca, NY 14853

e-mail: andrey@mae.cornell.edu
phone: 607-255-0360

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Rusty Russell]

In message <16110.20088.351260.156860@gargle.gargle.HOWL> you write:
> On Monday June 16, rusty@rustcorp.com.au wrote:
> > Hi Neil,
> 
> Hi Rusty.  Thanks for the comments... I probably should have Cc:ed you
> in the first place....

Yeah, that does tend to get faster response, but I know some hackers
consider it completely optional 8(

> > 	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.
> 
> We seem to have very different views of the problem, as you seem to be
> calling into question aspects that I thought were obviously correct.
> 
> __module_get:
>    In all the cases I am interested in (nfsd, lockd,
>    lockd-helper-thread), the thread is started by code running
>    inside the module and so there will be a reference held on the
>    module while the thread is being started, thus __module_get is the
>    correct thing to do as "we know we already have a refcount"...

But do you wait for it?  Theoretically the init code could have
finished, and someone done rmmod, before this thread gets as far as
__module_get, no?

> module count bumped permananelt while thread is running:
>    well ofcourse, the thread runs code in the module which can only
>    be done safely while we have a ref-count.

For future reference: this isn't quite true.  If a function/thread is
synchronously stopped by the exit code/failed init code then they
don't need to hold a reference count: it still *can* hold a reference
count, which really depends on whether the module should be considered
"in use" by the thread... cf. timers.

>    The threads I am thinking of aren't running "whenever the module is
>    loaded".  They are running "whenever their service is needed".

My bad: I wasn't sure given my (admittedly brief) glance at the code.
Thanks for clarifing!

> modprobe becomes your parent:
>    No, modprobe has nothing to do with it in my case. rpc.nfsd, or 
>    mount_nfs or lockd might be the parent.  I thought reparent_to_init
>    handled all that.  Apparently there are question marks over that
>    which I wasn't aware of.

Andrew has been trying to kill it, and I think he's right.  In
practical terms, it's much easier to start from a clean environment
than to clean up an unknown one, and keep that cleanup code uptodate.

> I don't want to have to call "cleanup_thread" or de-allocate the
> "struct kthread".  I want to be able to SIGKILL a process and have it
> go away and release everything, including possibly the last refernce
> to the module.
> 
> In short, it really feels like we are trying to solve different
> problems :-)

Agreed: threads under their own control are much simpler than ones
under external control.

> I will have a look at keventd and see if it's services can be of
> assistance to solve my problem.

I will think, which usually seems to help me when presented with new
information 8)

Thanks!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Rusty Russell]

In message <Pine.LNX.4.44.0306161145570.2079-100000@notebook.home.mdiehl.de> 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.

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Neil Brown]

On Monday June 16, rusty@rustcorp.com.au wrote:
> Hi Neil,

Hi Rusty.  Thanks for the comments... I probably should have Cc:ed you
in the first place....

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

We seem to have very different views of the problem, as you seem to be
calling into question aspects that I thought were obviously correct.

__module_get:
   In all the cases I am interested in (nfsd, lockd,
   lockd-helper-thread), the thread is started by code running
   inside the module and so there will be a reference held on the
   module while the thread is being started, thus __module_get is the
   correct thing to do as "we know we already have a refcount"...

module count bumped permananelt while thread is running:
   well ofcourse, the thread runs code in the module which can only
   be done safely while we have a ref-count.
how does it ever get unloaded:
   the thread exits, the refcount drops, and the module can be
   unloaded.
   In the case of nfsd, the threads get signalled and die gracefully.
   For lockd, the number of users (nfs mounts or nfsd threads) drops
   to zero, the process is signalled, and exits gracefully.
   For the lockd helper threads, the complete there assigned task and
   exit. 
   The threads I am thinking of aren't running "whenever the module is
   loaded".  They are running "whenever their service is needed".
modprobe becomes your parent:
   No, modprobe has nothing to do with it in my case. rpc.nfsd, or 
   mount_nfs or lockd might be the parent.  I thought reparent_to_init
   handled all that.  Apparently there are question marks over that
   which I wasn't aware of.

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

I don't want to have to call "cleanup_thread" or de-allocate the
"struct kthread".  I want to be able to SIGKILL a process and have it
go away and release everything, including possibly the last refernce
to the module.

In short, it really feels like we are trying to solve different
problems :-)

I will have a look at keventd and see if it's services can be of
assistance to solve my problem.

Thanks,
NeilBrown

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Martin Diehl]

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

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Rusty Russell]

In message <Pine.LNX.4.44.0306161043020.2079-100000@notebook.home.mdiehl.de> 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.

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Martin Diehl]

On Mon, 16 Jun 2003, Rusty Russell wrote:

> In message <Pine.LNX.4.44.0306160907470.2079-100000@notebook.home.mdiehl.de> 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

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Andrew Morton]

Martin Diehl <lists@mdiehl.de> 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.

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Rusty Russell]

In message <Pine.LNX.4.44.0306160907470.2079-100000@notebook.home.mdiehl.de> 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.

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Martin Diehl]

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

Re: [PATCH] Add module_kernel_thread for threads that live in modules.

[Rusty Russell]

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.