Re: [PATCH - v3?] workqueue: allow rescuer thread to do more work.

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Dec 03, 2014 at 11:40:11AM +1100, NeilBrown wrote:
> To execute the work item, we need to drop the locks.
> To perform the "list_move_tail" and the "get_pwq" we need to hold both locks.
> 
> It seems easier to do the test while holding the required locks.

Hmmm... is it difficult to regrab the locks tho?  If the problem is
wq_mayday_lock nesting outside pool locks, I don't think switching the
order would be too difficult.  The only place the two are nested is
pool_mayday_timeout() anyway, right?

> >    Isn't that -
> > whether the wq still needs rescuing after rescuer went through it once
> > - what we wanna know anyway?  e.g. something like the following.
> > 
> > 	for_each_pwq_on_mayday_list {
> > 		try to fetch work items from pwq->pool;
> > 		if (none was fetched)
> > 			goto remove_pwq;
> > 
> > 		execute the fetched work items;
> > 
> > 		if (need_to_create_worker()) {
> > 			move the pwq to the tail;
> > 			continue;
> > 		}
> > 
> > 	remove_pwq:
> > 		remove the pwq;
> > 	}
> 
> That looks nice.  I have a little difficulty matching the code to that
> outline.
> In particular I need to hold the pool->lock to call put_pwq() and after
> calling put_pwq() it isn't clear that I have a safe reference to pool so that
> it is safe to de-reference it... unless I always
> attach_to_pool/detach_from_pool.
> But to do that I have to drop the locks at awkward times.

Would inverting pool locks and wq_mayday_lock make it less awkward?

> Please correct me if I'm wrong, but it looks like I have to call
> worker_attach_to_pool() or I cannot safely call put_pwq().
> I then have to call worker_detach_from_pool() as the last step.

If I'm not mistaken, the two aren't related.  detach is necessary iff
attach has been called.  put_pwq() can be called independently.

Thanks.

-- 
tejun