stop_machine: stop_one_cpu_nowait() fix, misc cleanups

PATCH In review Oleg Nesterov 4 participants 23 messages Nov 15, 2015 – Nov 23, 2015

lore

Share & tools

lore

Oleg Nesterov

author

2015-11-15 18:36 UTC #0 lore raw

cover stop_machine: stop_one_cpu_nowait() fix, misc cleanups

Hello,

I tried to backport the recent stop_machine fixes and recalled that I forgot to make the fix and cleanups I promised during the previous discussion.

Please review.

I am also going to rediff/resend my old patch which removes lglock from stop_machine.c, but it probably needs more discussion so I'll send it separately.

Oleg.

 include/linux/stop_machine.h |    2 +-
 kernel/stop_machine.c        |   84 ++++++++++++++++++++---------------------
 2 files changed, 42 insertions(+), 44 deletions(-)

Oleg Nesterov

author

2015-11-15 18:37 UTC #1 lore raw

PATCH 1/8 stop_machine: cpu_stopper_thread() must check done != NULL

stop_one_cpu_nowait(fn) will crash the kernel if the callback returns nonzero, work->done == NULL in this case.

This needs more cleanups, cpu_stop_signal_done() is called right after we check done != NULL and it does the same check.

Signed-off-by Oleg Nesterov <[email protected]>

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

kernel/stop_machine.c +1 -1

index 867bc20..1a66a95 100644
@@ -454,7 +454,7 @@ repeat:
 		preempt_disable();
 
 		ret = fn(arg);
-		if (ret)
+		if (ret && done)
 			done->ret = ret;
 
 		/* restore preemption and check it's still balanced */

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-15 18:37 UTC #5 lore raw

PATCH 2/8 stop_machine: don't disable preemption in stop_two_cpus()

In reply to Oleg Nesterov

Now that stop_two_cpus() path does not check cpu_active() we can remove preempt_disable(), it was only needed to ensure that stop_machine() can not be called after we observe cpu_active() == T and before we queue the new work.

Also, turn the pointless and confusing ->executed check into WARN_ON(). We know that both works must be executed, otherwise we have a bug. And in fact I think that done->executed should die, see the next changes.

Signed-off-by Oleg Nesterov <[email protected]>

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

kernel/stop_machine.c +3 -8

index 1a66a95..17f01a9 100644
@@ -258,7 +258,6 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
 	struct cpu_stop_work work1, work2;
 	struct multi_stop_data msdata;
 
-	preempt_disable();
 	msdata = (struct multi_stop_data){
 		.fn = fn,
 		.data = arg,

@@ -277,16 +276,12 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
 
 	if (cpu1 > cpu2)
 		swap(cpu1, cpu2);
-	if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2)) {
-		preempt_enable();
+	if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
 		return -ENOENT;
-	}
-
-	preempt_enable();
 
 	wait_for_completion(&done.completion);
-
-	return done.executed ? done.ret : -ENOENT;
+	WARN_ON(!done.executed);
+	return done.ret;
 }
 
 /**

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-15 18:37 UTC #11 lore raw

PATCH 4/8 stop_machine: change stop_one_cpu() to rely on cpu_stop_queue_work()

In reply to Oleg Nesterov

Change stop_one_cpu() to return -ENOENT if cpu_stop_queue_work() fails. Otherwise we know that ->executed must be true after wait_for_completion() so we can just return done.ret.

Signed-off-by Oleg Nesterov <[email protected]>

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

kernel/stop_machine.c +4 -2

index 0ec1f16..68b73c4 100644
@@ -128,9 +128,11 @@ int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg)
 	struct cpu_stop_work work = { .fn = fn, .arg = arg, .done = &done };
 
 	cpu_stop_init_done(&done, 1);
-	cpu_stop_queue_work(cpu, &work);
+	if (!cpu_stop_queue_work(cpu, &work))
+		return -ENOENT;
 	wait_for_completion(&done.completion);
-	return done.executed ? done.ret : -ENOENT;
+	WARN_ON(!done.executed);
+	return done.ret;
 }
 
 /* This controls the threads on each CPU. */

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-15 18:37 UTC #13 lore raw

PATCH 5/8 stop_machine: change __stop_cpus() to rely on cpu_stop_queue_work()

In reply to Oleg Nesterov

Change queue_stop_cpus_work() to return true if it queues at least one work, this means that the caller should wait.

__stop_cpus() can check the value returned by queue_stop_cpus_work() and avoid done.executed, just like stop_one_cpu() does.

Signed-off-by Oleg Nesterov <[email protected]>

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

kernel/stop_machine.c +10 -4

index 68b73c4..ed2019a 100644
@@ -318,12 +318,13 @@ bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 /* static data for stop_cpus */
 static DEFINE_MUTEX(stop_cpus_mutex);
 
