kthread_create

Hi all,

Ingo read through this before and liked it: this is the basis
of the Hotplug CPU patch, and as such has been stressed fairly well. Tested stand-alone, and included here for wider review.

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

Name: Kernel Thread Control Primitives
Author: Rusty Russell
Status: Tested on 2.6.0-bk3

D: The hotplug CPU code introduces two major problems:
D:

D: 1) Threads which previously never stopped (migration thread,
D:    ksoftirqd, keventd) have to be stopped cleanly as CPUs go offline.
D: 2) Threads which previously never had to be created now have
D:    to be created when a CPU goes online.

D:
D: Unfortunately, stopping a thread is fairly baroque, involving memory
D: barriers, a completion and spinning until the task is actually dead. D:

D: There are also three problems in starting a thread:
D: 1) Doing it from a random process context risks environment contamination:
D:    better to do it from keventd to guarantee a clean environment, a-la
D:    call_usermodehelper.
D: 2) Getting the task struct without races is a hard: see kernel/sched.c
D:    migration_call(), kernel/workqueue.c create_workqueue_thread().
D: 3) There are races in starting a thread for a CPU which is not yet
D:    online: migration thread does a complex dance at the moment for
D:    a similar reason (there may be no migration thread to migrate us).

D:
D: Place all this logic in some primitives to make life easier:
D: kthread_create(), kthread_start() and kthread_destroy(). These
D: primitives require no extra data-structures in the caller: they operate
D: on normal "struct task_struct"s.
D:

D: Other changes:
D:   - Expose keventd_up(), as keventd and migration threads will use
D:     kthread to launch, and kthread normally uses workqueues and must
D:     recognize this case.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .32392-linux-2.6.0-test11-bk6/include/linux/kthread.h .32392-linux-2.6.0-test11-bk6.updated/include/linux/kthread.h
@@ -0,0 +1,38 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple abstraction for kernel thread usage: the initfn is called at
+ * the start, if that's successful (ie. returns 0), then the thread
+ * sleeps.
+ *
+ * Every time the thread is woken, it will run corefn, until that
+ * returns an error.  The thread must be ended by calling
+ * kthread_destroy().
+ */
+#include <linux/err.h>
+struct task_struct;
+
+/* Part I: create a kthread: if fork fails return ERR_PTR(-errno). */
+struct task_struct *kthread_create(int (*initfn)(void *data),
+				   int (*corefn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/* Part II: have thread call initfn(); return thread if successful,
+   otherwise ERR_PTR(-errno). */
+struct task_struct *kthread_start(struct task_struct *k);
+
+/* Convenient wrapper for both of the above. */
+#define kthread_run(initfn, corefn, data, namefmt, ...)			      \
+({									      \
+	struct task_struct *__k						      \
+		= kthread_create(initfn,corefn,data,namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k))						      \
+		__k = kthread_start(__k);				      \
+	__k;								      \
+})
+
+/* Stop the thread.  Return value is last return of corefn() (ie. zero
+ * if exited as normal).  Can be called before kthread_start(). */
+int kthread_destroy(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .32392-linux-2.6.0-test11-bk6/include/linux/workqueue.h .32392-linux-2.6.0-test11-bk6.updated/include/linux/workqueue.h
@@ -60,6 +60,7 @@ extern int FASTCALL(schedule_work(struct
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .32392-linux-2.6.0-test11-bk6/kernel/Makefile .32392-linux-2.6.0-test11-bk6.updated/kernel/Makefile
@@ -6,7 +6,8 @@ obj-y     = sched.o fork.o exec_domain.o
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .32392-linux-2.6.0-test11-bk6/kernel/workqueue.c .32392-linux-2.6.0-test11-bk6.updated/kernel/workqueue.c
@@ -359,6 +359,11 @@ void flush_scheduled_work(void)
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

Hey, this is pretty cool.

Recalling threads from LKML past, there are two mechanisms I (and some others) felt were missing from the equally nifty workqueue stuff:
1) one-shot threads
2) keventd overflow

For #1, your patch seems to cover that nicely.

For #2, that's to be used for situations where (a) you need a thread context _and_ (b) you simply cannot wait for keventd to become available (since there are no time guarantees).

Anyway, thanks for doing this, it fills a need, I believe.

Jeff

In message <[email protected]> you write:

It's really for persistent threads, but you can use it as one-shot by either (1) calling exit() in the core function (and noone calls kthread_destroy), or (2) not having a core function and just having an init function. I used this in a test patch.

For #2, if you really can't wait for keventd, perhaps your own workqueue is in order?

Yes: stopping threads manually is a real PITA, and things like complete_and_exit() always make me wince.

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

Way too wasteful, and doing so is working around a fundamental failing of workqueues: keventd gives no guarantee that your scheduled work will be executed this week, this month, or this year :)

keventd is used by two competing classes of users: those who want low-latency, quick execution in task context (Tux-ish), and those who just want to run something in task context, where they might sleep (perhaps for a long time).

So it would be nice to have thread pool semantics occasionally found in userspace: if thread pool is full when new work is queued, _temporarily_ increase the pool size (or create a one-shot kthread). Sure you have kthread creation overhead, but at least you have a reasonable guarantee that your work won't wait 5-30 seconds or more before being performed.

Jeff

That why those ones could be replaced by short-lived threads... I use a short lived thread for ADB probing and I'm considering doing so for things like ethernet NIC reset etc... which is definitely scheduling way too long in keventd

The overhead isn't big (and even lower with kthread I suppose due to not doing daemonize() & reparent_to_init()) so for occasional long-sleeping crap, short lived kthreads seem to be the solution. If those end up not using keventd any more (except for actual creation of the short lived thread), there is probably no need for a thread pool thing, only low latency stuffs will still use work_queue.

Ben.

In message <[email protected]> you write:

I feel the same, which is one reason I didn't use them here...

Hmm, I think that if you had an all-dancing, self-balancing workqueue mechanism, you wouldn't need to create your own workqueues anyway.

If we're wishlisting, I'd prefer something which also scaled down below one thread per cpu until required. Mainly because it just feels wrong to have those threads sitting around doing mainly nothing.

Simple matter of coding, right?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

It would be nice to be able to see all the hotplug CPU patches in one place, to get a feel for their shape and size. That way, we can decide whether we need to look at this patch ;)

A few things:

I was right! ;)

It would be nice to kerneldocify kthread_create(), kthread_start() and kthread_destroy() sometime.

In message <[email protected]> you write:

Um, I've had this on kernel.org for a few years now. It's even at the top of the page:

http:://www.kernel.org/pub/linux/kernel/people/rusty

Possibly, but there's not much we can do. We never wait long, and we're keventd or a child here, so we're only talking about SIGCHLD.

