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

[Tejun Heo <tj@kernel.org>]

Hello,

On Mon, Dec 24, 2012 at 08:41:01PM +0100, Borislav Petkov wrote:
> On Mon, Dec 24, 2012 at 10:45:20AM -0800, Tejun Heo wrote:
> > I was confused a bit there. We can't. Nothing guarantees that the
> > queuer sees the cleared PENDING before the work item starts execution,
> > and I think ipc memory hotplug could also be broken from that.
> 
> Stupid question: why not clear PENDING after execution is done? I'm
> looking at process_one_work() here.

The behavior is primarily because we want to enable workqueue users to
guarantee that a full work item execution happens at least once after
certain event happens.  ie. Let's say there's a work item which
processes data generated by a device.  If it's IRQ handler calls
queue_work() after getting notified of new data segment by the device,
it would want to guarantee that whole work item execution would happen
afterwards.  If you clear PENDING after execution, the event may
overlap with the end of the previous execution and the new data won't
be processed.

> > It's highly unlikely to actually happen and there may be external
> > locking which prevents the race from actually happening, but there's
> > nothing synchronizing queueing and the execution of the work item.
> > Looking at that part of code only, it's possible that it fails to
> > queue the work item after a memory hotplug event even though the
> > previous queueing already started execution and processed a couple
> > notifiers.
> 
> Maybe failure to queue could be signalled with a proper return value
> from __queue_work()?

It already does that but it's not about the return value.  You simply
cannot know whether to proceed with queueing or not with test_bit()
alone.

> Btw, I'm afraid I don't understand the "memory hotplug event" aspect and
> how that can influence the queueing - all it does it is list_add_tail,
> basically.

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.

Thanks.

-- 
tejun