stop_machine: implement stop_machine_from_offline_cpu()
Subsystems (2)
PATCH 1/1 stop_machine: implement stop_machine_from_offline_cpu()
Hello,
This three patch series implements stop_machine_from_offline_cpu(), which is stop_machine() which can be used from a CPU which isn't yet online.
This is for mtrr which needs stop_machine while bringing up a CPU. It currently implements its own stop_machine directly using stop_one_cpu(). Duplicating such core functionality is wasteful and fragile. e.g. mtrr's implementation currently can deadlock against generic stop_machine().
This patchset is born from Suresh's attempt at implementing similar feature into [__]stop_machine()[1], which IMHO is a bit too subtle. Given the peculiarity of the feature, I think it's better to have dedicated function with an ugly name. Also, this way, the implementation is very well isolated from the rest of stop_cpus machinery and much simpler.
This patchset contains the following three patches.
0001-stop_machine-kill-__stop_machine.patch 0002-stop_machine-reorganize-stop_cpus-implementation.patch 0003-stop_machine-implement-stop_machine_from_offline_cpu.patch
0001 is a cleanup patch which can go in regardless of the rest (with ack from x86 people of course). 0002 is pure code reorganization. 0003 implements the feature.
Suresh, how does this look to you? Can you please put the mtrr changes on top of this and see how it works?
arch/x86/kernel/alternative.c | 5 - include/linux/stop_machine.h | 42 +++------------ kernel/cpu.c | 2 kernel/stop_machine.c | 117 +++++++++++++++++++++++++++++++++++------- 4 files changed, 114 insertions(+), 52 deletions(-)
Thanks.
--
tejun
[1] http://thread.gmane.org/gmane.linux.kernel/1154383/focus=1154384
PATCH 1/3 stop_machine: kill __stop_machine()
stop_machine() is different from __stop_machine() in that it automatically calls get/put_online_cpus() to disable CPU hotplug. For __stop_machine(), the caller is responsible for achieving exclusion against CPU hotplug using either get/put_online_cpus() or cpu_hotplug_begin/done().
However, get_online_cpus() can nest safely inside both another get_online_cpus() or cpu_hotplug_begin(); thus, it's safe to use stop_machine() instead of __stop_machine() making the distinction pointless - the overhead of extra get/put_online_cpus() is negligible compared to stop_machine and they basically become noop if hotplug is in progress.
This patch converts all current __stop_machine() users to stop_machine() and kills __stop_machine(). While at it, move function comment for stop_machine() from function declaration to definition and update it slightly.
--- arch/x86/kernel/alternative.c | 5 ++--- include/linux/stop_machine.h | 34 ++-------------------------------- kernel/cpu.c | 2 +- kernel/stop_machine.c | 38 +++++++++++++++++++++++++++----------- 4 files changed, 32 insertions(+), 47 deletions(-)
arch/x86/kernel/alternative.c +2 -3
@@ -719,8 +719,7 @@ void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len) tpp.nparams =1 ;atomic_set (&stop_machine_first,1 ); wrote_text =0 ; -/* Use __stop_machine() because the caller already got online_cpus. */ -__stop_machine (stop_machine_text_poke, (void *)&tpp, cpu_online_mask); +stop_machine (stop_machine_text_poke, (void *)&tpp, cpu_online_mask);return addr; }
@@ -741,5 +740,5 @@ void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n)atomic_set (&stop_machine_first,1 ); wrote_text =0 ; -__stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); +stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); }
include/linux/stop_machine.h +2 -32
@@ -98,36 +98,12 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,*/ #if defined(CONFIG_STOP_MACHINE ) && defined(CONFIG_SMP ) -/** -* 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 causes a thread to be scheduled on every cpu, -* each of which disables interrupts. The result is that no one is -* holding a spinlock or inside any other preempt-disabled region when -* @fn() runs. -* -* 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); -/** -* __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); -#else /* CONFIG_STOP_MACHINE && CONFIG_SMP */ -static inline int __stop_machine (int (*fn)(void *),void *data, -const struct cpumask *cpus) +static inline int stop_machine (int (*fn)(void *),void *data, +const struct cpumask *cpus) {int ret;local_irq_disable ();
@@ -136,11 +112,5 @@ 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); -} -#endif /* CONFIG_STOP_MACHINE && CONFIG_SMP */ #endif /* _LINUX_STOP_MACHINE */
kernel/cpu.c +1 -1
@@ -235,7 +235,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen) goto out_release; } - 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 +27 -11
@@ -464,24 +464,40 @@ static int stop_machine_cpu_stop(void *data)return err; } -int __stop_machine (int (*fn)(void *),void *data,const struct cpumask *cpus) +/** +* stop_machine - freeze the machine on all online 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) +* +* This causes a thread to be scheduled on every cpu, each of which +* disables interrupts. The result is that no one is holding a spinlock or +* inside any other preempt-disabled region when @fn() runs. +* +* This can be thought of as a very heavy write lock, equivalent to +* grabbing every spinlock in the kernel. +* +* CONTEXT: +* Might sleep. Temporarily stops all online CPUs. +* +* RETURNS: +* 0 if all executions of @fn returned 0, any non zero return value if any +* returned non zero. +*/ +int stop_machine (int (*fn)(void *),void *data,const struct cpumask *cpus) {struct stop_machine_data smdata = { .fn = fn, .data = data, - .num_threads =num_online_cpus (), .active_cpus = cpus }; - -/* Set the initial state and stop all online cpus. */ -set_state (&smdata,STOPMACHINE_PREPARE ); -return stop_cpus (cpu_online_mask, stop_machine_cpu_stop, &smdata); -} - -int stop_machine (int (*fn)(void *),void *data,const struct cpumask *cpus) -{int ret;/* No CPUs can come up or down during this. */ get_online_cpus (); - ret =__stop_machine (fn, data, cpus); + smdata.num_threads =num_online_cpus (), + +/* Set the initial state and stop all online cpus. */ +set_state (&smdata,STOPMACHINE_PREPARE ); + ret =stop_cpus (cpu_online_mask, stop_machine_cpu_stop, &smdata); +put_online_cpus ();return ret; }
Signature · 1 line
1.7.5.2
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
Review void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len) · line 724
Quoted 10 lines · Tejun Heo
+++
b/arch/x86/kernel/alternative.c(quoted diff) +1
-2@@ -719,8 +719,7 @@ void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len) tpp.nparams = 1; atomic_set(&stop_machine_first, 1); wrote_text = 0; - /* Use __stop_machine() because the caller already got online_cpus. */ - __stop_machine(stop_machine_text_poke, (void *)&tpp, cpu_online_mask); + stop_machine(stop_machine_text_poke, (void *)&tpp, cpu_online_mask); return addr; }
Please have a look at:
---
commit d91309f69b7bdb64aeb30106fde8d18c5dd354b5
Author: Peter Zijlstra <[email protected]> Date: Fri Feb 11 22:07:46 2011 +0100
x86: Fix text_poke_smp_batch() deadlock
Fix this deadlock - we are already holding the mutex:
=======================================================
[ INFO: possible circular locking dependency detected ] 2.6.38-rc4-test+ #1
-------------------------------------------------------
bash/1850 is trying to acquire lock:
(text_mutex){+.+.+.}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
but task is already holding lock:
(smp_alt){+.+...}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (smp_alt){+.+...}:
[<ffffffff81082d02>] lock_acquire+0xcd/0xf8
[<ffffffff8192e119>] __mutex_lock_common+0x4c/0x339
[<ffffffff8192e4ca>] mutex_lock_nested+0x3e/0x43
[<ffffffff8101050f>] alternatives_smp_switch+0x77/0x1d8
[<ffffffff81926a6f>] do_boot_cpu+0xd7/0x762
[<ffffffff819277dd>] native_cpu_up+0xe6/0x16a
[<ffffffff81928e28>] _cpu_up+0x9d/0xee
[<ffffffff81928f4c>] cpu_up+0xd3/0xe7
[<ffffffff82268d4b>] kernel_init+0xe8/0x20a
[<ffffffff8100ba24>] kernel_thread_helper+0x4/0x10
-> #1 (cpu_hotplug.lock){+.+.+.}:
[<ffffffff81082d02>] lock_acquire+0xcd/0xf8
[<ffffffff8192e119>] __mutex_lock_common+0x4c/0x339
[<ffffffff8192e4ca>] mutex_lock_nested+0x3e/0x43
[<ffffffff810568cc>] get_online_cpus+0x41/0x55
[<ffffffff810a1348>] stop_machine+0x1e/0x3e
[<ffffffff819314c1>] text_poke_smp_batch+0x3a/0x3c
[<ffffffff81932b6c>] arch_optimize_kprobes+0x10d/0x11c
[<ffffffff81933a51>] kprobe_optimizer+0x152/0x222
[<ffffffff8106bb71>] process_one_work+0x1d3/0x335
[<ffffffff8106cfae>] worker_thread+0x104/0x1a4
[<ffffffff810707c4>] kthread+0x9d/0xa5
[<ffffffff8100ba24>] kernel_thread_helper+0x4/0x10
-> #0 (text_mutex){+.+.+.}:
other info that might help us debug this:
6 locks held by bash/1850:
#0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
#1: (s_active#75){.+.+.+}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
#2: (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
#3: (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
#4: (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
#5: (smp_alt){+.+...}, at: [<ffffffff8100a9c1>] return_to_handler+0x0/0x2f
stack backtrace:
Pid: 1850, comm: bash Not tainted 2.6.38-rc4-test+ #1
Call Trace:
[<ffffffff81080eb2>] print_circular_bug+0xa8/0xb7
[<ffffffff8192e4ca>] mutex_lock_nested+0x3e/0x43
[<ffffffff81010302>] alternatives_smp_unlock+0x3d/0x93
[<ffffffff81010630>] alternatives_smp_switch+0x198/0x1d8
[<ffffffff8102568a>] native_cpu_die+0x65/0x95
[<ffffffff818cc4ec>] _cpu_down+0x13e/0x202
[<ffffffff8117a619>] sysfs_write_file+0x108/0x144
[<ffffffff8111f5a2>] vfs_write+0xac/0xff
[<ffffffff8111f7a9>] sys_write+0x4a/0x6e
Reported-by: Steven Rostedt <[email protected]>
Tested-by: Steven Rostedt <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <1297458466.5226.93.camel@laptop>
Signed-off-by: Ingo Molnar <[email protected]>arch/x86/kernel/alternative.c +1 -1
@@ -671,7 +671,7 @@ void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n)atomic_set (&stop_machine_first,1 ); wrote_text =0 ; -stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); +__stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); }#if defined(CONFIG_DYNAMIC_FTRACE ) || defined(HAVE_JUMP_LABEL )
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
Quoted 14 lines · Peter Zijlstra
Review void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len) · line 724
Quoted 10 lines · Tejun Heo
+++
b/arch/x86/kernel/alternative.c(quoted diff) +1
-2@@ -719,8 +719,7 @@ void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len) tpp.nparams = 1; atomic_set(&stop_machine_first, 1); wrote_text = 0; - /* Use __stop_machine() because the caller already got online_cpus. */ - __stop_machine(stop_machine_text_poke, (void *)&tpp, cpu_online_mask); + stop_machine(stop_machine_text_poke, (void *)&tpp, cpu_online_mask); return addr; }Please have a look at:
Quoted 8 lines
---
commitd91309f69b7bdb64aeb30106fde8d18c5dd354b5Author: Peter Zijlstra <[email protected]> Date: Fri Feb 11 22:07:46 2011 +0100x86: Fix
text_poke_smp_batch()deadlockFix this deadlock - we are already holding the mutex:
Ah, right, I thought cpu_hotplug_begin() sets active_writer and then returns with cpu_hotplug.lock released. Weird locking there. Anyways, this one should be dropped then and I don't think it affects the rest of the series much.
Thanks.
Signature · 1 line
tejun
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
In reply to Peter ZijlstraQuoted 12 lines · Peter Zijlstra
On Tue, 2011-06-14 at 19:06 +0200, Tejun Heo wrote:
Please have a look at:---
commitd91309f69b7bdb64aeb30106fde8d18c5dd354b5Author: Peter Zijlstra <[email protected]> Date: Fri Feb 11 22:07:46 2011 +0100x86: Fix
text_poke_smp_batch()deadlockFix this deadlock - we are already holding the mutex:
...
Quoted 6 lines
arch/x86/kernel/alternative.c +0
-0
arch/x86/kernel/alternative.c void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n) · line 677
Quoted 8 lines
arch/x86/kernel/alternative.c +1
-1@@ -671,7 +671,7 @@ void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n)atomic_set (&stop_machine_first,1 ); wrote_text =0 ; -stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); +__stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); }#if defined(CONFIG_DYNAMIC_FTRACE ) || defined(HAVE_JUMP_LABEL )
Peter, So it looks like we are allowing a new cpu to come online in parallel, while we poke the text? Isn't it a problem? What am I missing?
thanks,
suresh
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
Quoted 26 lines · Suresh Siddha
Quoted 12 lines · Peter Zijlstra
On Tue, 2011-06-14 at 19:06 +0200, Tejun Heo wrote:
Please have a look at:---
commitd91309f69b7bdb64aeb30106fde8d18c5dd354b5Author: Peter Zijlstra <[email protected]> Date: Fri Feb 11 22:07:46 2011 +0100x86: Fix
text_poke_smp_batch()deadlockFix this deadlock - we are already holding the mutex:
...
Quoted 12 lines
arch/x86/kernel/alternative.c +1
-1@@ -671,7 +671,7 @@ void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n)atomic_set (&stop_machine_first,1 ); wrote_text =0 ; -stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); +__stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); }
Quoted 5 lines
arch/x86/kernel/alternative.c void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n) · line 677
Quoted 2 lines
arch/x86/kernel/alternative.c +0
-0#if defined(CONFIG_DYNAMIC_FTRACE ) || defined(HAVE_JUMP_LABEL )Peter, So it looks like we are allowing a new cpu to come online in parallel, while we poke the text? Isn't it a problem? What am I missing?
the caller already did get_online_cpus(),
do_optimize_kprobes()
get_online_cpus()
arch_optimize_kprobes()
text_poke_smp_batch()
put_online_cpus()
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
Quoted 33 lines · Peter Zijlstra
Quoted 26 lines · Suresh Siddha
Quoted 12 lines · Peter Zijlstra
On Tue, 2011-06-14 at 19:06 +0200, Tejun Heo wrote:
Please have a look at:---
commitd91309f69b7bdb64aeb30106fde8d18c5dd354b5Author: Peter Zijlstra <[email protected]> Date: Fri Feb 11 22:07:46 2011 +0100x86: Fix
text_poke_smp_batch()deadlockFix this deadlock - we are already holding the mutex:
...
Quoted 12 lines
arch/x86/kernel/alternative.c +1
-1@@ -671,7 +671,7 @@ void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n)atomic_set (&stop_machine_first,1 ); wrote_text =0 ; -stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); +__stop_machine (stop_machine_text_poke, (void *)&tpp,NULL ); }Quoted 5 lines
arch/x86/kernel/alternative.c void __kprobes text_poke_smp_batch(struct text_poke_param *params, int n) · line 677
Quoted 2 lines
arch/x86/kernel/alternative.c +0
-0#if defined(CONFIG_DYNAMIC_FTRACE ) || defined(HAVE_JUMP_LABEL )Peter, So it looks like we are allowing a new cpu to come online in parallel, while we poke the text? Isn't it a problem? What am I missing?
Quoted 7 lines
the caller already did
get_online_cpus(),
do_optimize_kprobes()
get_online_cpus()
arch_optimize_kprobes()
text_poke_smp_batch()
put_online_cpus()
So the circular dependency reported is not possible in practice right?
Above patch is working around a false positive.
thanks,
suresh
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
Hello,
Quoted 8 lines · Suresh Siddha
Quoted 7 lines · Peter Zijlstra
the caller already did
get_online_cpus(),
do_optimize_kprobes()
get_online_cpus()
arch_optimize_kprobes()
text_poke_smp_batch()
put_online_cpus()
Peter, I don't think it's that simple. get_online_cpus() itself can't create circular dependency by itself. It allows recursing. The chain involves cpu_hotplug_begin() which returns with hotplug mutex held.
Quoted 3 lines
So the circular dependency reported is not possible in practice right?
Above patch is working around a false positive.
But I don't think the lockdep warning is spurious. text_mutex -> hotplug.lock dependency is created during kprobe patching. hotplug.lock -> smp_alt during CPU hotplug, and then the alternative code calling get_online_cpus() creates reverse dependency through get_online_cpus(). So, the reasoning and comment are wrong but grapping hotplug mutex there does create a circular dependency.
This probably can be resolved in prettier way but let's leave it alone for now.
Thanks.
Signature · 1 line
tejun
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
Quoted 3 lines · Tejun Heo
Peter, I don't think it's that simple.
get_online_cpus()itself can't create circular dependency by itself. It allows recursing. The chain involvescpu_hotplug_begin()which returns with hotplug mutex held.
Right, its like:
mutex_lock(&a);
get_online_cpus();
vs
cpu_hotplug_begin()
mutex_lock(&a);
that will really deadlock
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
Quoted 11 lines · Peter Zijlstra
Quoted 3 lines · Tejun Heo
Peter, I don't think it's that simple.
get_online_cpus()itself can't create circular dependency by itself. It allows recursing. The chain involvescpu_hotplug_begin()which returns with hotplug mutex held.Right, its like:
mutex_lock(&a);
get_online_cpus();
Quoted 7 lines
vs
cpu_hotplug_begin()
mutex_lock(&a);that will really deadlock
Its actually like:
get_online_cpus();mutex_lock(&a);get_online_cpus(); <--- recursive get_online_cpus()
vs
cpu_hotplug_begin()mutex_lock(&a);
So it can't cause deadlock in practice.
thanks,
suresh
Re: [PATCH 1/3] stop_machine: kill __stop_machine()
In reply to Suresh SiddhaQuoted 5 lines · Suresh Siddha
Quoted 5 lines
Quoted 2 lines
Peter, So it looks like we are allowing a new cpu to come online in parallel, while we poke the text? Isn't it a problem? What am I missing?
the caller already did
get_online_cpus(),
Quoted 7 lines
Quoted 5 lines
do_optimize_kprobes()
get_online_cpus()
arch_optimize_kprobes()
text_poke_smp_batch()
put_online_cpus()So the circular dependency reported is not possible in practice right?
why not? get_online_cpus() takes a mutex, that connects smp_alt and text_mutex and can cause a deadlock.
PATCH 2/3 stop_machine: reorganize stop_cpus() implementation
In reply to Tejun HeoSplit __stop_cpus() into stop_cpus_queue() and stop_cpus_locked(). The former handles only the queueing part. The latter uses the queue function and is functionally equivalent to __stop_cpus().
The reorganization is to help future improvements to stop_machine() and doesn't introduce any behavior difference.
--- kernel/stop_machine.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
kernel/stop_machine.c +14 -6
@@ -136,10 +136,11 @@ void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,static DEFINE_MUTEX (stop_cpus_mutex); static DEFINE_PER_CPU(struct cpu_stop_work, stop_cpus_work); -int __stop_cpus (const struct cpumask *cpumask,cpu_stop_fn_t fn,void *arg) +static void stop_cpus_queue (const struct cpumask *cpumask, +cpu_stop_fn_t fn,void *arg, +struct cpu_stop_done *done) {struct cpu_stop_work *work; -struct cpu_stop_done done;unsigned int cpu;/* initialize works and done */
@@ -147,9 +148,8 @@ int __stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg) work = &per_cpu (stop_cpus_work, cpu); work->fn = fn; work->arg = arg; - work->done = &done; + work->done = done; } -cpu_stop_init_done (&done,cpumask_weight (cpumask));/* * Disable preemption while queueing to avoid getting
@@ -161,7 +161,15 @@ int __stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg)cpu_stop_queue_work (&per_cpu (cpu_stopper, cpu), &per_cpu (stop_cpus_work, cpu));preempt_enable (); +} +static int stop_cpus_locked (const struct cpumask *cpumask, +cpu_stop_fn_t fn,void *arg) +{ +struct cpu_stop_done done; + +cpu_stop_init_done (&done,cpumask_weight (cpumask)); +stop_cpus_queue (cpumask, fn, arg, &done);wait_for_completion (&done.completion);return done.executed ? done.ret : -ENOENT ; }
@@ -200,7 +208,7 @@ int stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg)/* static works are used, process one request at a time */ mutex_lock (&stop_cpus_mutex); - ret =__stop_cpus (cpumask, fn, arg); + ret =stop_cpus_locked (cpumask, fn, arg);mutex_unlock (&stop_cpus_mutex);return ret; }
@@ -230,7 +238,7 @@ int try_stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg)/* static works are used, process one request at a time */ if (!mutex_trylock (&stop_cpus_mutex))return -EAGAIN ; - ret =__stop_cpus (cpumask, fn, arg); + ret =stop_cpus_locked (cpumask, fn, arg);mutex_unlock (&stop_cpus_mutex);return ret; }
Signature · 1 line
1.7.5.2
PATCH 3/3 stop_machine: implement stop_machine_from_offline_cpu()
In reply to Tejun HeoCurrently, mtrr wants stop_machine functionality while a CPU is being brought up. As stop_machine() requires the calling CPU to be online, mtrr implements its own stop_machine using stop_one_cpu() on each online CPU. This doesn't only unnecessarily duplicate complex logic but also introduces a possibility of deadlock when it races against the generic stop_machine().
This patch implements stop_machine_from_offline_cpu() to serve such use cases. Its functionality is basically the same to stop_machine(); however, it should be called from a CPU which isn't online and doesn't depend on working scheduling on the calling CPU.
This is achieved by using busy loops for synchronization and open-coding stop_cpus queueing and waiting with direct invocation of fn() for local CPU inbetween.
This is inspired from Suresh's patch to implement similar functionality into stop_machine().
http://article.gmane.org/gmane.linux.kernel/1154384
--- include/linux/stop_machine.h | 14 ++++++++- kernel/stop_machine.c | 61 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 3 deletions(-)
include/linux/stop_machine.h +12 -2
@@ -99,18 +99,28 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,#if defined(CONFIG_STOP_MACHINE ) && defined(CONFIG_SMP )int stop_machine (int (*fn)(void *),void *data,const struct cpumask *cpus); +int stop_machine_from_offline_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,const struct cpumask *cpus) { +unsigned long flags;int ret; -local_irq_disable (); + +local_irq_save (flags); ret =fn (data); -local_irq_enable (); +local_irq_restore (flags);return ret; } +static inline int stop_machine_from_offline_cpu (int (*fn)(void *),void *data, +const struct cpumask *cpus) +{ +return stop_machine (fn, data, cpus); +} +#endif /* CONFIG_STOP_MACHINE && CONFIG_SMP */ #endif /* _LINUX_STOP_MACHINE */
kernel/stop_machine.c +60 -1
79 lines not on this page · raw message
Signature · 1 line
1.7.5.2
Quoted 10 lines · Tejun Heo
This three patch series implements
stop_machine_from_offline_cpu(), which isstop_machine()which can be used from a CPU which isn't yet online.This is for mtrr which needs
stop_machinewhile bringing up a CPU. It currently implements its ownstop_machinedirectly usingstop_one_cpu(). Duplicating such core functionality is wasteful and fragile. e.g. mtrr's implementation currently can deadlock against genericstop_machine().
Quoted 6 lines
This patchset is born from Suresh's attempt at implementing similar feature into [__]stop_machine()[1], which IMHO is a bit too subtle. Given the peculiarity of the feature, I think it's better to have dedicated function with an ugly name. Also, this way, the implementation is very well isolated from the rest of
stop_cpusmachinery and much simpler.
Maybe a silly question, but why does mtrr need all this? Surely mtrr can serialize state by other means than stopping all cpus. A simple mutex around the shared state blocking other cpus from updating the mtrr state while we're copying the state to our newly born cpu should cure things.
I'm sure I'm missing something trivial here, but shouldn't we at least explain the details and explore alternatives before merging ugly code?
Hello, Peter.
Quoted 4 lines · Peter Zijlstra
Maybe a silly question, but why does mtrr need all this? Surely mtrr can serialize state by other means than stopping all cpus. A simple mutex around the shared state blocking other cpus from updating the mtrr state while we're copying the state to our newly born cpu should cure things.
Hmmm... good question. I don't know mtrr too well either but the stop-machine requirement seems to directly come from intel's specification. I suppose Suresh can fill us in better.
Thanks.
Signature · 1 line
tejun
Quoted 11 lines · Tejun Heo
Hello, Peter.
Quoted 4 lines · Peter Zijlstra
Maybe a silly question, but why does mtrr need all this? Surely mtrr can serialize state by other means than stopping all cpus. A simple mutex around the shared state blocking other cpus from updating the mtrr state while we're copying the state to our newly born cpu should cure things.
Hmmm... good question. I don't know mtrr too well either but the stop-machine requirement seems to directly come from intel's specification. I suppose Suresh can fill us in better.
Yes, It is coming from SDM guidelines of updating MTRR's. MTRR/PAT change the memory attributes and to ensure that no one else is simultaneously accessing memory while a cpu is changing the attributes of that memory, we need system wide rendezvous. MTRR/PAT is not only updated during cpu online, we allow MTRR's to be changed (and all the cpu's need to be in sync) at runtime too.
For example, change log for the commit d0af9eed5aa91b6b7b5049cae69e5ea956fd85c3 explains a recent issue we encountered.
thanks,
suresh