No. (1) By design, we're always a purebred kernel thread descended directly from the init thread and have never had an mm anywhere. (2) daemonize is an abhorrent abortion: it's dangerous and presumptive to try to "clean up" a random thread into a kernel thread.

Yep, that's by design. (1) signal_pending() is not possible, we've blocked all signals above. (2) the corefn() MUST set the task state, as per normal semantics:

1) set current->state to TASK_INTERRUPTIBLE
2) check condition
3) return (so core can schedule)

OK, I've changed it to kthread_creation. It's private to kthread.c anyway.

Sure, if you want. Does anyone actually read that? I prefer the comments on how to use functions belong in the headers, not above the definitions as seems to be the kerneldoc way.

Hmm, if it's an RT task, it'll screw up, yes, because yield() won't yield().

Fixing this well would require a way of notifying someone who is not the parent when a task dies, OR taking over the parenthood of the task. Both of these required non-trivial changes to exit.c and I shied away.

All things are possible, however...

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

In message <[email protected]> you write:

Expand task_struct for this one usage? I don't think that's worthwhile.

The whole code is written so there is no datastructure associated with the kthread. When something like kt_message is needed (to kill a thread, for example), they grab the lock and use the static one.

This means that threads can exit without having to do cleanup.

Hope that clarifies,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

I agree on one side, there's the drawback on a size increase (3 pointers, plus eventually a spinlock) of the task struct. But IMO the code would be cleaner, since you know who is the target of the message. Also it would not require any cleanup since there would be nothing allocated, just a struct member inside task_struct.
Also, what happens in the task woke up by a send does not reschedule before another CPU does another send? Wouldn't a message be lost?

- Davide

The messages should not be lost because we take the cpucontrol semaphore in kthread_start or kthread_destroy before sending a (start or destroy) message.

Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560033

I see, ok. At that point though, having the message struct inside the task struct could save the *to pointer and (because of the big lock above), using barrier and proper order in setting *from and *info, the spin lock. OTOH the big lock above really make the global message structure private, so it does not make much difference.

- Davide

In message <[email protected]> you write:

My original version used barriers. But IMHO if you're using barriers and your code isn't speed-critical, you don't have enough locks.

So I just threw a spinlock around the struct, and no more barrier issues.

Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

In message <[email protected]> you write:

<shrug> The code's really not that complicated.

There's a lock, so only one communication happens at a time, and all communication is request-response, so it's pretty straightforward.

But an alternate implementation would be to have a "kthread" kernel thread, which would actually be parent to the kthread threads. This means it can allocate and clean up, since it catches *all* thread deaths, including "exit()".

What do you think?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Did you take a look at the stuff I sent you? I'll append here with a simple comment (this goes over you bits).

1)
Define a structure:

struct kt_message {
       spinlock_t lock;
       struct task_struct *from;
       void *info;
};

and add it inside the task struct. There's no need to cleanup this structure on exit.

2)
The global mutex, the global spinlock and the global "struct kt_message" go away.

3) The "creator" of the kthread is not picked up with a message but is passed with the kernel data.

The reason for those bits would be that basically the communication is really 1<->1, so IMHO there's no need to converge into static global data for this. OTOH it does not really have scalability problems, so it is a matter of personal taste. Burn those bits if you like :-)

- Davide

diff -Nru linux-2.6-mm1/include/linux/init_task.h linux-2.6-mm1.mod/include/linux/init_task.h
@@ -109,6 +109,9 @@
 	.switch_lock	= SPIN_LOCK_UNLOCKED,				\
 	.journal_info	= NULL,						\
 	.io_wait	= NULL,						\
+	.ktm		= {						\
+		.lock		= SPIN_LOCK_UNLOCKED			\
+	},								\
 }
 
 

diff -Nru linux-2.6-mm1/include/linux/sched.h linux-2.6-mm1.mod/include/linux/sched.h
@@ -326,6 +326,11 @@
 	struct sigqueue *sigq;		/* signal queue entry. */
 };
 
+struct kt_message {
+	spinlock_t lock;
+	struct task_struct *from;
+	void *info;
+};
 
 struct io_context;			/* See blkdev.h */
 void exit_io_context(void);

@@ -471,6 +476,9 @@
  * to a stack based synchronous wait) if its doing sync IO.
  */
 	wait_queue_t *io_wait;
+
+/* Used to exchange messages by kthread. */
+	struct kt_message ktm;
 };
 
 static inline pid_t process_group(struct task_struct *tsk)

In message <[email protected]> yo u write:

Yes, but I think it's a really bad idea, as I said before.

Anyway, Here's a version which fixes the issues raised by Andrew by doing *everything* in keventd, uses waitpid(), and uses signals for communication (except for the case of init failing).

Passes initial testing here.

TODO: better documentation.

Comments, welcome.
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Use Signals For Kthread
Author: Rusty Russell
Status: Tested on 2.6.0-bk3

D: The hotplug CPU code introduces two major problems:
D:

D: 1) Threads which previously never stopped (migration thread,
D:    ksoftirqd, keventd) have to be stopped cleanly as CPUs go offline.
D: 2) Threads which previously never had to be created now have
D:    to be created when a CPU goes online.

D:
D: Unfortunately, stopping a thread is fairly baroque, involving memory
D: barriers, a completion and spinning until the task is actually dead. D:

D: There are also three problems in starting a thread:
D: 1) Doing it from a random process context risks environment contamination:
D:    better to do it from keventd to guarantee a clean environment, a-la
D:    call_usermodehelper.
D: 2) Getting the task struct without races is a hard: see kernel/sched.c
D:    migration_call(), kernel/workqueue.c create_workqueue_thread().
D: 3) There are races in starting a thread for a CPU which is not yet
D:    online: migration thread does a complex dance at the moment for
D:    a similar reason (there may be no migration thread to migrate us).

D:
D: Place all this logic in some primitives to make life easier:
D: kthread_create(), kthread_start() and kthread_destroy(). These
D: primitives require no extra data-structures in the caller: they operate
D: on normal "struct task_struct"s.
D:

D: Other changes:
D:   - Expose keventd_up(), as keventd and migration threads will use
D:     kthread to launch, and kthread normally uses workqueues and must
D:     recognize this case.