-static void queue_stop_cpus_work(const struct cpumask *cpumask,
+static bool queue_stop_cpus_work(const struct cpumask *cpumask,
 				 cpu_stop_fn_t fn, void *arg,
 				 struct cpu_stop_done *done)
 {
 	struct cpu_stop_work *work;
 	unsigned int cpu;
+	bool queued = false;
 
 	/*
 	 * Disable preemption while queueing to avoid getting

@@ -336,9 +337,12 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 		work->fn = fn;
 		work->arg = arg;
 		work->done = done;
-		cpu_stop_queue_work(cpu, work);
+		if (cpu_stop_queue_work(cpu, work))
+			queued = true;
 	}
 	lg_global_unlock(&stop_cpus_lock);
+
+	return queued;
 }
 
 static int __stop_cpus(const struct cpumask *cpumask,

@@ -347,9 +351,11 @@ static int __stop_cpus(const struct cpumask *cpumask,
 	struct cpu_stop_done done;
 
 	cpu_stop_init_done(&done, cpumask_weight(cpumask));
-	queue_stop_cpus_work(cpumask, fn, arg, &done);
+	if (!queue_stop_cpus_work(cpumask, fn, arg, &done))
+		return -ENOENT;
 	wait_for_completion(&done.completion);
-	return done.executed ? done.ret : -ENOENT;
+	WARN_ON(!done.executed);
+	return done.ret;
 }
 
 /**

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-15 18:37 UTC #15 lore raw

PATCH 6/8 stop_machine: kill cpu_stop_done->executed

In reply to Oleg Nesterov

Now that cpu_stop_done->executed becomes write-only (ignoring WARN_ON() checks) we can remove it.

Signed-off-by Oleg Nesterov <[email protected]>

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

kernel/stop_machine.c +3 -9

index ed2019a..09eb83f 100644
@@ -28,7 +28,6 @@
  */
 struct cpu_stop_done {
 	atomic_t		nr_todo;	/* nr left to execute */
-	bool			executed;	/* actually executed? */
 	int			ret;		/* collected return value */
 	struct completion	completion;	/* fired if nr_todo reaches 0 */
 };

@@ -63,11 +62,9 @@ static void cpu_stop_init_done(struct cpu_stop_done *done, unsigned int nr_todo)
 }
 
 /* signal completion unless @done is NULL */
-static void cpu_stop_signal_done(struct cpu_stop_done *done, bool executed)
+static void cpu_stop_signal_done(struct cpu_stop_done *done)
 {
 	if (done) {
-		if (executed)
-			done->executed = true;
 		if (atomic_dec_and_test(&done->nr_todo))
 			complete(&done->completion);
 	}

@@ -92,7 +89,7 @@ static bool cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 	if (enabled)
 		__cpu_stop_queue_work(stopper, work);
 	else
-		cpu_stop_signal_done(work->done, false);
+		cpu_stop_signal_done(work->done);
 	spin_unlock_irqrestore(&stopper->lock, flags);
 
 	return enabled;

@@ -131,7 +128,6 @@ int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg)
 	if (!cpu_stop_queue_work(cpu, &work))
 		return -ENOENT;
 	wait_for_completion(&done.completion);
-	WARN_ON(!done.executed);
 	return done.ret;
 }

@@ -286,7 +282,6 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
 		return -ENOENT;
 
 	wait_for_completion(&done.completion);
-	WARN_ON(!done.executed);
 	return done.ret;
 }

@@ -354,7 +349,6 @@ static int __stop_cpus(const struct cpumask *cpumask,
 	if (!queue_stop_cpus_work(cpumask, fn, arg, &done))
 		return -ENOENT;
 	wait_for_completion(&done.completion);
-	WARN_ON(!done.executed);
 	return done.ret;
 }

@@ -467,6 +461,7 @@ repeat:
 		ret = fn(arg);
 		if (ret && done)
 			done->ret = ret;
+		cpu_stop_signal_done(done);
 
 		/* restore preemption and check it's still balanced */
 		preempt_enable();

@@ -475,7 +470,6 @@ repeat:
 			  kallsyms_lookup((unsigned long)fn, NULL, NULL, NULL,
 					  ksym_buf), arg);
 
-		cpu_stop_signal_done(done, true);
 		goto repeat;
 	}
 }

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-15 18:37 UTC #19 lore raw

PATCH 8/8 stop_machine: cleanup the usage of preemption counter in cpu_stopper_thread()

In reply to Oleg Nesterov

1. Change this code to use preempt_count_inc/preempt_count_dec; this way it works even if CONFIG_PREEMPT_COUNT=n, and we avoid the unnecessary __preempt_schedule() check (stop_sched_class is not preemptible).

And this makes clear that we only want to make preempt_count() != 0 for __might_sleep() / schedule_debug().

2. Change WARN_ONCE() to use %pf to print the function name and remove kallsyms_lookup/ksym_buf.

3. Move "int ret" into the "if (work)" block, this looks more consistent.

Signed-off-by Oleg Nesterov <[email protected]>

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

kernel/stop_machine.c +5 ~~-10~~

index 7ff7ace..6110119 100644
@@ -435,7 +435,6 @@ static void cpu_stopper_thread(unsigned int cpu)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
 	struct cpu_stop_work *work;
-	int ret;
 
 repeat:
 	work = NULL;

