Re: [PATCH] kthread: convert stop_machine into a kthread

Re: [PATCH] kthread: convert stop_machine into a kthread

[Rusty Russell]

On Fri, 2006-06-16 at 08:20 +1000, Paul Mackerras wrote:
> Update stop_machine.c to spawn stop_machine as kthreads rather
> than the deprecated kernel_threads.
> 
> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

kthread does a lot of stuff we don't need, and stop_machine is fairly
special, low-level interface.

Seems like change for change's sake, to me, *and* it added more code
than it removed.

Rusty.
-- 
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

Re: [PATCH] kthread: convert stop_machine into a kthread

[Serge E. Hallyn]

Quoting Rusty Russell (rusty@rustcorp.com.au):
> On Fri, 2006-06-16 at 08:20 +1000, Paul Mackerras wrote:
> > Update stop_machine.c to spawn stop_machine as kthreads rather
> > than the deprecated kernel_threads.
> > 
> > Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> 
> kthread does a lot of stuff we don't need, and stop_machine is fairly
> special, low-level interface.
> 
> Seems like change for change's sake, to me, *and* it added more code
> than it removed.

So if kernel_thread is really going to be removed, how else should this
be done?  Just clone?

-serge

Re: [PATCH] kthread: convert stop_machine into a kthread

[Paul Mackerras]

Serge E. Hallyn writes:

> So if kernel_thread is really going to be removed, how else should this
> be done?  Just clone?

Who said kernel_thread was going to be removed?  kthread *uses*
kernel_thread.  kthread is a helper to make life easier for drivers,
not a replacement for kernel_thread.

Paul.

Re: [PATCH] kthread: convert stop_machine into a kthread

[Rusty Russell]

On Thu, 2006-06-15 at 22:04 -0500, Serge E. Hallyn wrote:
> Quoting Rusty Russell (rusty@rustcorp.com.au):
> > Seems like change for change's sake, to me, *and* it added more code
> > than it removed.
> 
> So if kernel_thread is really going to be removed, how else should this
> be done?  Just clone?

Sorry, is kernel_thread going to be removed, or just not exported to
modules?  What's kthread going to use?

Confused,
Rusty.
-- 
Help! Save Australia from the worst of the DMCA: http://linux.org.au/law

Re: [PATCH] kthread: convert stop_machine into a kthread

[Serge E. Hallyn]

Quoting Rusty Russell (rusty@rustcorp.com.au):
> On Thu, 2006-06-15 at 22:04 -0500, Serge E. Hallyn wrote:
> > Quoting Rusty Russell (rusty@rustcorp.com.au):
> > > Seems like change for change's sake, to me, *and* it added more code
> > > than it removed.
> > 
> > So if kernel_thread is really going to be removed, how else should this
> > be done?  Just clone?
> 
> Sorry, is kernel_thread going to be removed, or just not exported to
> modules?  What's kthread going to use?
> 
> Confused,
> Rusty.

Hah.

Yes, I see.  I misread.  So I should be focusing on modules  :)

Really, all *I* care about is cases where the resulting pid is cached
as a pointer to the thread, which it wasn't here anyway.  

thanks, sorry for the noise.

-serge

Re: [PATCH] kthread: convert stop_machine into a kthread

[Eric W. Biederman]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Quoting Rusty Russell (rusty@rustcorp.com.au):
>> On Thu, 2006-06-15 at 22:04 -0500, Serge E. Hallyn wrote:
>> > Quoting Rusty Russell (rusty@rustcorp.com.au):
>> > > Seems like change for change's sake, to me, *and* it added more code
>> > > than it removed.
>> > 
>> > So if kernel_thread is really going to be removed, how else should this
>> > be done?  Just clone?
>> 
>> Sorry, is kernel_thread going to be removed, or just not exported to
>> modules?  What's kthread going to use?
>> 
>> Confused,
>> Rusty.
>
> Hah.
>
> Yes, I see.  I misread.  So I should be focusing on modules  :)
>
> Really, all *I* care about is cases where the resulting pid is cached
> as a pointer to the thread, which it wasn't here anyway.  

There is one other piece we care about as well.