D:
D: Changes since first version:
D: 1) Sleep UNINTERRUPTIBLE when waiting for reply (thanks Andrew Morton).
D: 2) Reparent threads and waitpid for them.
D: 3) Do ALL ops via keventd, so we don't disturb current process. D:
D: This version uses signals rather than its own communication
D: mechanism makes kthread code a little simpler, and means users can
D: loop inside corefn if they want, and return when signal_pending().

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .29518-linux-2.6.0-bk3/include/linux/kthread.h .29518-linux-2.6.0-bk3.updated/include/linux/kthread.h
@@ -0,0 +1,38 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple abstraction for kernel thread usage: the initfn is called at
+ * the start, if that's successful (ie. returns 0), then the thread
+ * sleeps.
+ *
+ * Every time the thread is woken, it will run corefn, until that
+ * returns an error.  The thread must be ended by calling
+ * kthread_destroy().
+ */
+#include <linux/err.h>
+struct task_struct;
+
+/* Part I: create a kthread: if fork fails return ERR_PTR(-errno). */
+struct task_struct *kthread_create(int (*initfn)(void *data),
+				   int (*corefn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/* Part II: have thread call initfn(); return thread if successful,
+   otherwise ERR_PTR(-errno). */
+struct task_struct *kthread_start(struct task_struct *k);
+
+/* Convenient wrapper for both of the above. */
+#define kthread_run(initfn, corefn, data, namefmt, ...)			      \
+({									      \
+	struct task_struct *__k						      \
+		= kthread_create(initfn,corefn,data,namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k))						      \
+		__k = kthread_start(__k);				      \
+	__k;								      \
+})
+
+/* Stop the thread.  Return value is last return of corefn() (ie. zero
+ * if exited as normal).  Can be called before kthread_start(). */
+int kthread_destroy(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .29518-linux-2.6.0-bk3/include/linux/workqueue.h .29518-linux-2.6.0-bk3.updated/include/linux/workqueue.h
@@ -60,6 +60,7 @@ extern int FASTCALL(schedule_work(struct
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .29518-linux-2.6.0-bk3/kernel/Makefile .29518-linux-2.6.0-bk3.updated/kernel/Makefile
@@ -6,7 +6,8 @@ obj-y     = sched.o fork.o exec_domain.o
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .29518-linux-2.6.0-bk3/kernel/workqueue.c .29518-linux-2.6.0-bk3.updated/kernel/workqueue.c
@@ -359,6 +359,11 @@ void flush_scheduled_work(void)
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

Rusty, you still have to use global static data when there is no need. I like this version better though ;)

- Davide

In message <[email protected]> you write:

And you're still putting obscure crap in the task struct when there's no need. Honestly, I'd be ashamed to post such a patch.

I think I should seek a second opinion though.
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Ashamed !? Take a look at your original patch and then define shame. You had a communication mechanism that whilst being a private 1<->1 communication among two tasks, relied on a single global message strucure, lock and mutex. Honestly I do not like myself to add stuff inside a strcture for one-time use. Not because of adding 12 bytes to the struct, that are laughable. But because it is used by a small piece of code w/out a re-use ability for other things.

But of course, even a third one ;)

- Davide

Rusty, I took a better look at the patch and I think we can have per-kthread stuff w/out littering the task_struct and by making the thing more robust. We keep a global list_head protected by a global spinlock. We define a structure that contain all the per-kthread stuff we need (including a task_struct* to the kthread itself). When a kthread starts it will add itself to the list, and when it will die it will remove itself from the list. The start/stop functions will lookup the list (or hash, depending on how much stuff you want to drop in) with the target task_struct*, and if the lookup fails, it means the task already quit (another task already did kthread_stop() ??, natural death ????). This is too bad, but at least there won't be deadlock (or crash) beacause of this. This because currently we keep the kthread task_struct* lingering around w/out a method that willl inform us if the task goes away for some reason (so that we can avoid signaling it and waiting for some interaction). The list/hash will be able to tell us this. What do you think?

- Davide

Rusty, I gave the patch a quick shot. There's no more single and global data structures but the list that stores all kthreads. Like usual, if you don't like it you can trash it. But pls leave shame apart. It did nothing to you :-)

- Davide

@@ -0,0 +1,81 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple interface for creating and stopping kernel threads without mess. */
+#include <linux/err.h>
+struct task_struct;
+
+/**
+ * kthread_create: create a kthread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after @initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread, and controls it for you.  See kthread_start() and
+ * kthread_run().
+ * 
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+struct task_struct *kthread_create(int (*initfn)(void *data),
+				   int (*corefn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/**
+ * kthread_start: start a thread created by kthread_create().
+ * @k: the task returned by kthread_create().
+ *
+ * Description: This makes the thread @k run the initfn() specified in
+ * kthread_create(), if non-NULL, and return the result.  If that
+ * result is less than zero, then the thread @k is terminated.
+ * Otherwise, the corefn() is run if it is not NULL.
+ *
+ * There are two ways to write corefn(): simple or complex.  For
+ * simple, you set current->state to TASK_INTERRUPTIBLE, check for
+ * work, then return 0 when it is done.  The kthread code will call
+ * schedule(), and call corefn() again when woken.  More complex
+ * corefn()s can loop themselves, but must return 0 when
+ * signal_pending(current) is true.
+ *
+ * corefn() can terminate itself in two ways: if someone will later
+ * call kthread_stop(), simply return a negative error number.
+ * Otherwise, if you call do_exit() directly, the thread will
+ * terminate immediately. */
+int kthread_start(struct task_struct *k);
+
+/**
+ * kthread_run: create and start a thread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * kthread_start().  Returns the kthread, or ERR_PTR(). */
+#define kthread_run(initfn, corefn, data, namefmt, ...)			      \
+({									      \
+	struct task_struct *__k						      \
+		= kthread_create(initfn,corefn,data,namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						      \
+		int __err = kthread_start(__k);				      \
+		if (__err) __k = ERR_PTR(__err);			      \
+	}								      \
+	__k;								      \
+})
+
+/**
+ * kthread_stop: stop a thread created by kthread_create().
+ * @k: thread created by kthread_start.
+ *
+ * Sends a signal to @k, and waits for it to exit.  Your corefn() must
+ * not call do_exit() itself if you use this function!  This can also
+ * be called after kthread_create() instead of calling
+ * kthread_start(): the thread will exit without calling initfn() or
+ * corefn().
+ *
+ * Returns the last result of corefn(), or 0 if kthread_start() was
+ * never called. */
+int kthread_stop(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

@@ -60,6 +60,7 @@
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

@@ -6,7 +6,8 @@
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

@@ -359,6 +359,11 @@
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

In message <[email protected]> you write:

Except sharing data with a lock is perfectly robust.

Yeah, I deliberately didn't implement this, because (1) it seems like a lot of complexity when using a lock and letting them share a single structure works fine and is even simpler, and (2) the thread can't just "do_exit()".

You can get around (2) by having a permenant parent "kthread" thread which is a parent to all the kthreads (it'll get a SIGCHLD when someone does "do_exit()"). But the implementation was pretty ugly, since it involved having a communications mechanism with the kthread parent, which means you have the global ktm_message-like-thing for this...

I put the "corefn fails" option in there in case someone wants it, but I'm not sure how it would be used in practice. If it becomes commonplace to have them handing around for a long time, then storing the result somewhere rather than keeping the whole thread around makes sense, but at the moment it seems like premature optimization.

Unfortunately, my fd idea is broken, because the process which does the start may not be the same as the one which does the create. A shared queue really is the simplest solution.

Latest version below,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Use Signals For Kthread
Author: Rusty Russell
Status: Tested on 2.6.0-bk3

D: The hotplug CPU code introduces two major problems:
D:

D: 1) Threads which previously never stopped (migration thread,
D:    ksoftirqd, keventd) have to be stopped cleanly as CPUs go offline.
D: 2) Threads which previously never had to be created now have
D:    to be created when a CPU goes online.

D:
D: Unfortunately, stopping a thread is fairly baroque, involving memory
D: barriers, a completion and spinning until the task is actually dead. D:

D: There are also three problems in starting a thread:
D: 1) Doing it from a random process context risks environment contamination:
D:    better to do it from keventd to guarantee a clean environment, a-la
D:    call_usermodehelper.
D: 2) Getting the task struct without races is a hard: see kernel/sched.c
D:    migration_call(), kernel/workqueue.c create_workqueue_thread().
D: 3) There are races in starting a thread for a CPU which is not yet
D:    online: migration thread does a complex dance at the moment for
D:    a similar reason (there may be no migration thread to migrate us).