@@ -451,23 +450,19 @@ repeat:
 		cpu_stop_fn_t fn = work->fn;
 		void *arg = work->arg;
 		struct cpu_stop_done *done = work->done;
-		char ksym_buf[KSYM_NAME_LEN] __maybe_unused;
+		int ret;
 
-		/* cpu stop callbacks are not allowed to sleep */
-		preempt_disable();
+		/* cpu stop callbacks must not sleep, make in_atomic() == T */
+		preempt_count_inc();
 		ret = fn(arg);
 		if (done) {
 			if (ret)
 				done->ret = ret;
 			cpu_stop_signal_done(done);
 		}
-		/* restore preemption and check it's still balanced */
-		preempt_enable();
+		preempt_count_dec();
 		WARN_ONCE(preempt_count(),
-			  "cpu_stop: %s(%p) leaked preempt count\n",
-			  kallsyms_lookup((unsigned long)fn, NULL, NULL, NULL,
-					  ksym_buf), arg);
-
+			  "cpu_stop: %pf(%p) leaked preempt count\n", fn, arg);
 		goto repeat;
 	}
 }

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-15 18:38 UTC #17 lore raw

PATCH 7/8 stop_machine: shift the done != NULL check from cpu_stop_signal_done() to callers

In reply to Oleg Nesterov

Change cpu_stop_queue_work() and cpu_stopper_thread() to check done != NULL before cpu_stop_signal_done(done). This makes the code more clean imo, note that cpu_stopper_thread() has to do this check anyway.

Signed-off-by Oleg Nesterov <[email protected]>

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

kernel/stop_machine.c +8 ~~-10~~

index 09eb83f..7ff7ace 100644
@@ -64,10 +64,8 @@ static void cpu_stop_init_done(struct cpu_stop_done *done, unsigned int nr_todo)
 /* signal completion unless @done is NULL */
 static void cpu_stop_signal_done(struct cpu_stop_done *done)
 {
-	if (done) {
-		if (atomic_dec_and_test(&done->nr_todo))
-			complete(&done->completion);
-	}
+	if (atomic_dec_and_test(&done->nr_todo))
+		complete(&done->completion);
 }
 
 static void __cpu_stop_queue_work(struct cpu_stopper *stopper,

@@ -88,7 +86,7 @@ static bool cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 	enabled = stopper->enabled;
 	if (enabled)
 		__cpu_stop_queue_work(stopper, work);
-	else
+	else if (work->done)
 		cpu_stop_signal_done(work->done);
 	spin_unlock_irqrestore(&stopper->lock, flags);

@@ -457,12 +455,12 @@ repeat:
 
 		/* cpu stop callbacks are not allowed to sleep */
 		preempt_disable();
-
 		ret = fn(arg);
-		if (ret && done)
-			done->ret = ret;
-		cpu_stop_signal_done(done);
-
+		if (done) {
+			if (ret)
+				done->ret = ret;
+			cpu_stop_signal_done(done);
+		}
 		/* restore preemption and check it's still balanced */
 		preempt_enable();
 		WARN_ONCE(preempt_count(),

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-15 18:38 UTC #7 lore raw

PATCH 3/8 stop_machine: make cpu_stop_queue_work() and stop_one_cpu_nowait() return bool

In reply to Oleg Nesterov

Change cpu_stop_queue_work() to return true if the work was queued and change stop_one_cpu_nowait() to return the result of cpu_stop_queue_work(). This makes it more useful, for example now you can alloc cpu_stop_work for stop_one_cpu_nowait() and free it in the callback or if stop_one_cpu_nowait() fails, currently this is impossible because you can't know if @fn will be called or not.

Also, this allows to kill cpu_stop_done->executed, see the next changes.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/stop_machine.h |    2 +-
 kernel/stop_machine.c        |   16 ++++++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

include/linux/stop_machine.h +1 -1

index 0adedca..40b239a 100644
@@ -29,7 +29,7 @@ struct cpu_stop_work {
 
 int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg);
 int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg);
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			 struct cpu_stop_work *work_buf);
 int stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);
 int try_stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);

kernel/stop_machine.c +12 -4

index 17f01a9..0ec1f16 100644
@@ -81,17 +81,21 @@ static void __cpu_stop_queue_work(struct cpu_stopper *stopper,
 }
 
 /* queue @work to @stopper.  if offline, @work is completed immediately */
-static void cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
+static bool cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
 	unsigned long flags;
+	bool enabled;
 
 	spin_lock_irqsave(&stopper->lock, flags);
-	if (stopper->enabled)
+	enabled = stopper->enabled;
+	if (enabled)
 		__cpu_stop_queue_work(stopper, work);
 	else
 		cpu_stop_signal_done(work->done, false);
 	spin_unlock_irqrestore(&stopper->lock, flags);
+
+	return enabled;
 }
 
 /**

@@ -297,12 +301,16 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
  *
  * CONTEXT:
  * Don't care.
+ *
+ * RETURNS:
+ * true if cpu_stop_work was queued successfully and @fn will be called,
+ * false otherwise.
  */
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			struct cpu_stop_work *work_buf)
 {
 	*work_buf = (struct cpu_stop_work){ .fn = fn, .arg = arg, };
-	cpu_stop_queue_work(cpu, work_buf);
+	return cpu_stop_queue_work(cpu, work_buf);
 }
 
 /* static data for stop_cpus */

