Re: Redundant up operation in stop_machine.c ?(2.6.12)

Re: Redundant up operation in stop_machine.c ?(2.6.12)

[Rusty Russell]

On Thu, 2005-08-25 at 21:59 +0800, Yingchao Zhou wrote:
> In stop_machine function, there are codes:
> 	if (ret < 0) {
> 		stopmachine_set_state(STOPMACHINE_EXIT);
> 		up(&stopmachine_mutex);
> 		return ret;
> 	}
> And in __stop_machine_run ,there are:
> 	if (!IS_ERR(p)) {
> 		kthread_bind(p, cpu);
> 		wake_up_process(p);
> 		wait_for_completion(&smdata.done);
> 	}
> 	up(&stopmachine_mutex);
> 
> Is the first up op is really redundant?

Yes, it seems you have found a bug.  I tested it (inserting a spurious
failure), and indeed, it gets up'ed twice.

Good catch!
Rusty.

Name: Redundant up operation in stop_machine.c
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (authored)

Yingchao Zhou <puppylove_0814@yahoo.com.cn> noticed that we up() in
stop_machine on failure, and also in the caller (unconditionally).

Index: linux-2.6.13-rc7-git1-Misc/kernel/stop_machine.c
===================================================================
--- linux-2.6.13-rc7-git1-Misc.orig/kernel/stop_machine.c	2005-08-26 11:18:00.000000000 +1000
+++ linux-2.6.13-rc7-git1-Misc/kernel/stop_machine.c	2005-08-26 12:05:01.000000000 +1000
@@ -115,7 +115,6 @@
 	/* If some failed, kill them all. */
 	if (ret < 0) {
 		stopmachine_set_state(STOPMACHINE_EXIT);
-		up(&stopmachine_mutex);
 		return ret;
 	}
 

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman