Re: Is adding requeue_delayed_work() a good idea

[Oleg Nesterov]

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?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/