D:
D: Place all this logic in some primitives to make life easier:
D: kthread_create(), kthread_start() and kthread_destroy(). These
D: primitives require no extra data-structures in the caller: they operate
D: on normal "struct task_struct"s.
D:

D: Other changes:
D:   - Expose keventd_up(), as keventd and migration threads will use
D:     kthread to launch, and kthread normally uses workqueues and must
D:     recognize this case.
D: 
D: Changes since first version:
D: 1) Sleep UNINTERRUPTIBLE when waiting for reply (thanks Andrew Morton).
D: 2) Reparent threads and waitpid for them.
D: 3) Do ALL ops via keventd, so we don't disturb current process.
D: 4) This version uses signals rather than its own communication
D:     mechanism makes kthread code a little simpler, and means users can
D:     loop inside corefn if they want, and return when signal_pending().
D: 5) kthread_destroy() renamed to kthread_stop().
D: 6) kthread_start() returns an int.
D: 7) Better documentation, in nanodoc form.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/include/linux/kthread.h .8740-linux-2.6.1-rc1.updated/include/linux/kthread.h
@@ -0,0 +1,81 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple interface for creating and stopping kernel threads without mess. */
+#include <linux/err.h>
+struct task_struct;
+
+/**
+ * kthread_create: create a kthread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after @initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread, and controls it for you.  See kthread_start() and
+ * kthread_run().
+ * 
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+struct task_struct *kthread_create(int (*initfn)(void *data),
+				   int (*corefn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/**
+ * kthread_start: start a thread created by kthread_create().
+ * @k: the task returned by kthread_create().
+ *
+ * Description: This makes the thread @k run the initfn() specified in
+ * kthread_create(), if non-NULL, and return the result.  If that
+ * result is less than zero, then the thread @k is terminated.
+ * Otherwise, the corefn() is run if it is not NULL.
+ *
+ * There are two ways to write corefn(): simple or complex.  For
+ * simple, you set current->state to TASK_INTERRUPTIBLE, check for
+ * work, then return 0 when it is done.  The kthread code will call
+ * schedule(), and call corefn() again when woken.  More complex
+ * corefn()s can loop themselves, but must return 0 when
+ * signal_pending(current) is true.
+ *
+ * corefn() can terminate itself in two ways: if someone will later
+ * call kthread_stop(), simply return a negative error number.
+ * Otherwise, if you call do_exit() directly, the thread will
+ * terminate immediately. */
+int kthread_start(struct task_struct *k);
+
+/**
+ * kthread_run: create and start a thread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * kthread_start().  Returns the kthread, or ERR_PTR(). */
+#define kthread_run(initfn, corefn, data, namefmt, ...)			      \
+({									      \
+	struct task_struct *__k						      \
+		= kthread_create(initfn,corefn,data,namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						      \
+		int __err = kthread_start(__k);				      \
+		if (__err) __k = ERR_PTR(__err);			      \
+	}								      \
+	__k;								      \
+})
+
+/**
+ * kthread_stop: stop a thread created by kthread_create().
+ * @k: thread created by kthread_start.
+ *
+ * Sends a signal to @k, and waits for it to exit.  Your corefn() must
+ * not call do_exit() itself if you use this function!  This can also
+ * be called after kthread_create() instead of calling
+ * kthread_start(): the thread will exit without calling initfn() or
+ * corefn().
+ *
+ * Returns the last result of corefn(), or 0 if kthread_start() was
+ * never called. */
+int kthread_stop(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/include/linux/workqueue.h .8740-linux-2.6.1-rc1.updated/include/linux/workqueue.h
@@ -60,6 +60,7 @@ extern int FASTCALL(schedule_work(struct
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/kernel/Makefile .8740-linux-2.6.1-rc1.updated/kernel/Makefile
@@ -6,7 +6,8 @@ obj-y     = sched.o fork.o exec_domain.o
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/kernel/workqueue.c .8740-linux-2.6.1-rc1.updated/kernel/workqueue.c
@@ -359,6 +359,11 @@ void flush_scheduled_work(void)
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

You will lose in any case. What happens if the thread does do_exit() and you do kthread_stop() after that?
With the patch I posted to you, the kthread_stop() will simply miss the lookup and return -ENOENT.

- Davide

Nope, we are screwed in any case. Is it really important to give the ability to do do_exit() for kthreads? I mean, why a simple return would not work?

- Davide

In message <[email protected]> you write:

That's illegal. Either your thread exits, or you call kthread_stop().

Or find some other random kthread which has reused the task struct and kill that 8(

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

I can see two options:

1) We do not allow do_exit() from kthreads

2) We give kthread_exit()

What do you think?

- Davide

In message <[email protected]> you write:

Well, I've now got a patch which works without any global datastructures, but still allows the same semantics. Includes some fixes in the previous code, and test code.

Also includes a longstanding fix in workqueue.c: we never get SIGCHLD if signal handler is SIG_IGN.

I'm not sure it's neater, though. We don't have WIFEXITED() in the kernel, so lots of manual shifting, probably should add that somewhere.

Thoughts?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.6.1-rc1-bk5/include/linux/kthread.h working-2.6.1-rc1-bk5-kthread-noglobal/include/linux/kthread.h
@@ -0,0 +1,81 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple interface for creating and stopping kernel threads without mess. */
+#include <linux/err.h>
+struct task_struct;
+
+/**
+ * kthread_create: create a kthread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after @initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread, and controls it for you.  See kthread_start() and
+ * kthread_run().
+ * 
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+struct task_struct *kthread_create(int (*initfn)(void *data),
+				   int (*corefn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/**
+ * kthread_start: start a thread created by kthread_create().
+ * @k: the task returned by kthread_create().
+ *
+ * Description: This makes the thread @k run the initfn() specified in
+ * kthread_create(), if non-NULL, and return the result.  If that
+ * result is less than zero, then the thread @k is terminated.
+ * Otherwise, the corefn() is run if it is not NULL.
+ *
+ * There are two ways to write corefn(): simple or complex.  For
+ * simple, you set current->state to TASK_INTERRUPTIBLE, check for
+ * work, then return 0 when it is done.  The kthread code will call
+ * schedule(), and call corefn() again when woken.  More complex
+ * corefn()s can loop themselves, but must return 0 when
+ * signal_pending(current) is true.
+ *
+ * corefn() can terminate itself in two ways: if someone will later
+ * call kthread_stop(), simply return a negative error number.
+ * Otherwise, if you call do_exit() directly, the thread will
+ * terminate immediately. */
+int kthread_start(struct task_struct *k);
+
+/**
+ * kthread_run: create and start a thread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * kthread_start().  Returns the kthread, or ERR_PTR(). */
+#define kthread_run(initfn, corefn, data, namefmt, ...)			      \
+({									      \
+	struct task_struct *__k						      \
+		= kthread_create(initfn,corefn,data,namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						      \
+		int __err = kthread_start(__k);				      \
+		if (__err) __k = ERR_PTR(__err);			      \
+	}								      \
+	__k;								      \
+})
+
+/**
+ * kthread_stop: stop a thread created by kthread_create().
+ * @k: thread created by kthread_start.
+ *
+ * Sends a signal to @k, and waits for it to exit.  Your corefn() must
+ * not call do_exit() itself if you use this function!  This can also
+ * be called after kthread_create() instead of calling
+ * kthread_start(): the thread will exit without calling initfn() or
+ * corefn().
+ *
+ * Returns the last result of corefn(), or 0 if kthread_start() was
+ * never called. */
+int kthread_stop(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.6.1-rc1-bk5/include/linux/workqueue.h working-2.6.1-rc1-bk5-kthread-noglobal/include/linux/workqueue.h
@@ -60,6 +60,7 @@ extern int FASTCALL(schedule_work(struct
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.6.1-rc1-bk5/kernel/Makefile working-2.6.1-rc1-bk5-kthread-noglobal/kernel/Makefile
@@ -6,7 +6,8 @@ obj-y     = sched.o fork.o exec_domain.o
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.6.1-rc1-bk5/kernel/workqueue.c working-2.6.1-rc1-bk5-kthread-noglobal/kernel/workqueue.c
@@ -181,7 +181,7 @@ static int worker_thread(void *__startup
 	complete(&startup->done);
 
 	/* Install a handler so SIGCLD is delivered */
-	sa.sa.sa_handler = SIG_IGN;
+	sa.sa.sa_handler = SIG_DFL;
 	sa.sa.sa_flags = 0;
 	siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
 	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);

@@ -359,6 +359,11 @@ void flush_scheduled_work(void)
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

Honestly I do not like playing with SIGCLD/waitpid for things that do not concern real task exits. But I think it can be avoided, and actually I don't know why I did not think about this before. We don't need to return a struct task_struct* for kthread_create(). We can have:

struct kthread_struct {
	struct task_struct *tsk;
	int ret;
	struct semaphore ack; /* Or completion if you like */
};

This structure is allocated in kthread_create() and propagated to the kthread() void* data. Then they (the creator and the kthread) will have a common ground to exchange acks w/out playing with SIGCLD/waitpid. Heh ?

- Davide

In message <[email protected]> you write:

Well, it's pretty well documented.

Nope. That's EXACTLY the kind of burden on the caller I wanted to avoid if at all possible.

I just have decide whether to submit the wait one or the shared queue one to Andrew....

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

Which burden? The kthread is a resource and a "struct kthread" is an handle to the resource. You create the resource (kthread_create()), you control the resource (kthread_start()) and you free the resource (kthread_stop()). To me it's simple and clean and does not require hacks like taking owerships of tasks and using SIGCLD/waitpid to communicate. Anyway, that's your baby and you'll take your choice.

- Davide

In message <[email protected]> you write:

Thinking more about this issue lead me to rewrite the code to be simpler to use. Main benefit is that the transition from existing code is minimal.

Latest version of patch, and code which uses it. It's actually quite neat now. Changes since first version:

1) kthread_start() deleted in favor of wake_up_process() directly.
2) New kthread_bind() for just-created threads, to replace original migration thread open-coded version.
3) Slight simplification: thread named only if spawned OK.

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

Name: Simplified Kthread
Author: Rusty Russell
Status: Booted on 2.6.1-rc2-bk1
Depends: Hotcpu-New-Kthread/workqueue_children.patch.gz

D: The hotplug CPU code introduces two major problems:
D:

D: 1) Threads which previously never stopped (migration thread,
D:    ksoftirqd, keventd) have to be stopped cleanly as CPUs go offline.
D: 2) Threads which previously never had to be created now have
D:    to be created when a CPU goes online.

