[lkml]   [2009]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: Is adding requeue_delayed_work() a good idea
On 08/20, Roland Dreier wrote:
> My patch goes back to an open-coded version of delayed work that splits
> the timer and the work struct. However it occurs to me that an API like
> requeue_delayed_work() that does a mod_timer() on the delayed work
> struct might be useful -- OTOH making this fully general and keeping
> track of the work's pending bit etc seems perhaps a bit dicy.

Completely agreed.

We need some simple changes in timer.c. __mod_timer() already has
pending_only, but requeue_delayed_work() needs another flag to prevent
migrating to another CPU. Again, this is simple, let's suppose we have
requeue_timer(timer) which works like mod_timer(pending_only => true)
but never changes timer->base.

The main question is: what should requeue_delayed_work(dwork) do when
dwork->timer is not pending but dwork->work is queued or running?
Should it cancel dwork->work is this case?

If yes, then I don't understand how this new helper (which is good
anyway) can help with this particular problem,

> happens because the mad module does
> cancel_delayed_work(&mad_agent_priv->timed_work);
> while holding mad_agent_priv->lock. cancel_delayed_work() internally
> does del_timer_sync(&mad_agent_priv->timed_work.timer).
> This can turn into a deadlock because mad_agent_priv->lock is taken
> inside cm_id_priv->lock, so we can get the following set of contexts
> that deadlock each other:
> A: holding cm_id_priv->lock, waiting for mad_agent_priv->lock
> B: holding mad_agent_priv->lock, waiting for del_timer_sync()
> C: interrupt during mad_agent_priv->timed_work.timer that takes
> cm_id_priv->lock

OK, suppose that we s/cancel_delayed_work/requeue_delayed_work/,
then we seem to have the same deadlock

A: holding cm_id_priv->lock, waiting for mad_agent_priv->lock
B: holding mad_agent_priv->lock, waiting for requeue_delayed_work()
which found !timer_pending() && queued work
C: interrupt during work->func() that takes cm_id_priv->lock

Perhaps, requeue_delayed_work() should cancel the pending work, but do
not wait_on_work(). This is not trivial, we have to avoid livelocks if
cancel_work_no_sync() races with queue_work()/etc. Perhaps,
requeue_delayed_work() could return the error if it can't update the
timer and can't cancel the work without spinning ?

What dou you think?


 \ /
  Last update: 2009-08-21 14:01    [W:0.091 / U:1.988 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site