Messages in this thread | | | Date | Tue, 3 Apr 2007 19:03:36 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH 7/8] Clean up workqueue.c with respect to the freezer based cpu-hotplug |
| |
On 04/03, Srivatsa Vaddagiri wrote: > > On Tue, Apr 03, 2007 at 03:47:29PM +0400, Oleg Nesterov wrote: > > > > for (;;) { > > try_to_freeze(); > > > > prepare_to_wait(&cwq->more_work, &wait, TASK_INTERRUPTIBLE); > > if (!kthread_should_stop() && list_empty(&cwq->worklist)) > > schedule(); > > finish_wait(&cwq->more_work, &wait); > > > > if (kthread_should_stop(cwq)) > > break; > > > > run_workqueue(cwq); > > } > > cleanup_workqueue_thread (in Gautham's patches) does this: > > thaw_process() > kthread_stop()
Yes, thanks.
> We could do what you are suggesting if the thaw_process() part was > integrated into kthread_stop() code [basically thaw_process after > setting the kthread_stop_info.k flag].
I think it would be nice to do. I believe we can cleanup ksoftirqd() and migration_thread() as well (kill wait_to_die: loop). Probably it is better to introduce a new helper for that, kthread_thaw_stop() or something.
> > > void fastcall flush_workqueue(struct workqueue_struct *wq) > > > { > > > - const cpumask_t *cpu_map = wq_cpu_map(wq); > > > int cpu; > > > > > > might_sleep(); > > > - for_each_cpu_mask(cpu, *cpu_map) > > > + for_each_online_cpu(cpu) > > > flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu)); > > > } > > > > Hm... I can't understand this change. I believe it is wrong. > > Why?
What if is_single_threaded(wq) == true? In that case we should call flush_cpu_workqueue(cpu) only if cpu == singlethread_cpu, otherwise this is unneeded and wrong, because per_cpu_ptr(wq->cpu_wq, cpu) was not initialized.
> > > + kthread_stop(cwq->thread); > > > cwq->thread = NULL; > > > - spin_unlock_wait(&cwq->lock); > > > } > > > } > > > > Deadlockable. Suppose that the freezing is in progress, cwq->thread is not > > frozen yet. cleanup_workqueue_thread() calls thaw_process(cwq->thread), > > then cwq->thread() goes to refrigerator, kthread_stop() blocks forever. > > Good catch! Can cleanup_workqueue_thread take some mutex to serialize > with freezer here (say freezer_mutex)? > > Or better, since this seems to be a general problem for anyone who wants to do a > kthread_stop, how abt modifying kthread_stop like below: > > kthread_stop(p) > { > int old_exempt_flags; > > task_lock(p); > old_exempt_flags = p->flags; > p->flags |= PFE_ALL; /* Exempt 'p' from being frozen? */
I agree, we should mark this thread as non-freezable, but we can't modify p->flags, this is racy. "current" owns its ->flags and it is not atomic. Note that thaw_process() checks frozen(p) when it clears PF_FROZEN.
Actually, we should do this before destroy_workqueue() calls flush_workqueue(). Otherwise flush_cpu_workqueue() can hang forever in a similar manner.
Needs more thinking, I guess.
> > This means that the work_struct on single_threaded wq can't use any of > > > > __create_workqueue() > > destroy_workqueue() > > flush_workqueue() > > cancel_work_sync() > > The workqueue_mutex() should serialize these with workqueue_cpu_callback() to > an extent, but ..
No, no, workqueue_mutex can't help. Just for example: CPU_UP_PREPARE completes and drops workqueue_mutex. __create_workqueue(wq) doesn't see the new cpu, it is not on cpu_online_map, so it doesn't create cwq->thread. CPU_ONLINE oopses.
> Yes I agree, we should target freezing everybody here. It feels much > safer that way!
Good!
Oleg.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |