Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo <tj@kernel.org>]

Hello, Borislav.

On Tue, Dec 25, 2012 at 11:46:14AM +0100, Borislav Petkov wrote:
> > It doesn't have anything to do with memory hotplug event itself.  It's
> > a generic memory access ordering / synchronization issue.  There just
> > isn't anything preventing test_bit() from seeing PENDING bit from
> > before clearing.
> 
> That's true. So is it that the whole PENDING bit testing was actually
> the wrong sync primitive to use in most cases and we actually really
> needed to disable interrupts around testing of that bit?

The IRQ disabling is for synchronization with other workqueue
operations.  Once PENDING is set, other workqueue operations may
busy-wait for the work item to be put on the appropriate queue so that
further operations can happen (e.g. flush / cancel).  So, if you have
to be atomic locally w.r.t. IRQs.

Memory ordering issue is the one which makes using test_bit() in
itself inadequate for gating the queue operation.  It could be all we
need is throwing in a smp_mb() before test_bit() to guarantee that
either clear PENDING is visible or all updates upto that point is
visible to the coming work item execution.  Not sure whether that
would be worthwhile tho.  The only case that may help is, I think, if
an already pending work item is queued very frequently from multiple
CPUs, which is unlikely and, if exists, indicates larger problems.

> Which would make your patches actually bugfixes.

Yes, why I'm talking about making [delayed_]work_pending() go away or
at least make it very ugly to use.

> /me goes and rereads 0/25 announcement mail.
> 
> Yes, it sounds like [delayed_]work_pending() needs to go or at least
> needs a big fat comment over it explaining that it is a special, cheaper
> alternative to be used *only* in conjunction with other synchronization.
> 
> Ok, thanks Tejun, for taking the time and explaining it again to me.

Thanks.

-- 
tejun