On Thu, 19 Apr 2007 01:58:51 -0600

	while (!atomic_read(&s->killed)) {


It's unusual to have a kernel thread which has a space in its name.  That
could trip up infufficient-defensive userspace tools.

-

Note that this also stands against a full kthread conversion.
Marcel put my old patches for a full kthread conversion in, but
they didn't deal properly with some of the premaure exit cases,
and causes OOPSes.

I don't remember what the problems where, but the case of a thread
terminating earlier and possibly asynchronously is one of the
cases we'll probably have to add to the kthread infrastructure
before all uses of kernel_thread in drivers can be converted.

-

This patch implements the kthread helper functions kthread_start
and kthread_end which make it simple to support a kernel thread
that may decided to exit on it's own before we request it to.
It is still assumed that eventually we will get around to requesting
that the kernel thread stop.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/kthread.h |   23 +++++++++++++++++++++++
 kernel/kthread.c        |   18 ++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index a8ea31d..4f1eff1 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -28,6 +28,29 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 
 void kthread_bind(struct task_struct *k, unsigned int cpu);
 int kthread_stop(struct task_struct *k);
+/**
+ * kthread_start - create and wake a thread.
+ * @threadfn: the function to run until kthread_should_stop().
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: Convenient wrapper for kthread_create() followed by
+ * get_task_struct() and wake_up_process. kthread_start should be paired
+ * with kthread_end() so we don't leak task structs.
+ *
+ * Returns the kthread or ERR_PTR(-ENOMEM).
+ */
+#define kthread_start(threadfn, data, namefmt, ...)			   \
+({									   \
+	struct task_struct *__k						   \
+		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
+	if (!IS_ERR(__k)) {						   \
+		get_task_struct(__k);					   \
+		wake_up_process(__k);					   \
+	}								   \
+	__k;								   \
+})
+int kthread_end(struct task_struct *k);
 
 static inline int __kthread_should_stop(struct task_struct *tsk)
 {
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 9b3c19f..d6d63c6 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -179,6 +179,24 @@ int kthread_stop(struct task_struct *tsk)
 }
 EXPORT_SYMBOL(kthread_stop);
 
+/**
+ * kthread_end - signal ...
[ message continues ]

I don't think having to parallel APIs is a good idea, people will
get utterly confused which one to use.  Better always grab a reference
in kthread_create and drop it in kthread_stop.  For normal thread
no change in behaviour and only slightly more code in the slowpath.

Of course it will need an audit for half-assed kthread conversion
first to avoid task_struct reference count leaks.

In addition to that kthrad_end implementation look wrong. When
the kthread has exited prematurely no one will call complete
on kthread_stop_info.done before it's been setup.  Interestingly
the comment there indicates someone thought about threads exiting
early, but it became defunkt during all the rewrites of the
-

I *am* already confused... a driver of mine does:

static __init int thkd_init(void)
{
	touch_task = kthread_run(touch_thread, Device, "thkd");
	...
}

and

static __exit void thkd_exit(void)
{
	kthread_stop(touch_task);
	/* I bet something is missing */
}

now what good would kthread_run do me?



Jan
-- 
-

Please read the kerneldoc documentation for kthread_create
-

In that case it is better to grab a reference in kthread(). This also
close the race when a new thread is woken (freezer) and exits before

This is not true anymore, see another patch from Eric

	kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch

Oleg.

-

Ok.  Thinking about it I agree with Christoph that parallel API's can
be a problem.

However we do still need to support kernel threads where kthread_stop will
never be called.  There appear to be a few legitimate cases where
someone wants to fire off a thread and have it do some work but don't
care at all for stopping it before it is done.

So I propose we add a kthread_orphan as a basic primitive to decrement the
count on the task_struct if we want a kthread to simply exit after it
has done some work.

And as a helper function we can have a kthread_run_orphan.

I think having a kthread_orphan will document what we are doing better
and make it easy to find kernel threads that don't use kthread_stop.

The pain is that this requires an audit of all kernel kthread creators
so that we call kthread_orphan on the right ones, or else we will have
a task_struct leak.  At least that isn't a fatal condition.

Eric
-

Speaking about helpers, could we also add kthread_start(), which should be
used instead of direct wake_up_process() ? Not that it is terribly important,
but still.

Note that "kthread_create() pins the task_struct" allows us to cleanup the code.
Look at this ugly "wait_to_die:" label in migration_thread(). Is is because
migration_thread() can't exit until CPU_DEAD reaps it. Other reasons were already
solved by kthread-enhance-kthread_stop-to-abort-interruptible-sleeps.patch

