Messages in this thread | | | Date | Tue, 27 Jan 2009 09:40:49 +0100 | From | Frederic Weisbecker <> | Subject | Re: [RFC][PATCH] create workqueue threads only when needed |
| |
On Tue, Jan 27, 2009 at 02:46:51AM +0100, Oleg Nesterov wrote: > Frederic Weisbecker <fweisbec@gmail.com> wrote: > > > > static void insert_work(struct cpu_workqueue_struct *cwq, > > struct work_struct *work, struct list_head *head) > > { > > - trace_workqueue_insertion(cwq->thread, work); > > + trace_workqueue_insertion(cwq->thread, work, cwq->wq->singlethread); > > > > set_wq_data(work, cwq); > > /* > > @@ -148,6 +176,9 @@ static void __queue_work(struct cpu_workqueue_struct *cwq, > > { > > unsigned long flags; > > > > + if (!cwq->thread) > > + create_wq_thread_late(cwq); > > + > > [...snip...] > > > +static void create_wq_thread_late_work(struct work_struct *work) > > +{ > > + struct late_workqueue_creation_data *l; > > + struct cpu_workqueue_struct *cwq; > > + int cpu = smp_processor_id(); > > + int err = 0; > > + > > + l = container_of(work, struct late_workqueue_creation_data, work); > > + cwq = l->cwq; > > + > > + if (is_wq_single_threaded(cwq->wq)) { > > + err = create_workqueue_thread(cwq, singlethread_cpu); > > + start_workqueue_thread(cwq, -1); > > + } else { > > + err = create_workqueue_thread(cwq, cpu); > > + start_workqueue_thread(cwq, cpu); > > + } > > + WARN_ON_ONCE(err); > > + kfree(l); > > +} > > Let's suppose the workqueue was just created, and cwq->thared == NULL > on (say) CPU 0. > > Then CPU 0 does > > queue_work(wq, work1); > queue_work(wq, work2); > > Both these calls will notice cwq->thread == NULL, both will schedule > the work wilth ->func = create_wq_thread_late_work. > > The first work correctly creates cwq->thread, the second one creates > the new thread too and replaces cwq->thread? Now we have two threads > which run in parallel doing the same work, but the first thread is > "stealth", no?
You're right. I will put a mutex + a recheck of the cwq->thread inside create_wq_thread_late_work to be sure there is no race during creation.
> > @@ -904,9 +967,12 @@ static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq) > > * checks list_empty(), and a "normal" queue_work() can't use > > * a dead CPU. > > */ > > - trace_workqueue_destruction(cwq->thread); > > - kthread_stop(cwq->thread); > > - cwq->thread = NULL; > > + > > + if (cwq->thread) { > > + trace_workqueue_destruction(cwq->thread, cwq->wq->singlethread); > > + kthread_stop(cwq->thread); > > + cwq->thread = NULL; > > + } > > cleanup_workqueue_thread() has already checked cwq->thread != NULL, > how can it become NULL ?
Right.
> And let's suppose a user does: > > wq = create_workqueue(...., when_needed => 1); > queue_work(wq, some_work); > destroy_workqueue(wq); > > This can return before create_wq_thread_late() populates the necessary > cwq->thread. We can destroy/free workqueue with the pending work_structs, > no? > > Oleg. >
Totally right. I 'll fix these bugs.
Thanks!
| |