[PATCH 2/3] make kernel threads invisible to /sbin/init

[PATCH 2/3] make kernel threads invisible to /sbin/init

[Oleg Nesterov]

1. rename reparent_to_init() to reparent_kthread() and export it

2. use init_pid_ns.child_reaper instead of child_reaper(current)

3. set ->exit_signal = -1, so init can't see us and we don't use
   it to reap the task.

4. add reparent_kthread() to kthread() and stopmachine()

See also

	http://marc.info/?t=117580282200003&r=1
	http://marc.info/?t=95299284800003&r=1

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

 include/linux/sched.h |    1 +
 kernel/exit.c         |   16 ++++++++--------
 kernel/kthread.c      |    1 +
 kernel/stop_machine.c |    1 +
 4 files changed, 11 insertions(+), 8 deletions(-)

--- 2.6.21-rc5/include/linux/sched.h~2_DETACH	2007-04-05 12:18:28.000000000 +0400
+++ 2.6.21-rc5/include/linux/sched.h	2007-04-10 21:52:27.000000000 +0400
@@ -1401,6 +1401,7 @@ extern void exit_itimers(struct signal_s
 
 extern NORET_TYPE void do_group_exit(int);
 
+extern void reparent_kthread(void);
 extern void daemonize(const char *, ...);
 extern int allow_signal(int);
 extern int disallow_signal(int);
--- 2.6.21-rc5/kernel/exit.c~2_DETACH	2007-04-10 21:32:44.000000000 +0400
+++ 2.6.21-rc5/kernel/exit.c	2007-04-10 21:59:41.000000000 +0400
@@ -255,7 +255,7 @@ static int has_stopped_jobs(struct pid *
 }
 
 /**
- * reparent_to_init - Reparent the calling kernel thread to the init task of the pid space that the thread belongs to.
+ * reparent_kthread - Reparent the calling kernel thread to the init task of the pid space that the thread belongs to.
  *
  * If a kernel thread is launched as a result of a system call, or if
  * it ever exits, it should generally reparent itself to init so that
@@ -264,20 +264,20 @@ static int has_stopped_jobs(struct pid *
  * The various task state such as scheduling policy and priority may have
  * been inherited from a user process, so we reset them to sane values here.
  *
- * NOTE that reparent_to_init() gives the caller full capabilities.
+ * NOTE that reparent_kthread() gives the caller full capabilities.
  */
-static void reparent_to_init(void)
+void reparent_kthread(void)
 {
 	write_lock_irq(&tasklist_lock);
 
 	ptrace_unlink(current);
 	remove_parent(current);
-	current->parent = child_reaper(current);
-	current->real_parent = child_reaper(current);
+	current->parent = init_pid_ns.child_reaper;
+	current->real_parent = current->parent;
 	add_parent(current);
 
-	/* Set the exit signal to SIGCHLD so we signal init on exit */
-	current->exit_signal = SIGCHLD;
+	/* make the task auto-reap */
+	current->exit_signal = -1;
 
 	security_task_reparent_to_init(current);
 	write_unlock_irq(&tasklist_lock);
@@ -391,7 +391,7 @@ void daemonize(const char *name, ...)
 	current->files = init_task.files;
 	atomic_inc(&current->files->count);
 
-	reparent_to_init();
+	reparent_kthread();
 
 	if (!has_rt_policy(current) && (task_nice(current) < 0))
 		set_user_nice(current, 0);
--- 2.6.21-rc5/kernel/kthread.c~2_DETACH	2007-04-05 12:18:28.000000000 +0400
+++ 2.6.21-rc5/kernel/kthread.c	2007-04-10 22:02:31.000000000 +0400
@@ -82,6 +82,7 @@ static int kthread(void *_create)
 	sigset_t blocked;
 	int ret = -EINTR;
 
+	reparent_kthread();
 	kthread_exit_files();
 
 	/* Copy data: it's on keventd's stack */
--- 2.6.21-rc5/kernel/stop_machine.c~2_DETACH	2006-10-22 18:24:03.000000000 +0400
+++ 2.6.21-rc5/kernel/stop_machine.c	2007-04-10 22:04:23.000000000 +0400
@@ -33,6 +33,7 @@ static int stopmachine(void *cpu)
 	int irqs_disabled = 0;
 	int prepared = 0;
 
+	reparent_kthread();
 	set_cpus_allowed(current, cpumask_of_cpu((int)(long)cpu));
 
 	/* Ack: we are alive */

Re: [PATCH 2/3] make kernel threads invisible to /sbin/init

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@tv-sign.ru):
> 1. rename reparent_to_init() to reparent_kthread() and export it
> 
> 2. use init_pid_ns.child_reaper instead of child_reaper(current)

Each of these patches looks good to me, but this part in particular
is a must-have bugfix.

Just started some tests, if any failures come back I'll report them
tonight.

thanks Oleg,
-serge

> 3. set ->exit_signal = -1, so init can't see us and we don't use
>    it to reap the task.
> 
> 4. add reparent_kthread() to kthread() and stopmachine()
> 
> See also
> 
> 	http://marc.info/?t=117580282200003&r=1
> 	http://marc.info/?t=95299284800003&r=1
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
>  include/linux/sched.h |    1 +
>  kernel/exit.c         |   16 ++++++++--------
>  kernel/kthread.c      |    1 +
>  kernel/stop_machine.c |    1 +
>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> --- 2.6.21-rc5/include/linux/sched.h~2_DETACH	2007-04-05 12:18:28.000000000 +0400
> +++ 2.6.21-rc5/include/linux/sched.h	2007-04-10 21:52:27.000000000 +0400
> @@ -1401,6 +1401,7 @@ extern void exit_itimers(struct signal_s
>  
>  extern NORET_TYPE void do_group_exit(int);
>  
> +extern void reparent_kthread(void);
>  extern void daemonize(const char *, ...);
>  extern int allow_signal(int);
>  extern int disallow_signal(int);
> --- 2.6.21-rc5/kernel/exit.c~2_DETACH	2007-04-10 21:32:44.000000000 +0400
> +++ 2.6.21-rc5/kernel/exit.c	2007-04-10 21:59:41.000000000 +0400
> @@ -255,7 +255,7 @@ static int has_stopped_jobs(struct pid *
>  }
>  
>  /**
> - * reparent_to_init - Reparent the calling kernel thread to the init task of the pid space that the thread belongs to.
> + * reparent_kthread - Reparent the calling kernel thread to the init task of the pid space that the thread belongs to.
>   *
>   * If a kernel thread is launched as a result of a system call, or if
>   * it ever exits, it should generally reparent itself to init so that
> @@ -264,20 +264,20 @@ static int has_stopped_jobs(struct pid *
>   * The various task state such as scheduling policy and priority may have
>   * been inherited from a user process, so we reset them to sane values here.
>   *
> - * NOTE that reparent_to_init() gives the caller full capabilities.
> + * NOTE that reparent_kthread() gives the caller full capabilities.
>   */
> -static void reparent_to_init(void)
> +void reparent_kthread(void)
>  {
>  	write_lock_irq(&tasklist_lock);
>  
>  	ptrace_unlink(current);
>  	remove_parent(current);
> -	current->parent = child_reaper(current);
> -	current->real_parent = child_reaper(current);
> +	current->parent = init_pid_ns.child_reaper;
> +	current->real_parent = current->parent;
>  	add_parent(current);
>  
> -	/* Set the exit signal to SIGCHLD so we signal init on exit */
> -	current->exit_signal = SIGCHLD;
> +	/* make the task auto-reap */
> +	current->exit_signal = -1;
>  
>  	security_task_reparent_to_init(current);
>  	write_unlock_irq(&tasklist_lock);
> @@ -391,7 +391,7 @@ void daemonize(const char *name, ...)
>  	current->files = init_task.files;
>  	atomic_inc(&current->files->count);
>  
> -	reparent_to_init();
> +	reparent_kthread();
>  
>  	if (!has_rt_policy(current) && (task_nice(current) < 0))
>  		set_user_nice(current, 0);
> --- 2.6.21-rc5/kernel/kthread.c~2_DETACH	2007-04-05 12:18:28.000000000 +0400
> +++ 2.6.21-rc5/kernel/kthread.c	2007-04-10 22:02:31.000000000 +0400
> @@ -82,6 +82,7 @@ static int kthread(void *_create)
>  	sigset_t blocked;
>  	int ret = -EINTR;
>  
> +	reparent_kthread();
>  	kthread_exit_files();
>  
>  	/* Copy data: it's on keventd's stack */
> --- 2.6.21-rc5/kernel/stop_machine.c~2_DETACH	2006-10-22 18:24:03.000000000 +0400
> +++ 2.6.21-rc5/kernel/stop_machine.c	2007-04-10 22:04:23.000000000 +0400
> @@ -33,6 +33,7 @@ static int stopmachine(void *cpu)
>  	int irqs_disabled = 0;
>  	int prepared = 0;
>  
> +	reparent_kthread();
>  	set_cpus_allowed(current, cpumask_of_cpu((int)(long)cpu));
>  
>  	/* Ack: we are alive */

Re: [PATCH 2/3] make kernel threads invisible to /sbin/init

[Eric W. Biederman]

"Serge E. Hallyn" <serge@hallyn.com> writes:

> Quoting Oleg Nesterov (oleg@tv-sign.ru):
>> 1. rename reparent_to_init() to reparent_kthread() and export it
>> 
>> 2. use init_pid_ns.child_reaper instead of child_reaper(current)
>
> Each of these patches looks good to me, but this part in particular
> is a must-have bugfix.

Removing daemonize is a must have bug fix.  This falls short of that so
it is a good fix, but it doesn't solve the core problem that kernel daemons
are being assigned pids inside of child pid namespaces.

It doesn't solve the problem that some kernel daemons are using signals
to communicate with user space.

It doesn't solve the problem that we have to do a lot of massaging and
maintenance to get kernel threads from grabbing references to namespaces
and other kernel pieces they should not be grabbing.

Eric

Re: [PATCH 2/3] make kernel threads invisible to /sbin/init

[Eric W. Biederman]

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

> 1. rename reparent_to_init() to reparent_kthread() and export it
>
> 2. use init_pid_ns.child_reaper instead of child_reaper(current)
>
> 3. set ->exit_signal = -1, so init can't see us and we don't use
>    it to reap the task.
>
> 4. add reparent_kthread() to kthread() and stopmachine()
>

If the goal is to hide from /sbin/init.  We don't need to touch
kernel/kthread.c or 
kernel/stop_machine.c

Their parents are already kernel threads.

Yes. There is a startup issue for kernel/kthread.c I just about have
a patch I can stand that addresses that issue.

For the kernel thread they all inherit signals with SIGCHLD set to
SIG_IGN, so there is child auto reaping in that form.  Adding
the ->exit_signal = -1 would be a bonus but is not required.

I am a little concerned with removing the knowledge of who our
real parent is as that may compromise debugging.  The reason
why kernel threads are visible in the first place.

I do support reparenting kernel threads that call daemonize,
and the modifications to reparent_kthread here look reasonable.

Eric

Re: [PATCH 2/3] make kernel threads invisible to /sbin/init

[Oleg Nesterov]

On 04/10, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > 1. rename reparent_to_init() to reparent_kthread() and export it
> >
> > 2. use init_pid_ns.child_reaper instead of child_reaper(current)
> >
> > 3. set ->exit_signal = -1, so init can't see us and we don't use
> >    it to reap the task.
> >
> > 4. add reparent_kthread() to kthread() and stopmachine()
> >
> 
> If the goal is to hide from /sbin/init.  We don't need to touch
> kernel/kthread.c or 
> kernel/stop_machine.c
> 
> Their parents are already kernel threads.
> 
> For the kernel thread they all inherit signals with SIGCHLD set to
> SIG_IGN, so there is child auto reaping in that form.  Adding
> the ->exit_signal = -1 would be a bonus but is not required.

Unless a kernel thread does kernel_thread() (not kthread_create) and
exits. In that case the child will be re-parented to init which doesn't
ignore SIGCHLD.

Robin Holt wrote:
>
> wait_task_zombie() is taking many seconds to get through the list.
> For the case of a modprobe, stop_machine creates one thread per cpu
> (remember big number). All are parented to init and their exit will
> cause wait_task_zombie to scan multiple times most of the way through
> this very long list looking for threads which need to be reaped.

initially, "stopmachine" threads were not parented to init.

However, I agree, your patch is better, and solves most problems in more
simple way. Including the above problem, I believe. "stopmachine" likely
does exit_notify() and notices SIG_IGN (inherited from kthreadd_setup())
before "do_stop" does forget_original_parent().

Oleg.

[PATCH] kthread: Don't depend on work queues

[Eric W. Biederman]

Currently there is a circular reference between work queue initialization
and kthread initialization.   This prevents the kernel thread
infrastructure from initializing until after work queues have been
initialized.

For kernel threads we want something that is as close as possible to the
init_task and is not contaminated by user processes.  The later we start
our kthreadd that forks the rest of the kernel threads the harder this is
to do and the more of a mess we have to clean up because the defaults have
changed on us.

So this patch modifies the kthread support to not use work queues but to
instead use a simple list of structures, and to have kthreadd start from
init_task immediately after our kernel thread that execs /sbin/init.

By being a true child of init_task we only have to change those process
settings that we want to have different from init_task, such as our
process name, blocking all signals and setting SIGCHLD to SIG_IGN
so that all of our children are reaped automatically.

By being a tre child of init_task we also naturally get our ppid set to 0
and do not wind up as a child of PID == 1.  Ensuring that kernel threads
will not slow down the functioning of the wait family of functions.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/kthread.h |    2 +
 init/main.c             |    2 +
 kernel/kthread.c        |  121 +++++++++++++++++++++++++----------------------
 3 files changed, 69 insertions(+), 56 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 1c65e7a..113bd7c 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -30,4 +30,6 @@ void kthread_bind(struct task_struct *k, unsigned int cpu);
 int kthread_stop(struct task_struct *k);
 int kthread_should_stop(void);
 
+int kthreadd(void *unused);
+
 #endif /* _LINUX_KTHREAD_H */
diff --git a/init/main.c b/init/main.c
index a92989e..12a8aba 100644
--- a/init/main.c
+++ b/init/main.c
@@ -54,6 +54,7 @@
 #include <linux/lockdep.h>
 #include <linux/pid_namespace.h>
 #include <linux/device.h>
+#include <linux/kthread.h>
 
 #include <asm/io.h>
 #include <asm/bugs.h>
@@ -437,6 +438,7 @@ static void noinline rest_init(void)
 {
 	kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
+	kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	unlock_kernel();
 
 	/*
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 87c50cc..8638a04 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -1,7 +1,7 @@
 /* Kernel thread helper functions.
  *   Copyright (C) 2004 IBM Corporation, Rusty Russell.
  *
- * Creation is done via keventd, so that we get a clean environment
+ * Creation is done via kthreadd, so that we get a clean environment
  * even if we're invoked from userspace (think modprobe, hotplug cpu,
  * etc.).
  */
@@ -15,24 +15,22 @@
 #include <linux/mutex.h>
 #include <asm/semaphore.h>
 
-/*
- * We dont want to execute off keventd since it might
- * hold a semaphore our callers hold too:
- */
-static struct workqueue_struct *helper_wq;
+static DEFINE_SPINLOCK(kthread_create_lock);
+static LIST_HEAD(kthread_create_list);
+static DECLARE_WAIT_QUEUE_HEAD(kthread_create_work);
 
 struct kthread_create_info
 {
-	/* Information passed to kthread() from keventd. */
+	/* Information passed to kthread() from kthreadd. */
 	int (*threadfn)(void *data);
 	void *data;
 	struct completion started;
 
-	/* Result passed back to kthread_create() from keventd. */
+	/* Result passed back to kthread_create() from kthreadd. */
 	struct task_struct *result;
 	struct completion done;
 
-	struct work_struct work;
+	struct list_head list;
 };
 
 struct kthread_stop_info
@@ -60,42 +58,17 @@ int kthread_should_stop(void)
 }
 EXPORT_SYMBOL(kthread_should_stop);
 
-static void kthread_exit_files(void)
-{
-	struct fs_struct *fs;
-	struct task_struct *tsk = current;
-
-	exit_fs(tsk);		/* current->fs->count--; */
-	fs = init_task.fs;
-	tsk->fs = fs;
-	atomic_inc(&fs->count);
- 	exit_files(tsk);
-	current->files = init_task.files;
-	atomic_inc(&tsk->files->count);
-}
-
 static int kthread(void *_create)
 {
 	struct kthread_create_info *create = _create;
 	int (*threadfn)(void *data);
 	void *data;
-	sigset_t blocked;
 	int ret = -EINTR;
 
-	kthread_exit_files();
-
-	/* Copy data: it's on keventd's stack */
+	/* Copy data: it's on kthread's stack */
 	threadfn = create->threadfn;
 	data = create->data;
 
-	/* Block and flush all signals (in case we're not from keventd). */
-	sigfillset(&blocked);
-	sigprocmask(SIG_BLOCK, &blocked, NULL);
-	flush_signals(current);
-
-	/* By default we can run anywhere, unlike keventd. */
-	set_cpus_allowed(current, CPU_MASK_ALL);
-
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_INTERRUPTIBLE);
 	complete(&create->started);
@@ -112,11 +85,8 @@ static int kthread(void *_create)
 	return 0;
 }
 
-/* We are keventd: create a thread. */
-static void keventd_create_kthread(struct work_struct *work)
+static void create_kthread(struct kthread_create_info *create)
 {
-	struct kthread_create_info *create =
-		container_of(work, struct kthread_create_info, work);
 	int pid;
 
 	/* We want our own signal handler (we take no signals by default). */
@@ -162,17 +132,14 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 	create.data = data;
 	init_completion(&create.started);
 	init_completion(&create.done);
-	INIT_WORK(&create.work, keventd_create_kthread);
-
-	/*
-	 * The workqueue needs to start up first:
-	 */
-	if (!helper_wq)
-		create.work.func(&create.work);
-	else {
-		queue_work(helper_wq, &create.work);
-		wait_for_completion(&create.done);
-	}
+
+	spin_lock(&kthread_create_lock);
+	list_add_tail(&create.list, &kthread_create_list);
+	wake_up(&kthread_create_work);
+	spin_unlock(&kthread_create_lock);
+
+	wait_for_completion(&create.done);
+
 	if (!IS_ERR(create.result)) {
 		va_list args;
 		va_start(args, namefmt);
@@ -180,7 +147,6 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 			  namefmt, args);
 		va_end(args);
 	}
-
 	return create.result;
 }
 EXPORT_SYMBOL(kthread_create);
@@ -245,12 +211,55 @@ int kthread_stop(struct task_struct *k)
 }
 EXPORT_SYMBOL(kthread_stop);
 