Signature · 1 line

1.5.5.1

Peter Zijlstra

2015-11-16 09:44 UTC #22 lore raw

In reply to Oleg Nesterov

Quoted 8 lines · Oleg Nesterov

Hello,
I tried to backport the recent stop_machine fixes and recalled that I forgot to make the fix and cleanups I promised during the previous discussion.
Please review.

Thanks!

Tejun Heo

Acked-by on v1 1/8

2015-11-16 18:52 UTC #2 lore raw

Re: [PATCH 1/8] stop_machine: cpu_stopper_thread() must check done != NULL

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

stop_one_cpu_nowait(fn) will crash the kernel if the callback returns nonzero, work->done == NULL in this case.
This needs more cleanups, cpu_stop_signal_done() is called right after we check done != NULL and it does the same check.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Tejun Heo <[email protected]>

cc stable?

Thanks.

Signature · 1 line

tejun

Tejun Heo

Acked-by on v1 8/8

2015-11-16 19:10 UTC #20 lore raw

Re: [PATCH 8/8] stop_machine: cleanup the usage of preemption counter in cpu_stopper_thread()

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

1. Change this code to use preempt_count_inc/preempt_count_dec; this way it works even if CONFIG_PREEMPT_COUNT=n, and we avoid the unnecessary __preempt_schedule() check (stop_sched_class is not preemptible).
And this makes clear that we only want to make preempt_count() != 0 for __might_sleep() / schedule_debug().

Quoted 6 lines

2. Change WARN_ONCE() to use %pf to print the function name and remove kallsyms_lookup/ksym_buf.
3. Move "int ret" into the "if (work)" block, this looks more consistent.
Signed-off-by Oleg Nesterov <[email protected]>

The entire series looks good to me. Please feel free to add

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

author

2015-11-17 16:09 UTC #8 lore raw

PATCH v2 3/8 stop_machine: make cpu_stop_queue_work() and stop_one_cpu_nowait() return bool

In reply to Oleg Nesterov

include/linux/stop_machine.h struct cpu_stop_work { · line 33

Quoted 10 lines · Oleg Nesterov

include/linux/stop_machine.h +1 -1

@@ -29,7 +29,7 @@ struct cpu_stop_work {
 
 int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg);
 int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg);
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			 struct cpu_stop_work *work_buf);

Peter, sorry!!!

I forgot to update the !CONFIG_SMP version of stop_one_cpu_nowait(). Please see V2 below.

Or please tell me if I should send the incremental patch:

	--- a/include/linux/stop_machine.h
	+++ b/include/linux/stop_machine.h
	@@ -65,7 +65,7 @@ static void stop_one_cpu_nowait_workfn(struct work_struct *work)
		preempt_enable();
	 }
	 
	-static inline void stop_one_cpu_nowait(unsigned int cpu,
	+static inline bool stop_one_cpu_nowait(unsigned int cpu,
					       cpu_stop_fn_t fn, void *arg,
					       struct cpu_stop_work *work_buf)
	 {
	@@ -74,7 +74,10 @@ static inline void stop_one_cpu_nowait(unsigned int cpu,
			work_buf->fn = fn;
			work_buf->arg = arg;
			schedule_work(&work_buf->work);
	+		return true;
		}
	+
	+	return false;
	 }

-------------------------------------------------------------------------------

Quoted 1 line

From d5710e2dca493046257f97ed18699eed4c36a880 Mon Sep 17 00:00:00 2001

From: Oleg Nesterov <[email protected]>
Date: Sat, 14 Nov 2015 19:37:01 +0100
Subject: [PATCH v2 3/8] stop_machine: make cpu_stop_queue_work() and stop_one_cpu_nowait() return bool

Also, this allows to kill cpu_stop_done->executed, see the next changes.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/stop_machine.h |    7 +++++--
 kernel/stop_machine.c        |   16 ++++++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

include/linux/stop_machine.h +5 -2

index 0adedca..9ef42e1 100644
@@ -29,7 +29,7 @@ struct cpu_stop_work {
 
 int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg);
 int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg);
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			 struct cpu_stop_work *work_buf);
 int stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);
 int try_stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);

@@ -65,7 +65,7 @@ static void stop_one_cpu_nowait_workfn(struct work_struct *work)
 	preempt_enable();
 }
 
