Messages in this thread Patch in this message | | | Date | Tue, 3 Apr 2007 19:29:19 +0530 | From | Srivatsa Vaddagiri <> | Subject | Re: [PATCH 7/8] Clean up workqueue.c with respect to the freezer based cpu-hotplug |
| |
On Tue, Apr 03, 2007 at 03:47:29PM +0400, Oleg Nesterov wrote: > I still think that wait_to_die + bind_cpu is unneeded complication. > Why can't we do the following: > > static int worker_thread(void *__cwq) > { > ... > > 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); > } > > return 0; > } > > ?
cleanup_workqueue_thread (in Gautham's patches) does this:
thaw_process() kthread_stop()
There is a chance that after thaw_process() [but before we have posted the kthread_stop], worker thread can come out of the refrigerator and start running run_workqueue() - that will simply prolong the subsequent kthread_stop() and the system freeze time.
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].
> > 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?
> > - for_each_possible_cpu(cpu) { > > + for_each_online_cpu(cpu) { > > This is wrong. CPU_UP_PREPARE doesn't call init_cpu_workqueue(). > Easy to fix, but I personally think is is better to initialize > the whole cpu_possible_map.
I tend to agree yes.
> > static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq, int cpu)
[snip]
> > - if (alive) { > > thaw_process(cwq->thread); > > - wait_for_completion(&barr.done); > > - > > - while (unlikely(cwq->status != CWQ_STOPPED)) > > - cpu_relax(); > > - /* > > - * Wait until cwq->thread unlocks cwq->lock, > > - * it won't touch *cwq after that. > > - */ > > - smp_rmb(); > > + 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? */ task_unlock(p);
kthread_stop_info.k = p; thaw_process(p);
wait_for_completion();
}
Marking 'p' as exempt shouldn't be a problem because freezer would wait on the thread doing kthread_stop() anyway before declaring system as frozen.
> > +static void take_over_work(struct workqueue_struct *wq, unsigned int cpu) > > +{ > > + struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu); > > + struct list_head list; > > + struct work_struct *work; > > + > > + spin_lock_irq(&cwq->lock); > > This CPU is dead (or cancelled), we don't need cwq->lock.
yeah ..
> > > static int __devinit workqueue_cpu_callback(struct notifier_block *nfb, > > unsigned long action, > > void *hcpu) > > @@ -782,11 +768,6 @@ static int __devinit workqueue_cpu_callb > > struct cpu_workqueue_struct *cwq; > > struct workqueue_struct *wq; > > > > - switch (action) { > > - case CPU_UP_PREPARE: > > - cpu_set(cpu, cpu_populated_map); > > - } > > - > > mutex_lock(&workqueue_mutex); > > list_for_each_entry(wq, &workqueues, list) { > > cwq = per_cpu_ptr(wq->cpu_wq, cpu); > > @@ -799,6 +780,7 @@ static int __devinit workqueue_cpu_callb > > return NOTIFY_BAD; > > > > case CPU_ONLINE: > > + kthread_bind(cwq->thread, cpu); > > wake_up_process(cwq->thread); > > break; > > > > @@ -806,6 +788,7 @@ static int __devinit workqueue_cpu_callb > > if (cwq->thread) > > wake_up_process(cwq->thread); > > case CPU_DEAD: > > + take_over_work(wq, cpu); > > cleanup_workqueue_thread(cwq, cpu); > > break; > > } > > 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 ..
> , they are all racy wrt workqueue_cpu_callback(), and we don't freeze > single_threaded workqueues. This is bad. > > Probaly we should: > > - freeze all workqueues, even the single_threaded ones.
Yes I agree, we should target freezing everybody here. It feels much safer that way!
The only dependency I have seen is stop_machine() called after processes are frozen. It needs the services of a workqueue to create kthreads. We need to atleast exempt that worker thread from being frozen. Hopefully the list of such non-freezable singlethreaded workqueues will be tiny enough for us to audit time-to-time.
> - helper_init() explicitely does __create_workqueue(FE_ALL). > this means that we should never use the functions above > with this workqueue.
Ok you seem to have covered what I went out to say above! Thx for your review so far ..
-- Regards, vatsa - 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/
| |