-static __init int helper_init(void)
+
+static __init void kthreadd_setup(void)
 {
-	helper_wq = create_singlethread_workqueue("kthread");
-	BUG_ON(!helper_wq);
+	struct task_struct *tsk = current;
+	struct k_sigaction sa;
+	sigset_t blocked;
 
-	return 0;
+	strcpy(tsk->comm, "kthreadd");
+
+	/* Block and flush all signals */
+	sigfillset(&blocked);
+	sigprocmask(SIG_BLOCK, &blocked, NULL);
+	flush_signals(tsk);
+
+	/* SIG_IGN makes children autoreap: see do_notify_parent(). */
+	sa.sa.sa_handler = SIG_IGN;
+	sa.sa.sa_flags = 0;
+	siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
+	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
+
+	set_user_nice(current, -5);
 }
 
-core_initcall(helper_init);
+int kthreadd(void *unused)
+{
+	/* Setup a clean context for our children to inherit. */
+	kthreadd_setup();
+
+	current->flags |= PF_NOFREEZE;
+
+	for (;;) {
+		wait_event(kthread_create_work,
+			   !list_empty(&kthread_create_list));
+
+		spin_lock(&kthread_create_lock);
+		while (!list_empty(&kthread_create_list)) {
+			struct kthread_create_info *create;
+
+			create = list_entry(kthread_create_list.next,
+					    struct kthread_create_info, list);
+			list_del_init(&create->list);
+			spin_unlock(&kthread_create_lock);
+
+			create_kthread(create);
+
+			spin_lock(&kthread_create_lock);
+		}
+		spin_unlock(&kthread_create_lock);
+	}
+
+	return 0;
+}
-- 
1.5.0.g53756

