Re: Is adding requeue_delayed_work() a good idea

[Roland Dreier]

 > OK, in this case I think we have a simple solution,
 > 
 > 	// like cancel_delayed_work, but uses del_timer().
 > 	// this means, if it returns 0 the timer function may be
 > 	// running and the queueing is in progress. The caller
 > 	// can't rely on flush_workqueue/etc
 > 	static inline int __cancel_delayed_work(struct delayed_work *work)
 > 	{
 > 		int ret;
 > 
 > 		ret = del_timer(&work->timer);
 > 		if (ret)
 > 			work_clear_pending(&work->work);
 > 		return ret;
 > 	}
 > 
 > Now, you can do
 > 
 > 	spin_lock_irqsave(&mydata_lock);
 > 	new_timeout = add_item_to_timeout_list();
 > 
 > 	__cancel_delayed_work(&process_timeout_list_work);
 > 	queue_delayed_work(wq, &process_timeout_list_work, new_timeout);
 > 
 > 	spin_unlock_irqsave(&mydata_lock);
 > 
 > If queue_delayed_work() fails, this means that WORK_STRUCT_PENDING is set,
 > dwork->work is already queued or the queueing is in progress. In both
 > cases it will run "soon" as if we just called queue_work(&dwork->work).

This looks like it would work well.  If we can get this into 2.6.32 then
I will drop my patch and switch to this approach instead.

 > But this assumes nobody else does queue_delayed_work(dwork, HUGE_DELAY) in
 > parallel, otherwise we can lose the race and another caller can setup
 > HUGE_DELAY timeout.

In my case this is fine -- all uses of queue_delayed_work() are
synchronized with the same lock.  So any place that tries to shorten the
timeout will succeed.

 > In particular, if process_timeout_list_work->func() itself uses
 > queue_delay_work() to re-arm itself we can race. Bu t I think it is always
 > possible to do something to synchronize with work->func, for example
 > work->func() can re-arm itself _before_ it scans timeout_list (under the
 > same lock). This way, if re-queue code above fails because work->func()
 > wins, work->func() must see the new additions to timeout_list.

In my case, work function does do queue_delayed_work(), but with the
same lock as everyone else held.  So there should be no race.

Thanks,
  Roland

--
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/