Re: [RFC][PATCH] create workqueue threads only when needed

[Frederic Weisbecker <fweisbec@gmail.com>]

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!