-static inline void stop_one_cpu_nowait(unsigned int cpu,
+static inline bool stop_one_cpu_nowait(unsigned int cpu,
 				       cpu_stop_fn_t fn, void *arg,
 				       struct cpu_stop_work *work_buf)
 {

@@ -74,7 +74,10 @@ static inline void stop_one_cpu_nowait(unsigned int cpu,
 		work_buf->fn = fn;
 		work_buf->arg = arg;
 		schedule_work(&work_buf->work);
+		return true;
 	}
+
+	return false;
 }
 
 static inline int stop_cpus(const struct cpumask *cpumask,

kernel/stop_machine.c +12 -4

index 17f01a9..0ec1f16 100644
@@ -81,17 +81,21 @@ static void __cpu_stop_queue_work(struct cpu_stopper *stopper,
 }
 
 /* queue @work to @stopper.  if offline, @work is completed immediately */
-static void cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
+static bool cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
 	unsigned long flags;
+	bool enabled;
 
 	spin_lock_irqsave(&stopper->lock, flags);
-	if (stopper->enabled)
+	enabled = stopper->enabled;
+	if (enabled)
 		__cpu_stop_queue_work(stopper, work);
 	else
 		cpu_stop_signal_done(work->done, false);
 	spin_unlock_irqrestore(&stopper->lock, flags);
+
+	return enabled;
 }
 
 /**

@@ -297,12 +301,16 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
  *
  * CONTEXT:
  * Don't care.
+ *
+ * RETURNS:
+ * true if cpu_stop_work was queued successfully and @fn will be called,
+ * false otherwise.
  */
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			struct cpu_stop_work *work_buf)
 {
 	*work_buf = (struct cpu_stop_work){ .fn = fn, .arg = arg, };
-	cpu_stop_queue_work(cpu, work_buf);
+	return cpu_stop_queue_work(cpu, work_buf);
 }
 
 /* static data for stop_cpus */

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-11-17 16:57 UTC #3 lore raw

Re: [PATCH 1/8] stop_machine: cpu_stopper_thread() must check done != NULL

In reply to Tejun Heo

Quoted 11 lines · Tejun Heo

Quoted 7 lines · Oleg Nesterov
stop_one_cpu_nowait(fn) will crash the kernel if the callback returns nonzero, work->done == NULL in this case.
This needs more cleanups, cpu_stop_signal_done() is called right after we check done != NULL and it does the same check.
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Tejun Heo <[email protected]>

Thanks!

Quoted 1 line

cc stable?

Yes, I was going to add -stable initially. And perhaps this makes sense anyway, the patch is really trivial.

OTOH I have checked the history, none of cpu_stop_signal_done's callbacks ever returned non-zero, so I decided to not spam -stable.

Oleg.

Peter Zijlstra

2015-11-17 18:06 UTC #9 lore raw

Re: [PATCH v2 3/8] stop_machine: make cpu_stop_queue_work() and stop_one_cpu_nowait() return bool

In reply to Oleg Nesterov

Quoted 14 lines · Oleg Nesterov

include/linux/stop_machine.h struct cpu_stop_work { · line 33

Quoted 10 lines · Oleg Nesterov

include/linux/stop_machine.h +1 -1

@@ -29,7 +29,7 @@ struct cpu_stop_work {
 
 int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg);
 int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg);
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			 struct cpu_stop_work *work_buf);

Peter, sorry!!!

Quoted 2 lines

I forgot to update the !CONFIG_SMP version of stop_one_cpu_nowait(). Please see V2 below.

No problem, updated the patch.

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:22 UTC #4 lore raw

[tip:sched/core] stop_machine: Fix possible cpu_stopper_thread() crash

In reply to Oleg Nesterov

stop_machine: Fix possible cpu_stopper_thread() crash

Commit-ID:  64038f292a1b33c7d46bd11f62f7798101152c00
Gitweb:     http://git.kernel.org/tip/64038f292a1b33c7d46bd11f62f7798101152c00
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 15 Nov 2015 20:33:11 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:17 +0100

stop_machine: Fix possible cpu_stopper_thread() crash

stop_one_cpu_nowait(fn) will crash the kernel if the callback returns nonzero, work->done == NULL in this case.

This needs more cleanups, cpu_stop_signal_done() is called right after we check done != NULL and it does the same check.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/stop_machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/stop_machine.c +1 -1

index 867bc20..1a66a95 100644
@@ -454,7 +454,7 @@ repeat:
 		preempt_disable();
 
 		ret = fn(arg);
-		if (ret)
+		if (ret && done)
 			done->ret = ret;
 
 		/* restore preemption and check it's still balanced */

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:22 UTC #6 lore raw

[tip:sched/core] stop_machine: Don' t disable preemption in stop_two_cpus()

In reply to Oleg Nesterov

stop_machine: Don't disable preemption in stop_two_cpus()

Commit-ID:  6a19005157c464b47b2082f2617d12bc11198a0d
Gitweb:     http://git.kernel.org/tip/6a19005157c464b47b2082f2617d12bc11198a0d
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 15 Nov 2015 20:33:14 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:18 +0100

stop_machine: Don't disable preemption in stop_two_cpus()

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/stop_machine.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

kernel/stop_machine.c +3 -8

index 1a66a95..17f01a9 100644
@@ -258,7 +258,6 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
 	struct cpu_stop_work work1, work2;
 	struct multi_stop_data msdata;
 
-	preempt_disable();
 	msdata = (struct multi_stop_data){
 		.fn = fn,
 		.data = arg,

@@ -277,16 +276,12 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
 
 	if (cpu1 > cpu2)
 		swap(cpu1, cpu2);
-	if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2)) {
-		preempt_enable();
+	if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
 		return -ENOENT;
-	}
-
-	preempt_enable();
 
 	wait_for_completion(&done.completion);