Re: [PATCH] kthread: Don't depend on work queues

[Andrew Morton]

On Tue, 10 Apr 2007 23:44:36 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:

> Currently there is a circular reference between work queue initialization
> and kthread initialization.   This prevents the kernel thread
> infrastructure from initializing until after work queues have been
> initialized.
> 
> For kernel threads we want something that is as close as possible to the
> init_task and is not contaminated by user processes.  The later we start
> our kthreadd that forks the rest of the kernel threads the harder this is
> to do and the more of a mess we have to clean up because the defaults have
> changed on us.
> 
> So this patch modifies the kthread support to not use work queues but to
> instead use a simple list of structures, and to have kthreadd start from
> init_task immediately after our kernel thread that execs /sbin/init.
> 
> By being a true child of init_task we only have to change those process
> settings that we want to have different from init_task, such as our
> process name, blocking all signals and setting SIGCHLD to SIG_IGN
> so that all of our children are reaped automatically.
> 
> By being a tre child of init_task we also naturally get our ppid set to 0
> and do not wind up as a child of PID == 1.  Ensuring that kernel threads
> will not slow down the functioning of the wait family of functions.

argh.  Your description freely confuddles the terms "kernel thread" and
"kthread".  Can we not do that?  Henceforth the term "kernel thread" refers
to something which was started with kernel_thread() and "kthread" refers to
something which was created by kthread_create(), OK?

