Re: [patch v2 1/2] stop_machine: enable __stop_machine() to be called from the cpu online path

[Tejun Heo <tj@kernel.org>]

Hello, Suresh, Ingo.

On Tue, Jun 07, 2011 at 01:14:12PM -0700, Suresh Siddha wrote:
> Currently stop machine infrastructure can be called only from a cpu that is
> online. But for !CONFIG_SMP, we do allow calling __stop_machine() before the
> cpu is online.
> 
> x86 for example requires stop machine infrastructure in the cpu online path
> and currently implements its own stop machine (using stop_one_cpu_nowait())
> for MTRR initialization in the cpu online path.
> 
> Enhance the __stop_machine() so that it can be called before the cpu
> is onlined. This will pave the way for code consolidation and address potential
> deadlocks caused by multiple mechanisms of doing system wide rendezvous.
> 
> This will also address the behavioral differences of __stop_machine()
> between SMP and UP builds.
> 
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Cc: stable@kernel.org    # v2.6.35+

First of all, I agree this is the correct thing to do but I don't
agree with some of the details.  Also, this is slightly scary for
-stable.  Maybe we should opt for something less intrusive for
-stable?

> +/**
> + * __stop_cpus - stop multiple cpus
> + * @cpumask: cpus to stop
> + * @fn: function to execute
> + * @arg: argument to @fn
> + *
> + * Execute @fn(@arg) on online cpus in @cpumask. If @cpumask is NULL, @fn
> + * is run on all the online cpus including the current cpu (even if it
> + * is not online).
> + * On each target cpu, @fn is run in a process context (except when run on
> + * the cpu that is in the process of coming online, in which case @fn is run
> + * in the same context as __stop_cpus()) with the highest priority
> + * preempting any task on the cpu and monopolizing it.  This function
> + * returns after all executions are complete.
> + */

It's minor but can you please follow the comment style in the same
file?  ie. Blank line between paragraphs, separate CONTEXT: and
RETURNS: sections.  At the second thought, why is this function even
exported?  It doesn't seem to have any out-of-file user left.  Maybe
best to make it static?

>  int __stop_cpus(const struct cpumask *cpumask, cpu_stop_fn_t fn, void *arg)
>  {
> +	int online = percpu_read(cpu_stopper.enabled);
> +	int include_this_offline = 0;
>  	struct cpu_stop_work *work;
>  	struct cpu_stop_done done;
> +	unsigned int weight;
>  	unsigned int cpu;
>  
> +	if (!cpumask) {
> +		cpumask = cpu_online_mask;
> +		include_this_offline = 1;
> +	}

This seems a bit too subtle.  I would much prefer if it were much more
explicit than using non-obvious combination of conditions to trigger
this special behavior.

Hmm... Wouldn't it be better to change cpu_stop_queue_work() consider
whether the local CPU is offline and then add cpu_stop_wait_done()
which does wait_for_completion() if local is online and otherwise
execute fn locally, call cpu_stop_signal_done() and wait in busy loop?

That would make the whole thing much more generic and easier to
describe.  The current implementation seems quite hacky/subtle and
doesn't fit well with the rest.  It would be much better if we can
just state "if used from local CPU which is not online and the target
@cpumask includes the local CPU, the work item is executed on-stack
and completion is waited in busy-loop" for all cpu_stop functions.

Also, it would be better to factor out work item execution and
completion from cpu_stopper_thread() and call that instead of invoking
fn(arg) directly.

Thank you.

-- 
tejun