Messages in this thread | | | Date | Thu, 23 Oct 2008 18:35:17 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/7] work_on_cpu: helper for doing task on a CPU. |
| |
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.
| |