Re: [PATCH 1/7] work_on_cpu: helper for doing task on a CPU.

[Oleg Nesterov <oleg@redhat.com>]

On 10/23, Gautham R Shenoy wrote:
>
> On Thu, Oct 23, 2008 at 11:40:36AM +0200, Oleg Nesterov wrote:
> >
> > IOW, I'd suggest
> >
> > 	long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
> > 	{
> > 		struct work_for_cpu wfc;
> >
> > 		INIT_WORK(&wfc.work, do_work_for_cpu);
> > 		wfc.fn = fn;
> > 		wfc.arg = arg;
> > 		wfc.ret = -EINVAL;
> >
> > 		get_online_cpus();
> > 		if (likely(cpu_online(cpu))) {
> > 			schedule_work_on(cpu, &wfc.work);
> > 			flush_work(&wfc.work);
> > 		}
>
> OK, how about doing the following? That will solve the problem
> of deadlock you pointed out in patch 6.
>
> 		get_online_cpus();
> 		if (likely(per_cpu(cpu_state, cpuid) == CPU_ONLINE)) {
> 			schedule_work_on(cpu, &wfc.work);
> 			flush_work(&wfc.work);
> 		} else if (per_cpu(cpu_state, cpuid) != CPU_DEAD)) {
> 			/*
> 			 * We're the CPU-Hotplug thread. Call the
> 			 * function synchronously so that we don't
> 			 * deadlock with any pending work-item blocked
> 			 * on get_online_cpus()
> 			 */
> 			 cpumask_t  orignal_mask = current->cpus_allowed;
> 			 set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu);
> 			 wfc.ret = fn(arg);
> 			 set_cpus_allowed_ptr(current, &original_mask);

Not sure I understand...

_cpu_up() does raw_notifier_call_chain(CPU_ONLINE) after __cpu_up(),
at this point per_cpu(cpu_state) == CPU_ONLINE.

(OK, this is not exactly true, start_secondary() updates cpu_online_map
 and only then cpu_state = CPU_ONLINE, but __cpu_up() waits for
 cpu_online(cpu) == T).


Anyway, personally I dislike this special case. We must not use work_on_cpu()
if we hold the lock which can be used by some work_struct, cpu_hotplug is not
special at all.

Oleg.