stop_machine: cleanups and fix

PATCH Superseded Oleg Nesterov 3 participants 15 messages Jun 30, 2015 – Aug 3, 2015

Subsystems (2)

Locking primitives CPU hotplug

lore

Share & tools

lore

Oleg Nesterov

author

2015-06-30 01:31 UTC #0 lore raw

cover stop_machine: cleanups and fix

Quoted 4 lines · Oleg Nesterov

But let me send some cleanups first. Plus I believe I found another stop_machine bug, see the last patch. So I hope these changes make sense in any case.

The last patch fixes the bug, I think. Say, stop_one_cpu(X) can race with _cpu_down(X)->stop_machine() so that the kernel will crash if this CPU X becomes online again.

The window after cpu_stopper_thread() returns and before smpboot_thread() calls ->park() is tiny, but still this is possible afaics.

Oleg.

Oleg Nesterov

author

2015-06-30 01:31 UTC #1 lore raw

PATCH 1/5 stop_machine: move cpu_stopper_task and stop_cpus_work into struct cpu_stopper

Multpiple DEFINE_PER_CPU's do not make sense, move all the per-cpu variables into struct cpu_stopper.

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

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

kernel/stop_machine.c +9 -8

index fd643d8..6e677b0 100644
@@ -35,13 +35,16 @@ struct cpu_stop_done {
 
 /* the actual stopper, one per every possible cpu, enabled on online cpus */
 struct cpu_stopper {
+	struct task_struct	*thread;
+
 	spinlock_t		lock;
 	bool			enabled;	/* is this stopper enabled? */
 	struct list_head	works;		/* list of pending works */
+
+	struct cpu_stop_work	stop_work;	/* for stop_cpus */
 };
 
 static DEFINE_PER_CPU(struct cpu_stopper, cpu_stopper);
-static DEFINE_PER_CPU(struct task_struct *, cpu_stopper_task);
 static bool stop_machine_initialized = false;
 
 /*

@@ -74,7 +77,6 @@ static void cpu_stop_signal_done(struct cpu_stop_done *done, bool executed)
 static void cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
-	struct task_struct *p = per_cpu(cpu_stopper_task, cpu);
 
 	unsigned long flags;

@@ -82,7 +84,7 @@ static void cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 
 	if (stopper->enabled) {
 		list_add_tail(&work->list, &stopper->works);
-		wake_up_process(p);
+		wake_up_process(stopper->thread);
 	} else
 		cpu_stop_signal_done(work->done, false);

@@ -293,7 +295,6 @@ void 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 DEFINE_PER_CPU(struct cpu_stop_work, stop_cpus_work);
 
 static void queue_stop_cpus_work(const struct cpumask *cpumask,
 				 cpu_stop_fn_t fn, void *arg,

@@ -304,7 +305,7 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 
 	/* initialize works and done */
 	for_each_cpu(cpu, cpumask) {
-		work = &per_cpu(stop_cpus_work, cpu);
+		work = &per_cpu(cpu_stopper.stop_work, cpu);
 		work->fn = fn;
 		work->arg = arg;
 		work->done = done;

@@ -317,7 +318,7 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 	 */
 	lg_global_lock(&stop_cpus_lock);
 	for_each_cpu(cpu, cpumask)
-		cpu_stop_queue_work(cpu, &per_cpu(stop_cpus_work, cpu));
+		cpu_stop_queue_work(cpu, &per_cpu(cpu_stopper.stop_work, cpu));
 	lg_global_unlock(&stop_cpus_lock);
 }

@@ -458,7 +459,7 @@ extern void sched_set_stop_task(int cpu, struct task_struct *stop);
 
 static void cpu_stop_create(unsigned int cpu)
 {
-	sched_set_stop_task(cpu, per_cpu(cpu_stopper_task, cpu));
+	sched_set_stop_task(cpu, per_cpu(cpu_stopper.thread, cpu));
 }
 
 static void cpu_stop_park(unsigned int cpu)

@@ -485,7 +486,7 @@ static void cpu_stop_unpark(unsigned int cpu)
 }
 
 static struct smp_hotplug_thread cpu_stop_threads = {
-	.store			= &cpu_stopper_task,
+	.store			= &cpu_stopper.thread,
 	.thread_should_run	= cpu_stop_should_run,
 	.thread_fn		= cpu_stopper_thread,
 	.thread_comm		= "migration/%u",

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-06-30 01:31 UTC #3 lore raw

PATCH 2/5 stop_machine: don't do for_each_cpu() twice in queue_stop_cpus_work()

In reply to Oleg Nesterov

queue_stop_cpus_work() can do everything in one for_each_cpu() loop.

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

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

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

index 6e677b0..6212208 100644
@@ -303,22 +303,19 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 	struct cpu_stop_work *work;
 	unsigned int cpu;
 
-	/* initialize works and done */
-	for_each_cpu(cpu, cpumask) {
-		work = &per_cpu(cpu_stopper.stop_work, cpu);
-		work->fn = fn;
-		work->arg = arg;
-		work->done = done;
-	}
-
 	/*
 	 * Disable preemption while queueing to avoid getting
 	 * preempted by a stopper which might wait for other stoppers
 	 * to enter @fn which can lead to deadlock.
 	 */
 	lg_global_lock(&stop_cpus_lock);
-	for_each_cpu(cpu, cpumask)
-		cpu_stop_queue_work(cpu, &per_cpu(cpu_stopper.stop_work, cpu));
+	for_each_cpu(cpu, cpumask) {
+		work = &per_cpu(cpu_stopper.stop_work, cpu);
+		work->fn = fn;
+		work->arg = arg;
+		work->done = done;
+		cpu_stop_queue_work(cpu, work);
+	}
 	lg_global_unlock(&stop_cpus_lock);
 }

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-06-30 01:31 UTC #7 lore raw

PATCH 4/5 stop_machine: use cpu_stop_fn_t where possible

