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

[Andrew Morton]

"Serge E. Hallyn" <serue@xxxxxxxxxx> wrote:
>
> Quoting Andrew Morton (akpm@xxxxxxxx):
> > On Tue, 20 Jun 2006 03:27:45 -0500
> > "Serge E. Hallyn" <serue@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> 
> ---
> 
>  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?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/