[PATCH 3/3] make kthread_stop() scalable

[PATCH 3/3] make kthread_stop() scalable

[Oleg Nesterov]

It's a shame kthread_stop() (may take a while!) runs with a global semaphore
held. With this patch kthread() allocates all neccesary data (struct kthread)
on its own stack, globals kthread_stop_xxx are deleted.

HACKS:

	- re-use task_struct->set_child_tid to point to "struct kthread"

	- use do_exit() directly to preserve "struct kthread" on stack

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.21-rc5/kernel/kthread.c~3_STOP	2007-04-13 16:28:41.000000000 +0400
+++ 2.6.21-rc5/kernel/kthread.c	2007-04-13 16:24:37.000000000 +0400
@@ -17,7 +17,25 @@
 
 static DEFINE_SPINLOCK(kthread_create_lock);
 static LIST_HEAD(kthread_create_list);
-struct task_struct *kthreadd_task;
+struct task_struct *kthreadd_task __read_mostly;
+
+struct kthread {
+	int should_stop;
+	struct task_struct *task;
+
+	struct completion exited;
+	int err;
+};
+
+static inline struct kthread *to_kthread(struct task_struct *t)
+{
+	return (void*)t->set_child_tid;
+}
+
+static inline void set_kthread(struct kthread *self)
+{
+	current->set_child_tid = (void __user*)self;
+}
 
 struct kthread_create_info
 {
@@ -27,24 +45,12 @@ struct kthread_create_info
 	struct completion created;
 	struct completion started;
 
-	/* Result passed back to kthread_create() from kthreadd. */
-	pid_t result;
+	/* Result passed back to kthread_create() from kthread. */
+	struct kthread *result;
 
 	struct list_head list;
 };
 
-struct kthread_stop_info
-{
-	struct task_struct *k;
-	int err;
-	struct completion done;
-};
-
-/* Thread stopping is done by setthing this var: lock serializes
- * multiple kthread_stop calls. */
-static DEFINE_MUTEX(kthread_stop_lock);
-static struct kthread_stop_info kthread_stop_info;
-
 /**
  * kthread_should_stop - should this kthread return now?
  *
@@ -54,20 +60,28 @@ static struct kthread_stop_info kthread_
  */
 int kthread_should_stop(void)
 {
-	return (kthread_stop_info.k == current);
+	return to_kthread(current)->should_stop;
 }
 EXPORT_SYMBOL(kthread_should_stop);
 
 static int kthread(void *_create)
 {
-	struct kthread_create_info *create = _create;
-	int (*threadfn)(void *data);
-	void *data;
-	int ret = -EINTR;
+	struct kthread self = {
+		.task = current,
+		.err = -EINTR,
+	};
 
 	/* Copy data: it's on kthread's stack */
-	threadfn = create->threadfn;
-	data = create->data;
+	struct kthread_create_info *create = _create;
+	int (*threadfn)(void *data) = create->threadfn;
+	void *data = create->data;
+
+	/*
+	 * This should be enough to assure that self is still on
+	 * stack when we enter do_exit()
+	 */
+	set_kthread(&self);
+	create->result = &self;
 
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
@@ -75,13 +89,13 @@ static int kthread(void *_create)
 	schedule();
 
 	if (!kthread_should_stop())
-		ret = threadfn(data);
+		self.err = threadfn(data);
 
-	/* It might have exited on its own, w/o kthread_stop.  Check. */
-	if (kthread_should_stop()) {
-		kthread_stop_info.err = ret;
-		complete(&kthread_stop_info.done);
-	}
+	/* It might have exited on its own, w/o kthread_stop. Check. */
+	if (kthread_should_stop())
+		complete(&self.exited);
+
+	do_exit(0);
 	return 0;
 }
 
@@ -91,7 +105,7 @@ static void create_kthread(struct kthrea
 
 	/* We want our own signal handler (we take no signals by default). */
 	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
-	create->result = pid;
+	create->result = ERR_PTR(pid);
 
 	complete(&create->created);
 }
@@ -135,11 +149,11 @@ struct task_struct *kthread_create(int (
 	wake_up_process(kthreadd_task);
 
 	wait_for_completion(&create.created);
-	if (create.result < 0)
-		return ERR_PTR(create.result);
+	if (IS_ERR(create.result))
+		return (void*)create.result;
 
 	wait_for_completion(&create.started);
-	ret = find_task_by_pid(create.result);
+	ret = create.result->task;
 
 	va_start(args, namefmt);
 	vsnprintf(ret->comm, sizeof(ret->comm), namefmt, args);
@@ -183,27 +197,23 @@ EXPORT_SYMBOL(kthread_bind);
  */
 int kthread_stop(struct task_struct *k)
 {
+	struct kthread *kthread;
 	int ret;
 
-	mutex_lock(&kthread_stop_lock);
-
 	/* It could exit after stop_info.k set, but before wake_up_process. */
 	get_task_struct(k);
+	kthread = to_kthread(k);
 
-	/* Must init completion *before* thread sees kthread_stop_info.k */
-	init_completion(&kthread_stop_info.done);
+	/* Must init completion *before* thread sees ->should_stop */
+	init_completion(&kthread->exited);
 	smp_wmb();
-
-	/* Now set kthread_should_stop() to true, and wake it up. */
-	kthread_stop_info.k = k;
+	kthread->should_stop = 1;
 	wake_up_process(k);
-	put_task_struct(k);
 
 	/* Once it dies, reset stop ptr, gather result and we're done. */
-	wait_for_completion(&kthread_stop_info.done);
-	kthread_stop_info.k = NULL;
-	ret = kthread_stop_info.err;
-	mutex_unlock(&kthread_stop_lock);
+	wait_for_completion(&kthread->exited);
+	ret = kthread->err;
+	put_task_struct(k);
 
 	return ret;
 }

Re: [PATCH 3/3] make kthread_stop() scalable

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> It's a shame kthread_stop() (may take a while!) runs with a global semaphore
> held. With this patch kthread() allocates all neccesary data (struct kthread)
> on its own stack, globals kthread_stop_xxx are deleted.

Oleg so fare you patches  have been inspiring.  However..

> HACKS:
>
> 	- re-use task_struct->set_child_tid to point to "struct kthread"

	 task_struct->vfork_done is a better cannidate.

> 	- use do_exit() directly to preserve "struct kthread" on stack

Calling do_exit directly like that is not a hack, as it appears the preferred
way to exit is to call do_exit, or complete_and_exit.

While this does improve the scalability and remove a global variable.  It
also introduces a complex special case in the form of struct kthread.

It also doesn't solve the biggest problem with the current kthread interface
in that calling kthread_stop does not cause the code to break out of
interruptible sleeps.

>  static int kthread(void *_create)
>  {
> -	struct kthread_create_info *create = _create;
> -	int (*threadfn)(void *data);
> -	void *data;
> -	int ret = -EINTR;
> +	struct kthread self = {
> +		.task = current,
> +		.err = -EINTR,
> +	};
>  
>  	/* Copy data: it's on kthread's stack */
> -	threadfn = create->threadfn;
> -	data = create->data;
> +	struct kthread_create_info *create = _create;
> +	int (*threadfn)(void *data) = create->threadfn;
> +	void *data = create->data;
> +
> +	/*
> +	 * This should be enough to assure that self is still on
> +	 * stack when we enter do_exit()
> +	 */
> +	set_kthread(&self);
> +	create->result = &self;
>  

> @@ -91,7 +105,7 @@ static void create_kthread(struct kthrea
>  
>  	/* We want our own signal handler (we take no signals by default). */
>  	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
> -	create->result = pid;
> +	create->result = ERR_PTR(pid);

Ouch.    You have a nasty race here.

If kthread runs before kernel_thread returns then setting
"create->result = ERR_PTR(pid);" could easily stomp 
"create->result = &self".


Eric

Re: [PATCH 3/3] make kthread_stop() scalable

[Oleg Nesterov]

On 04/13, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > It's a shame kthread_stop() (may take a while!) runs with a global semaphore
> > held. With this patch kthread() allocates all neccesary data (struct kthread)
> > on its own stack, globals kthread_stop_xxx are deleted.
> 
> Oleg so fare you patches  have been inspiring.  However..
> 
> > HACKS:
> >
> > 	- re-use task_struct->set_child_tid to point to "struct kthread"
> 
> 	 task_struct->vfork_done is a better cannidate.
> 
> > 	- use do_exit() directly to preserve "struct kthread" on stack
> 
> Calling do_exit directly like that is not a hack, as it appears the preferred
> way to exit is to call do_exit, or complete_and_exit.
> 
> While this does improve the scalability and remove a global variable.  It
> also introduces a complex special case in the form of struct kthread.

I can't say I agree. I thought it is good to have a struct which represents
a kernel thread. Actually, I thought we can have __kthread_create() which
returns "struct kthread". May be I am wrong, because yes, ->set_child_tid can
point right to completion, and we can use some TIF flag instead of ->should_stop.
This needs to update a lot of include/asm/ files.

> It also doesn't solve the biggest problem with the current kthread interface
> in that calling kthread_stop does not cause the code to break out of
> interruptible sleeps.

Hm? kthread_stop() does wake_up_process(), it wakes up TASK_INTERRUPTIBLE tasks.

> > @@ -91,7 +105,7 @@ static void create_kthread(struct kthrea
> >
> >  	/* We want our own signal handler (we take no signals by default). */
> >  	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
> > -	create->result = pid;
> > +	create->result = ERR_PTR(pid);
> 
> Ouch.    You have a nasty race here.
> 
> If kthread runs before kernel_thread returns then setting
> "create->result = ERR_PTR(pid);" could easily stomp 
> "create->result = &self".

Yes, thanks... Can't understand how I was soooo stupid!!! thanks...

Damn. We don't need 2 completions! just one.

	create_kthread:

		pid = kernel_thread(...);
		if (pid < 0) {
			create->result = ERR_PTR(pid);
			complete(create->started);
		}
		// else: kthread() will do complete()

		return;

Oleg.

Re: [PATCH 3/3] make kthread_stop() scalable

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/13, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@tv-sign.ru> writes:
>> 
>> > It's a shame kthread_stop() (may take a while!) runs with a global semaphore
>> > held. With this patch kthread() allocates all neccesary data (struct
> kthread)
>> > on its own stack, globals kthread_stop_xxx are deleted.
>> 
>> Oleg so fare you patches  have been inspiring.  However..
>> 
>> > HACKS:
>> >
>> > 	- re-use task_struct->set_child_tid to point to "struct kthread"
>> 
>> 	 task_struct->vfork_done is a better cannidate.
>> 
>> > 	- use do_exit() directly to preserve "struct kthread" on stack
>> 
>> Calling do_exit directly like that is not a hack, as it appears the preferred
>> way to exit is to call do_exit, or complete_and_exit.
>> 
>> While this does improve the scalability and remove a global variable.  It
>> also introduces a complex special case in the form of struct kthread.
>
> I can't say I agree. I thought it is good to have a struct which represents
> a kernel thread. Actually, I thought we can have __kthread_create() which
> returns "struct kthread". May be I am wrong, because yes, ->set_child_tid can
> point right to completion, and we can use some TIF flag instead of
> ->should_stop.

> This needs to update a lot of include/asm/ files.

Yes it does.

This is where I was going beyond what you were doing.  I needed a flag to say
that this a kthread that is stopping to test in recalc_sigpending.  To be certain
of terminating interruptible sleeps.  I could not get at your struct kthread
in that case.

If it wasn't for the wait_event_interruptible thing I likely would
have just thrown a union in struct task_struct.

I also got lucky in that vfork_done is designed to point a completion
just where I need it (when a task exits).  The name is now a little
abused but otherwise it does just what I want it to.

>> It also doesn't solve the biggest problem with the current kthread interface
>> in that calling kthread_stop does not cause the code to break out of
>> interruptible sleeps.
>
> Hm? kthread_stop() does wake_up_process(), it wakes up TASK_INTERRUPTIBLE tasks.

Yes. But if they are looping, unless signal_pending is set it is quite possible
they will go back to sleep.

Take for example:

> #define __wait_event_interruptible(wq, condition, ret)		\
> do {									\
> 	DEFINE_WAIT(__wait);						\
> 									\
> 	for (;;) {							\
> 		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
> 		if (condition)						\
> 			break;						\
> 		if (!signal_pending(current)) {				\
> 			schedule();					\
> 			continue;					\
> 		}							\
> 		ret = -ERESTARTSYS;					\
> 		break;							\
> 	}								\
> 	finish_wait(&wq, &__wait);					\
> } while (0)

We don't break out until either condition is true or signal_pending(current)
is true.

Loops that do that are very common in the kernel.  I counted about 500
calls of signal pending in places that otherwise care nothing about signals.
Several kernel threads call into functions that use loops like
wait_event_interruptible.  So I need a more forceful kthread_stop.  If
I don't want to continue to use signals.

>> > @@ -91,7 +105,7 @@ static void create_kthread(struct kthrea
>> >
>> >  	/* We want our own signal handler (we take no signals by default). */
>> >  	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
>> > -	create->result = pid;
>> > +	create->result = ERR_PTR(pid);
>> 
>> Ouch.    You have a nasty race here.
>> 
>> If kthread runs before kernel_thread returns then setting
>> "create->result = ERR_PTR(pid);" could easily stomp 
>> "create->result = &self".
>
> Yes, thanks... Can't understand how I was soooo stupid!!! thanks...
>
> Damn. We don't need 2 completions! just one.

Yep.  My second patch in this last round implements that.

> 	create_kthread:
>
> 		pid = kernel_thread(...);
> 		if (pid < 0) {
> 			create->result = ERR_PTR(pid);
> 			complete(create->started);
> 		}
> 		// else: kthread() will do complete()
>
> 		return;

Eric

Re: [PATCH 3/3] make kthread_stop() scalable

[Oleg Nesterov]

On 04/14, Eric W. Biederman wrote:
>
> This is where I was going beyond what you were doing.  I needed a flag to say
> that this a kthread that is stopping to test in recalc_sigpending.  To be certain
> of terminating interruptible sleeps.  I could not get at your struct kthread
> in that case.
> 
> If it wasn't for the wait_event_interruptible thing I likely would
> have just thrown a union in struct task_struct.
> 
> I also got lucky in that vfork_done is designed to point a completion
> just where I need it (when a task exits).  The name is now a little
> abused but otherwise it does just what I want it to.
> 
> >> It also doesn't solve the biggest problem with the current kthread interface
> >> in that calling kthread_stop does not cause the code to break out of
> >> interruptible sleeps.
> >
> > Hm? kthread_stop() does wake_up_process(), it wakes up TASK_INTERRUPTIBLE tasks.
> 
> Yes. But if they are looping, unless signal_pending is set it is quite possible
> they will go back to sleep.
> 
> Take for example:
> 
> > #define __wait_event_interruptible(wq, condition, ret)		\
> > do {									\
> > 	DEFINE_WAIT(__wait);						\
> > 									\
> > 	for (;;) {							\
> > 		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
> > 		if (condition)						\
> > 			break;						\
> > 		if (!signal_pending(current)) {				\
> > 			schedule();					\
> > 			continue;					\
> > 		}							\
> > 		ret = -ERESTARTSYS;					\
> > 		break;							\
> > 	}								\
> > 	finish_wait(&wq, &__wait);					\
> > } while (0)
> 
> We don't break out until either condition is true or signal_pending(current)
> is true.
> 
> Loops that do that are very common in the kernel.  I counted about 500
> calls of signal pending in places that otherwise care nothing about signals.
> Several kernel threads call into functions that use loops like
> wait_event_interruptible.  So I need a more forceful kthread_stop.  If
> I don't want to continue to use signals.

Yes, I got it reading your next patches. Ok, probably this change is good.
My question was: do we really want to force a kernel thread to exit if it
waits for event in TASK_INTERRUPTIBLE state? probably yes.

> > Yes, thanks... Can't understand how I was soooo stupid!!! thanks...
> >
> > Damn. We don't need 2 completions! just one.
> 
> Yep.  My second patch in this last round implements that.

Yes, I have read it. It is clearly better then mine, and I think correct.

Oleg.

[PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Eric W. Biederman]

This patch reworks kthread_stop so it is more flexible and it causes
the target kthread to abort interruptible sleeps.  Allowing a larger
class of kernel threads to use to the kthread API.

The changes start by defining TIF_KTHREAD_STOP on all architectures.
TIF_KTHREAD_STOP is a per process flag that I can set from another
process to indicate that a kernel thread should stop.

wake_up_process in kthread_stop has been replaced by signal_wake_up
ensuring that the kernel thread if sleeping is woken up in a timely
manner and with TIF_SIGNAL_PENDING set, which causes us to break out
of interruptible sleeps.

recalc_signal_pending was modified to keep TIF_SIGNAL_PENDING set for
as long as TIF_KTHREAD_STOP is set.

Arbitrary paths to do_exit are now allowed.  I have placed a
completion on the thread stack and pointed vfork_done at it, when the
mm_release is called from do_exit the completion will be called.
Since the completion is stored on the stack it is important that
kthread() now calls do_exit ensuring the stack frame that holds the
completion is never released, and so that our exit_code is certain to
make it unchanged all the way to do_exit.

To allow kthread_stop to read the process exit code when exit_mm wakes
it up  I have moved the setting of exit_code to the beginning of
do_exit. 

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/asm-alpha/thread_info.h     |    1 +
 include/asm-arm/thread_info.h       |    1 +
 include/asm-arm26/thread_info.h     |    1 +
 include/asm-avr32/thread_info.h     |    1 +
 include/asm-cris/thread_info.h      |    1 +
 include/asm-frv/thread_info.h       |    1 +
 include/asm-h8300/thread_info.h     |    1 +
 include/asm-i386/thread_info.h      |    1 +
 include/asm-ia64/thread_info.h      |    1 +
 include/asm-m32r/thread_info.h      |    1 +
 include/asm-m68k/thread_info.h      |    1 +
 include/asm-m68knommu/thread_info.h |    1 +
 include/asm-mips/thread_info.h      |    1 +
 include/asm-parisc/thread_info.h    |    1 +
 include/asm-powerpc/thread_info.h   |    1 +
 include/asm-s390/thread_info.h      |    1 +
 include/asm-sh/thread_info.h        |    1 +
 include/asm-sh64/thread_info.h      |    1 +
 include/asm-sparc/thread_info.h     |    1 +
 include/asm-sparc64/thread_info.h   |    1 +
 include/asm-um/thread_info.h        |    1 +
 include/asm-v850/thread_info.h      |    1 +
 include/asm-x86_64/thread_info.h    |    1 +
 include/asm-xtensa/thread_info.h    |    1 +
 include/linux/kthread.h             |   20 ++++++++-
 kernel/exit.c                       |    2 +-
 kernel/kthread.c                    |   81 ++++++++++++-----------------------
 kernel/signal.c                     |    3 +-
 28 files changed, 72 insertions(+), 58 deletions(-)

diff --git a/include/asm-alpha/thread_info.h b/include/asm-alpha/thread_info.h
index 69ffd93..31c41e0 100644
--- a/include/asm-alpha/thread_info.h
+++ b/include/asm-alpha/thread_info.h
@@ -76,6 +76,7 @@ register struct thread_info *__current_thread_info __asm__("$8");
 #define TIF_UAC_NOFIX		7
 #define TIF_UAC_SIGBUS		8
 #define TIF_MEMDIE		9
+#define TIF_KTHREAD_STOP	10
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
diff --git a/include/asm-arm/thread_info.h b/include/asm-arm/thread_info.h
index 5014794..e6db6f1 100644
--- a/include/asm-arm/thread_info.h
+++ b/include/asm-arm/thread_info.h
@@ -148,6 +148,7 @@ extern void iwmmxt_task_switch(struct thread_info *);
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
+#define TIF_KTHREAD_STOP	20
 
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
diff --git a/include/asm-arm26/thread_info.h b/include/asm-arm26/thread_info.h
index 9b367eb..614cc8c 100644
--- a/include/asm-arm26/thread_info.h
+++ b/include/asm-arm26/thread_info.h
@@ -123,6 +123,7 @@ extern void free_thread_info(struct thread_info *);
 #define TIF_USED_FPU		16
 #define TIF_POLLING_NRFLAG	17
 #define TIF_MEMDIE		18
+#define TIF_KTHREAD_STOP	19
 
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
diff --git a/include/asm-avr32/thread_info.h b/include/asm-avr32/thread_info.h
index d1f5b35..2f295de 100644
--- a/include/asm-avr32/thread_info.h
+++ b/include/asm-avr32/thread_info.h
@@ -83,6 +83,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SINGLE_STEP		6	/* single step after next break */
 #define TIF_MEMDIE		7
 #define TIF_RESTORE_SIGMASK	8	/* restore signal mask in do_signal */
+#define TIF_KTHREAD_STOP	9
 #define TIF_USERSPACE		31      /* true if FS sets userspace */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
diff --git a/include/asm-cris/thread_info.h b/include/asm-cris/thread_info.h
index 7ad853c..5076b97 100644
--- a/include/asm-cris/thread_info.h
+++ b/include/asm-cris/thread_info.h
@@ -84,6 +84,7 @@ struct thread_info {
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		17
+#define TIF_KTHREAD_STOP	18
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
diff --git a/include/asm-frv/thread_info.h b/include/asm-frv/thread_info.h
index d881f51..3bb68c0 100644
--- a/include/asm-frv/thread_info.h
+++ b/include/asm-frv/thread_info.h
@@ -117,6 +117,7 @@ register struct thread_info *__current_thread_info asm("gr15");
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		17	/* OOM killer killed process */
 #define TIF_FREEZE		18	/* freezing for suspend */
+#define TIF_KTHREAD_STOP	19
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
diff --git a/include/asm-h8300/thread_info.h b/include/asm-h8300/thread_info.h
index 45f09dc..fda90d3 100644
--- a/include/asm-h8300/thread_info.h
+++ b/include/asm-h8300/thread_info.h
@@ -92,6 +92,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_POLLING_NRFLAG	4	/* true if poll_idle() is polling
 					   TIF_NEED_RESCHED */
 #define TIF_MEMDIE		5
+#define TIF_KTHREAD_STOP	6
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
diff --git a/include/asm-i386/thread_info.h b/include/asm-i386/thread_info.h
index 4b187bb..b823054 100644
--- a/include/asm-i386/thread_info.h
+++ b/include/asm-i386/thread_info.h
@@ -135,6 +135,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_DEBUG		17	/* uses debug registers */
 #define TIF_IO_BITMAP		18	/* uses I/O bitmap */
 #define TIF_FREEZE		19	/* is freezing for suspend */
+#define TIF_KTHREAD_STOP	20
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
diff --git a/include/asm-ia64/thread_info.h b/include/asm-ia64/thread_info.h
index 9169859..2a5391b 100644
--- a/include/asm-ia64/thread_info.h
+++ b/include/asm-ia64/thread_info.h
@@ -90,6 +90,7 @@ struct thread_info {
 #define TIF_MCA_INIT		18	/* this task is processing MCA or INIT */
 #define TIF_DB_DISABLED		19	/* debug trap disabled for fsyscall */
 #define TIF_FREEZE		20	/* is freezing for suspend */
+#define TIF_KTHREAD_STOP	21
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
diff --git a/include/asm-m32r/thread_info.h b/include/asm-m32r/thread_info.h
index 22aff32..6f06f93 100644
--- a/include/asm-m32r/thread_info.h
+++ b/include/asm-m32r/thread_info.h
@@ -154,6 +154,7 @@ static inline unsigned int get_thread_fault_code(void)
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 					/* 31..28 fault code */
 #define TIF_MEMDIE		17
+#define TIF_KTHREAD_STOP	18
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
diff --git a/include/asm-m68k/thread_info.h b/include/asm-m68k/thread_info.h
index c4d622a..3c9d2f2 100644
--- a/include/asm-m68k/thread_info.h
+++ b/include/asm-m68k/thread_info.h
@@ -58,5 +58,6 @@ struct thread_info {
 #define TIF_DELAYED_TRACE	14	/* single step a syscall */
 #define TIF_SYSCALL_TRACE	15	/* syscall trace active */
 #define TIF_MEMDIE		16
+#define TIF_KTHREAD_STOP	17
 
 #endif	/* _ASM_M68K_THREAD_INFO_H */
diff --git a/include/asm-m68knommu/thread_info.h b/include/asm-m68knommu/thread_info.h
index b8f009e..95f44c6 100644
--- a/include/asm-m68knommu/thread_info.h
+++ b/include/asm-m68knommu/thread_info.h
@@ -89,6 +89,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_POLLING_NRFLAG	4	/* true if poll_idle() is polling
 					   TIF_NEED_RESCHED */
 #define TIF_MEMDIE		5
+#define TIF_KTHREAD_STOP	6
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
diff --git a/include/asm-mips/thread_info.h b/include/asm-mips/thread_info.h
index 6cf05f4..9c425bf 100644
--- a/include/asm-mips/thread_info.h
+++ b/include/asm-mips/thread_info.h
@@ -120,6 +120,7 @@ register struct thread_info *__current_thread_info __asm__("$28");
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
 #define TIF_ALLOW_FP_IN_KERNEL	20
+#define TIF_KTHREAD_STOP	21
 #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
diff --git a/include/asm-parisc/thread_info.h b/include/asm-parisc/thread_info.h
index 949314c..44706a5 100644
--- a/include/asm-parisc/thread_info.h
+++ b/include/asm-parisc/thread_info.h
@@ -63,6 +63,7 @@ struct thread_info {
 #define TIF_32BIT               5       /* 32 bit binary */
 #define TIF_MEMDIE		6
 #define TIF_RESTORE_SIGMASK	7	/* restore saved signal mask */
+#define TIF_KTHREAD_STOP	8
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
diff --git a/include/asm-powerpc/thread_info.h b/include/asm-powerpc/thread_info.h
index 3f32ca8..6e20084 100644
--- a/include/asm-powerpc/thread_info.h
+++ b/include/asm-powerpc/thread_info.h
@@ -123,6 +123,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NOERROR		14	/* Force successful syscall return */
 #define TIF_RESTORE_SIGMASK	15	/* Restore signal mask in do_signal */
 #define TIF_FREEZE		16	/* Freezing for suspend */
+#define TIF_KTHREAD_STOP	17
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
diff --git a/include/asm-s390/thread_info.h b/include/asm-s390/thread_info.h
index 0a51891..f345e1f 100644
--- a/include/asm-s390/thread_info.h
+++ b/include/asm-s390/thread_info.h
@@ -101,6 +101,7 @@ static inline struct thread_info *current_thread_info(void)
 					   TIF_NEED_RESCHED */
 #define TIF_31BIT		18	/* 32bit process */ 
 #define TIF_MEMDIE		19
+#define TIF_KTHREAD_STOP	20
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
diff --git a/include/asm-sh/thread_info.h b/include/asm-sh/thread_info.h
index 31d55e3..8fb5d52 100644
--- a/include/asm-sh/thread_info.h
+++ b/include/asm-sh/thread_info.h
@@ -116,6 +116,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_POLLING_NRFLAG	17	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
+#define TIF_KTHREAD_STOP	20
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
diff --git a/include/asm-sh64/thread_info.h b/include/asm-sh64/thread_info.h
index 1f825cb..cd7f7c8 100644
--- a/include/asm-sh64/thread_info.h
+++ b/include/asm-sh64/thread_info.h
@@ -78,6 +78,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
 #define TIF_MEMDIE		4
+#define TIF_KTHREAD_STOP	5
 
 
 #endif /* __KERNEL__ */
diff --git a/include/asm-sparc/thread_info.h b/include/asm-sparc/thread_info.h
index 91b9f58..594bce5 100644
--- a/include/asm-sparc/thread_info.h
+++ b/include/asm-sparc/thread_info.h
@@ -137,6 +137,7 @@ BTFIXUPDEF_CALL(void, free_thread_info, struct thread_info *)
 #define TIF_POLLING_NRFLAG	9	/* true if poll_idle() is polling
 					 * TIF_NEED_RESCHED */
 #define TIF_MEMDIE		10
+#define TIF_KTHREAD_STOP	11
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
diff --git a/include/asm-sparc64/thread_info.h b/include/asm-sparc64/thread_info.h
index 2ebf7f2..3a5178d 100644
--- a/include/asm-sparc64/thread_info.h
+++ b/include/asm-sparc64/thread_info.h
@@ -236,6 +236,7 @@ register struct thread_info *current_thread_info_reg asm("g6");
 #define TIF_ABI_PENDING		12
 #define TIF_MEMDIE		13
 #define TIF_POLLING_NRFLAG	14
+#define TIF_KTHREAD_STOP	15
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_SIGPENDING		(1<<TIF_SIGPENDING)
diff --git a/include/asm-um/thread_info.h b/include/asm-um/thread_info.h
index 261e2f4..f10a6ca 100644
--- a/include/asm-um/thread_info.h
+++ b/include/asm-um/thread_info.h
@@ -69,6 +69,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_MEMDIE	 	5
 #define TIF_SYSCALL_AUDIT	6
 #define TIF_RESTORE_SIGMASK	7
+#define TIF_KTHREAD_STOP	8
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
diff --git a/include/asm-v850/thread_info.h b/include/asm-v850/thread_info.h
index 82b8f28..98d5922 100644
--- a/include/asm-v850/thread_info.h
+++ b/include/asm-v850/thread_info.h
@@ -83,6 +83,7 @@ struct thread_info {
 #define TIF_POLLING_NRFLAG	4	/* true if poll_idle() is polling
 					   TIF_NEED_RESCHED */
 #define TIF_MEMDIE		5
+#define TIF_KTHREAD_STOP	6
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
diff --git a/include/asm-x86_64/thread_info.h b/include/asm-x86_64/thread_info.h
index 74a6c74..9ad6545 100644
--- a/include/asm-x86_64/thread_info.h
+++ b/include/asm-x86_64/thread_info.h
@@ -123,6 +123,7 @@ static inline struct thread_info *stack_thread_info(void)
 #define TIF_DEBUG		21	/* uses debug registers */
 #define TIF_IO_BITMAP		22	/* uses I/O bitmap */
 #define TIF_FREEZE		23	/* is freezing for suspend */
+#define TIF_KTHREAD_STOP	24
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
diff --git a/include/asm-xtensa/thread_info.h b/include/asm-xtensa/thread_info.h
index 5ae34ab..facf5cd 100644
--- a/include/asm-xtensa/thread_info.h
+++ b/include/asm-xtensa/thread_info.h
@@ -117,6 +117,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_IRET		5	/* return with iret */
 #define TIF_MEMDIE		6
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
+#define TIF_KTHREAD_STOP	17
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 00dd957..a8ea31d 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -10,7 +10,7 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 
 /**
  * kthread_run - create and wake a thread.
- * @threadfn: the function to run until signal_pending(current).
+ * @threadfn: the function to run until kthread_should_stop().
  * @data: data ptr for @threadfn.
  * @namefmt: printf-style name for the thread.
  *
@@ -28,7 +28,23 @@ 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);
-int kthread_should_stop(void);
+
+static inline int __kthread_should_stop(struct task_struct *tsk)
+{
+	return test_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
+}
+
+/**
+ * kthread_should_stop - should this kthread return now?
+ *
+ * When someone calls kthread_stop() on your kthread, it will be woken
+ * and this will return true.  You should then return, and your return
+ * value will be passed through to kthread_stop().
+ */
+static inline int kthread_should_stop(void)
+{
+	return __kthread_should_stop(current);
+}
 
 int kthreadd(void *unused);
 extern struct task_struct *kthreadd_task;
diff --git a/kernel/exit.c b/kernel/exit.c
index 8024e79..3dad696 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -891,6 +891,7 @@ fastcall NORET_TYPE void do_exit(long code)
 	}
 
 	tsk->flags |= PF_EXITING;
+	tsk->exit_code = code;
 
 	if (unlikely(in_atomic()))
 		printk(KERN_INFO "note: %s[%d] exited with preempt_count %d\n",
@@ -937,7 +938,6 @@ fastcall NORET_TYPE void do_exit(long code)
 	if (tsk->binfmt)
 		module_put(tsk->binfmt->module);
 
-	tsk->exit_code = code;
 	proc_exit_connector(tsk);
 	exit_task_namespaces(tsk);
 	exit_notify(tsk);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index df8a8e8..06c7e51 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -33,38 +33,18 @@ struct kthread_create_info
 	struct list_head list;
 };
 
-struct kthread_stop_info
-{
-	struct task_struct *k;
-	int err;
-	struct completion done;
-};
-
-/* Thread stopping is done by setthing this var: lock serializes
- * multiple kthread_stop calls. */
-static DEFINE_MUTEX(kthread_stop_lock);
-static struct kthread_stop_info kthread_stop_info;
-
-/**
- * kthread_should_stop - should this kthread return now?
- *
- * When someone calls kthread_stop() on your kthread, it will be woken
- * and this will return true.  You should then return, and your return
- * value will be passed through to kthread_stop().
- */
-int kthread_should_stop(void)
-{
-	return (kthread_stop_info.k == current);
-}
-EXPORT_SYMBOL(kthread_should_stop);
-
 static int kthread(void *_create)
 {
 	struct kthread_create_info *create = _create;
+	struct completion done;
 	int (*threadfn)(void *data);
 	void *data;
 	int ret = -EINTR;
 
+	/* Setup a completion on this thread's stack */
+	init_completion(&done);
+	current->vfork_done = &done;
+
 	/* Copy data: it's on kthread's stack */
 	threadfn = create->threadfn;
 	data = create->data;
@@ -77,12 +57,7 @@ static int kthread(void *_create)
 	if (!kthread_should_stop())
 		ret = threadfn(data);
 
-	/* It might have exited on its own, w/o kthread_stop.  Check. */
-	if (kthread_should_stop()) {
-		kthread_stop_info.err = ret;
-		complete(&kthread_stop_info.done);
-	}
-	return 0;
+	do_exit(ret);
 }
 
 static void create_kthread(struct kthread_create_info *create)
@@ -104,7 +79,7 @@ static void create_kthread(struct kthread_create_info *create)
 
 /**
  * kthread_create - create a kthread.
- * @threadfn: the function to run until signal_pending(current).
+ * @threadfn: the function to run until kthread_should_stop().
  * @data: data ptr for @threadfn.
  * @namefmt: printf-style name for the thread.
  *
@@ -174,38 +149,36 @@ EXPORT_SYMBOL(kthread_bind);
  * kthread_stop - stop a thread created by kthread_create().
  * @k: thread created by kthread_create().
  *
- * Sets kthread_should_stop() for @k to return true, wakes it, 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().
+ * Sets TIF_KTHREAD_STOP for @k, wakes it and waits for it to exit.
+ *
+ * kthread_stop should be called exactly once on @k.
+ *
+ * This can also be called after kthread_create() instead of calling
+ * wake_up_process(): the thread  will exit without calling threadfn().
+ *
+ * If your threadfn may terminate before you call kthread_stop
+ * you should call get_task_struct on @k so it is valid when you call
+ * kthread_stop.
  *
  * Returns the result of threadfn(), or %-EINTR if wake_up_process()
  * was never called.
  */
-int kthread_stop(struct task_struct *k)
+int kthread_stop(struct task_struct *tsk)
 {
 	int ret;
 
-	mutex_lock(&kthread_stop_lock);
-
-	/* It could exit after stop_info.k set, but before wake_up_process. */
-	get_task_struct(k);
+	/* Ensure the task struct persists until I read the exit code. */
+	get_task_struct(tsk);
 
-	/* Must init completion *before* thread sees kthread_stop_info.k */
-	init_completion(&kthread_stop_info.done);
-	smp_wmb();
+	set_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
+	spin_lock_irq(&tsk->sighand->siglock);
+	signal_wake_up(tsk, 1);
+	spin_unlock_irq(&tsk->sighand->siglock);
 
-	/* Now set kthread_should_stop() to true, and wake it up. */
-	kthread_stop_info.k = k;
-	wake_up_process(k);
-	put_task_struct(k);
+	wait_for_completion(tsk->vfork_done);
+	ret = tsk->exit_code;
 
-	/* Once it dies, reset stop ptr, gather result and we're done. */
-	wait_for_completion(&kthread_stop_info.done);
-	kthread_stop_info.k = NULL;
-	ret = kthread_stop_info.err;
-	mutex_unlock(&kthread_stop_lock);
+	put_task_struct(tsk);
 
 	return ret;
 }
diff --git a/kernel/signal.c b/kernel/signal.c
index 2d437f6..fca2a45 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -26,6 +26,7 @@
 #include <linux/freezer.h>
 #include <linux/pid_namespace.h>
 #include <linux/nsproxy.h>
+#include <linux/kthread.h>
 
 #include <asm/param.h>
 #include <asm/uaccess.h>
@@ -218,7 +219,7 @@ static inline int has_pending_signals(sigset_t *signal, sigset_t *blocked)
 fastcall void recalc_sigpending_tsk(struct task_struct *t)
 {
 	if (t->signal->group_stop_count > 0 ||
-	    (freezing(t)) ||
+	    (freezing(t)) || __kthread_should_stop(t) ||
 	    PENDING(&t->pending, &t->blocked) ||
 	    PENDING(&t->signal->shared_pending, &t->blocked))
 		set_tsk_thread_flag(t, TIF_SIGPENDING);
-- 
1.5.0.g53756

[PATCH] kthread: Simplify kthread_create.

[Eric W. Biederman]

This removes an unneeded completion from kthread_create and moves
wake_up_process out of the kthread_create_lock making it clear that
wake_up_process doesn't need the protection of the
kthread_create_lock.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 kernel/kthread.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 06c7e51..cd89b49 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -24,7 +24,6 @@ struct kthread_create_info
 	/* Information passed to kthread() from kthreadd. */
 	int (*threadfn)(void *data);
 	void *data;
-	struct completion started;
 
 	/* Result passed back to kthread_create() from kthreadd. */
 	struct task_struct *result;
@@ -41,6 +40,9 @@ static int kthread(void *_create)
 	void *data;
 	int ret = -EINTR;
 
+	/* Report which task_struct I am */
+	create->result = current;
+
 	/* Setup a completion on this thread's stack */
 	init_completion(&done);
 	current->vfork_done = &done;
@@ -51,7 +53,7 @@ static int kthread(void *_create)
 
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_INTERRUPTIBLE);
-	complete(&create->started);
+	complete(&create->done);
 	schedule();
 
 	if (!kthread_should_stop())
@@ -68,13 +70,8 @@ static void create_kthread(struct kthread_create_info *create)
 	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
 	if (pid < 0) {
 		create->result = ERR_PTR(pid);
-	} else {
-		wait_for_completion(&create->started);
-		read_lock(&tasklist_lock);
-		create->result = find_task_by_pid(pid);
-		read_unlock(&tasklist_lock);
+		complete(&create->done);
 	}
-	complete(&create->done);
 }
 
 /**
@@ -105,14 +102,13 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 
 	create.threadfn = threadfn;
 	create.data = data;
-	init_completion(&create.started);
 	init_completion(&create.done);
 
 	spin_lock(&kthread_create_lock);
 	list_add_tail(&create.list, &kthread_create_list);
-	wake_up_process(kthreadd_task);
 	spin_unlock(&kthread_create_lock);
 
+	wake_up_process(kthreadd_task);
 	wait_for_completion(&create.done);
 
 	if (!IS_ERR(create.result)) {
-- 
1.5.0.g53756

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Oleg Nesterov]

On 04/13, Eric W. Biederman wrote:
>
> +static inline int __kthread_should_stop(struct task_struct *tsk)
> +{
> +	return test_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
> +}

Am I blind? Where does copy_process/dup_task_struct clears unwanted
flags in thread_info->flags ?

> +int kthread_stop(struct task_struct *tsk)
>  {
>  	int ret;
>  
> -	mutex_lock(&kthread_stop_lock);
> -
> -	/* It could exit after stop_info.k set, but before wake_up_process. */
> -	get_task_struct(k);
> +	/* Ensure the task struct persists until I read the exit code. */
> +	get_task_struct(tsk);
>  
> -	/* Must init completion *before* thread sees kthread_stop_info.k */
> -	init_completion(&kthread_stop_info.done);
> -	smp_wmb();
> +	set_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
> +	spin_lock_irq(&tsk->sighand->siglock);
> +	signal_wake_up(tsk, 1);
> +	spin_unlock_irq(&tsk->sighand->siglock);

Off-topic again, the comment above signal_wake_up() is very confusing...
I think the only reason for ->siglock is to prevent the case when
TIF_SIGPENDING may be cleared by recalc_sigpending(). Or something else?

This changes the current API, kthread_stop() doesn't wake up UNINTERRUPTIBLE
tasks any longer. I'd say this is good, but may break things ... For example,
kthred_stop(kthread_create(...)) can't work now.

> -	/* Now set kthread_should_stop() to true, and wake it up. */
> -	kthread_stop_info.k = k;
> -	wake_up_process(k);
> -	put_task_struct(k);
> +	wait_for_completion(tsk->vfork_done);
> +	ret = tsk->exit_code;

This is really good. Now the kernel thread can exit() at any point without
fear to break kthread_stop().

> @@ -218,7 +219,7 @@ static inline int has_pending_signals(sigset_t *signal, sigset_t *blocked)
>  fastcall void recalc_sigpending_tsk(struct task_struct *t)
>  {
>  	if (t->signal->group_stop_count > 0 ||
> -	    (freezing(t)) ||
> +	    (freezing(t)) || __kthread_should_stop(t) ||
>  	    PENDING(&t->pending, &t->blocked) ||
>  	    PENDING(&t->signal->shared_pending, &t->blocked))
>  		set_tsk_thread_flag(t, TIF_SIGPENDING);

Aha, now I understand your point about interruptible sleeps (the previous
message). What is the reason for this change?

Oleg.

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/13, Eric W. Biederman wrote:
>>
>> +static inline int __kthread_should_stop(struct task_struct *tsk)
>> +{
>> +	return test_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
>> +}
>
> Am I blind? Where does copy_process/dup_task_struct clears unwanted
> flags in thread_info->flags ?

Good question.  It is only a real problem if someone forks a kernel
thread after we ask it to die but, it does appear to be an issue.
With this usage and the same usage by the process freezer.

We do have these lines in copy_process...

	clear_tsk_thread_flag(p, TIF_SIGPENDING);
	init_sigpending(&p->pending);

I don't know what we want to do about TIF_KTHREAD_STOP and TIF_FREEZE.

Right now we will go allow our merry way until we hit:

	recalc_sigpending();
	if (signal_pending(current)) {
		spin_unlock(&current->sighand->siglock);
		write_unlock_irq(&tasklist_lock);
		retval = -ERESTARTNOINTR;
		goto bad_fork_cleanup_namespaces;
	}

And copy_process will fail.  Since that is an expected failure point
that actually seems like reasonable behavior in this case if you
are being frozen or are being told to die you can't fork.

It does ensure that these additional kernel flags won't make it
onto new instances of struct task_struct.  Which is the important
thing from a correctness standpoint.


>> +int kthread_stop(struct task_struct *tsk)
>>  {
>>  	int ret;
>>  
>> -	mutex_lock(&kthread_stop_lock);
>> -
>> -	/* It could exit after stop_info.k set, but before wake_up_process. */
>> -	get_task_struct(k);
>> +	/* Ensure the task struct persists until I read the exit code. */
>> +	get_task_struct(tsk);
>>  
>> -	/* Must init completion *before* thread sees kthread_stop_info.k */
>> -	init_completion(&kthread_stop_info.done);
>> -	smp_wmb();
>> +	set_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
>> +	spin_lock_irq(&tsk->sighand->siglock);
>> +	signal_wake_up(tsk, 1);
>> +	spin_unlock_irq(&tsk->sighand->siglock);
>
> Off-topic again, the comment above signal_wake_up() is very confusing...
> I think the only reason for ->siglock is to prevent the case when
> TIF_SIGPENDING may be cleared by recalc_sigpending(). Or something else?

I'm really not certain.  I just looked and saw that every user of
signal_pending had the siglock, so I didn't delve any deeper.

> This changes the current API, kthread_stop() doesn't wake up UNINTERRUPTIBLE
> tasks any longer. I'd say this is good, but may break things ... For example,
> kthred_stop(kthread_create(...)) can't work now.

Ugh. Good point.  I haven't picked up the UNINTERRUPTIBLE change yet.
I hadn't realized this exceeded the wait for the completion.  Which
it obviously does, ugh.  

I don't know what to do about the theoretical freezer race, for now I
am inclined to ignore it.  At least until someone audits all of the kernel
threads to be certain an API change is ok.

There is also Andrews general objection that we should use UNINTERRUPTIBLE
sleeps very sparingly because it contributes to load average and such.

>> -	/* Now set kthread_should_stop() to true, and wake it up. */
>> -	kthread_stop_info.k = k;
>> -	wake_up_process(k);
>> -	put_task_struct(k);
>> +	wait_for_completion(tsk->vfork_done);
>> +	ret = tsk->exit_code;
>
> This is really good. Now the kernel thread can exit() at any point without
> fear to break kthread_stop().

Yes.

>> @@ -218,7 +219,7 @@ static inline int has_pending_signals(sigset_t *signal,
> sigset_t *blocked)
>>  fastcall void recalc_sigpending_tsk(struct task_struct *t)
>>  {
>>  	if (t->signal->group_stop_count > 0 ||
>> -	    (freezing(t)) ||
>> +	    (freezing(t)) || __kthread_should_stop(t) ||
>>  	    PENDING(&t->pending, &t->blocked) ||
>>  	    PENDING(&t->signal->shared_pending, &t->blocked))
>>  		set_tsk_thread_flag(t, TIF_SIGPENDING);
>
> Aha, now I understand your point about interruptible sleeps (the previous
> message). What is the reason for this change?

This bit is so that TIF_SIGPENDING does not get cleared on us.

Eric

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Oleg Nesterov]

On 04/14, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > On 04/13, Eric W. Biederman wrote:
> >>
> >> +static inline int __kthread_should_stop(struct task_struct *tsk)
> >> +{
> >> +	return test_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
> >> +}
> >
> > Am I blind? Where does copy_process/dup_task_struct clears unwanted
> > flags in thread_info->flags ?
> 
> Good question.  It is only a real problem if someone forks a kernel
> thread after we ask it to die but, it does appear to be an issue.
> With this usage and the same usage by the process freezer.
> 
> We do have these lines in copy_process...
> 
> 	clear_tsk_thread_flag(p, TIF_SIGPENDING);
> 	init_sigpending(&p->pending);
> 
> I don't know what we want to do about TIF_KTHREAD_STOP and TIF_FREEZE.

Perhaps we need _TIF_CLEAR_ON_FORK_MASK. Probably doesn't matter right
now, but still it is not imho safe in general.

> Right now we will go allow our merry way until we hit:
> 
> 	recalc_sigpending();
> 	if (signal_pending(current)) {
> 		spin_unlock(&current->sighand->siglock);
> 		write_unlock_irq(&tasklist_lock);
> 		retval = -ERESTARTNOINTR;
> 		goto bad_fork_cleanup_namespaces;
> 	}
> 
> And copy_process will fail.  Since that is an expected failure point
> that actually seems like reasonable behavior in this case if you
> are being frozen or are being told to die you can't fork.
> 
> It does ensure that these additional kernel flags won't make it
> onto new instances of struct task_struct.  Which is the important
> thing from a correctness standpoint.

Note that we set TIF_FREEZE and TIF_KTHREAD_STOP outside of ->siglock,
so both flags can leak onto the child. Again, not a problem right now.
TIF_KTHREAD_STOP doesn't matter unless process was created vi kthread_create(),
but in that case it can't inherit TIF_KTHREAD_STOP.

Oleg.

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Andrew Morton]

On Fri, 13 Apr 2007 21:13:13 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:

> This patch reworks kthread_stop so it is more flexible and it causes
> the target kthread to abort interruptible sleeps.  Allowing a larger
> class of kernel threads to use to the kthread API.
> 
> The changes start by defining TIF_KTHREAD_STOP on all architectures.
> TIF_KTHREAD_STOP is a per process flag that I can set from another
> process to indicate that a kernel thread should stop.
> 
> wake_up_process in kthread_stop has been replaced by signal_wake_up
> ensuring that the kernel thread if sleeping is woken up in a timely
> manner and with TIF_SIGNAL_PENDING set, which causes us to break out
> of interruptible sleeps.
> 
> recalc_signal_pending was modified to keep TIF_SIGNAL_PENDING set for
> as long as TIF_KTHREAD_STOP is set.
> 
> Arbitrary paths to do_exit are now allowed.  I have placed a
> completion on the thread stack and pointed vfork_done at it, when the
> mm_release is called from do_exit the completion will be called.
> Since the completion is stored on the stack it is important that
> kthread() now calls do_exit ensuring the stack frame that holds the
> completion is never released, and so that our exit_code is certain to
> make it unchanged all the way to do_exit.
> 
> To allow kthread_stop to read the process exit code when exit_mm wakes
> it up  I have moved the setting of exit_code to the beginning of
> do_exit. 

This patch causes this oops: http://userweb.kernel.org/~akpm/s5000508.jpg
with this config: http://userweb.kernel.org/~akpm/config-x.txt

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Fri, 13 Apr 2007 21:13:13 -0600 ebiederm@xmission.com (Eric W. Biederman)
> wrote:
>
>> This patch reworks kthread_stop so it is more flexible and it causes
>> the target kthread to abort interruptible sleeps.  Allowing a larger
>> class of kernel threads to use to the kthread API.
>> 
>> The changes start by defining TIF_KTHREAD_STOP on all architectures.
>> TIF_KTHREAD_STOP is a per process flag that I can set from another
>> process to indicate that a kernel thread should stop.
>> 
>> wake_up_process in kthread_stop has been replaced by signal_wake_up
>> ensuring that the kernel thread if sleeping is woken up in a timely
>> manner and with TIF_SIGNAL_PENDING set, which causes us to break out
>> of interruptible sleeps.
>> 
>> recalc_signal_pending was modified to keep TIF_SIGNAL_PENDING set for
>> as long as TIF_KTHREAD_STOP is set.
>> 
>> Arbitrary paths to do_exit are now allowed.  I have placed a
>> completion on the thread stack and pointed vfork_done at it, when the
>> mm_release is called from do_exit the completion will be called.
>> Since the completion is stored on the stack it is important that
>> kthread() now calls do_exit ensuring the stack frame that holds the
>> completion is never released, and so that our exit_code is certain to
>> make it unchanged all the way to do_exit.
>> 
>> To allow kthread_stop to read the process exit code when exit_mm wakes
>> it up  I have moved the setting of exit_code to the beginning of
>> do_exit. 
>
> This patch causes this oops: http://userweb.kernel.org/~akpm/s5000508.jpg
> with this config: http://userweb.kernel.org/~akpm/config-x.txt

Thanks.  If I am reading the oops properly this happened during bootup and
vfork_done was set to NULL?

The NULL vfork_done is really weird as exec is the only thing that sets
vfork_done to NULL.

Either I've got a stupid bug in there somewhere or we have just found
the weirdest memory stomp.  I will take a look and see if I can reproduce
this shortly.

Eric

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Andrew Morton]

On Tue, 24 Apr 2007 04:30:22 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > On Fri, 13 Apr 2007 21:13:13 -0600 ebiederm@xmission.com (Eric W. Biederman)
> > wrote:
> >
> >> This patch reworks kthread_stop so it is more flexible and it causes
> >> the target kthread to abort interruptible sleeps.  Allowing a larger
> >> class of kernel threads to use to the kthread API.
> >> 
> >> The changes start by defining TIF_KTHREAD_STOP on all architectures.
> >> TIF_KTHREAD_STOP is a per process flag that I can set from another
> >> process to indicate that a kernel thread should stop.
> >> 
> >> wake_up_process in kthread_stop has been replaced by signal_wake_up
> >> ensuring that the kernel thread if sleeping is woken up in a timely
> >> manner and with TIF_SIGNAL_PENDING set, which causes us to break out
> >> of interruptible sleeps.
> >> 
> >> recalc_signal_pending was modified to keep TIF_SIGNAL_PENDING set for
> >> as long as TIF_KTHREAD_STOP is set.
> >> 
> >> Arbitrary paths to do_exit are now allowed.  I have placed a
> >> completion on the thread stack and pointed vfork_done at it, when the
> >> mm_release is called from do_exit the completion will be called.
> >> Since the completion is stored on the stack it is important that
> >> kthread() now calls do_exit ensuring the stack frame that holds the
> >> completion is never released, and so that our exit_code is certain to
> >> make it unchanged all the way to do_exit.
> >> 
> >> To allow kthread_stop to read the process exit code when exit_mm wakes
> >> it up  I have moved the setting of exit_code to the beginning of
> >> do_exit. 
> >
> > This patch causes this oops: http://userweb.kernel.org/~akpm/s5000508.jpg
> > with this config: http://userweb.kernel.org/~akpm/config-x.txt
> 
> Thanks.  If I am reading the oops properly this happened during bootup and
> vfork_done was set to NULL?

Yes, it was fairly early in boot.  I didn't check what we're oopsing on.

> The NULL vfork_done is really weird as exec is the only thing that sets
> vfork_done to NULL.
> 
> Either I've got a stupid bug in there somewhere or we have just found
> the weirdest memory stomp.  I will take a look and see if I can reproduce
> this shortly.

That was on a Fedora Core 3 machine.  One of those older distros I keep
around to trip people up.

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

>
> Yes, it was fairly early in boot.  I didn't check what we're oopsing on.

Thanks.

It was definitely vfork_done == NULL from what I can read of the Oops and
the code.

> That was on a Fedora Core 3 machine.  One of those older distros I keep
> around to trip people up.

That could be something there.  Hopefully this is independent of compiler.
Otherwise it will suck to track down.

Eric

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Oleg Nesterov]

On 04/24, Eric W. Biederman wrote:
>
> The NULL vfork_done is really weird as exec is the only thing that sets
> vfork_done to NULL.

Hm, mm_release() clears ->vfork_done before complete().

	mm_release:

		struct completion *vfork_done = tsk->vfork_done;

		if (vfork_done) {
			tsk->vfork_done = NULL;
			complete(vfork_done);
		}


	kthread_stop:

		set_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
		signal_wake_up(tsk, 1);

		// tsk exits, sets ->vfork_done == NULL

		wait_for_completion(tsk->vfork_done);


Probably Rafael worried about this, but I misunderstood him...

Oleg.

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Oleg Nesterov]

On 04/24, Oleg Nesterov wrote:
>
> Hm, mm_release() clears ->vfork_done before complete().
> 
> 	mm_release:
> 
> 		struct completion *vfork_done = tsk->vfork_done;
> 
> 		if (vfork_done) {
> 			tsk->vfork_done = NULL;
> 			complete(vfork_done);
> 		}
> 
> 
> 	kthread_stop:
> 
> 		set_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
> 		signal_wake_up(tsk, 1);
> 
> 		// tsk exits, sets ->vfork_done == NULL
> 
> 		wait_for_completion(tsk->vfork_done);

Since the task_struct should be pinned anyway, I think kthread_stop()
should do:

	vfork_done = tsk->vfork_done;
	barrier();
	if (vfork_done)
		wait_for_completion(vfork_done);

Oleg.

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/24, Oleg Nesterov wrote:
>>
>> Hm, mm_release() clears ->vfork_done before complete().

Duh.  Yes somehow I had a blind spot there.  I clearly
need to handle that case.

>> 	mm_release:
>> 
>> 		struct completion *vfork_done = tsk->vfork_done;
>> 
>> 		if (vfork_done) {
>> 			tsk->vfork_done = NULL;
>> 			complete(vfork_done);
>> 		}
>> 
>> 
>> 	kthread_stop:
>> 
>> 		set_tsk_thread_flag(tsk, TIF_KTHREAD_STOP);
>> 		signal_wake_up(tsk, 1);
>> 
>> 		// tsk exits, sets ->vfork_done == NULL
>> 
>> 		wait_for_completion(tsk->vfork_done);
>
> Since the task_struct should be pinned anyway, I think kthread_stop()
> should do:
>
> 	vfork_done = tsk->vfork_done;
> 	barrier();
> 	if (vfork_done)
> 		wait_for_completion(vfork_done);

That should work, and this may explain what is going on. I was trying
to figure out how this could happen but if the thread is running on
another cpu there is a race and it may exit quickly enough to cause us
problems before we get to kthread_stop.

I don't know if this is the problem but it certainly needs to be fixed.

Eric

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Oleg Nesterov]

On 04/24, Eric W. Biederman wrote:
> 
> I don't know if this is the problem but it certainly needs to be fixed.

I guess you will re-submit these patches soon. May I suggest you to put
this

> +	spin_lock_irq(&tsk->sighand->siglock);
> +	signal_wake_up(tsk, 1);
> +	spin_unlock_irq(&tsk->sighand->siglock);

and this

>  fastcall void recalc_sigpending_tsk(struct task_struct *t)
>  {
>  	if (t->signal->group_stop_count > 0 ||
> -	    (freezing(t)) ||
> +	    (freezing(t)) || __kthread_should_stop(t) ||

into the separate patch?

Perhaps I am too paranoid, and most probably this change is good, but
still I'm afraid this very subtle change may break things. In that case
it would be easy to revert that only part (for example for the testing
purposes).

Consider,

	current->flags |= PF_NOFREEZE;

	while (!kthread_should_stop()) {

		begin_something();

		// I am a kernel thread, all signals are ignored.
		// I don't want to contribute to loadavg, so I am
		// waiting for the absoulutely critical event in
		// TASK__INTERRUPTIBLE state.

		if (wait_event_interruptible(condition))
			panic("Impossible!");

		commit_something();
	}

Oleg.

Re: [PATCH] kthread: Enhance kthread_stop to abort interruptible sleeps

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/24, Eric W. Biederman wrote:
>> 
>> I don't know if this is the problem but it certainly needs to be fixed.
>
> I guess you will re-submit these patches soon. May I suggest you to put
> this
>
>> +	spin_lock_irq(&tsk->sighand->siglock);
>> +	signal_wake_up(tsk, 1);
>> +	spin_unlock_irq(&tsk->sighand->siglock);
>
> and this
>
>>  fastcall void recalc_sigpending_tsk(struct task_struct *t)
>>  {
>>  	if (t->signal->group_stop_count > 0 ||
>> -	    (freezing(t)) ||
>> +	    (freezing(t)) || __kthread_should_stop(t) ||
>
> into the separate patch?
>
> Perhaps I am too paranoid, and most probably this change is good, but
> still I'm afraid this very subtle change may break things. In that case
> it would be easy to revert that only part (for example for the testing
> purposes).

It makes sense.  I doubt we are going to run into issues when
we are killing a thread but we certainly could.

Making it easy to test for that scenario would certainly be
a good idea.

> Consider,
>
> 	current->flags |= PF_NOFREEZE;
>
> 	while (!kthread_should_stop()) {
>
> 		begin_something();
>
> 		// I am a kernel thread, all signals are ignored.
> 		// I don't want to contribute to loadavg, so I am
> 		// waiting for the absoulutely critical event in
> 		// TASK__INTERRUPTIBLE state.
>
> 		if (wait_event_interruptible(condition))
> 			panic("Impossible!");
>
> 		commit_something();
> 	}

Of course if it's impossible it is most likely there won't be a check
there so something more subtle will happen.

Eric