D:
D: Unfortunately, stopping a thread is fairly baroque, involving memory
D: barriers, a completion and spinning until the task is actually dead
D: (for example, complete_and_exit() must be used if inside a module). D:

D: There are also three problems in starting a thread:
D: 1) Doing it from a random process context risks environment contamination:
D:    better to do it from keventd to guarantee a clean environment, a-la
D:    call_usermodehelper.
D: 2) Getting the task struct without races is a hard: see kernel/sched.c
D:    migration_call(), kernel/workqueue.c create_workqueue_thread().
D: 3) There are races in starting a thread for a CPU which is not yet
D:    online: migration thread does a complex dance at the moment for
D:    a similar reason (there may be no migration thread to migrate us).

D:
D: Place all this logic in some primitives to make life easier:
D: kthread_create() and kthread_stop(). These primitives require no
D: extra data-structures in the caller: they operate on normal "struct
D: task_struct"s.
D:

D: Other changes:
D:   - Expose keventd_up(), as keventd and migration threads will use
D:     kthread to launch, and kthread normally uses workqueues and must
D:     recognize this case.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .16124-linux-2.6.1-rc2-bk1/include/linux/kthread.h .16124-linux-2.6.1-rc2-bk1.updated/include/linux/kthread.h
@@ -0,0 +1,71 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple interface for creating and stopping kernel threads without mess. */
+#include <linux/err.h>
+#include <linux/sched.h>
+
+/**
+ * kthread_create: create a kthread.
+ * @threadfn: the function to run until signal_pending(current).
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread.  The thread will be stopped: use wake_up_process() to start
+ * it.  See also kthread_run(), kthread_create_on_cpu().
+ *
+ * When woken, the thread will run @threadfn() with @data as its
+ * argument. @threadfn can either call do_exit() directly if it is a
+ * standalone thread for which noone will call kthread_stop(), or
+ * return when 'signal_pending(current)' is true (which means
+ * kthread_stop() has been called).  The return value should be zero
+ * or a negative error number: it will be passed to kthread_stop().
+ * 
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+struct task_struct *kthread_create(int (*threadfn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/**
+ * kthread_run: create and wake a thread.
+ * @threadfn: the function to run until signal_pending(current).
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * wake_up_process().  Returns the kthread, or ERR_PTR(-ENOMEM). */
+#define kthread_run(threadfn, data, namefmt, ...)			   \
+({									   \
+	struct task_struct *__k						   \
+		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k))						   \
+		wake_up_process(__k);					   \
+	__k;								   \
+})
+
+/**
+ * kthread_bind: bind a just-created kthread to a cpu.
+ * @k: thread created by kthread_create().
+ * @cpu: cpu (might not be online, must be possible) for @k to run on.
+ *
+ * Description: This function is equivalent to set_cpus_allowed(),
+ * except that @cpu doesn't need to be online, and the thread must be
+ * stopped (ie. just returned from kthread_create().
+ */
+void kthread_bind(struct task_struct *k, unsigned int cpu);
+
+/**
+ * kthread_stop: stop a thread created by kthread_create().
+ * @k: thread created by kthread_create().
+ *
+ * Sends a signal to @k, and waits for it to exit.  Your threadfn()
+ * must not call do_exit() itself if you use this function!  This can
+ * also be called after kthread_create() instead of calling
+ * wake_up_process(): the thread will exit without calling threadfn().
+ *
+ * Returns the result of threadfn(), or -EINTR if wake_up_process()
+ * was never called. */
+int kthread_stop(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .16124-linux-2.6.1-rc2-bk1/include/linux/workqueue.h .16124-linux-2.6.1-rc2-bk1.updated/include/linux/workqueue.h
@@ -60,6 +60,7 @@ extern int FASTCALL(schedule_work(struct
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .16124-linux-2.6.1-rc2-bk1/kernel/Makefile .16124-linux-2.6.1-rc2-bk1.updated/kernel/Makefile
@@ -6,7 +6,8 @@ obj-y     = sched.o fork.o exec_domain.o
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .16124-linux-2.6.1-rc2-bk1/kernel/workqueue.c .16124-linux-2.6.1-rc2-bk1.updated/kernel/workqueue.c
@@ -347,6 +347,11 @@ void flush_scheduled_work(void)
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

Name: Use Kthread For Core Kernel Threads
Author: Rusty Russell
Status: Booted on 2.6.1-rc2-bk1
Depends: Hotcpu-New-Kthread/kthread-simple.patch.gz

D: This simply changes over the migration threads, the workqueue
D: threads and the ksoftirqd threads to use kthread.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .9505-linux-2.6.1-rc2-bk1/kernel/softirq.c .9505-linux-2.6.1-rc2-bk1.updated/kernel/softirq.c
@@ -14,6 +14,7 @@
 #include <linux/notifier.h>
 #include <linux/percpu.h>
 #include <linux/cpu.h>
+#include <linux/kthread.h>
 
 /*
    - No shared variables, all the data are CPU local.

@@ -337,20 +338,14 @@ static int ksoftirqd(void * __bind_cpu)
 {
 	int cpu = (int) (long) __bind_cpu;
 
-	daemonize("ksoftirqd/%d", cpu);
 	set_user_nice(current, 19);
 	current->flags |= PF_IOTHREAD;
 
-	/* Migrate to the right CPU */
-	set_cpus_allowed(current, cpumask_of_cpu(cpu));
 	BUG_ON(smp_processor_id() != cpu);
 
-	__set_current_state(TASK_INTERRUPTIBLE);
-	mb();
-
-	__get_cpu_var(ksoftirqd) = current;
+	set_current_state(TASK_INTERRUPTIBLE);
 
-	for (;;) {
+	while (!signal_pending(current)) {
 		if (!local_softirq_pending())
 			schedule();
 

@@ -363,6 +358,7 @@ static int ksoftirqd(void * __bind_cpu)
 
 		__set_current_state(TASK_INTERRUPTIBLE);
 	}
+	return 0;
 }
 
 static int __devinit cpu_callback(struct notifier_block *nfb,

@@ -370,15 +366,17 @@ static int __devinit cpu_callback(struct
 				  void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct task_struct *p;
 
 	if (action == CPU_ONLINE) {
-		if (kernel_thread(ksoftirqd, hcpu, CLONE_KERNEL) < 0) {
+		p = kthread_create(ksoftirqd, hcpu, "ksoftirqd/%d", hotcpu);
+		if (IS_ERR(p)) {
 			printk("ksoftirqd for %i failed\n", hotcpu);
 			return NOTIFY_BAD;
 		}
-
-		while (!per_cpu(ksoftirqd, hotcpu))
-			yield();
+		per_cpu(ksoftirqd, hotcpu) = p;
+		kthread_bind(p, hotcpu);
+		wake_up_process(p);
  	}
 	return NOTIFY_OK;
 }

Yes, I like this better. Without any doubt, the removal of sync'd start function simplified things a lot.

- Davide

In message <[email protected]> you write:

Yeah, I thought you would: makes our previous debates moot so we can be friends again 8)

The reason I did the startfn thing originally is for the hotplug CPU
code: I wanted to make sure the thread gets onto the CPU while we're in the notifier, so it doesn't go down again before the thread does set_cpus_allowed(). Not a problem in practice, but it's nice to have the BUG_ON(smp_processor_id() != cpu) in the code.

kthread_bind() solves this neatly, and we need it for migration_thread anyway (which is the one thread where being on the right CPU effects correctness, not just performance).

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

Nope, we are screwed in any case. Is it really important to give the ability to do do_exit() for kthreads? I mean, why a simple return would not work?

- Davide

You will lose in any case. What happens if the thread does do_exit() and you do kthread_stop() after that?
With the patch I posted to you, the kthread_stop() will simply miss the lookup and return -ENOENT.

- Davide

Rusty, I gave the patch a quick shot. There's no more single and global data structures but the list that stores all kthreads. Like usual, if you don't like it you can trash it. But pls leave shame apart. It did nothing to you :-)

- Davide

@@ -0,0 +1,81 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple interface for creating and stopping kernel threads without mess. */
+#include <linux/err.h>
+struct task_struct;
+
+/**
+ * kthread_create: create a kthread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after @initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread, and controls it for you.  See kthread_start() and
+ * kthread_run().
+ * 
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+struct task_struct *kthread_create(int (*initfn)(void *data),
+				   int (*corefn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/**
+ * kthread_start: start a thread created by kthread_create().
+ * @k: the task returned by kthread_create().
+ *
+ * Description: This makes the thread @k run the initfn() specified in
+ * kthread_create(), if non-NULL, and return the result.  If that
+ * result is less than zero, then the thread @k is terminated.
+ * Otherwise, the corefn() is run if it is not NULL.
+ *
+ * There are two ways to write corefn(): simple or complex.  For
+ * simple, you set current->state to TASK_INTERRUPTIBLE, check for
+ * work, then return 0 when it is done.  The kthread code will call
+ * schedule(), and call corefn() again when woken.  More complex
+ * corefn()s can loop themselves, but must return 0 when
+ * signal_pending(current) is true.
+ *
+ * corefn() can terminate itself in two ways: if someone will later
+ * call kthread_stop(), simply return a negative error number.
+ * Otherwise, if you call do_exit() directly, the thread will
+ * terminate immediately. */
+int kthread_start(struct task_struct *k);
+
+/**
+ * kthread_run: create and start a thread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * kthread_start().  Returns the kthread, or ERR_PTR(). */
+#define kthread_run(initfn, corefn, data, namefmt, ...)			      \
+({									      \
+	struct task_struct *__k						      \
+		= kthread_create(initfn,corefn,data,namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						      \
+		int __err = kthread_start(__k);				      \
+		if (__err) __k = ERR_PTR(__err);			      \
+	}								      \
+	__k;								      \
+})
+
+/**
+ * kthread_stop: stop a thread created by kthread_create().
+ * @k: thread created by kthread_start.
+ *
+ * Sends a signal to @k, and waits for it to exit.  Your corefn() must
+ * not call do_exit() itself if you use this function!  This can also
+ * be called after kthread_create() instead of calling
+ * kthread_start(): the thread will exit without calling initfn() or
+ * corefn().
+ *
+ * Returns the last result of corefn(), or 0 if kthread_start() was
+ * never called. */
+int kthread_stop(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

@@ -60,6 +60,7 @@
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

@@ -6,7 +6,8 @@
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

@@ -359,6 +359,11 @@
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

In message <[email protected]> you write:

Except sharing data with a lock is perfectly robust.

Yeah, I deliberately didn't implement this, because (1) it seems like a lot of complexity when using a lock and letting them share a single structure works fine and is even simpler, and (2) the thread can't just "do_exit()".

You can get around (2) by having a permenant parent "kthread" thread which is a parent to all the kthreads (it'll get a SIGCHLD when someone does "do_exit()"). But the implementation was pretty ugly, since it involved having a communications mechanism with the kthread parent, which means you have the global ktm_message-like-thing for this...

I put the "corefn fails" option in there in case someone wants it, but I'm not sure how it would be used in practice. If it becomes commonplace to have them handing around for a long time, then storing the result somewhere rather than keeping the whole thread around makes sense, but at the moment it seems like premature optimization.

Unfortunately, my fd idea is broken, because the process which does the start may not be the same as the one which does the create. A shared queue really is the simplest solution.

Latest version below,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Use Signals For Kthread
Author: Rusty Russell
Status: Tested on 2.6.0-bk3

D: The hotplug CPU code introduces two major problems:
D:

D: 1) Threads which previously never stopped (migration thread,
D:    ksoftirqd, keventd) have to be stopped cleanly as CPUs go offline.
D: 2) Threads which previously never had to be created now have
D:    to be created when a CPU goes online.

D:
D: Unfortunately, stopping a thread is fairly baroque, involving memory
D: barriers, a completion and spinning until the task is actually dead. D:

D: There are also three problems in starting a thread:
D: 1) Doing it from a random process context risks environment contamination:
D:    better to do it from keventd to guarantee a clean environment, a-la
D:    call_usermodehelper.
D: 2) Getting the task struct without races is a hard: see kernel/sched.c
D:    migration_call(), kernel/workqueue.c create_workqueue_thread().
D: 3) There are races in starting a thread for a CPU which is not yet
D:    online: migration thread does a complex dance at the moment for
D:    a similar reason (there may be no migration thread to migrate us).

