Re: [PATCH 7/9] PM: Add suspend blocking work.

[Tejun Heo <tj@kernel.org>]

Hello,

On 04/24/2010 12:49 AM, Arve Hjønnevåg wrote:
> I want the suspend blocker active when the work is pending or running.
> I did not see a way to do this on top of the workqueue api without
> adding additional locking.

Well, then add additional locking.  suspend_blocker is far away from
being a hot path and there's nothing wrong with additional locking as
long as everything is wrapped inside proper APIs.  Adding stuff to the
core code for things as specific as this is much worse.

> If the work is both queued and starts running on another workqueue
> between "get_wq_data(work) == cwq" and "!work_pending(work)", then
> suspend_unblock will be called when it shouldn't. It should work fine
> if I change to it check pending first though, since it cannot move
> back to the current workqueue without locking cwq->lock first.

The code is more fundamentally broken.  Once work->func has started
executing, the work pointer can't be dereferenced as work callback is
allowed to free or re-purpose the work data structure and in the same
manner you can't check for pending status after execution has
completed.

> Or are you talking about the race when the callback is running on
> multiple (cpu) workqueues at the same time. In that case the suspend
> blocker is released when the callback returns from the last workqueue
> is was queued on, not when all the callbacks have returned. On that
> note, is it ever safe to use flush_work and cancel_work_sync for work
> queues on anything other than a single single threaded workqueue?

flush_work() is guaranteed only to flush from the last queued cpu but
cancel_work_sync() will guarantee that no cpu is executing or holding
onto the work.  So, yeah, as long as the limitations of flush_work()
is understood, it's safe.

Going back to the original subject, just add simplistic outside
locking in suspend_blocker_work API (or whatever other name you
prefer).  That will be much cleaner and safer.  Let's think about
moving them into workqueue implementation proper after the number of
the API users grows to hundreds.

Thanks.

-- 
tejun