Your patch gets midly tangled up with Oleg's recent

reduce-reparent_to_init.patch
make-kernel-threads-invisible-to-sbin-init.patch
reparent-kernel-threads-to-swapper.patch

but they seemed fairly unpopular anyway so I'll drop 'em.

Your wait_event() will contribute to load average, I expect.  We get mail. 
I converted it to wait_event_interruptible().

I guess using PF_NOFREEZE rather than try_to_freeze() is OK, but one
wonders what thinking led to that?

Often when we have a singleton thread like this it is neater to use
wake_up_process() directly on it, rather than creating a rather pointless
waitqueue_head for it.  I started looking into that but it would have taken
more than 30 seconds.

Re: [PATCH] kthread: Don't depend on work queues

[Rafael J. Wysocki]

On Wednesday, 11 April 2007 09:03, Andrew Morton wrote:
> On Tue, 10 Apr 2007 23:44:36 -0600 ebiederm@xmission.com (Eric W. Biederman) wrote:
> 
> > Currently there is a circular reference between work queue initialization
> > and kthread initialization.   This prevents the kernel thread
> > infrastructure from initializing until after work queues have been
> > initialized.
> > 
> > For kernel threads we want something that is as close as possible to the
> > init_task and is not contaminated by user processes.  The later we start
> > our kthreadd that forks the rest of the kernel threads the harder this is
> > to do and the more of a mess we have to clean up because the defaults have
> > changed on us.
> > 
> > So this patch modifies the kthread support to not use work queues but to
> > instead use a simple list of structures, and to have kthreadd start from
> > init_task immediately after our kernel thread that execs /sbin/init.
> > 
> > By being a true child of init_task we only have to change those process
> > settings that we want to have different from init_task, such as our
> > process name, blocking all signals and setting SIGCHLD to SIG_IGN
> > so that all of our children are reaped automatically.
> > 
> > By being a tre child of init_task we also naturally get our ppid set to 0
> > and do not wind up as a child of PID == 1.  Ensuring that kernel threads
> > will not slow down the functioning of the wait family of functions.
> 
> argh.  Your description freely confuddles the terms "kernel thread" and
> "kthread".  Can we not do that?  Henceforth the term "kernel thread" refers
> to something which was started with kernel_thread() and "kthread" refers to
> something which was created by kthread_create(), OK?
> 
> Your patch gets midly tangled up with Oleg's recent
> 
> reduce-reparent_to_init.patch
> make-kernel-threads-invisible-to-sbin-init.patch
> reparent-kernel-threads-to-swapper.patch
> 
> but they seemed fairly unpopular anyway so I'll drop 'em.
> 
> Your wait_event() will contribute to load average, I expect.  We get mail. 
> I converted it to wait_event_interruptible().
> 
> I guess using PF_NOFREEZE rather than try_to_freeze() is OK, but one
> wonders what thinking led to that?

It should be calling try_to_freeze() somewhere anyway.  We may need to freeze
all tasks in some cases.

Greetings,
Rafael

Re: [PATCH] kthread: Don't depend on work queues

[Gautham R Shenoy]

On Wed, Apr 11, 2007 at 12:13:34PM +0200, Rafael J. Wysocki wrote:
> 
> It should be calling try_to_freeze() somewhere anyway.  We may need to freeze
> all tasks in some cases.