D:
D: Place all this logic in some primitives to make life easier:
D: kthread_create(), kthread_start() and kthread_destroy(). These
D: primitives require no extra data-structures in the caller: they operate
D: on normal "struct task_struct"s.
D:

D: Other changes:
D:   - Expose keventd_up(), as keventd and migration threads will use
D:     kthread to launch, and kthread normally uses workqueues and must
D:     recognize this case.
D: 
D: Changes since first version:
D: 1) Sleep UNINTERRUPTIBLE when waiting for reply (thanks Andrew Morton).
D: 2) Reparent threads and waitpid for them.
D: 3) Do ALL ops via keventd, so we don't disturb current process.
D: 4) This version uses signals rather than its own communication
D:     mechanism makes kthread code a little simpler, and means users can
D:     loop inside corefn if they want, and return when signal_pending().
D: 5) kthread_destroy() renamed to kthread_stop().
D: 6) kthread_start() returns an int.
D: 7) Better documentation, in nanodoc form.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/include/linux/kthread.h .8740-linux-2.6.1-rc1.updated/include/linux/kthread.h
@@ -0,0 +1,81 @@
+#ifndef _LINUX_KTHREAD_H
+#define _LINUX_KTHREAD_H
+/* Simple interface for creating and stopping kernel threads without mess. */
+#include <linux/err.h>
+struct task_struct;
+
+/**
+ * kthread_create: create a kthread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after @initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread, and controls it for you.  See kthread_start() and
+ * kthread_run().
+ * 
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+struct task_struct *kthread_create(int (*initfn)(void *data),
+				   int (*corefn)(void *data),
+				   void *data,
+				   const char namefmt[], ...);
+
+/**
+ * kthread_start: start a thread created by kthread_create().
+ * @k: the task returned by kthread_create().
+ *
+ * Description: This makes the thread @k run the initfn() specified in
+ * kthread_create(), if non-NULL, and return the result.  If that
+ * result is less than zero, then the thread @k is terminated.
+ * Otherwise, the corefn() is run if it is not NULL.
+ *
+ * There are two ways to write corefn(): simple or complex.  For
+ * simple, you set current->state to TASK_INTERRUPTIBLE, check for
+ * work, then return 0 when it is done.  The kthread code will call
+ * schedule(), and call corefn() again when woken.  More complex
+ * corefn()s can loop themselves, but must return 0 when
+ * signal_pending(current) is true.
+ *
+ * corefn() can terminate itself in two ways: if someone will later
+ * call kthread_stop(), simply return a negative error number.
+ * Otherwise, if you call do_exit() directly, the thread will
+ * terminate immediately. */
+int kthread_start(struct task_struct *k);
+
+/**
+ * kthread_run: create and start a thread.
+ * @initfn: a function to run when kthread_start() is called.
+ * @corefn: a function to run after initfn succeeds.
+ * @data: data ptr for @initfn and @corefn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * kthread_start().  Returns the kthread, or ERR_PTR(). */
+#define kthread_run(initfn, corefn, data, namefmt, ...)			      \
+({									      \
+	struct task_struct *__k						      \
+		= kthread_create(initfn,corefn,data,namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						      \
+		int __err = kthread_start(__k);				      \
+		if (__err) __k = ERR_PTR(__err);			      \
+	}								      \
+	__k;								      \
+})
+
+/**
+ * kthread_stop: stop a thread created by kthread_create().
+ * @k: thread created by kthread_start.
+ *
+ * Sends a signal to @k, and waits for it to exit.  Your corefn() must
+ * not call do_exit() itself if you use this function!  This can also
+ * be called after kthread_create() instead of calling
+ * kthread_start(): the thread will exit without calling initfn() or
+ * corefn().
+ *
+ * Returns the last result of corefn(), or 0 if kthread_start() was
+ * never called. */
+int kthread_stop(struct task_struct *k);
+
+#endif /* _LINUX_KTHREAD_H */

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/include/linux/workqueue.h .8740-linux-2.6.1-rc1.updated/include/linux/workqueue.h
@@ -60,6 +60,7 @@ extern int FASTCALL(schedule_work(struct
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
+extern int keventd_up(void);
 
 extern void init_workqueues(void);
 

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/kernel/Makefile .8740-linux-2.6.1-rc1.updated/kernel/Makefile
@@ -6,7 +6,8 @@ obj-y     = sched.o fork.o exec_domain.o
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
-	    rcupdate.o intermodule.o extable.o params.o posix-timers.o
+	    rcupdate.o intermodule.o extable.o params.o posix-timers.o \
+	    kthread.o
 
 obj-$(CONFIG_FUTEX) += futex.o
 obj-$(CONFIG_GENERIC_ISA_DMA) += dma.o

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .8740-linux-2.6.1-rc1/kernel/workqueue.c .8740-linux-2.6.1-rc1.updated/kernel/workqueue.c
@@ -359,6 +359,11 @@ void flush_scheduled_work(void)
 	flush_workqueue(keventd_wq);
 }
 
+int keventd_up(void)
+{
+	return keventd_wq != NULL;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;

In message <[email protected]> you write:

Still do. It's *simple*, and I refuse to be ashamed of that.

My words were harsh, but I completely disagree with you. I believe you are wrong. I would never have coded it the way you did. I read your code and I still think you are wrong, and find your code both bloated and ugly.

It's not about space, it's about taste. And placing random stuff in an unrelated structure because you can't think of a better way to do it is TASTELESS. If it were the only way, it might be forgivable, but it's not, and I far prefer a little localized messiness to global messiness.

Now, on something we do agree: I dislike the global structure myself. By all means try changing the code to use a pipe between child and parent for the initfn result. But I've told you that I will not submit any solution which adds to a generic structure for a specific problem.

I'm very, very sorry this has gotten a little heated: I generally enjoy our discussions. But I don't think I should have to say "no" four times.

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

Bloated ? This is the diffstat of my "ashamed" patch over your bits :-)

include/linux/init_task.h |    3 +
include/linux/sched.h     |    8 ++++
kernel/kthread.c          |   78 ++++++++++++++++------------------------------
3 files changed, 39 insertions(+), 50 deletions(-)

It's ok Rusty, I enjoy the discussion in any case :-) Since I told you in a private email that I was convinced myself about adding stuff inside the struct, you could have avoided the "ashamed" thing. But it's fine, a little bit of sarcasm is the salt of life.

- Davide

Bloated ? This is the diffstat of my "ashamed" patch over your bits :-)

