Messages in this thread | | | Date | Tue, 23 Jun 2015 18:20:24 +0200 | From | Oleg Nesterov <> | Subject | Re: [RFC][PATCH 12/13] stop_machine: Remove lglock |
| |
On 06/23, Peter Zijlstra wrote: > > On Tue, Jun 23, 2015 at 12:21:52AM +0200, Oleg Nesterov wrote: > > > Suppose that stop_two_cpus(cpu1 => 0, cpu2 => 1) races with stop_machine(). > > > > - stop_machine takes the lock on CPU 0, adds the work > > and drops the lock > > > > - cpu_stop_queue_work() queues both works > > cpu_stop_queue_work() only ever queues _1_ work. > > > - stop_machine takes the lock on CPU 1, etc > > > > In this case both CPU 0 and 1 will run multi_cpu_stop() but they will > > use different multi_stop_data's, so they will wait for each other > > forever? > > So what you're saying is: > > queue_stop_cpus_work() stop_two_cpus() > > cpu_stop_queue_work(0,..); > spin_lock(0); > spin_lock(1); > > __cpu_stop_queue_work(0,..); > __cpu_stop_queue_work(1,..); > > spin_unlock(1); > spin_unlock(0); > cpu_stop_queue_work(1,..);
Yes, sorry for confusion.
> We can of course slap a percpu-rwsem in, but I wonder if there's > anything smarter we can do here.
I am wondering too if we can make this multi_cpu_stop() more clever. Or at least add some deadlock detection...
Until then you can probably just uglify queue_stop_cpus_work() and avoid the race,
static void queue_stop_cpus_work(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg, struct cpu_stop_done *done) { struct cpu_stopper *stopper; struct cpu_stop_work *work; unsigned long flags; unsigned int cpu;
local_irq_save(flags); for_each_cpu(cpu, cpumask) { stopper = &per_cpu(cpu_stopper, cpu); spin_lock(&stopper->lock);
work = &per_cpu(stop_cpus_work, cpu); work->fn = fn; work->arg = arg; work->done = done; }
for_each_cpu(cpu, cpumask) __cpu_stop_queue_work(cpu, &per_cpu(stop_cpus_work, cpu));
for_each_cpu(cpu, cpumask) { stopper = &per_cpu(cpu_stopper, cpu); spin_unlock(&stopper->lock); } local_irq_restore(flags); }
ignoring lockdep problems.
It would be nice to remove stop_cpus_mutex, it actually protects stop_cpus_work... Then probably stop_two_cpus() can just use stop_cpus(). We could simply make stop_cpus_mutex per-cpu too, but this doesn't look nice.
Oleg.
| |