Re: [PATCH] stopmachine: add stopmachine_timeout

Rusty Russell
2008-07-15T07:50:30Z

On Tuesday 15 July 2008 11:11:34 Hidetoshi Seto wrote:
> Hi Rusty,

Hi Hidetoshi,

> However we need to be careful that the stuck CPU can restart unexpectedly.

OK, if you are worried about that race, I think we can still fix it...

> > + for_each_online_cpu(i) {
> > + if (!cpu_isset(i, prepared_cpus) && i != smp_processor_id()) {
> > + bool ignore;
> > +
>
> Note that here is a window that a not-prepared frozen cpu can be thawed and
> become be prepared.
>
> > + /* If we wanted to run on a particular CPU, and that's
> > + * the one which is stuck, it's a real failure. */
> > + ignore = !cpus || !cpu_isset(i, *cpus);
> > + printk(KERN_CRIT "stopmachine: cpu#%d seems to be "
> > + "stuck, %s.\n",
> > + i, ignore ? "ignoring" : "FAILING");
> > + /* Unbind thread: it will exit when it sees
> > + * that prepared_cpus bit set. */
> > + set_cpus_allowed(threads[i], cpu_online_map);
>
> Unbinded threads still can wake up on a cpu where they originally targeted.

What if we use:
if (i != smp_processor_id() && !cpu_test_and_set(i, prepared_cpus)) {

instead of cpu_isset? That means that if a CPU restarts during that window,
either the thread will exit (because we set the bit here), or this will
detect it.

Hmm, there's still the vague possibility that the thread doesn't schedule
until we start a new stop_machine (and clear prepared_cpus). We could simply
loop in the main thread if any threads are alive, before freeing them (inside
the lock). A counter and notifier is the other way, but it seems like
overkill for a very unlikely event.

Thanks for the analysis!
Rusty.