include/linux/init_task.h |    3 +
include/linux/sched.h     |    8 ++++
kernel/kthread.c          |   78 ++++++++++++++++------------------------------
3 files changed, 39 insertions(+), 50 deletions(-)

It's ok Rusty, I enjoy the discussion in any case :-) Since I told you in a private email that I was convinced myself about adding stuff inside the struct, you could have avoided the "ashamed" thing. But it's fine, a little bit of sarcasm is the salt of life.

- Davide

Rusty, I took a better look at the patch and I think we can have per-kthread stuff w/out littering the task_struct and by making the thing more robust. We keep a global list_head protected by a global spinlock. We define a structure that contain all the per-kthread stuff we need (including a task_struct* to the kthread itself). When a kthread starts it will add itself to the list, and when it will die it will remove itself from the list. The start/stop functions will lookup the list (or hash, depending on how much stuff you want to drop in) with the target task_struct*, and if the lookup fails, it means the task already quit (another task already did kthread_stop() ??, natural death ????). This is too bad, but at least there won't be deadlock (or crash) beacause of this. This because currently we keep the kthread task_struct* lingering around w/out a method that willl inform us if the task goes away for some reason (so that we can avoid signaling it and waiting for some interaction). The list/hash will be able to tell us this. What do you think?

- Davide