Oleg.

-

There's two cases where it's valid that kthread_stop is not called:

 a) the user is always builtin and the thread runs until the kernel halts.
    examples: voyager, arm ecard
 b) the thread is normally started/stopped, e.g. at module_init/module_exit
    but there is some reason why it could terminate earlier.
    examples:  the various bluetooth threads, nfs-related threads that
    can be killed using signals
 c) we have some kind of asynchronous helper thread.
    examples: various s390 drivers, usbatm, therm_pm72
 d) a driver has threadpools were we need to start/stop threads on demand.
    examples: nfsd, xpc


case a)
	is trivial, we can just ignore the refcounting issue.

case b)
	is what refcounting the task struct and proper handling in
	kthread_stop will deal with.

case c)
	should get a new kthread_create_async api which starts a thread
	without blocking, so we can get rid of the workqueues in the
	s390 drivers.  it should probably also be safe to be called from
	irq context.  What makes this a bit complicated is the need to
	make sure no more thread is running in case the caller terminates
	(shutdown of the structure it's associated with or module removal)

case d)
	should be deal with with a kthread_pool api

-

yes. we need something like :

-       while (!atomic_read(&s->killed)) {
+       while (1) {
                try_to_freeze();
 
                set_current_state(TASK_INTERRUPTIBLE);
 
+               if (atomic_read(&s->killed))
+                       break;
+


but we can't just change it, can we ? i could be used by a user space tool 
to check if the thread is running.

C.
-

here's the refreshed version not taking into account the space in its 
kernel thread name. 

C.

Signed-off-by: Cedric Le Goater <clg@fr.ibm.com>

---
 net/bluetooth/bnep/core.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Index: 2.6.21-rc6-mm1/net/bluetooth/bnep/core.c
===================================================================
--- 2.6.21-rc6-mm1.orig/net/bluetooth/bnep/core.c
+++ 2.6.21-rc6-mm1/net/bluetooth/bnep/core.c
@@ -47,6 +47,7 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
+#include <linux/kthread.h>
 
 #include <asm/unaligned.h>
 
@@ -473,16 +474,18 @@ static int bnep_session(void *arg)
 
 	BT_DBG("");
 
-	daemonize("kbnepd %s", dev->name);
 	set_user_nice(current, -15);
 
 	init_waitqueue_entry(&wait, current);
 	add_wait_queue(sk->sk_sleep, &wait);
-	while (!atomic_read(&s->killed)) {
+	while (1) {
 		try_to_freeze();
 
 		set_current_state(TASK_INTERRUPTIBLE);
 
+		if (atomic_read(&s->killed))
+			break;
+
 		// RX
 		while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
 			skb_orphan(skb);
@@ -539,6 +542,7 @@ static struct device *bnep_get_device(st
 
 int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
 {
+	struct task_struct *task;
 	struct net_device *dev;
 	struct bnep_session *s, *ss;
 	u8 dst[ETH_ALEN], src[ETH_ALEN];
@@ -598,9 +602,10 @@ int bnep_add_connection(struct bnep_conn
 
 	__bnep_link_session(s);
 
-	err = kernel_thread(bnep_session, s, CLONE_KERNEL);
-	if (err < 0) {
-		/* Session thread start failed, gotta cleanup. */
+	task = kthread_run(bnep_session, s, "kbnepd %s", dev->name);
+	if (IS_ERR(task)) {
+ 		/* Session thread start failed, gotta cleanup. */
+		err = PTR_ERR(task);
 		unregister_netdev(dev);
 		__bnep_unlink_session(s);
 		goto failed;
-

Hello,


But all kernel threads are supposed to be only *in-kernel*
implementation details. Isn't a userspace tool whose behaviour relies
on the existence (or even the knowledge of the existence) of any

Yes, so although userspace shouldn't be bothering with kernel threads
in the first place, that does not mean that such tools do not exist.
So we'll have to live with this (unfortunate) naming for some time,
till we can get rid of it later.

Which is similar to the habit of some kernel threads in there that
actually *do* want to export the knowledge of their existence (and
even a signals-based interface!) to userspace. Eric did receive some
nacks on his patches that tried to remove the signals business from
kernel threads on this account, but perhaps that too is something that
we could get rid of later (hopefully by that time those using signals
in kernel threads would have realized their folly and shifted to
something else :-)

Satyam
-