Re: [PATCH] Add kerneldoc for flush_scheduled_work()

[James Bottomley]

On Wed, 2009-08-12 at 14:16 -0400, Alan Stern wrote:
> On Wed, 12 Aug 2009, James Bottomley wrote:
> 
> > If it's a local lock, how would something someone else submitted to the
> > queue, which would be out of scope, take this local lock?  A local lock,
> > by definition is local to the code scope you control.
> 
> This can happen easily.  Somebody else submits a work item to the
> queue, the work routine calls one of your publicly exported functions,
> and that function takes the local lock.

Fine, so local means not exported and not usable by exported functions.

> > > Not at all.  With cancel_work_sync() you must verify only that the
> > > item you want to cancel obeys the rules.  There's no need to check the
> > > other items -- and a public workqueue like keventd_wq certainly will
> > > contain other items.
> > 
> > Um, so this is called on driver removal, which is an asynchronous event.
> > Thus, how can you assure that what's on the queue (which you are
> > advocating calling cancel sync for) doesn't violate the locking rules at
> > the time the driver is removed, unless you assure that every piece of
> > work submitted doesn't violate them.
> 
> You can assure it by auditing the work routine's code.  A single work
> item involves a single function, plus whatever that function calls -- 
> it is easily checked.
> 
> You don't need to inspect every work item ever added to the queue, only 
> the one that you want to cancel.

This is true, but not relevant to the documentation you were trying to
add.  You're advocating using cancel sync as a replacement for flush ...
thus you have to cancel every pending piece of your work currently on
the queue.  My contention is that this means your rules for submission
if you do this must be the same as they would be if you'd just called
flush; making the advice moot.

James


> > Thus the rules for calling flush and cancel sync are the same.
> 
> They are not.  With cancel you need to verify that the one item you are
> cancelling obeys the rules.  With flush you need to verify that
> _everything_ on the queue is safe with respect to locking.
> 
> The only valid reason for a driver to call flush_scheduled_work()  
> would be that it knows there's an outstanding work item, which has to
> be completed or cancelled, but it doesn't have a pointer to the item.  
> Then cancellation is impossible.
> 
> Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/