lkml.org 
[lkml]   [2010]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.



\
 
 \ /
  Last update: 2010-02-28 15:17    [W:0.767 / U:2.420 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site