Messages in this thread | | | Date | Thu, 23 Oct 2008 11:40:36 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/7] work_on_cpu: helper for doing task on a CPU. |
| |
On 10/23, Rusty Russell wrote: > > +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); > + init_completion(&wfc.done); > + wfc.fn = fn; > + wfc.arg = arg; > + get_online_cpus(); > + if (unlikely(!cpu_online(cpu))) { > + wfc.ret = -EINVAL; > + complete(&wfc.done); > + } else > + schedule_work_on(cpu, &wfc.work);
I do not claim this is wrong, but imho the code is a bit lisleading and needs a comment (or the "fix", please see below).
Once we drop cpu_hotplug lock, CPU can go away and this work can migrate to another cpu.
> + put_online_cpus(); > + wait_for_completion(&wfc.done);
Actually you don't need work_for_cpu->done, you can use flush_work().
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); } put_online_cpus();
return wfc.ret; }
Oleg.
| |