We don't want to capture user space context like a non-default fs namespace
in a kernel thread as well.  Since the kthread api calls kernel_thread
from keventd non of the threads that it spawns can capture any user
space context, by accident.  There was a very nasty bug a while ago
when the fs namespace was captured by a kernel thread and then it was
impossible to unmount your filesystem because no one had access to
that filesystem mount tree.

In this instance the kstopmachine is stared using the kthread api so
it is safe, and then forking children is safe as well. 

Once everything that we can convert to the kthread api is converted we
need to audit all of the remaining kernel_thread instances to ensure
they don't capture user space context.

The basic rule is that is only safe to use kernel_thread directly if
it is coming from another kernel thread.

So while the conversion was overkill in this context and we certainly
want to concentrate on modules, where it is much less likely to be
correct.  We want to convert as many instances as we can away from
the raw kernel_thread api.

All that is ultimately going away is the export of kernel_thread to
modules, because there are so very few instances when using
kernel_thread directly can be justified.

Eric

[PATCH] kthread: convert stop_machine into a kthread

[Serge E. Hallyn]

Update stop_machine.c to spawn stop_machine as kthreads rather
than the deprecated kernel_threads.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 kernel/stop_machine.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

ce04ccc88ac3e2e6c3942fe2c8c4b2d5492d8397
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index dcfb5d7..2dd5a48 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -4,6 +4,7 @@ #include <linux/sched.h>
 #include <linux/cpu.h>
 #include <linux/err.h>
 #include <linux/syscalls.h>
+#include <linux/kthread.h>
 #include <asm/atomic.h>
 #include <asm/semaphore.h>
 #include <asm/uaccess.h>
@@ -96,11 +97,14 @@ static int stop_machine(void)
 	stopmachine_state = STOPMACHINE_WAIT;
 
 	for_each_online_cpu(i) {
+		struct task_struct *tsk;
 		if (i == raw_smp_processor_id())
 			continue;
-		ret = kernel_thread(stopmachine, (void *)(long)i,CLONE_KERNEL);
-		if (ret < 0)
+		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
+		if (IS_ERR(tsk)) {
+			ret = PTR_ERR(tsk);
 			break;
+		}
 		stopmachine_num_threads++;
 	}
 
-- 
1.3.3

Re: [PATCH] kthread: convert stop_machine into a kthread

[Andrew Morton]

On Thu, 15 Jun 2006 09:43:31 -0500
"Serge E. Hallyn" <serue@us.ibm.com> wrote:

> Update stop_machine.c to spawn stop_machine as kthreads rather
> than the deprecated kernel_threads.
> 
> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> 
> ---
> 
>  kernel/stop_machine.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> ce04ccc88ac3e2e6c3942fe2c8c4b2d5492d8397
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index dcfb5d7..2dd5a48 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -4,6 +4,7 @@ #include <linux/sched.h>
>  #include <linux/cpu.h>
>  #include <linux/err.h>
>  #include <linux/syscalls.h>
> +#include <linux/kthread.h>
>  #include <asm/atomic.h>
>  #include <asm/semaphore.h>
>  #include <asm/uaccess.h>
> @@ -96,11 +97,14 @@ static int stop_machine(void)
>  	stopmachine_state = STOPMACHINE_WAIT;
>  
>  	for_each_online_cpu(i) {
> +		struct task_struct *tsk;
>  		if (i == raw_smp_processor_id())
>  			continue;
> -		ret = kernel_thread(stopmachine, (void *)(long)i,CLONE_KERNEL);
> -		if (ret < 0)
> +		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
> +		if (IS_ERR(tsk)) {
> +			ret = PTR_ERR(tsk);
>  			break;
> +		}
>  		stopmachine_num_threads++;
>  	}
>  

OK.  But if we're going to convert to the kthread API then stopmachine()
really whould be switched to the more efficient kthread_bind().

RE: [PATCH] kthread: convert stop_machine into a kthread

[Erik Ohrnberger]

unsubscribe linux-kernel

Re: [PATCH] kthread: convert stop_machine into a kthread

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> OK.  But if we're going to convert to the kthread API then stopmachine()
> really whould be switched to the more efficient kthread_bind().

Ah, like so?

Rusty, do you feel this makes the conversion less of a step backward?
If not, Andrew, as Rusty pointed out, stop_machine.c does not fall into
the set of kernel_thread users which need to be updated either for the
deprecation or to deal with pid namespaces, and perhaps my previous
patch should not be applied after all.