In message <[email protected]> you write:

Still do. It's *simple*, and I refuse to be ashamed of that.

My words were harsh, but I completely disagree with you. I believe you are wrong. I would never have coded it the way you did. I read your code and I still think you are wrong, and find your code both bloated and ugly.

It's not about space, it's about taste. And placing random stuff in an unrelated structure because you can't think of a better way to do it is TASTELESS. If it were the only way, it might be forgivable, but it's not, and I far prefer a little localized messiness to global messiness.

Now, on something we do agree: I dislike the global structure myself. By all means try changing the code to use a pipe between child and parent for the initfn result. But I've told you that I will not submit any solution which adds to a generic structure for a specific problem.

I'm very, very sorry this has gotten a little heated: I generally enjoy our discussions. But I don't think I should have to say "no" four times.

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

Ashamed !? Take a look at your original patch and then define shame. You had a communication mechanism that whilst being a private 1<->1 communication among two tasks, relied on a single global message strucure, lock and mutex. Honestly I do not like myself to add stuff inside a strcture for one-time use. Not because of adding 12 bytes to the struct, that are laughable. But because it is used by a small piece of code w/out a re-use ability for other things.

But of course, even a third one ;)

- Davide

In message <[email protected]> you write:

And you're still putting obscure crap in the task struct when there's no need. Honestly, I'd be ashamed to post such a patch.

I think I should seek a second opinion though.
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Rusty, you still have to use global static data when there is no need. I like this version better though ;)

- Davide