-
-	return done.executed ? done.ret : -ENOENT;
+	WARN_ON(!done.executed);
+	return done.ret;
 }
 
 /**

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:23 UTC #12 lore raw

[tip:sched/core] stop_machine: Change stop_one_cpu() to rely on cpu_stop_queue_work()

In reply to Oleg Nesterov

stop_machine: Change stop_one_cpu() to rely on cpu_stop_queue_work()

Commit-ID:  958c5f848e17e216df138cc2161b07b7120e2d15
Gitweb:     http://git.kernel.org/tip/958c5f848e17e216df138cc2161b07b7120e2d15
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 15 Nov 2015 20:33:20 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:18 +0100

stop_machine: Change stop_one_cpu() to rely on cpu_stop_queue_work()

Change stop_one_cpu() to return -ENOENT if cpu_stop_queue_work() fails. Otherwise we know that ->executed must be true after wait_for_completion() so we can just return done.ret.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

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

kernel/stop_machine.c +4 -2

index 0ec1f16..68b73c4 100644
@@ -128,9 +128,11 @@ int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg)
 	struct cpu_stop_work work = { .fn = fn, .arg = arg, .done = &done };
 
 	cpu_stop_init_done(&done, 1);
-	cpu_stop_queue_work(cpu, &work);
+	if (!cpu_stop_queue_work(cpu, &work))
+		return -ENOENT;
 	wait_for_completion(&done.completion);
-	return done.executed ? done.ret : -ENOENT;
+	WARN_ON(!done.executed);
+	return done.ret;
 }
 
 /* This controls the threads on each CPU. */

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:23 UTC #14 lore raw

[tip:sched/core] stop_machine: Change __stop_cpus() to rely on cpu_stop_queue_work()

In reply to Oleg Nesterov

stop_machine: Change __stop_cpus() to rely on cpu_stop_queue_work()

Commit-ID:  4aff1ca6970afbf9cd916c34a9c442c8ccba905e
Gitweb:     http://git.kernel.org/tip/4aff1ca6970afbf9cd916c34a9c442c8ccba905e
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 15 Nov 2015 20:33:23 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:19 +0100

stop_machine: Change __stop_cpus() to rely on cpu_stop_queue_work()

Change queue_stop_cpus_work() to return true if it queues at least one work, this means that the caller should wait.

__stop_cpus() can check the value returned by queue_stop_cpus_work() and avoid done.executed, just like stop_one_cpu() does.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/stop_machine.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

kernel/stop_machine.c +10 -4

index 68b73c4..ed2019a 100644
@@ -318,12 +318,13 @@ bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 /* static data for stop_cpus */
 static DEFINE_MUTEX(stop_cpus_mutex);
 
-static void queue_stop_cpus_work(const struct cpumask *cpumask,
+static bool queue_stop_cpus_work(const struct cpumask *cpumask,
 				 cpu_stop_fn_t fn, void *arg,
 				 struct cpu_stop_done *done)
 {
 	struct cpu_stop_work *work;
 	unsigned int cpu;
+	bool queued = false;
 
 	/*
 	 * Disable preemption while queueing to avoid getting

@@ -336,9 +337,12 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 		work->fn = fn;
 		work->arg = arg;
 		work->done = done;
-		cpu_stop_queue_work(cpu, work);
+		if (cpu_stop_queue_work(cpu, work))
+			queued = true;
 	}
 	lg_global_unlock(&stop_cpus_lock);
+
+	return queued;
 }
 
 static int __stop_cpus(const struct cpumask *cpumask,

@@ -347,9 +351,11 @@ static int __stop_cpus(const struct cpumask *cpumask,
 	struct cpu_stop_done done;
 
 	cpu_stop_init_done(&done, cpumask_weight(cpumask));
-	queue_stop_cpus_work(cpumask, fn, arg, &done);
+	if (!queue_stop_cpus_work(cpumask, fn, arg, &done))
+		return -ENOENT;
 	wait_for_completion(&done.completion);
-	return done.executed ? done.ret : -ENOENT;
+	WARN_ON(!done.executed);
+	return done.ret;
 }
 
 /**

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:23 UTC #16 lore raw

[tip:sched/core] stop_machine: Kill cpu_stop_done->executed

In reply to Oleg Nesterov

stop_machine: Kill cpu_stop_done->executed

Commit-ID:  6fa3b826bcb3309157166e6e523a4be236fe267a
Gitweb:     http://git.kernel.org/tip/6fa3b826bcb3309157166e6e523a4be236fe267a
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 15 Nov 2015 20:33:26 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:19 +0100

stop_machine: Kill cpu_stop_done->executed

Now that cpu_stop_done->executed becomes write-only (ignoring WARN_ON() checks) we can remove it.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/stop_machine.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

kernel/stop_machine.c +3 -9

index ed2019a..09eb83f 100644
@@ -28,7 +28,6 @@
  */
 struct cpu_stop_done {
 	atomic_t		nr_todo;	/* nr left to execute */
-	bool			executed;	/* actually executed? */
 	int			ret;		/* collected return value */
 	struct completion	completion;	/* fired if nr_todo reaches 0 */
 };

@@ -63,11 +62,9 @@ static void cpu_stop_init_done(struct cpu_stop_done *done, unsigned int nr_todo)
 }
 
 /* signal completion unless @done is NULL */
-static void cpu_stop_signal_done(struct cpu_stop_done *done, bool executed)
+static void cpu_stop_signal_done(struct cpu_stop_done *done)
 {
 	if (done) {
-		if (executed)
-			done->executed = true;
 		if (atomic_dec_and_test(&done->nr_todo))
 			complete(&done->completion);
 	}

@@ -92,7 +89,7 @@ static bool cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 	if (enabled)
 		__cpu_stop_queue_work(stopper, work);
 	else
-		cpu_stop_signal_done(work->done, false);
+		cpu_stop_signal_done(work->done);
 	spin_unlock_irqrestore(&stopper->lock, flags);
 
 	return enabled;

@@ -131,7 +128,6 @@ int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg)
 	if (!cpu_stop_queue_work(cpu, &work))
 		return -ENOENT;
 	wait_for_completion(&done.completion);
-	WARN_ON(!done.executed);
 	return done.ret;
 }