How about
for (;;) {
	try_to_freeze();

?

This change allows us to make all the worker threads freezeable by default.
>From cpu-hotplug perspective, helper_wq was the only singlethreaded
non-freezeable workqueue.

> 
> Greetings,
> Rafael
> -

Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [PATCH] kthread: Don't depend on work queues

[Oleg Nesterov]

On 04/11, Gautham R Shenoy wrote:
>
> On Wed, Apr 11, 2007 at 12:13:34PM +0200, Rafael J. Wysocki wrote:
> > 
> > It should be calling try_to_freeze() somewhere anyway.  We may need to freeze
> > all tasks in some cases.
> 
> How about
> for (;;) {
> 	try_to_freeze();
> 
> ?

Why?

> This change allows us to make all the worker threads freezeable by default.
> >From cpu-hotplug perspective, helper_wq was the only singlethreaded
> non-freezeable workqueue.

I think Eric's patch is what you need. We should _not_ freeze kthreadd(), we
need kthread_create() after freezing. Now it doesn't depend on workqueues, we
can freeze them all, single-thread or not.

I like this patch.

Oleg.

Re: [PATCH] kthread: Don't depend on work queues

[Gautham R Shenoy]

On Wed, Apr 11, 2007 at 03:48:05PM +0400, Oleg Nesterov wrote:
> On 04/11, Gautham R Shenoy wrote:
> >
> > On Wed, Apr 11, 2007 at 12:13:34PM +0200, Rafael J. Wysocki wrote:
> > > 
> > > It should be calling try_to_freeze() somewhere anyway.  We may need to freeze
> > > all tasks in some cases.
> > 
> > How about
> > for (;;) {
> > 	try_to_freeze();
> > 
> > ?
> 
> Why?

If some event (defintely NOT cpu hotplug) needs this thread frozen.

> 
> > This change allows us to make all the worker threads freezeable by default.
> > >From cpu-hotplug perspective, helper_wq was the only singlethreaded
> > non-freezeable workqueue.
> 
> I think Eric's patch is what you need. We should _not_ freeze kthreadd(), we
> need kthread_create() after freezing. Now it doesn't depend on workqueues, we
> can freeze them all, single-thread or not.
> 

These were my exact thoughts. 

> I like this patch.

Same here. 

> 
> Oleg.
> 

Thanks and Regards
gautham.

-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: [PATCH] kthread: Don't depend on work queues

[Oleg Nesterov]

On 04/11, Gautham R Shenoy wrote:
> On Wed, Apr 11, 2007 at 03:48:05PM +0400, Oleg Nesterov wrote:
> > On 04/11, Gautham R Shenoy wrote:
> > >
> > > On Wed, Apr 11, 2007 at 12:13:34PM +0200, Rafael J. Wysocki wrote:
> > > > 
> > > > It should be calling try_to_freeze() somewhere anyway.  We may need to freeze
> > > > all tasks in some cases.
> > > 
> > > How about
> > > for (;;) {
> > > 	try_to_freeze();
> > > 
> > > ?
> > 
> > Why?
> 
> If some event (defintely NOT cpu hotplug) needs this thread frozen.
> 
> > 
> > > This change allows us to make all the worker threads freezeable by default.
> > > >From cpu-hotplug perspective, helper_wq was the only singlethreaded
> > > non-freezeable workqueue.
> > 
> > I think Eric's patch is what you need. We should _not_ freeze kthreadd(), we
> > need kthread_create() after freezing. Now it doesn't depend on workqueues, we
> > can freeze them all, single-thread or not.
> > 
> 
> These were my exact thoughts. 

Sorry, I misunderstood your message.

Yes, we can freeze it with FE_HOTPLUG_CPU. In that case wait_event()
should also check !freezing(), and try_to_freeze() should be called
after case wait_event().

On the other hand, if "kthreadd" does not sleep on kthread_create_work,
we have another unfrozen process waiting for kthread_create_info.done.
So, is there any practical reason why kthreadd() should explicitely go
to refrigerator?

Oleg.

Re: [PATCH] kthread: Don't depend on work queues

[Rafael J. Wysocki]

On Wednesday, 11 April 2007 16:36, Oleg Nesterov wrote:
> On 04/11, Gautham R Shenoy wrote:
> > On Wed, Apr 11, 2007 at 03:48:05PM +0400, Oleg Nesterov wrote:
> > > On 04/11, Gautham R Shenoy wrote:
> > > >
> > > > On Wed, Apr 11, 2007 at 12:13:34PM +0200, Rafael J. Wysocki wrote:
> > > > > 
> > > > > It should be calling try_to_freeze() somewhere anyway.  We may need to freeze
> > > > > all tasks in some cases.
> > > > 
> > > > How about
> > > > for (;;) {
> > > > 	try_to_freeze();
> > > > 
> > > > ?
> > > 
> > > Why?
> > 
> > If some event (defintely NOT cpu hotplug) needs this thread frozen.
> > 
> > > 
> > > > This change allows us to make all the worker threads freezeable by default.
> > > > >From cpu-hotplug perspective, helper_wq was the only singlethreaded
> > > > non-freezeable workqueue.
> > > 
> > > I think Eric's patch is what you need. We should _not_ freeze kthreadd(), we
> > > need kthread_create() after freezing. Now it doesn't depend on workqueues, we
> > > can freeze them all, single-thread or not.
> > > 
> > 
> > These were my exact thoughts. 
> 
> Sorry, I misunderstood your message.
> 
> Yes, we can freeze it with FE_HOTPLUG_CPU. In that case wait_event()
> should also check !freezing(), and try_to_freeze() should be called
> after case wait_event().
> 
> On the other hand, if "kthreadd" does not sleep on kthread_create_work,
> we have another unfrozen process waiting for kthread_create_info.done.
> So, is there any practical reason why kthreadd() should explicitely go
> to refrigerator?

Good question.  Right now, there probably is not any.

Greetings,
Rafael

Re: [PATCH] kthread: Don't depend on work queues

[Rafael J. Wysocki]

On Wednesday, 11 April 2007 15:31, Gautham R Shenoy wrote:
> On Wed, Apr 11, 2007 at 03:48:05PM +0400, Oleg Nesterov wrote:
> > On 04/11, Gautham R Shenoy wrote:
> > >
> > > On Wed, Apr 11, 2007 at 12:13:34PM +0200, Rafael J. Wysocki wrote:
> > > > 
> > > > It should be calling try_to_freeze() somewhere anyway.  We may need to freeze
> > > > all tasks in some cases.
> > > 
> > > How about
> > > for (;;) {
> > > 	try_to_freeze();
> > > 
> > > ?
> > 
> > Why?
> 
> If some event (defintely NOT cpu hotplug) needs this thread frozen.

I didn't mean the CPU hotplug too, BTW, but I'm considering freezing all tasks
at the late stage of suspend (ie. after freezing devices).

Greetings,
Rafael

Re: [PATCH] kthread: Don't depend on work queues

[Eric W. Biederman]

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

> argh.  Your description freely confuddles the terms "kernel thread" and
> "kthread".  Can we not do that?  Henceforth the term "kernel thread" refers
> to something which was started with kernel_thread() and "kthread" refers to
> something which was created by kthread_create(), OK?

Yes.  Will fix.

> Your patch gets midly tangled up with Oleg's recent
>
> reduce-reparent_to_init.patch
> make-kernel-threads-invisible-to-sbin-init.patch
> reparent-kernel-threads-to-swapper.patch
>
> but they seemed fairly unpopular anyway so I'll drop 'em.

Ok.  I kind of liked the first one but that is a minor cleanup.

> Your wait_event() will contribute to load average, I expect.  We get mail. 
> I converted it to wait_event_interruptible().

Ok.  That is more polite.

> I guess using PF_NOFREEZE rather than try_to_freeze() is OK, but one
> wonders what thinking led to that?

That is what we are currently doing for the work queues, and I was lazy.
For people who care they can fix it.

> Often when we have a singleton thread like this it is neater to use
> wake_up_process() directly on it, rather than creating a rather pointless
> waitqueue_head for it.  I started looking into that but it would have taken
> more than 30 seconds.

Sure I took a look and it isn't too hard.  Updated patch in a minute...

I have left the locking the way it is despite the reasonable chance that
Oleg points I can only acquire the lock when deleting the list entry.
I'm to lazy to think through the SMP races to make certain that is safe.

Eric

[PATCH] kthread: Don't depend on work queues (take 2)

[Eric W. Biederman]

Currently there is a circular reference between work queue initialization
and kthread initialization.   This prevents the kthread infrastructure from
initializing until after work queues have been initialized.

We want the properties of tasks created with kthread_create to be as close
as possible to the init_task and to not be contaminated by user processes.
The later we start our kthreadd that creates these tasks the harder it
is to avoid contamination from user processes and the more of a mess we
have to clean up because the defaults have changed on us.

So this patch modifies the kthread support to not use work queues but to
instead use a simple list of structures, and to have kthreadd start from
init_task immediately after our kernel thread that execs /sbin/init.

By being a true child of init_task we only have to change those process
settings that we want to have different from init_task, such as our
process name, the cpus that are allowed, blocking all signals and setting
SIGCHLD to SIG_IGN so that all of our children are reaped automatically.

By being a true child of init_task we also naturally get our ppid set to 0
and do not wind up as a child of PID == 1.  Ensuring that tasks generated
by kthread_create will not slow down the functioning of the wait family
of functions.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/kthread.h |    3 +
 init/main.c             |    5 ++
 kernel/kthread.c        |  124 ++++++++++++++++++++++++++---------------------
 3 files changed, 76 insertions(+), 56 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 1c65e7a..00dd957 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -30,4 +30,7 @@ void kthread_bind(struct task_struct *k, unsigned int cpu);
 int kthread_stop(struct task_struct *k);
 int kthread_should_stop(void);
 
+int kthreadd(void *unused);
+extern struct task_struct *kthreadd_task;
+
 #endif /* _LINUX_KTHREAD_H */
diff --git a/init/main.c b/init/main.c
index a92989e..d05430e 100644
--- a/init/main.c
+++ b/init/main.c
@@ -54,6 +54,7 @@
 #include <linux/lockdep.h>
 #include <linux/pid_namespace.h>
 #include <linux/device.h>
+#include <linux/kthread.h>
 
 #include <asm/io.h>
 #include <asm/bugs.h>
@@ -435,8 +436,12 @@ static void __init setup_command_line(char *command_line)
 static void noinline rest_init(void)
 	__releases(kernel_lock)
 {
+	int pid;
 	kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
+
+	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
+	kthreadd_task = find_task_by_pid(pid);
 	unlock_kernel();
 
 	/*
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 87c50cc..fce2af7 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -1,7 +1,7 @@
 /* Kernel thread helper functions.
  *   Copyright (C) 2004 IBM Corporation, Rusty Russell.
  *
- * Creation is done via keventd, so that we get a clean environment
+ * Creation is done via kthreadd, so that we get a clean environment
  * even if we're invoked from userspace (think modprobe, hotplug cpu,
  * etc.).
  */
@@ -15,24 +15,22 @@
 #include <linux/mutex.h>
 #include <asm/semaphore.h>
 
-/*
- * We dont want to execute off keventd since it might
- * hold a semaphore our callers hold too:
- */
-static struct workqueue_struct *helper_wq;
+static DEFINE_SPINLOCK(kthread_create_lock);
+static LIST_HEAD(kthread_create_list);
+struct task_struct *kthreadd_task;
 
 struct kthread_create_info
 {
-	/* Information passed to kthread() from keventd. */
+	/* Information passed to kthread() from kthreadd. */
 	int (*threadfn)(void *data);
 	void *data;
 	struct completion started;
 
-	/* Result passed back to kthread_create() from keventd. */
+	/* Result passed back to kthread_create() from kthreadd. */
 	struct task_struct *result;
 	struct completion done;
 
-	struct work_struct work;
+	struct list_head list;
 };
 
 struct kthread_stop_info
@@ -60,42 +58,17 @@ int kthread_should_stop(void)
 }
 EXPORT_SYMBOL(kthread_should_stop);
 
-static void kthread_exit_files(void)
-{
-	struct fs_struct *fs;
-	struct task_struct *tsk = current;
-
-	exit_fs(tsk);		/* current->fs->count--; */
-	fs = init_task.fs;
-	tsk->fs = fs;
-	atomic_inc(&fs->count);
- 	exit_files(tsk);
-	current->files = init_task.files;
-	atomic_inc(&tsk->files->count);
-}
-
 static int kthread(void *_create)
 {
 	struct kthread_create_info *create = _create;
 	int (*threadfn)(void *data);
 	void *data;
-	sigset_t blocked;
 	int ret = -EINTR;
 
-	kthread_exit_files();
-
-	/* Copy data: it's on keventd's stack */
+	/* Copy data: it's on kthread's stack */
 	threadfn = create->threadfn;
 	data = create->data;
 
-	/* Block and flush all signals (in case we're not from keventd). */
-	sigfillset(&blocked);
-	sigprocmask(SIG_BLOCK, &blocked, NULL);
-	flush_signals(current);
-
-	/* By default we can run anywhere, unlike keventd. */
-	set_cpus_allowed(current, CPU_MASK_ALL);
-
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_INTERRUPTIBLE);
 	complete(&create->started);
@@ -112,11 +85,8 @@ static int kthread(void *_create)
 	return 0;
 }
 
-/* We are keventd: create a thread. */
-static void keventd_create_kthread(struct work_struct *work)
+static void create_kthread(struct kthread_create_info *create)
 {
-	struct kthread_create_info *create =
-		container_of(work, struct kthread_create_info, work);
 	int pid;
 
 	/* We want our own signal handler (we take no signals by default). */
@@ -162,17 +132,14 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 	create.data = data;
 	init_completion(&create.started);
 	init_completion(&create.done);
-	INIT_WORK(&create.work, keventd_create_kthread);
-
-	/*
-	 * The workqueue needs to start up first:
-	 */
-	if (!helper_wq)
-		create.work.func(&create.work);
-	else {
-		queue_work(helper_wq, &create.work);
-		wait_for_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);
+
+	wait_for_completion(&create.done);
+
 	if (!IS_ERR(create.result)) {
 		va_list args;
 		va_start(args, namefmt);
@@ -180,7 +147,6 @@ struct task_struct *kthread_create(int (*threadfn)(void *data),
 			  namefmt, args);
 		va_end(args);
 	}
-
 	return create.result;
 }
 EXPORT_SYMBOL(kthread_create);
@@ -245,12 +211,58 @@ int kthread_stop(struct task_struct *k)
 }
 EXPORT_SYMBOL(kthread_stop);
 
-static __init int helper_init(void)
+
+static __init void kthreadd_setup(void)
 {
-	helper_wq = create_singlethread_workqueue("kthread");
-	BUG_ON(!helper_wq);
+	struct task_struct *tsk = current;
+	struct k_sigaction sa;
+	sigset_t blocked;
 
-	return 0;
+	strcpy(tsk->comm, "kthreadd");
+
+	/* Block and flush all signals */
+	sigfillset(&blocked);
+	sigprocmask(SIG_BLOCK, &blocked, NULL);
+	flush_signals(tsk);
+
+	/* SIG_IGN makes children autoreap: see do_notify_parent(). */
+	sa.sa.sa_handler = SIG_IGN;
+	sa.sa.sa_flags = 0;
+	siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
+	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
+
+	set_user_nice(current, -5);
+	set_cpus_allowed(current, CPU_MASK_ALL);
 }
 
-core_initcall(helper_init);
+int kthreadd(void *unused)
+{
+	/* Setup a clean context for our children to inherit. */
+	kthreadd_setup();
+
+	current->flags |= PF_NOFREEZE;
+
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&kthread_create_list))
+			schedule();
+		__set_current_state(TASK_RUNNING);
+
+		spin_lock(&kthread_create_lock);
+		while (!list_empty(&kthread_create_list)) {
+			struct kthread_create_info *create;
+
+			create = list_entry(kthread_create_list.next,
+					    struct kthread_create_info, list);
+			list_del_init(&create->list);
+			spin_unlock(&kthread_create_lock);
+
+			create_kthread(create);
+
+			spin_lock(&kthread_create_lock);
+		}
+		spin_unlock(&kthread_create_lock);
+	}
+
+	return 0;
+}
-- 
1.5.0.g53756

[PATCH] Change reparent_to_init to reparent_to_kthreadd

[Eric W. Biederman]

When a kernel thread calls daemonize, instead of reparenting the thread to
init reparent the thread to kthreadd next to the threads created by
kthread_create.

This is really just a stop gap until daemonize goes away, but it does
ensure no kernel threads are under init and they are all in one place
that is easy to find.

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

diff --git a/kernel/exit.c b/kernel/exit.c
index b55ed4c..1a7c878 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -42,6 +42,7 @@
 #include <linux/audit.h> /* for audit_free() */
 #include <linux/resource.h>
 #include <linux/blkdev.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -255,26 +256,25 @@ static int has_stopped_jobs(struct pid *pgrp)
 }
 
 /**
- * reparent_to_init - Reparent the calling kernel thread to the init task of the pid space that the thread belongs to.
+ * reparent_to_kthreadd - Reparent the calling kernel thread to kthreadd
  *
  * If a kernel thread is launched as a result of a system call, or if
- * it ever exits, it should generally reparent itself to init so that
- * it is correctly cleaned up on exit.
+ * it ever exits, it should generally reparent itself to kthreadd so it
+ * isn't in the way of other processes and is correctly cleaned up on exit.
  *
  * The various task state such as scheduling policy and priority may have
  * been inherited from a user process, so we reset them to sane values here.
  *
- * NOTE that reparent_to_init() gives the caller full capabilities.
+ * NOTE that reparent_to_kthreadd() gives the caller full capabilities.
  */