thanks,
-serge

From: Serge E. Hallyn <serue@us.ibm.com>
Date: Tue, 20 Jun 2006 03:17:44 -0500
Subject: [PATCH] kthread: convert stop_machine to use kthread_bind

Convert stop_machine to use the more efficient kthread_bind()
in place of set_cpus_allowed().

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 kernel/stop_machine.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

831c955bcc8572f0aea75ab608ed5da37680df4e
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 2dd5a48..a462deb 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -31,7 +31,7 @@ static int stopmachine(void *cpu)
 	int irqs_disabled = 0;
 	int prepared = 0;
 
-	set_cpus_allowed(current, cpumask_of_cpu((int)(long)cpu));
+	kthread_bind(current, (unsigned int)(long)cpu);
 
 	/* Ack: we are alive */
 	smp_mb(); /* Theoretically the ack = 0 might not be on this CPU yet. */
-- 
1.3.3

Re: [PATCH] kthread: convert stop_machine into a kthread

[Andrew Morton]

On Tue, 20 Jun 2006 03:27:45 -0500
"Serge E. Hallyn" <serue@us.ibm.com> wrote:

> Ah, like so?

Nope.  kthread_bind() is supposed to be called by the thread creator,
before the thread is started.

The documentation for kthread_bind() is irritatingly hidden in the header
file.

Re: [PATCH] kthread: convert stop_machine into a kthread

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> On Tue, 20 Jun 2006 03:27:45 -0500
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> 
> > Ah, like so?
> 
> Nope.  kthread_bind() is supposed to be called by the thread creator,
> before the thread is started.
> 
> The documentation for kthread_bind() is irritatingly hidden in the header
> file.

Oh, I see - then it makes more sense that it gets away with being so
much simpler than set_cpus_allowed().

So here's another attempt.  However I'm not sure now whether
the first round of synchronization around stopmachine_thread_ack
is necessary anymore.  If any threads fail, we'll find out from the
kthread_create() return value, right?

Still I'm not sure about that, so first things first:

thanks,
-serge

From: Serge E. Hallyn <serue@us.ibm.com>
Date: Tue, 20 Jun 2006 11:01:08 -0500
Subject: [PATCH] kthread: update stop_machine to use kthread_bind

Update stop_machine to use the more efficient kthread_bind()
before running task in place of set_cpus_allowed() after.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 kernel/stop_machine.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

