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

From: Oleg Nesterov
Date: Thu Oct 23 2008 - 05:40:21 EST


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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/