Re: workqueue question.

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Jun 29, 2011 at 09:02:29AM -0700, Ben Greear wrote:
> On 06/29/2011 01:43 AM, Tejun Heo wrote:
> It appears that the code just wants to (re)add itself to the
> work queue with a different callback method:
> 
> static void rpc_final_put_task(struct rpc_task *task,
> 		struct workqueue_struct *q)
> {
> 	if (q != NULL) {
> 		INIT_WORK(&task->u.tk_work, rpc_async_release);
> 		queue_work(q, &task->u.tk_work);
> 	} else
> 		rpc_free_task(task);
> }

Ummm... so, at the time of INIT_WORK(), the tk_work could be already
pending or running?

> My debugging leads me to believe that the rpc_async_release
> is (very rarely) called on a task object that has already been logically
> freed.

What do you mean "logically freed"?  Do you mean the rpc_task struct
is freed twice?

> Is there a better way to queue this up that might have less chance
> of some strange race?

Why not just use a separate work item?

> >>Also, is it valid to free the memory containing foo
> >>in a workqueue callback?
> >
> >Yeap.
> 
> Is there a method that can be called from a workqueue callback
> to verify that the item has not been re-added to the work-queue?

Can you be a bit more specific?  Are you saying that queue_work() and
INIT_WORK() may race?

> I tried doing a cancel, but that caused recursive locking issues.
> 
> I'd like to call this right before freeing the object and BUG_ON()
> if the object is actually still on on a work-queue.

That may be useful as a debugging feature but is inherently racy.
Nothing guarantees the work item won't be queued after BUG_ON() but
before actual freeing.  The guarantee that the work item is no longer
in use should come from the wq user.  There are good number of use
cases where work item frees itself or the containing data structure
and they all work fine.

Thanks.

-- 
tejun