add unschedule_delayed_work to the workqueue API
PATCH 1/1 add unschedule_delayed_work to the workqueue API
I'm in the process of moving some of our scsi timers which do more work than just a few lines of code into schedule_work() instead. The problem is that the workqueue API lacks the equivalent of del_timer_sync(). This patch adds it as unschedule_delayed_work() (and also adds the unschedule_work() API---it's probably much less useful, but unschedule_delayed_work() is best constructed on top of it).
The idea is that unschedule_delayed_work() will return 0 if the work has already executed and 1 if it hasn't. It is guaranteed that either the work was removed or has been executed by the time unschedule_delayed_work() returns (and thus it needs process context to wait for the work function if necessary).
James
===== include/linux/workqueue.h 1.9 vs edited =====
include/linux/workqueue.h +5 -0
@@ -69,6 +69,11 @@extern int current_is_keventd (void );extern int keventd_up (void ); +extern int remove_work (struct workqueue_struct *wq,struct work_struct *work); +extern int remove_delayed_work (struct workqueue_struct *wq,struct work_struct *work); +extern int unschedule_work (struct work_struct *work); +extern int unschedule_delayed_work (struct work_struct *work); +extern void init_workqueues (void );/*
===== kernel/workqueue.c 1.28 vs edited =====
kernel/workqueue.c +83 -0
108 lines not on this page · raw message
Quoted 4 lines · James Bottomley
I'm in the process of moving some of our scsi timers which do more work than just a few lines of code into
schedule_work()instead. The problem is that the workqueue API lacks the equivalent ofdel_timer_sync().
The usual way of doing this is:
cancel_delayed_work(...); flush_workqueue(...);
Quoted 9 lines · Andrew Morton
Quoted 4 lines · James Bottomley
I'm in the process of moving some of our scsi timers which do more work than just a few lines of code into
schedule_work()instead. The problem is that the workqueue API lacks the equivalent ofdel_timer_sync().The usual way of doing this is:
cancel_delayed_work(...);
That API doesn't seem to be in the vanilla kernel ... is it mm only?
Quoted 1 line
flush_workqueue(...);
Yes, but the flush_workqueue() has you waiting until every piece of work on the workqueue has completed ... that creates an unacceptable delay in the routine that wants it either cancelled or run.
James
Quoted 4 lines · James Bottomley
Quoted 3 lines · Andrew Morton
The usual way of doing this is:
cancel_delayed_work(...);
OK, found it in the headers, sorry .. it's not synchronous, so it can't really be used in most of the cases where we use del_timer_sync().
James
Quoted 9 lines · James Bottomley
Quoted 4 lines · James Bottomley
Quoted 3 lines · Andrew Morton
The usual way of doing this is:
cancel_delayed_work(...);OK, found it in the headers, sorry .. it's not synchronous, so it can't really be used in most of the cases where we use
del_timer_sync().
cancel_delayed_work() will tell you whether it successfully cancelled the timer. If it didn't, you should run flush_workqueue() to wait on the final handler. The combination of the two is synchronous.
The missing link is cancellation of a delayed work which re-adds itself and where the calling code has no way of telling the handler to not re-arm itself. There's a patch in -mm to add that function, but I don't like it. That's cancel_rearming_delayed_work.patch.
Quoted 6 lines · Andrew Morton
Quoted 5 lines · James Bottomley
Quoted 3 lines · James Bottomley
Quoted 2 lines · Andrew Morton
The usual way of doing this is:
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
cancel_delayed_work(...);OK, found it in the headers, sorry .. it's not synchronous, so it can't really be used in most of the cases where we use
del_timer_sync().
cancel_delayed_work()will tell you whether it successfully cancelled the timer. If it didn't, you should runflush_workqueue()to wait on the final handler. The combination of the two is synchronous.
Right, but it potentially does too much work for my purposes. I want to cancel the work if it's cancellable or wait for it if it's already executing. I don't want to have to wait for all the work in the queue just because the timer fired and it got added to the workqueue schedule.
Quoted 4 lines
The missing link is cancellation of a delayed work which re-adds itself and where the calling code has no way of telling the handler to not re-arm itself. There's a patch in -mm to add that function, but I don't like it. That's cancel_rearming_delayed_work.patch.
James
Quoted 9 lines · James Bottomley
Quoted 7 lines
Quoted 3 lines
OK, found it in the headers, sorry .. it's not synchronous, so it can't really be used in most of the cases where we use
del_timer_sync().
cancel_delayed_work()will tell you whether it successfully cancelled the timer. If it didn't, you should runflush_workqueue()to wait on the final handler. The combination of the two is synchronous.Right, but it potentially does too much work for my purposes.
Are you sure?
Quoted 4 lines
I want to
cancel the work if it's cancellable or wait for it if it's already executing. I don't want to have to wait for all the work in the queue just because the timer fired and it got added to the workqueue schedule.
The probability that the handler is running when you call cancel_delayed_work() is surely very low. And the probability that there is more than one thing pending in the queue at that time is also low. Multiplying them both together, then multiplying that by the relative expense of the handler makes me say "show me" ;)
Quoted 5 lines · Andrew Morton
Quoted 4 lines · James Bottomley
Quoted 4 lines
Quoted 3 lines
OK, found it in the headers, sorry .. it's not synchronous, so it can't really be used in most of the cases where we use
del_timer_sync().
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
cancel_delayed_work()will tell you whether it successfully cancelled the timer. If it didn't, you should runflush_workqueue()to wait on the final handler. The combination of the two is synchronous.Right, but it potentially does too much work for my purposes.
Are you sure?
I'm positive the potential is there.
Quoted 10 lines
Quoted 4 lines
I want to
cancel the work if it's cancellable or wait for it if it's already executing. I don't want to have to wait for all the work in the queue just because the timer fired and it got added to the workqueue schedule.The probability that the handler is running when you call
cancel_delayed_work()is surely very low. And the probability that there is more than one thing pending in the queue at that time is also low. Multiplying them both together, then multiplying that by the relative expense of the handler makes me say "show me" ;)
OK. In the current code, domain validation is done from the workqueue interface. This can take several seconds per target to complete. Why should I have to wait this extra time. As I move other SCSI daemon threads to being work queue items, these times rise.
However, now there's a worse problem. If I want to cancel a piece of work synchronously, flush_scheduled_work() makes me dependent on the execution of all the prior pieces of work. If some of them are related, like Domain Validation and device unlocking say, I have to now be extremely careful that the place I cancel and flush from isn't likely to deadlock with any other work scheduled on the device. This makes it a hard to use interface. By contrast, the proposed patch will *only* wait if the item of work is currently executing. This is a (OK reasonably given the aic del_timer_sync() issues) well understood deadlock problem---the main point being I now don't have to consider any of the other work that might be queued.
James
Quoted 8 lines · James Bottomley
Quoted 7 lines
The probability that the handler is running when you call
cancel_delayed_work()is surely very low. And the probability that there is more than one thing pending in the queue at that time is also low. Multiplying them both together, then multiplying that by the relative expense of the handler makes me say "show me" ;)
Quoted 4 lines
OK. In the current code, domain validation is done from the workqueue interface. This can take several seconds per target to complete. Why should I have to wait this extra time. As I move other SCSI daemon threads to being work queue items, these times rise.
You mean the scsi code wants to schedule a work some time in the future and that when it executes, the handler will then run for several seconds?
If so, it sounds like this is the wrong mechanism to use. We certainly don't want keventd gone to lunch for that long, and even if the kernel threads were privately created by the scsi code, you'll have to create many such threads to get any sort of parallelism across many disks. I suspect I'm missing something.
Quoted 7 lines
However, now there's a worse problem. If I want to cancel a piece of work synchronously,
flush_scheduled_work()makes me dependent on the execution of all the prior pieces of work. If some of them are related, like Domain Validation and device unlocking say, I have to now be extremely careful that the place I cancel and flush from isn't likely to deadlock with any other work scheduled on the device. This makes it a hard to use interface.
Well yes, the caller wouldn't want to be holding any locks which cause currently-queued works to block.
Quoted 5 lines
By contrast, the proposed patch will *only* wait
if the item of work is currently executing. This is a (OK reasonably given the aicdel_timer_sync()issues) well understood deadlock problem---the main point being I now don't have to consider any of the other work that might be queued.
OK.
Quoted 9 lines · Andrew Morton
James Bottomley <[email protected]> wrote:
You mean the scsi code wants to schedule a work some time in the future and that when it executes, the handler will then run for several seconds?If so, it sounds like this is the wrong mechanism to use. We certainly don't want keventd gone to lunch for that long, and even if the kernel threads were privately created by the scsi code, you'll have to create many such threads to get any sort of parallelism across many disks. I suspect I'm missing something.
There is a case where this can happen, yes, but I admit it's not the common one...most DV's are kicked off without a thread from user context (either in the initial scan or by user request). The common case where we use a workqueue is if an asynchronous event that alters the transport agreement comes in (mainly a bus reset) via an interrupt.
Quoted 10 lines
Quoted 7 lines
However, now there's a worse problem. If I want to cancel a piece of work synchronously,
flush_scheduled_work()makes me dependent on the execution of all the prior pieces of work. If some of them are related, like Domain Validation and device unlocking say, I have to now be extremely careful that the place I cancel and flush from isn't likely to deadlock with any other work scheduled on the device. This makes it a hard to use interface.Well yes, the caller wouldn't want to be holding any locks which cause currently-queued works to block.
It's not just locks (hopefully there shouldn't be any of these). It's also device states.
For instance, DV will attempt to quiesce the device (i.e. wait for it's currently outstanding commands to complete). Thus, I can't use the cancel/flush method in any code path (such as may be called from error handling) which could itself be part of the completion path.
This dependence is caused by having to wait for everything that could potentially be on the workqueue to complete. I'd really prefer an API that didn't have this potentially nasty entanglement, which is why I coded mine to only do the wait if the piece of work being removed was in the process of executing, but not otherwise.
Quoted 7 lines
Quoted 5 lines
By contrast, the proposed patch will *only* wait
if the item of work is currently executing. This is a (OK reasonably given the aicdel_timer_sync()issues) well understood deadlock problem---the main point being I now don't have to consider any of the other work that might be queued.OK.
James