lkml.org 
[lkml]   [2009]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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!



\
 
 \ /
  Last update: 2009-01-27 09:43    [from the cache]
©2003-2011 Jasper Spaans