In reply to Oleg Nesterov

Cosmetic, but cpu_stop_fn_t actually makes the code more readable and it doesn't break cscope. And most of the declarations already use it.

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

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

include/linux/stop_machine.h +4 -4

index 0fca276..414d924 100644
@@ -112,13 +112,13 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
  *
  * This can be thought of as a very heavy write lock, equivalent to
  * grabbing every spinlock in the kernel. */
-int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
+int stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus);
 
-int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
+int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 				   const struct cpumask *cpus);
 #else	 /* CONFIG_STOP_MACHINE && CONFIG_SMP */
 
-static inline int stop_machine(int (*fn)(void *), void *data,
+static inline int stop_machine(cpu_stop_fn_t fn, void *data,
 				 const struct cpumask *cpus)
 {
 	unsigned long flags;

@@ -129,7 +129,7 @@ static inline int stop_machine(int (*fn)(void *), void *data,
 	return ret;
 }
 
-static inline int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
+static inline int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 						 const struct cpumask *cpus)
 {
 	return stop_machine(fn, data, cpus);

kernel/stop_machine.c +4 -4

index b50910d..9a70def 100644
@@ -141,7 +141,7 @@ enum multi_stop_state {
 };
 
 struct multi_stop_data {
-	int			(*fn)(void *);
+	cpu_stop_fn_t		fn;
 	void			*data;
 	/* Like num_online_cpus(), but hotplug cpu uses us, so we need this. */
 	unsigned int		num_threads;

@@ -513,7 +513,7 @@ early_initcall(cpu_stop_init);
 
 #ifdef CONFIG_STOP_MACHINE
 
-static int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
+static int __stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus)
 {
 	struct multi_stop_data msdata = {
 		.fn = fn,

@@ -546,7 +546,7 @@ static int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *c
 	return stop_cpus(cpu_online_mask, multi_cpu_stop, &msdata);
 }
 
-int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
+int stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus)
 {
 	int ret;

@@ -580,7 +580,7 @@ EXPORT_SYMBOL_GPL(stop_machine);
  * 0 if all executions of @fn returned 0, any non zero return value if any
  * returned non zero.
  */
-int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
+int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 				  const struct cpumask *cpus)
 {
 	struct multi_stop_data msdata = { .fn = fn, .data = data,

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-06-30 01:32 UTC #10 lore raw

PATCH 5/5 stop_machine: cpu_stop_park() should remove cpu_stop_work's from list

In reply to Oleg Nesterov

cpu_stop_park() does cpu_stop_signal_done() but leaves the work on stopper->works. The owner of this work can free/reuse this memory right after that and corrupt the list, so if this CPU becomes online again cpu_stopper_thread() will crash.

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 9a70def..12484e5 100644
@@ -462,13 +462,15 @@ static void cpu_stop_create(unsigned int cpu)
 static void cpu_stop_park(unsigned int cpu)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
-	struct cpu_stop_work *work;
+	struct cpu_stop_work *work, *tmp;
 	unsigned long flags;
 
 	/* drain remaining works */
 	spin_lock_irqsave(&stopper->lock, flags);
-	list_for_each_entry(work, &stopper->works, list)
+	list_for_each_entry_safe(work, tmp, &stopper->works, list) {
+		list_del_init(&work->list);
 		cpu_stop_signal_done(work->done, false);
+	}
 	stopper->enabled = false;
 	spin_unlock_irqrestore(&stopper->lock, flags);
 }

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-06-30 01:33 UTC #5 lore raw

PATCH 3/5 stop_machine: unexport __stop_machine()

In reply to Oleg Nesterov

The only caller outside of stop_machine.c is _cpu_down(), it can use stop_machine(). get_online_cpus() is fine under cpu_hotplug_begin().

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

---
 include/linux/stop_machine.h |   22 ++--------------------
 kernel/cpu.c                 |    3 +--
 kernel/stop_machine.c        |    2 +-
 3 files changed, 4 insertions(+), 23 deletions(-)

include/linux/stop_machine.h +2 ~~-20~~

index d2abbdb..0fca276 100644
@@ -114,23 +114,11 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
  * grabbing every spinlock in the kernel. */
 int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
 
-/**
- * __stop_machine: freeze the machine on all CPUs and run this function
- * @fn: the function to run
- * @data: the data ptr for the @fn
- * @cpus: the cpus to run the @fn() on (NULL = any online cpu)
- *
- * Description: This is a special version of the above, which assumes cpus
- * won't come or go while it's being called.  Used by hotplug cpu.
- */
-int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
-
 int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
 				   const struct cpumask *cpus);
-
 #else	 /* CONFIG_STOP_MACHINE && CONFIG_SMP */
 
-static inline int __stop_machine(int (*fn)(void *), void *data,
+static inline int stop_machine(int (*fn)(void *), void *data,
 				 const struct cpumask *cpus)
 {
 	unsigned long flags;

@@ -141,16 +129,10 @@ static inline int __stop_machine(int (*fn)(void *), void *data,
 	return ret;
 }
 
-static inline int stop_machine(int (*fn)(void *), void *data,
-			       const struct cpumask *cpus)
-{
-	return __stop_machine(fn, data, cpus);
-}
-
 static inline int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
 						 const struct cpumask *cpus)
 {
-	return __stop_machine(fn, data, cpus);
+	return stop_machine(fn, data, cpus);
 }
 
 #endif	/* CONFIG_STOP_MACHINE && CONFIG_SMP */

kernel/cpu.c +1 -2

index 94bbe46..68c8324 100644
@@ -394,8 +394,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
 	/*
 	 * So now all preempt/rcu users must observe !cpu_active().
 	 */
-
-	err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
+	err = stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
 	if (err) {
 		/* CPU didn't die: tell everyone.  Can't complain. */
 		smpboot_unpark_threads(cpu);

kernel/stop_machine.c +1 -1

index 6212208..b50910d 100644
@@ -513,7 +513,7 @@ early_initcall(cpu_stop_init);
 
 #ifdef CONFIG_STOP_MACHINE
 
-int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
+static int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 {
 	struct multi_stop_data msdata = {
 		.fn = fn,

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-06-30 12:53 UTC #12 lore raw

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov
But let me send some cleanups first. Plus I believe I found another stop_machine bug, see the last patch. So I hope these changes make sense in any case.

Quoted 6 lines

The last patch fixes the bug, I think. Say, stop_one_cpu(X) can race with _cpu_down(X)->stop_machine() so that the kernel will crash if this CPU X becomes online again.
The window after cpu_stopper_thread() returns and before smpboot_thread() calls ->park() is tiny, but still this is possible afaics.

Now lets try to remove lglocks from stop_machine.c. See the patch at the end. It lacks the comments and the changelog, just for review.

On top of this series, so let me show the code with this patch applied,

	static int cpu_stop_queue_two_works(int cpu1, struct cpu_stop_work *work1,
					    int cpu2, struct cpu_stop_work *work2)
	{
		struct cpu_stopper *stopper1 = per_cpu_ptr(&cpu_stopper, cpu1);
		struct cpu_stopper *stopper2 = per_cpu_ptr(&cpu_stopper, cpu2);
		int err;
	retry:
		spin_lock_irq(&stopper1->lock);
		spin_lock_nested(&stopper2->lock, SINGLE_DEPTH_NESTING);
		err = -ENOENT;
		if (!cpu_active(cpu1) || !cpu_active(cpu2))
			goto unlock;

		BUG_ON(!stopper1->enabled || !stopper2->enabled);

		err = -EDEADLK;
		if (list_empty(&stopper1->stop_work.list) !=
		    list_empty(&stopper2->stop_work.list))
			goto unlock;

		err = 0;
		list_add_tail(&work1->list, &stopper1->works);
		list_add_tail(&work2->list, &stopper2->works);
		wake_up_process(stopper1->thread);
		wake_up_process(stopper2->thread);
	unlock:
		spin_unlock(&stopper2->lock);
		spin_unlock_irq(&stopper1->lock);

		if (unlikely(err == -EDEADLK)) {
			cond_resched();
			goto retry;
		}
		return err;
	}

	int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg)
	{
		struct cpu_stop_done done;
		struct cpu_stop_work work1, work2;
		struct multi_stop_data msdata = {
			.fn = fn,
			.data = arg,
			.num_threads = 2,
			.active_cpus = cpumask_of(cpu1),
		};

		set_state(&msdata, MULTI_STOP_PREPARE);
		cpu_stop_init_done(&done, 2);

		work1.fn   = work2.fn   = multi_cpu_stop;
		work1.arg  = work2.arg  = &msdata;
		work1.done = work2.done = &done;

		if (cpu1 > cpu2)
			swap(cpu1, cpu2);
		if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
			return -ENOENT;

		wait_for_completion(&done.completion);
		BUG_ON(!done.executed);
		return done.ret;
	}

Note the -EDEADLK check in cpu_stop_queue_two_works(). This avoids the race with stop_cpus(). We need to ensure that if we race with stop_cpus() then either stop_cpus() wins and queues both works on these CPU's, or we win this race and queue both works.

The "false positive" -EDEADLK can happen if we race with, say, stop_cpus(cpumask_of(cpu1)). But this is very unlikely (in fact it is always called with cpumask == cpu_online_mask), and in this case we need to wait anyway, so I think the "busy wait" loop can work.

As for cpu_active() checks... This was copied from the current code, but I think they should die later. This needs another cleanup, imo the stopper->enabled logic should be improved.

What do you think?

Oleg.

------------------------------------------------------------------------------
Subject: [PATCH] stop_machine: kill stop_cpus_lock and lg_double_lock/unlock()

---
 include/linux/lglock.h  |    5 ---
 kernel/locking/lglock.c |   22 -----------
 kernel/stop_machine.c   |   92 +++++++++++++++++++++++++---------------------
 3 files changed, 50 insertions(+), 69 deletions(-)

include/linux/lglock.h +0 -5

index c92ebd1..0081f00 100644
@@ -52,15 +52,10 @@ struct lglock {
 	static struct lglock name = { .lock = &name ## _lock }
 
 void lg_lock_init(struct lglock *lg, char *name);
-
 void lg_local_lock(struct lglock *lg);
 void lg_local_unlock(struct lglock *lg);
 void lg_local_lock_cpu(struct lglock *lg, int cpu);
 void lg_local_unlock_cpu(struct lglock *lg, int cpu);
-
-void lg_double_lock(struct lglock *lg, int cpu1, int cpu2);
-void lg_double_unlock(struct lglock *lg, int cpu1, int cpu2);
-
 void lg_global_lock(struct lglock *lg);
 void lg_global_unlock(struct lglock *lg);

kernel/locking/lglock.c +0 ~~-22~~

index 951cfcd..86ae2ae 100644
@@ -60,28 +60,6 @@ void lg_local_unlock_cpu(struct lglock *lg, int cpu)
 }
 EXPORT_SYMBOL(lg_local_unlock_cpu);
 
-void lg_double_lock(struct lglock *lg, int cpu1, int cpu2)
-{
-	BUG_ON(cpu1 == cpu2);
-
-	/* lock in cpu order, just like lg_global_lock */
-	if (cpu2 < cpu1)
-		swap(cpu1, cpu2);
-
-	preempt_disable();
-	lock_acquire_shared(&lg->lock_dep_map, 0, 0, NULL, _RET_IP_);
-	arch_spin_lock(per_cpu_ptr(lg->lock, cpu1));
-	arch_spin_lock(per_cpu_ptr(lg->lock, cpu2));
-}
-
-void lg_double_unlock(struct lglock *lg, int cpu1, int cpu2)
-{
-	lock_release(&lg->lock_dep_map, 1, _RET_IP_);
-	arch_spin_unlock(per_cpu_ptr(lg->lock, cpu1));
-	arch_spin_unlock(per_cpu_ptr(lg->lock, cpu2));
-	preempt_enable();
-}
-
 void lg_global_lock(struct lglock *lg)
 {
 	int i;

kernel/stop_machine.c +50 ~~-42~~

145 lines not on this page · raw message

Signature · 1 line

1.5.5.1

Peter Zijlstra

maintainer

2015-06-30 14:28 UTC #8 lore raw

Re: [PATCH 4/5] stop_machine: use cpu_stop_fn_t where possible

In reply to Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

Cosmetic, but cpu_stop_fn_t actually makes the code more readable and it doesn't break cscope. And most of the declarations already use it.

I made that cpu_stop_f

Oleg Nesterov

author

2015-07-01 19:24 UTC #13 lore raw

In reply to Oleg Nesterov

OK, let me send this patch as 6/5 in reply to 0/5. Added a couple of helpers and the comment.

Please let me know what do you think.

Quoted 324 lines · Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov
But let me send some cleanups first. Plus I believe I found another stop_machine bug, see the last patch. So I hope these changes make sense in any case.

Quoted 6 lines

The last patch fixes the bug, I think. Say, stop_one_cpu(X) can race with _cpu_down(X)->stop_machine() so that the kernel will crash if this CPU X becomes online again.
The window after cpu_stopper_thread() returns and before smpboot_thread() calls ->park() is tiny, but still this is possible afaics.

Now lets try to remove lglocks from stop_machine.c. See the patch at the end. It lacks the comments and the changelog, just for review.

On top of this series, so let me show the code with this patch applied,

	static int cpu_stop_queue_two_works(int cpu1, struct cpu_stop_work *work1,
					    int cpu2, struct cpu_stop_work *work2)
	{
		struct cpu_stopper *stopper1 = per_cpu_ptr(&cpu_stopper, cpu1);
		struct cpu_stopper *stopper2 = per_cpu_ptr(&cpu_stopper, cpu2);
		int err;
	retry:
		spin_lock_irq(&stopper1->lock);
		spin_lock_nested(&stopper2->lock, SINGLE_DEPTH_NESTING);
		err = -ENOENT;
		if (!cpu_active(cpu1) || !cpu_active(cpu2))
			goto unlock;

		BUG_ON(!stopper1->enabled || !stopper2->enabled);

		err = -EDEADLK;
		if (list_empty(&stopper1->stop_work.list) !=
		    list_empty(&stopper2->stop_work.list))
			goto unlock;

		err = 0;
		list_add_tail(&work1->list, &stopper1->works);
		list_add_tail(&work2->list, &stopper2->works);
		wake_up_process(stopper1->thread);
		wake_up_process(stopper2->thread);
	unlock:
		spin_unlock(&stopper2->lock);
		spin_unlock_irq(&stopper1->lock);

		if (unlikely(err == -EDEADLK)) {
			cond_resched();
			goto retry;
		}
		return err;
	}

	int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg)
	{
		struct cpu_stop_done done;
		struct cpu_stop_work work1, work2;
		struct multi_stop_data msdata = {
			.fn = fn,
			.data = arg,
			.num_threads = 2,
			.active_cpus = cpumask_of(cpu1),
		};

		set_state(&msdata, MULTI_STOP_PREPARE);
		cpu_stop_init_done(&done, 2);

		work1.fn   = work2.fn   = multi_cpu_stop;
		work1.arg  = work2.arg  = &msdata;
		work1.done = work2.done = &done;

		if (cpu1 > cpu2)
			swap(cpu1, cpu2);
		if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
			return -ENOENT;

		wait_for_completion(&done.completion);
		BUG_ON(!done.executed);
		return done.ret;
	}

As for cpu_active() checks... This was copied from the current code, but I think they should die later. This needs another cleanup, imo the stopper->enabled logic should be improved.

What do you think?

Oleg.

------------------------------------------------------------------------------
Subject: [PATCH] stop_machine: kill stop_cpus_lock and lg_double_lock/unlock()

---
 include/linux/lglock.h  |    5 ---
 kernel/locking/lglock.c |   22 -----------
 kernel/stop_machine.c   |   92 +++++++++++++++++++++++++---------------------
 3 files changed, 50 insertions(+), 69 deletions(-)

include/linux/lglock.h +0 -5

index c92ebd1..0081f00 100644
@@ -52,15 +52,10 @@ struct lglock {
 	static struct lglock name = { .lock = &name ## _lock }
 
 void lg_lock_init(struct lglock *lg, char *name);
-
 void lg_local_lock(struct lglock *lg);
 void lg_local_unlock(struct lglock *lg);
 void lg_local_lock_cpu(struct lglock *lg, int cpu);
 void lg_local_unlock_cpu(struct lglock *lg, int cpu);
-
-void lg_double_lock(struct lglock *lg, int cpu1, int cpu2);
-void lg_double_unlock(struct lglock *lg, int cpu1, int cpu2);
-
 void lg_global_lock(struct lglock *lg);
 void lg_global_unlock(struct lglock *lg);

kernel/locking/lglock.c +0 ~~-22~~

index 951cfcd..86ae2ae 100644
@@ -60,28 +60,6 @@ void lg_local_unlock_cpu(struct lglock *lg, int cpu)
 }
 EXPORT_SYMBOL(lg_local_unlock_cpu);
 
-void lg_double_lock(struct lglock *lg, int cpu1, int cpu2)
-{
-	BUG_ON(cpu1 == cpu2);
-
-	/* lock in cpu order, just like lg_global_lock */
-	if (cpu2 < cpu1)
-		swap(cpu1, cpu2);
-
-	preempt_disable();
-	lock_acquire_shared(&lg->lock_dep_map, 0, 0, NULL, _RET_IP_);
-	arch_spin_lock(per_cpu_ptr(lg->lock, cpu1));
-	arch_spin_lock(per_cpu_ptr(lg->lock, cpu2));
-}
-
-void lg_double_unlock(struct lglock *lg, int cpu1, int cpu2)
-{
-	lock_release(&lg->lock_dep_map, 1, _RET_IP_);
-	arch_spin_unlock(per_cpu_ptr(lg->lock, cpu1));
-	arch_spin_unlock(per_cpu_ptr(lg->lock, cpu2));
-	preempt_enable();
-}
-
 void lg_global_lock(struct lglock *lg)
 {
 	int i;

kernel/stop_machine.c +50 ~~-42~~

index 12484e5..d53c86c 100644
@@ -20,7 +20,6 @@
 #include <linux/kallsyms.h>
 #include <linux/smpboot.h>
 #include <linux/atomic.h>
-#include <linux/lglock.h>
 
 /*
  * Structure to determine completion condition and record errors.  May

@@ -47,14 +46,6 @@ struct cpu_stopper {
 static DEFINE_PER_CPU(struct cpu_stopper, cpu_stopper);
 static bool stop_machine_initialized = false;
 
-/*
- * Avoids a race between stop_two_cpus and global stop_cpus, where
- * the stoppers could get queued up in reverse order, leading to
- * system deadlock. Using an lglock means stop_two_cpus remains
- * relatively cheap.
- */
-DEFINE_STATIC_LGLOCK(stop_cpus_lock);
-
 static void cpu_stop_init_done(struct cpu_stop_done *done, unsigned int nr_todo)
 {
 	memset(done, 0, sizeof(*done));

@@ -213,6 +204,42 @@ static int multi_cpu_stop(void *data)
 	return err;
 }
 
+static int cpu_stop_queue_two_works(int cpu1, struct cpu_stop_work *work1,
+				    int cpu2, struct cpu_stop_work *work2)
+{
+	struct cpu_stopper *stopper1 = per_cpu_ptr(&cpu_stopper, cpu1);
+	struct cpu_stopper *stopper2 = per_cpu_ptr(&cpu_stopper, cpu2);
+	int err;
+retry:
+	spin_lock_irq(&stopper1->lock);
+	spin_lock_nested(&stopper2->lock, SINGLE_DEPTH_NESTING);
+	err = -ENOENT;
+	if (!cpu_active(cpu1) || !cpu_active(cpu2))
+		goto unlock;
+
+	BUG_ON(!stopper1->enabled || !stopper2->enabled);
+
+	err = -EDEADLK;
+	if (list_empty(&stopper1->stop_work.list) !=
+	    list_empty(&stopper2->stop_work.list))
+		goto unlock;
+
+	err = 0;
+	list_add_tail(&work1->list, &stopper1->works);
+	list_add_tail(&work2->list, &stopper2->works);
+	wake_up_process(stopper1->thread);
+	wake_up_process(stopper2->thread);
+unlock:
+	spin_unlock(&stopper2->lock);
+	spin_unlock_irq(&stopper1->lock);
+
+	if (unlikely(err == -EDEADLK)) {
+		cond_resched();
+		goto retry;
+	}
+	return err;
+}
+
 /**
  * stop_two_cpus - stops two cpus
  * @cpu1: the cpu to stop

@@ -228,48 +255,28 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
 {
 	struct cpu_stop_done done;
 	struct cpu_stop_work work1, work2;
-	struct multi_stop_data msdata;
-
-	preempt_disable();
-	msdata = (struct multi_stop_data){
+	struct multi_stop_data msdata = {
 		.fn = fn,
 		.data = arg,
 		.num_threads = 2,
 		.active_cpus = cpumask_of(cpu1),
 	};
 
-	work1 = work2 = (struct cpu_stop_work){
-		.fn = multi_cpu_stop,
-		.arg = &msdata,
-		.done = &done
-	};
-
-	cpu_stop_init_done(&done, 2);
 	set_state(&msdata, MULTI_STOP_PREPARE);
+	cpu_stop_init_done(&done, 2);
 
-	/*
-	 * If we observe both CPUs active we know _cpu_down() cannot yet have
-	 * queued its stop_machine works and therefore ours will get executed
-	 * first. Or its not either one of our CPUs that's getting unplugged,
-	 * in which case we don't care.
-	 *
-	 * This relies on the stopper workqueues to be FIFO.
-	 */
-	if (!cpu_active(cpu1) || !cpu_active(cpu2)) {
-		preempt_enable();
-		return -ENOENT;
-	}
-
-	lg_double_lock(&stop_cpus_lock, cpu1, cpu2);
-	cpu_stop_queue_work(cpu1, &work1);
-	cpu_stop_queue_work(cpu2, &work2);
-	lg_double_unlock(&stop_cpus_lock, cpu1, cpu2);
+	work1.fn   = work2.fn   = multi_cpu_stop;
+	work1.arg  = work2.arg  = &msdata;
+	work1.done = work2.done = &done;
 
-	preempt_enable();
+	if (cpu1 > cpu2)
+		swap(cpu1, cpu2);
+	if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
+		return -ENOENT;
 
 	wait_for_completion(&done.completion);
-
-	return done.executed ? done.ret : -ENOENT;
+	BUG_ON(!done.executed);
+	return done.ret;
 }
 
 /**

@@ -308,7 +315,7 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 	 * preempted by a stopper which might wait for other stoppers
 	 * to enter @fn which can lead to deadlock.
 	 */
-	lg_global_lock(&stop_cpus_lock);
+	preempt_disable();
 	for_each_cpu(cpu, cpumask) {
 		work = &per_cpu(cpu_stopper.stop_work, cpu);
 		work->fn = fn;

@@ -316,7 +323,7 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 		work->done = done;
 		cpu_stop_queue_work(cpu, work);
 	}
-	lg_global_unlock(&stop_cpus_lock);
+	preempt_enable();
 }
 
 static int __stop_cpus(const struct cpumask *cpumask,

@@ -505,6 +512,7 @@ static int __init cpu_stop_init(void)
 
 		spin_lock_init(&stopper->lock);
 		INIT_LIST_HEAD(&stopper->works);
+		INIT_LIST_HEAD(&stopper->stop_work.list);
 	}
 
 	BUG_ON(smpboot_register_percpu_thread(&cpu_stop_threads));

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2015-07-01 19:25 UTC #14 lore raw

[PATCH 6/5] stop_machine: kill stop_cpus_lock and lg_double_lock/unlock()

In reply to Oleg Nesterov

stop_two_cpus() and stop_cpus() use stop_cpus_lock to avoid the deadlock, we need to ensure that the stopper functions can't be queued "backwards" from one another.

Instead, we can change stop_two_cpus() to take 2 stopper->lock's and queue both works "atomically"; just we need to check that both ->stop_work's on these CPU's are either free or already queued.

Note: this patch preserves the cpu_active() checks, but I think we need to shift them into migrate_swap_stop(). However, we can't do this without another cleanup: currently stopper->enabled does not guarantee that work->fn() will be actually executed if we race with cpu_down().

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

---
 include/linux/lglock.h  |    5 --
 kernel/locking/lglock.c |   22 ---------
 kernel/stop_machine.c   |  120 ++++++++++++++++++++++++++++-------------------
 3 files changed, 71 insertions(+), 76 deletions(-)

include/linux/lglock.h +0 -5

index c92ebd1..0081f00 100644
@@ -52,15 +52,10 @@ struct lglock {
 	static struct lglock name = { .lock = &name ## _lock }
 
 void lg_lock_init(struct lglock *lg, char *name);
-
 void lg_local_lock(struct lglock *lg);
 void lg_local_unlock(struct lglock *lg);
 void lg_local_lock_cpu(struct lglock *lg, int cpu);
 void lg_local_unlock_cpu(struct lglock *lg, int cpu);
-
-void lg_double_lock(struct lglock *lg, int cpu1, int cpu2);
-void lg_double_unlock(struct lglock *lg, int cpu1, int cpu2);
-
 void lg_global_lock(struct lglock *lg);
 void lg_global_unlock(struct lglock *lg);

kernel/locking/lglock.c +0 ~~-22~~

index 951cfcd..86ae2ae 100644
@@ -60,28 +60,6 @@ void lg_local_unlock_cpu(struct lglock *lg, int cpu)
 }
 EXPORT_SYMBOL(lg_local_unlock_cpu);
 
-void lg_double_lock(struct lglock *lg, int cpu1, int cpu2)
-{
-	BUG_ON(cpu1 == cpu2);
-
-	/* lock in cpu order, just like lg_global_lock */
-	if (cpu2 < cpu1)
-		swap(cpu1, cpu2);
-
-	preempt_disable();
-	lock_acquire_shared(&lg->lock_dep_map, 0, 0, NULL, _RET_IP_);
-	arch_spin_lock(per_cpu_ptr(lg->lock, cpu1));
-	arch_spin_lock(per_cpu_ptr(lg->lock, cpu2));
-}
-
-void lg_double_unlock(struct lglock *lg, int cpu1, int cpu2)
-{
-	lock_release(&lg->lock_dep_map, 1, _RET_IP_);
-	arch_spin_unlock(per_cpu_ptr(lg->lock, cpu1));
-	arch_spin_unlock(per_cpu_ptr(lg->lock, cpu2));
-	preempt_enable();
-}
-
 void lg_global_lock(struct lglock *lg)
 {
 	int i;

kernel/stop_machine.c +71 ~~-49~~

187 lines not on this page · raw message

Signature · 1 line

1.5.5.1

tip-bot for Oleg Nesterov

merge bot

2015-08-03 17:09 UTC #2 lore raw

[tip:sched/core] stop_machine: Move 'cpu_stopper_task' and ' stop_cpus_work' into 'struct cpu_stopper'

In reply to Oleg Nesterov

stop_machine: Move 'cpu_stopper_task' and 'stop_cpus_work' into 'struct cpu_stopper'

Commit-ID:  02cb7aa923ec553e6454ec766ded27b472326ebe
Gitweb:     http://git.kernel.org/tip/02cb7aa923ec553e6454ec766ded27b472326ebe
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Tue, 30 Jun 2015 03:29:44 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 3 Aug 2015 12:21:25 +0200

stop_machine: Move 'cpu_stopper_task' and 'stop_cpus_work' into 'struct cpu_stopper'

Multpiple DEFINE_PER_CPU's do not make sense, move all the per-cpu variables into 'struct cpu_stopper'.

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

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

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [email protected]

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

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

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

kernel/stop_machine.c +9 -8

index fd643d8..6e677b0 100644
@@ -35,13 +35,16 @@ struct cpu_stop_done {
 
 /* the actual stopper, one per every possible cpu, enabled on online cpus */
 struct cpu_stopper {
+	struct task_struct	*thread;
+
 	spinlock_t		lock;
 	bool			enabled;	/* is this stopper enabled? */
 	struct list_head	works;		/* list of pending works */
+
+	struct cpu_stop_work	stop_work;	/* for stop_cpus */
 };
 
 static DEFINE_PER_CPU(struct cpu_stopper, cpu_stopper);
-static DEFINE_PER_CPU(struct task_struct *, cpu_stopper_task);
 static bool stop_machine_initialized = false;
 
 /*

@@ -74,7 +77,6 @@ static void cpu_stop_signal_done(struct cpu_stop_done *done, bool executed)
 static void cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
-	struct task_struct *p = per_cpu(cpu_stopper_task, cpu);
 
 	unsigned long flags;

@@ -82,7 +84,7 @@ static void cpu_stop_queue_work(unsigned int cpu, struct cpu_stop_work *work)
 
 	if (stopper->enabled) {
 		list_add_tail(&work->list, &stopper->works);
-		wake_up_process(p);
+		wake_up_process(stopper->thread);
 	} else
 		cpu_stop_signal_done(work->done, false);

@@ -293,7 +295,6 @@ void 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 DEFINE_PER_CPU(struct cpu_stop_work, stop_cpus_work);
 
 static void queue_stop_cpus_work(const struct cpumask *cpumask,
 				 cpu_stop_fn_t fn, void *arg,

@@ -304,7 +305,7 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 
 	/* initialize works and done */
 	for_each_cpu(cpu, cpumask) {
-		work = &per_cpu(stop_cpus_work, cpu);
+		work = &per_cpu(cpu_stopper.stop_work, cpu);
 		work->fn = fn;
 		work->arg = arg;
 		work->done = done;

@@ -317,7 +318,7 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 	 */
 	lg_global_lock(&stop_cpus_lock);
 	for_each_cpu(cpu, cpumask)
-		cpu_stop_queue_work(cpu, &per_cpu(stop_cpus_work, cpu));
+		cpu_stop_queue_work(cpu, &per_cpu(cpu_stopper.stop_work, cpu));
 	lg_global_unlock(&stop_cpus_lock);
 }

@@ -458,7 +459,7 @@ extern void sched_set_stop_task(int cpu, struct task_struct *stop);
 
 static void cpu_stop_create(unsigned int cpu)
 {
-	sched_set_stop_task(cpu, per_cpu(cpu_stopper_task, cpu));
+	sched_set_stop_task(cpu, per_cpu(cpu_stopper.thread, cpu));
 }
 
 static void cpu_stop_park(unsigned int cpu)

@@ -485,7 +486,7 @@ static void cpu_stop_unpark(unsigned int cpu)
 }
 
 static struct smp_hotplug_thread cpu_stop_threads = {
-	.store			= &cpu_stopper_task,
+	.store			= &cpu_stopper.thread,
 	.thread_should_run	= cpu_stop_should_run,
 	.thread_fn		= cpu_stopper_thread,
 	.thread_comm		= "migration/%u",

tip-bot for Oleg Nesterov

merge bot

2015-08-03 17:09 UTC #4 lore raw

[tip:sched/core] stop_machine: Don't do for_each_cpu() twice in queue_stop_cpus_work()

In reply to Oleg Nesterov

stop_machine: Don't do for_each_cpu() twice in queue_stop_cpus_work()

Commit-ID:  b377c2a089d4538e6e62e51fa595c896c314d83d
Gitweb:     http://git.kernel.org/tip/b377c2a089d4538e6e62e51fa595c896c314d83d
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Tue, 30 Jun 2015 03:29:48 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 3 Aug 2015 12:21:26 +0200

stop_machine: Don't do for_each_cpu() twice in queue_stop_cpus_work()

queue_stop_cpus_work() can do everything in one for_each_cpu() loop.

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

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

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [email protected]

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

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

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

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

index 6e677b0..6212208 100644
@@ -303,22 +303,19 @@ static void queue_stop_cpus_work(const struct cpumask *cpumask,
 	struct cpu_stop_work *work;
 	unsigned int cpu;
 
-	/* initialize works and done */
-	for_each_cpu(cpu, cpumask) {
-		work = &per_cpu(cpu_stopper.stop_work, cpu);
-		work->fn = fn;
-		work->arg = arg;
-		work->done = done;
-	}
-
 	/*
 	 * Disable preemption while queueing to avoid getting
 	 * preempted by a stopper which might wait for other stoppers
 	 * to enter @fn which can lead to deadlock.
 	 */
 	lg_global_lock(&stop_cpus_lock);
-	for_each_cpu(cpu, cpumask)
-		cpu_stop_queue_work(cpu, &per_cpu(cpu_stopper.stop_work, cpu));
+	for_each_cpu(cpu, cpumask) {
+		work = &per_cpu(cpu_stopper.stop_work, cpu);
+		work->fn = fn;
+		work->arg = arg;
+		work->done = done;
+		cpu_stop_queue_work(cpu, work);
+	}
 	lg_global_unlock(&stop_cpus_lock);
 }

tip-bot for Oleg Nesterov

merge bot

2015-08-03 17:10 UTC #6 lore raw

[tip:sched/core] stop_machine: Unexport __stop_machine()

In reply to Oleg Nesterov

stop_machine: Unexport __stop_machine()

Commit-ID:  7eeb088e72048bf4660f64fc3824c8066cf17591
Gitweb:     http://git.kernel.org/tip/7eeb088e72048bf4660f64fc3824c8066cf17591
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Tue, 30 Jun 2015 03:29:51 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 3 Aug 2015 12:21:26 +0200

stop_machine: Unexport __stop_machine()

The only caller outside of stop_machine.c is _cpu_down(), it can use stop_machine(). get_online_cpus() is fine under cpu_hotplug_begin().

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

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

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [email protected]

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

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

---
 include/linux/stop_machine.h | 22 ++--------------------
 kernel/cpu.c                 |  2 +-
 kernel/stop_machine.c        |  2 +-
 3 files changed, 4 insertions(+), 22 deletions(-)

include/linux/stop_machine.h +2 ~~-20~~

index d2abbdb..0fca276 100644
@@ -114,23 +114,11 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
  * grabbing every spinlock in the kernel. */
 int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
 
-/**
- * __stop_machine: freeze the machine on all CPUs and run this function
- * @fn: the function to run
- * @data: the data ptr for the @fn
- * @cpus: the cpus to run the @fn() on (NULL = any online cpu)
- *
- * Description: This is a special version of the above, which assumes cpus
- * won't come or go while it's being called.  Used by hotplug cpu.
- */
-int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
-
 int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
 				   const struct cpumask *cpus);
-
 #else	 /* CONFIG_STOP_MACHINE && CONFIG_SMP */
 
-static inline int __stop_machine(int (*fn)(void *), void *data,
+static inline int stop_machine(int (*fn)(void *), void *data,
 				 const struct cpumask *cpus)
 {
 	unsigned long flags;

@@ -141,16 +129,10 @@ static inline int __stop_machine(int (*fn)(void *), void *data,
 	return ret;
 }
 
-static inline int stop_machine(int (*fn)(void *), void *data,
-			       const struct cpumask *cpus)
-{
-	return __stop_machine(fn, data, cpus);
-}
-
 static inline int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
 						 const struct cpumask *cpus)
 {
-	return __stop_machine(fn, data, cpus);
+	return stop_machine(fn, data, cpus);
 }
 
 #endif	/* CONFIG_STOP_MACHINE && CONFIG_SMP */

kernel/cpu.c +1 -1

index 9c9c9fa..664ce52 100644
@@ -395,7 +395,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
 	 * So now all preempt/rcu users must observe !cpu_active().
 	 */
 
-	err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
+	err = stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
 	if (err) {
 		/* CPU didn't die: tell everyone.  Can't complain. */
 		cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);

kernel/stop_machine.c +1 -1

index 6212208..b50910d 100644
@@ -513,7 +513,7 @@ early_initcall(cpu_stop_init);
 
 #ifdef CONFIG_STOP_MACHINE
 
-int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
+static int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 {
 	struct multi_stop_data msdata = {
 		.fn = fn,

tip-bot for Oleg Nesterov

merge bot

2015-08-03 17:10 UTC #9 lore raw

[tip:sched/core] stop_machine: Use 'cpu_stop_fn_t' where possible

In reply to Oleg Nesterov

stop_machine: Use 'cpu_stop_fn_t' where possible

Commit-ID:  9a301f22faac7fc2207ee49c1855a6b4ba9c5a52
Gitweb:     http://git.kernel.org/tip/9a301f22faac7fc2207ee49c1855a6b4ba9c5a52
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Tue, 30 Jun 2015 03:29:55 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 3 Aug 2015 12:21:27 +0200

stop_machine: Use 'cpu_stop_fn_t' where possible

Cosmetic, but 'cpu_stop_fn_t' actually makes the code more readable and it doesn't break cscope. And most of the declarations already use it.

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

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

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [email protected]

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

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

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

include/linux/stop_machine.h +4 -4

index 0fca276..414d924 100644
@@ -112,13 +112,13 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
  *
  * This can be thought of as a very heavy write lock, equivalent to
  * grabbing every spinlock in the kernel. */
-int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
+int stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus);
 
-int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
+int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 				   const struct cpumask *cpus);
 #else	 /* CONFIG_STOP_MACHINE && CONFIG_SMP */
 
-static inline int stop_machine(int (*fn)(void *), void *data,
+static inline int stop_machine(cpu_stop_fn_t fn, void *data,
 				 const struct cpumask *cpus)
 {
 	unsigned long flags;

@@ -129,7 +129,7 @@ static inline int stop_machine(int (*fn)(void *), void *data,
 	return ret;
 }
 
-static inline int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
+static inline int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 						 const struct cpumask *cpus)
 {
 	return stop_machine(fn, data, cpus);

kernel/stop_machine.c +4 -4

index b50910d..9a70def 100644
@@ -141,7 +141,7 @@ enum multi_stop_state {
 };
 
 struct multi_stop_data {
-	int			(*fn)(void *);
+	cpu_stop_fn_t		fn;
 	void			*data;
 	/* Like num_online_cpus(), but hotplug cpu uses us, so we need this. */
 	unsigned int		num_threads;

@@ -513,7 +513,7 @@ early_initcall(cpu_stop_init);
 
 #ifdef CONFIG_STOP_MACHINE
 
-static int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
+static int __stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus)
 {
 	struct multi_stop_data msdata = {
 		.fn = fn,

@@ -546,7 +546,7 @@ static int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *c
 	return stop_cpus(cpu_online_mask, multi_cpu_stop, &msdata);
 }
 
-int stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
+int stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus)
 {
 	int ret;

@@ -580,7 +580,7 @@ EXPORT_SYMBOL_GPL(stop_machine);
  * 0 if all executions of @fn returned 0, any non zero return value if any
  * returned non zero.
  */
-int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
+int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 				  const struct cpumask *cpus)
 {
 	struct multi_stop_data msdata = { .fn = fn, .data = data,

tip-bot for Oleg Nesterov

merge bot

2015-08-03 17:10 UTC #11 lore raw

[tip:sched/core] stop_machine: Remove cpu_stop_work' s from list in cpu_stop_park()

In reply to Oleg Nesterov

stop_machine: Remove cpu_stop_work's from list in cpu_stop_park()

Commit-ID:  d308b9f1e4412bcf583c82c4ca15ef97cb8b0e6f
Gitweb:     http://git.kernel.org/tip/d308b9f1e4412bcf583c82c4ca15ef97cb8b0e6f
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Tue, 30 Jun 2015 03:29:58 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 3 Aug 2015 12:21:28 +0200

stop_machine: Remove cpu_stop_work's from list in cpu_stop_park()

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

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

Cc Linus Torvalds <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [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 9a70def..12484e5 100644
@@ -462,13 +462,15 @@ static void cpu_stop_create(unsigned int cpu)
 static void cpu_stop_park(unsigned int cpu)
 {
 	struct cpu_stopper *stopper = &per_cpu(cpu_stopper, cpu);
-	struct cpu_stop_work *work;
+	struct cpu_stop_work *work, *tmp;
 	unsigned long flags;
 
 	/* drain remaining works */
 	spin_lock_irqsave(&stopper->lock, flags);
-	list_for_each_entry(work, &stopper->works, list)
+	list_for_each_entry_safe(work, tmp, &stopper->works, list) {
+		list_del_init(&work->list);
 		cpu_stop_signal_done(work->done, false);
+	}
 	stopper->enabled = false;
 	spin_unlock_irqrestore(&stopper->lock, flags);
 }