-static void reparent_to_init(void)
+static void reparent_to_kthreadd(void)
 {
 	write_lock_irq(&tasklist_lock);
 
 	ptrace_unlink(current);
 	/* Reparent to init */
 	remove_parent(current);
-	current->parent = child_reaper(current);
-	current->real_parent = child_reaper(current);
+	current->real_parent = current->parent = kthreadd_task;
 	add_parent(current);
 
 	/* Set the exit signal to SIGCHLD so we signal init on exit */
@@ -401,7 +401,7 @@ void daemonize(const char *name, ...)
 	current->files = init_task.files;
 	atomic_inc(&current->files->count);
 
-	reparent_to_init();
+	reparent_to_kthreadd();
 }
 
 EXPORT_SYMBOL(daemonize);
-- 
1.5.0.g53756

Re: [PATCH] kthread: Don't depend on work queues (take 2)

[Andrew Morton]

On Wed, 11 Apr 2007 12:27:59 -0600
ebiederm@xmission.com (Eric W. Biederman) wrote:

> +	strcpy(tsk->comm, "kthreadd");

We have this dopey set_task_comm() thing which is there to avoid
races when userspace is looking at this task's name via /proc.

It obviously doesn't matter in this case, but I suppose one should
set a good example.  I'll make that change.

Re: [PATCH] kthread: Don't depend on work queues (take 2)

