Messages in this thread | | | Date | Sun, 28 Feb 2010 15:11:35 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 10/43] stop_machine: reimplement without using workqueue |
| |
On 02/26, Tejun Heo wrote: > > +static int stop_cpu(void *unused) > { > enum stopmachine_state curstate = STOPMACHINE_NONE; > - struct stop_machine_data *smdata = &idle; > + struct stop_machine_data *smdata; > int cpu = smp_processor_id(); > int err; > > +repeat: > + /* Wait for __stop_machine() to initiate */ > + while (true) { > + set_current_state(TASK_INTERRUPTIBLE); > + /* <- kthread_stop() and __stop_machine()::smp_wmb() */ > + if (kthread_should_stop()) { > + __set_current_state(TASK_RUNNING); > + return 0; > + } > + if (state == STOPMACHINE_PREPARE) > + break;
Cosmetic nit: this doesn't matter at all, but perhaps it makes sense to set TASK_RUNNING here too.
Actually, I was a bit confused by this "while (true)" loop. It looks as if a spurious wakeup is possible. It is not, and more importantly, if it was possible stop_machine_cpu_callback(CPU_POST_DEAD) (which is called after cpu_hotplug_done()) could race with stop_machine(). stop_machine_cpu_callback(CPU_POST_DEAD) relies on fact that this thread has already called schedule() and it can't be woken until kthread_stop() sets ->should_stop.
> + schedule(); > + } > + smp_rmb(); /* <- __stop_machine()::set_state() */ > + > + /* Okay, let's go */ > + smdata = &idle; > if (!active_cpus) { > if (cpu == cpumask_first(cpu_online_mask)) > smdata = &active;
I never understood why do we need "struct stop_machine_data idle". stop_cpu() just needs a "bool should_call_active_fn" ?
> int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) > { > ... > /* Schedule the stop_cpu work on all cpus: hold this CPU so one > * doesn't hit this CPU until we're ready. */ > get_cpu(); > + for_each_online_cpu(i) > + wake_up_process(*per_cpu_ptr(stop_machine_threads, i));
I think the comment is wrong, and we need preempt_disable() instead of get_cpu(). We shouldn't worry about this CPU, but we need to ensure the woken real-time thread can't preempt us until we wake up them all.
Oleg.
| |