@@ -286,7 +282,6 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
 		return -ENOENT;
 
 	wait_for_completion(&done.completion);
-	WARN_ON(!done.executed);
 	return done.ret;
 }

@@ -354,7 +349,6 @@ static int __stop_cpus(const struct cpumask *cpumask,
 	if (!queue_stop_cpus_work(cpumask, fn, arg, &done))
 		return -ENOENT;
 	wait_for_completion(&done.completion);
-	WARN_ON(!done.executed);
 	return done.ret;
 }

@@ -467,6 +461,7 @@ repeat:
 		ret = fn(arg);
 		if (ret && done)
 			done->ret = ret;
+		cpu_stop_signal_done(done);
 
 		/* restore preemption and check it's still balanced */
 		preempt_enable();

@@ -475,7 +470,6 @@ repeat:
 			  kallsyms_lookup((unsigned long)fn, NULL, NULL, NULL,
 					  ksym_buf), arg);
 
-		cpu_stop_signal_done(done, true);
 		goto repeat;
 	}
 }

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:23 UTC #10 lore raw

[tip:sched/core] stop_machine: Make cpu_stop_queue_work() and stop_one_cpu_nowait() return bool

In reply to Oleg Nesterov

stop_machine: Make cpu_stop_queue_work() and stop_one_cpu_nowait() return bool

Commit-ID:  1b034bd989aa4a396c13d305759c376c52595a97
Gitweb:     http://git.kernel.org/tip/1b034bd989aa4a396c13d305759c376c52595a97
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Tue, 17 Nov 2015 18:05:23 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:18 +0100

stop_machine: Make cpu_stop_queue_work() and stop_one_cpu_nowait() return bool

Also, this allows to kill cpu_stop_done->executed, see the next changes.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/stop_machine.h |  7 +++++--
 kernel/stop_machine.c        | 16 ++++++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

include/linux/stop_machine.h +5 -2

index 0adedca..9ef42e1 100644
@@ -29,7 +29,7 @@ struct cpu_stop_work {
 
 int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg);
 int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg);
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			 struct cpu_stop_work *work_buf);
 int stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);
 int try_stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg);

@@ -65,7 +65,7 @@ static void stop_one_cpu_nowait_workfn(struct work_struct *work)
 	preempt_enable();
 }
 
-static inline void stop_one_cpu_nowait(unsigned int cpu,
+static inline bool stop_one_cpu_nowait(unsigned int cpu,
 				       cpu_stop_fn_t fn, void *arg,
 				       struct cpu_stop_work *work_buf)
 {

@@ -74,7 +74,10 @@ static inline void stop_one_cpu_nowait(unsigned int cpu,
 		work_buf->fn = fn;
 		work_buf->arg = arg;
 		schedule_work(&work_buf->work);
+		return true;
 	}
+
+	return false;
 }
 
 static inline int stop_cpus(const struct cpumask *cpumask,

kernel/stop_machine.c +12 -4

index 17f01a9..0ec1f16 100644
@@ -81,17 +81,21 @@ static void __cpu_stop_queue_work(struct cpu_stopper *stopper,
 }
 
 /* queue @work to @stopper.  if offline, @work is completed immediately */
-static void cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
+static bool cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
 	unsigned long flags;
+	bool enabled;
 
 	spin_lock_irqsave(&stopper->lock, flags);
-	if (stopper->enabled)
+	enabled = stopper->enabled;
+	if (enabled)
 		__cpu_stop_queue_work(stopper, work);
 	else
 		cpu_stop_signal_done(work->done, false);
 	spin_unlock_irqrestore(&stopper->lock, flags);
+
+	return enabled;
 }
 
 /**

@@ -297,12 +301,16 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
  *
  * CONTEXT:
  * Don't care.
+ *
+ * RETURNS:
+ * true if cpu_stop_work was queued successfully and @fn will be called,
+ * false otherwise.
  */
-void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			struct cpu_stop_work *work_buf)
 {
 	*work_buf = (struct cpu_stop_work){ .fn = fn, .arg = arg, };
-	cpu_stop_queue_work(cpu, work_buf);
+	return cpu_stop_queue_work(cpu, work_buf);
 }
 
 /* static data for stop_cpus */

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:24 UTC #18 lore raw