[Oleg Nesterov]

On 04/11, Eric W. Biederman wrote:
> 
> @@ -435,8 +436,12 @@ static void __init setup_command_line(char *command_line)
>  static void noinline rest_init(void)
>  	__releases(kernel_lock)
>  {
> +	int pid;
>  	kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
>  	numa_default_policy();
> +
> +	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
> +	kthreadd_task = find_task_by_pid(pid);
>  	unlock_kernel();

Just curious. What if kernel/kthread.c declares

	static struct task_struct *kthreadd_task = &init_task;

an then kthreadd_setup() does kthreadd_task = current. I assume it is always
safe to try_to_wake_up(idle_thread), because it always TASK_RUNNING. This
way we don't need to export kthreadd_task.

> +	spin_lock(&kthread_create_lock);
> +	list_add_tail(&create.list, &kthread_create_list);
> +	wake_up_process(kthreadd_task);
> +	spin_unlock(&kthread_create_lock);

Very minor nit, but we don't need to do wake_up under spin_unlock().

Oleg.

Re: [PATCH] kthread: Don't depend on work queues (take 2)

[Eric W. Biederman]

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

> On 04/11, Eric W. Biederman wrote:
>> 
>> @@ -435,8 +436,12 @@ static void __init setup_command_line(char *command_line)
>>  static void noinline rest_init(void)
>>  	__releases(kernel_lock)
>>  {
>> +	int pid;
>>  	kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
>>  	numa_default_policy();
>> +
>> +	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
>> +	kthreadd_task = find_task_by_pid(pid);
>>  	unlock_kernel();
>
> Just curious. What if kernel/kthread.c declares
>
> 	static struct task_struct *kthreadd_task = &init_task;
>
> an then kthreadd_setup() does kthreadd_task = current. I assume it is always
> safe to try_to_wake_up(idle_thread), because it always TASK_RUNNING. This
> way we don't need to export kthreadd_task.

I did it this way largely so I could use the export in reparent_to_XXX in
daemonize.  This way I don't have races in finding kthreadd.  Plus
I didn't think of the trick of using the idle_thread...

>> +	spin_lock(&kthread_create_lock);
>> +	list_add_tail(&create.list, &kthread_create_list);
>> +	wake_up_process(kthreadd_task);
>> +	spin_unlock(&kthread_create_lock);
>
> Very minor nit, but we don't need to do wake_up under spin_unlock().

I guess that is true.  However it doesn't hurt either.  I guess
I was keeping the form that I used with wait queues where it may
have mattered.  Either that or I just copied a bad example.

Eric

Re: [PATCH] kthread: Don't depend on work queues (take 2)

[Oleg Nesterov]

On 04/11, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > On 04/11, Eric W. Biederman wrote:
> >> 
> >> @@ -435,8 +436,12 @@ static void __init setup_command_line(char *command_line)
> >>  static void noinline rest_init(void)
> >>  	__releases(kernel_lock)
> >>  {
> >> +	int pid;
> >>  	kernel_thread(init, NULL, CLONE_FS | CLONE_SIGHAND);
> >>  	numa_default_policy();
> >> +
> >> +	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
> >> +	kthreadd_task = find_task_by_pid(pid);
> >>  	unlock_kernel();
> >
> > Just curious. What if kernel/kthread.c declares
> >
> > 	static struct task_struct *kthreadd_task = &init_task;
> >
> > an then kthreadd_setup() does kthreadd_task = current. I assume it is always
> > safe to try_to_wake_up(idle_thread), because it always TASK_RUNNING. This
> > way we don't need to export kthreadd_task.
> 
> I did it this way largely so I could use the export in reparent_to_XXX in
> daemonize.  This way I don't have races in finding kthreadd.  Plus
> I didn't think of the trick of using the idle_thread...

Ah yes, we still need to export kthreadd_task... I just worried about subtle
dependency this patch adds... This "kthreadd_task = find_task_by_pid" assumes
that init/main.c:init() takes lock_kernel() before the first kthread_create().

Oleg.

Re: [PATCH] kthread: Don't depend on work queues (take 2)

[Eric W. Biederman]

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

> Ah yes, we still need to export kthreadd_task... I just worried about subtle
> dependency this patch adds... This "kthreadd_task = find_task_by_pid" assumes
> that init/main.c:init() takes lock_kernel() before the first kthread_create().

There is a small element of that.  Mostly it assumes we are running in the idle
thread until we call schedule.  Anyway the assumption and the code is out
there in the open in the main code path so it is easy to find, and the code works
today.

At this point I am queuing up patches that kill daemonize.  I will be
happy to revisit this part once daemonize is gone and the only interesting
file is kthread.c

Eric

Re: [PATCH] kthread: Don't depend on work queues

[Oleg Nesterov]

On 04/10, Eric W. Biederman wrote:
> 
> +int kthreadd(void *unused)
> +{
> +	/* Setup a clean context for our children to inherit. */
> +	kthreadd_setup();
> +
> +	current->flags |= PF_NOFREEZE;
> +
> +	for (;;) {
> +		wait_event(kthread_create_work,
> +			   !list_empty(&kthread_create_list));
> +
> +		spin_lock(&kthread_create_lock);
> +		while (!list_empty(&kthread_create_list)) {

Do we need to check the condition under lock? We can miss an event,
but then it will be noticed by wait_event() above.

> +			struct kthread_create_info *create;
> +
> +			create = list_entry(kthread_create_list.next,
> +					    struct kthread_create_info, list);
> +			list_del_init(&create->list);
> +			spin_unlock(&kthread_create_lock);
> +
> +			create_kthread(create);
> +
> +			spin_lock(&kthread_create_lock);
> +		}
> +		spin_unlock(&kthread_create_lock);
> +	}

IOW,

	for (;;) {
		wait_event(kthread_create_work,
			   !list_empty(&kthread_create_list));

		while (!list_empty(&kthread_create_list)) {
			struct kthread_create_info *create;

			spin_lock(&kthread_create_lock);
			create = list_entry(kthread_create_list.next,
					    struct kthread_create_info, list);
			list_del_init(&create->list);
			spin_unlock(&kthread_create_lock);

			create_kthread(create);
		}
	}

Oleg.

Re: [PATCH] kthread: Don't depend on work queues

[Eric W. Biederman]

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

> On 04/10, Eric W. Biederman wrote:
>> 
>> +int kthreadd(void *unused)
>> +{
>> +	/* Setup a clean context for our children to inherit. */
>> +	kthreadd_setup();
>> +
>> +	current->flags |= PF_NOFREEZE;
>> +
>> +	for (;;) {
>> +		wait_event(kthread_create_work,
>> +			   !list_empty(&kthread_create_list));
>> +
>> +		spin_lock(&kthread_create_lock);
>> +		while (!list_empty(&kthread_create_list)) {
>
> Do we need to check the condition under lock? We can miss an event,
> but then it will be noticed by wait_event() above.

We need to be certain there is something on the list before we remove
it.  Otherwise we will start dereferencing bad pointers.

> IOW,
>
> 	for (;;) {
> 		wait_event(kthread_create_work,
> 			   !list_empty(&kthread_create_list));
>
> 		while (!list_empty(&kthread_create_list)) {
> 			struct kthread_create_info *create;
>
> 			spin_lock(&kthread_create_lock);
> 			create = list_entry(kthread_create_list.next,
> 					    struct kthread_create_info, list);
> 			list_del_init(&create->list);
> 			spin_unlock(&kthread_create_lock);
>
> 			create_kthread(create);
> 		}
> 	}

I guess since we are the only process to ever remove things from the
list that would be safe.

Eric

Re: [PATCH] kthread: Don't depend on work queues

[Oleg Nesterov]

On 04/10, Eric W. Biederman wrote:
> 
>  static int kthread(void *_create)
>  {
>  	struct kthread_create_info *create = _create;
>  	int (*threadfn)(void *data);
>  	void *data;
> -	sigset_t blocked;
>  	int ret = -EINTR;
>  
> -	kthread_exit_files();
> -
> -	/* Copy data: it's on keventd's stack */
> +	/* Copy data: it's on kthread's stack */
>  	threadfn = create->threadfn;
>  	data = create->data;
>  
> -	/* Block and flush all signals (in case we're not from keventd). */
> -	sigfillset(&blocked);
> -	sigprocmask(SIG_BLOCK, &blocked, NULL);
> -	flush_signals(current);
> -
> -	/* By default we can run anywhere, unlike keventd. */
> -	set_cpus_allowed(current, CPU_MASK_ALL);

The above is OK, but I believe you should add set_cpus_allowed() to
kthreadd_setup(). Note that kthreadd() is forked after init_idle().

Oleg.

Re: [PATCH] kthread: Don't depend on work queues

[Eric W. Biederman]

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

> On 04/10, Eric W. Biederman wrote:
>> 
>>  static int kthread(void *_create)
>>  {
>>  	struct kthread_create_info *create = _create;
>>  	int (*threadfn)(void *data);
>>  	void *data;
>> -	sigset_t blocked;
>>  	int ret = -EINTR;
>>  
>> -	kthread_exit_files();
>> -
>> -	/* Copy data: it's on keventd's stack */
>> +	/* Copy data: it's on kthread's stack */
>>  	threadfn = create->threadfn;
>>  	data = create->data;
>>  
>> -	/* Block and flush all signals (in case we're not from keventd). */
>> -	sigfillset(&blocked);
>> -	sigprocmask(SIG_BLOCK, &blocked, NULL);
>> -	flush_signals(current);
>> -
>> -	/* By default we can run anywhere, unlike keventd. */
>> -	set_cpus_allowed(current, CPU_MASK_ALL);
>
> The above is OK, but I believe you should add set_cpus_allowed() to
> kthreadd_setup(). Note that kthreadd() is forked after init_idle().

init_idle()?  Yep in sched_init.  I wondered where that happened
for the initial idle thread.  Ugh. That is the first thing that
we fix after we fork init as well.  So yes it would be a good
idea to fix that...

I was expecting init_idle() to happen after we forked kthreadd. I almost
wonder if I should move init_idle....

Anyway that is one bug caught.

Eric