e25a88e3d60f3f139f10cc8cd894d87622033a16
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 2dd5a48..593d8e4 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -86,7 +86,8 @@ static void stopmachine_set_state(enum s
 
 static int stop_machine(void)
 {
-	int i, ret = 0;
+	int ret = 0;
+	unsigned int i;
 	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
 
 	/* One high-prio thread per cpu.  We'll do this one. */
@@ -100,11 +101,13 @@ static int stop_machine(void)
 		struct task_struct *tsk;
 		if (i == raw_smp_processor_id())
 			continue;
-		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
+		tsk = kthread_create(stopmachine, NULL, "stopmachine");
 		if (IS_ERR(tsk)) {
 			ret = PTR_ERR(tsk);
 			break;
 		}
+		kthread_bind(tsk, i);
+		wake_up_process(tsk);
 		stopmachine_num_threads++;
 	}
 
-- 
1.3.3

Re: [PATCH] kthread: convert stop_machine into a kthread

[Andrew Morton]

"Serge E. Hallyn" <serue@us.ibm.com> wrote:
>
> Quoting Andrew Morton (akpm@osdl.org):
> > On Tue, 20 Jun 2006 03:27:45 -0500
> > "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> > 
> > > Ah, like so?
> > 
> > Nope.  kthread_bind() is supposed to be called by the thread creator,
> > before the thread is started.
> > 
> > The documentation for kthread_bind() is irritatingly hidden in the header
> > file.
> 
> Oh, I see - then it makes more sense that it gets away with being so
> much simpler than set_cpus_allowed().
> 
> So here's another attempt.  However I'm not sure now whether
> the first round of synchronization around stopmachine_thread_ack
> is necessary anymore.  If any threads fail, we'll find out from the
> kthread_create() return value, right?
> 
> Still I'm not sure about that, so first things first:
> 
> thanks,
> -serge
> 
> From: Serge E. Hallyn <serue@us.ibm.com>
> Date: Tue, 20 Jun 2006 11:01:08 -0500
> Subject: [PATCH] kthread: update stop_machine to use kthread_bind
> 
> Update stop_machine to use the more efficient kthread_bind()
> before running task in place of set_cpus_allowed() after.
> 
> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> 
> ---
> 
>  kernel/stop_machine.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> e25a88e3d60f3f139f10cc8cd894d87622033a16
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index 2dd5a48..593d8e4 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -86,7 +86,8 @@ static void stopmachine_set_state(enum s
>  
>  static int stop_machine(void)
>  {
> -	int i, ret = 0;
> +	int ret = 0;
> +	unsigned int i;
>  	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
>  
>  	/* One high-prio thread per cpu.  We'll do this one. */
> @@ -100,11 +101,13 @@ static int stop_machine(void)
>  		struct task_struct *tsk;
>  		if (i == raw_smp_processor_id())
>  			continue;
> -		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
> +		tsk = kthread_create(stopmachine, NULL, "stopmachine");
>  		if (IS_ERR(tsk)) {
>  			ret = PTR_ERR(tsk);
>  			break;
>  		}
> +		kthread_bind(tsk, i);
> +		wake_up_process(tsk);
>  		stopmachine_num_threads++;
>  	}
>  

You forgot this rather important bit:

--- 25/kernel/stop_machine.c~kthread-convert-stop_machine-into-a-kthread-update-fix	Tue Jun 20 15:39:30 2006
+++ 25-akpm/kernel/stop_machine.c	Tue Jun 20 15:39:30 2006
@@ -26,13 +26,11 @@ static unsigned int stopmachine_num_thre
 static atomic_t stopmachine_thread_ack;
 static DECLARE_MUTEX(stopmachine_mutex);
 
-static int stopmachine(void *cpu)
+static int stopmachine(void *unused)
 {
 	int irqs_disabled = 0;
 	int prepared = 0;
 
-	set_cpus_allowed(current, cpumask_of_cpu((int)(long)cpu));
-
 	/* Ack: we are alive */
 	smp_mb(); /* Theoretically the ack = 0 might not be on this CPU yet. */
 	atomic_inc(&stopmachine_thread_ack);
_


Without that change, every thread will run on cpu 0, and the whole
stopmachine thing is, presumably, busted.

I'll fold all three patches together and send 'em back to you for a bit of
runtime testing, OK?

Re: [PATCH] kthread: convert stop_machine into a kthread

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> >
> > Quoting Andrew Morton (akpm@osdl.org):
> > > On Tue, 20 Jun 2006 03:27:45 -0500
> > > "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> > > 
> > > > Ah, like so?
> > > 
> > > Nope.  kthread_bind() is supposed to be called by the thread creator,
> > > before the thread is started.
> > > 
> > > The documentation for kthread_bind() is irritatingly hidden in the header
> > > file.
> > 
> > Oh, I see - then it makes more sense that it gets away with being so
> > much simpler than set_cpus_allowed().
> > 
> > So here's another attempt.  However I'm not sure now whether
> > the first round of synchronization around stopmachine_thread_ack
> > is necessary anymore.  If any threads fail, we'll find out from the
> > kthread_create() return value, right?
> > 
> > Still I'm not sure about that, so first things first:
> > 
> > thanks,
> > -serge
> > 
> > From: Serge E. Hallyn <serue@us.ibm.com>
> > Date: Tue, 20 Jun 2006 11:01:08 -0500
> > Subject: [PATCH] kthread: update stop_machine to use kthread_bind
> > 
> > Update stop_machine to use the more efficient kthread_bind()
> > before running task in place of set_cpus_allowed() after.
> > 
> > Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> > 
> > ---
> > 
> >  kernel/stop_machine.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > e25a88e3d60f3f139f10cc8cd894d87622033a16
> > diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> > index 2dd5a48..593d8e4 100644
> > --- a/kernel/stop_machine.c
> > +++ b/kernel/stop_machine.c
> > @@ -86,7 +86,8 @@ static void stopmachine_set_state(enum s
> >  
> >  static int stop_machine(void)
> >  {
> > -	int i, ret = 0;
> > +	int ret = 0;
> > +	unsigned int i;
> >  	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
> >  
> >  	/* One high-prio thread per cpu.  We'll do this one. */
> > @@ -100,11 +101,13 @@ static int stop_machine(void)
> >  		struct task_struct *tsk;
> >  		if (i == raw_smp_processor_id())
> >  			continue;
> > -		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
> > +		tsk = kthread_create(stopmachine, NULL, "stopmachine");
> >  		if (IS_ERR(tsk)) {
> >  			ret = PTR_ERR(tsk);
> >  			break;
> >  		}
> > +		kthread_bind(tsk, i);
> > +		wake_up_process(tsk);
> >  		stopmachine_num_threads++;
> >  	}
> >  
> 
> You forgot this rather important bit:

Confounded, the only thing I can figure is I must not have done a
git-format-patch to regenerate the patch before I pasted it.

I see it's all sorted out in -mm, but just for the record here's
the patch I meant to send.

thanks,
-serge

>From nobody Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <hallyn@sergelap.(none)>
Date: Tue, 20 Jun 2006 11:01:08 -0500
Subject: [PATCH] kthread: update stop_machine to use kthread_bind

Update stop_machine to use the more efficient kthread_bind()
before running task in place of set_cpus_allowed() after.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 kernel/stop_machine.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

fe140b494141aefae45a4fa12a4b4e8fef5d8ee3
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 2dd5a48..698b842 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -26,15 +26,13 @@ static unsigned int stopmachine_num_thre
 static atomic_t stopmachine_thread_ack;
 static DECLARE_MUTEX(stopmachine_mutex);
 
-static int stopmachine(void *cpu)
+static int stopmachine(void)
 {
 	int irqs_disabled = 0;
 	int prepared = 0;
 
-	set_cpus_allowed(current, cpumask_of_cpu((int)(long)cpu));
-
 	/* Ack: we are alive */
-	smp_mb(); /* Theoretically the ack = 0 might not be on this CPU yet. */
+	smp_mb();
 	atomic_inc(&stopmachine_thread_ack);
 
 	/* Simple state machine */
@@ -86,7 +84,8 @@ static void stopmachine_set_state(enum s
 
 static int stop_machine(void)
 {
-	int i, ret = 0;
+	int ret = 0;
+	unsigned int i;
 	struct sched_param param = { .sched_priority = MAX_RT_PRIO-1 };
 
 	/* One high-prio thread per cpu.  We'll do this one. */
@@ -100,11 +99,13 @@ static int stop_machine(void)
 		struct task_struct *tsk;
 		if (i == raw_smp_processor_id())
 			continue;
-		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
+		tsk = kthread_create(stopmachine, NULL, "stopmachine");
 		if (IS_ERR(tsk)) {
 			ret = PTR_ERR(tsk);
 			break;
 		}
+		kthread_bind(tsk, i);
+		wake_up_process(tsk);
 		stopmachine_num_threads++;
 	}
 
-- 
1.3.3

Re: [PATCH] kthread: convert stop_machine into a kthread

[Andrew Morton]

On Tue, 20 Jun 2006 19:52:06 -0500
"Serge E. Hallyn" <serue@us.ibm.com> wrote:

> here's
> the patch I meant to send.
> 
> ...
>
> -static int stopmachine(void *cpu)
> +static int stopmachine(void)
> ...
> -		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
> +		tsk = kthread_create(stopmachine, NULL, "stopmachine");

This should have spat a compiler warning.

The confidence level on all of this ain't high.  Please, test the patch
which I merged?

Re: [PATCH] kthread: convert stop_machine into a kthread

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> On Tue, 20 Jun 2006 19:52:06 -0500
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> 
> > here's
> > the patch I meant to send.
> > 
> > ...
> >
> > -static int stopmachine(void *cpu)
> > +static int stopmachine(void)
> > ...
> > -		tsk = kthread_run(stopmachine, (void *)(long)i, "stopmachine");
> > +		tsk = kthread_create(stopmachine, NULL, "stopmachine");
> 
> This should have spat a compiler warning.
> 
> The confidence level on all of this ain't high.  Please, test the patch
> which I merged?

Compiles, boots, and shuts down on s390.

thanks,
-serge