Re: [PATCH] file: kill unnecessary timer in fdtable_defer

[Dipankar Sarma <dipankar@in.ibm.com>]

On Mon, Aug 21, 2006 at 02:18:16PM +0900, Tejun Heo wrote:
> On Mon, Aug 21, 2006 at 10:02:57AM +0530, Dipankar Sarma wrote:
> > > regardless of its return value.  0 return simply means that the work
> > > was already pending and thus no further action was required.
> > 
> > Hmm.. Is this really true ? IIRC, schedule_work() checks pending
> > work based on bit ops on work->pending and clear_bit() is not
> > a memory barrier.
> 
> Those bitops are not memory barriers but they can define order between
> them alright.  Once the execution order is correct, the rest of

Huh ? If they are not memory barriers, they how can you guranttee
ordering on weakly ordered CPUs ?


> In workqueue, this is guaranteed by
> 
> 1. If pending bit is set, the work is guaranteed to be executed in
>    some future - it's on timer or queue.
> 
> 2. The queuer sets the pending bit *after* producing a job to be
>    done.
> 
> 3. The worker clears the pending *before* executing the job.
> 
> I sometimes find it easier to understand with a diagram which looks
> like the following.  Time flows from top to bottom.
> 
>           Queuer			  Worker
> 
>         -------------
>        | produce job |
>        |             |             <--- clr pending --->
>         -------------                        |
>               |                              v
>               v                       --------------
>     <--- set pending --->            | consume jobs |
> 				     |		    |
> 				      --------------
> 
> Now move the queuer and worker up and down in your mind.  If 'set
> pending' is higher than clr pending 'consume job' is guaranteed to see
> the job queuer has produced whether pending is set or clear
> beforehand.  If 'set pending' is lower than 'clr pending', it is
> guaranteed to set pending and schedule worker.  The workqueue is
> straight-forward expansion where there are N queuers and a repeating
> consumer.

Given that there is no smp_mb__after_clear_bit() after clearing
work->pending, what prevents the worker thread from seeing
the state of the deferred fd queue before setting the pending bit ?
IOW, the queuer sees pending = 1 and ignores waking the
worker thread, worker sees a stale state of the deferred fd queue
ignoring the newly queued work. That should be possible on
a cpu with weak memory ordering. Perhaps, we should fix
__queue_work() to add the smp_mb__after_clear_bit() and
make sure that we have a memory barrier after adding the
deferred fds.

Thanks
Dipankar