[tip:sched/core] stop_machine: Shift the 'done != NULL' check from cpu_stop_signal_done() to callers

In reply to Oleg Nesterov

stop_machine: Shift the 'done != NULL' check from cpu_stop_signal_done() to callers

Commit-ID:  dd2e3121e3cb16d03a6e3f2db48f260f046f39c2
Gitweb:     http://git.kernel.org/tip/dd2e3121e3cb16d03a6e3f2db48f260f046f39c2
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 15 Nov 2015 20:33:29 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:19 +0100

stop_machine: Shift the 'done != NULL' check from cpu_stop_signal_done() to callers

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/stop_machine.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

kernel/stop_machine.c +8 ~~-10~~

index 09eb83f..7ff7ace 100644
@@ -64,10 +64,8 @@ static void cpu_stop_init_done(struct cpu_stop_done *done, unsigned int nr_todo)
 /* signal completion unless @done is NULL */
 static void cpu_stop_signal_done(struct cpu_stop_done *done)
 {
-	if (done) {
-		if (atomic_dec_and_test(&done->nr_todo))
-			complete(&done->completion);
-	}
+	if (atomic_dec_and_test(&done->nr_todo))
+		complete(&done->completion);
 }
 
 static void __cpu_stop_queue_work(struct cpu_stopper *stopper,

@@ -88,7 +86,7 @@ static bool cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 	enabled = stopper->enabled;
 	if (enabled)
 		__cpu_stop_queue_work(stopper, work);
-	else
+	else if (work->done)
 		cpu_stop_signal_done(work->done);
 	spin_unlock_irqrestore(&stopper->lock, flags);

@@ -457,12 +455,12 @@ repeat:
 
 		/* cpu stop callbacks are not allowed to sleep */
 		preempt_disable();
-
 		ret = fn(arg);
-		if (ret && done)
-			done->ret = ret;
-		cpu_stop_signal_done(done);
-
+		if (done) {
+			if (ret)
+				done->ret = ret;
+			cpu_stop_signal_done(done);
+		}
 		/* restore preemption and check it's still balanced */
 		preempt_enable();
 		WARN_ONCE(preempt_count(),

tip-bot for Oleg Nesterov

merge bot

2015-11-23 16:24 UTC #21 lore raw

[tip:sched/core] stop_machine: Clean up the usage of the preemption counter in cpu_stopper_thread()

In reply to Oleg Nesterov

stop_machine: Clean up the usage of the preemption counter in cpu_stopper_thread()

Commit-ID:  accaf6ea3db6f5fb997f096b6eefd5431d03f7e5
Gitweb:     http://git.kernel.org/tip/accaf6ea3db6f5fb997f096b6eefd5431d03f7e5
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Sun, 15 Nov 2015 20:33:32 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Nov 2015 09:48:20 +0100

stop_machine: Clean up the usage of the preemption counter in cpu_stopper_thread()

And this makes clear that we only want to make preempt_count() != 0 for __might_sleep() / schedule_debug().

2. Change WARN_ONCE() to use %pf to print the function name and remove kallsyms_lookup/ksym_buf.

3. Move "int ret" into the "if (work)" block, this looks more consistent.

Signed-off-by Oleg Nesterov <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Milos Vyletel <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/stop_machine.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

kernel/stop_machine.c +5 ~~-10~~

index 7ff7ace..6110119 100644
@@ -435,7 +435,6 @@ static void cpu_stopper_thread(unsigned int cpu)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
 	struct cpu_stop_work *work;
-	int ret;
 
 repeat:
 	work = NULL;

@@ -451,23 +450,19 @@ repeat:
 		cpu_stop_fn_t fn = work->fn;
 		void *arg = work->arg;
 		struct cpu_stop_done *done = work->done;
-		char ksym_buf[KSYM_NAME_LEN] __maybe_unused;
+		int ret;
 
-		/* cpu stop callbacks are not allowed to sleep */
-		preempt_disable();
+		/* cpu stop callbacks must not sleep, make in_atomic() == T */
+		preempt_count_inc();
 		ret = fn(arg);
 		if (done) {
 			if (ret)
 				done->ret = ret;
 			cpu_stop_signal_done(done);
 		}
-		/* restore preemption and check it's still balanced */
-		preempt_enable();
+		preempt_count_dec();
 		WARN_ONCE(preempt_count(),
-			  "cpu_stop: %s(%p) leaked preempt count\n",
-			  kallsyms_lookup((unsigned long)fn, NULL, NULL, NULL,
-					  ksym_buf), arg);
-
+			  "cpu_stop: %pf(%p) leaked preempt count\n", fn, arg);
 		goto repeat;
 	}
 }