[RFC] Add mod_delayed_work()

[RFC] Add mod_delayed_work()

[Gustavo F. Padovan]

Hi,

In the work to move Bluetooth RX processing to workqueues I faced some
issues when trying to move our timers to workqueue, including deadlocks.
We use mod_timer() a lot to reset timer's delay, but the alternative with
workqueues is

	cancel_delayed_work_sync(work);
	queue_delayed_work(wq, work, delay);

This is too much for a simple change in the delay, so I did some research on
this and found that other pieces[0] in the kernel may have the same needs and
cooked a patch to create mod_delayed_work(). I used this name because we
basically call mod_timer there (or queue the work if it is not pending).

Then I propose replace the two lines above with:
	mod_delayed_work(wq, work, delay);

Please comment.

	Gustavo

[0] https://lkml.org/lkml/2011/2/3/175	

Gustavo F. Padovan (1):
  workqueue: Add mod_delayed_work()

 include/linux/workqueue.h |    2 ++
 kernel/workqueue.c        |   26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

-- 
1.7.5.1

[RFC 1/1] workqueue: Add mod_delayed_work()

[Gustavo F. Padovan]

mod_delayed_work() updates a timer if the work is pending otherwise calls
queue_delayed_work_on() to queue the work with the specified delay.

Call cancel_delayed_work_sync() and then queue_delayed_work() again to
change a timer's delays is too expensive (and requires process context).
Istead we call mod_delayed_work() to only modify the timer's timeout.

LKML-Reference: <20110203161906.GG2570@htj.dyndns.org>
Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 include/linux/workqueue.h |    2 ++
 kernel/workqueue.c        |   26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 2be2887..44f24a4 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -349,6 +349,8 @@ extern void destroy_workqueue(struct workqueue_struct *wq);
 extern int queue_work(struct workqueue_struct *wq, struct work_struct *work);
 extern int queue_work_on(int cpu, struct workqueue_struct *wq,
 			struct work_struct *work);
+extern int mod_delayed_work(struct workqueue_struct *wq,
+			struct delayed_work *work, unsigned long delay);
 extern int queue_delayed_work(struct workqueue_struct *wq,
 			struct delayed_work *work, unsigned long delay);
 extern int queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 25fb1b0..f8b59ea 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1108,6 +1108,32 @@ static void delayed_work_timer_fn(unsigned long __data)
 }
 
 /**
+ * mod_delayed_work - queue work on a workqueue after delay
+ * @wq: workqueue to use
+ * @dwork: delayable work to queue
+ * @delay: number of jiffies to wait before queueing
+ *
+ * Returns 0 if @work was already on a queue, non-zero otherwise.
+ *
+ */
+int mod_delayed_work(struct workqueue_struct *wq,
+			struct delayed_work *dwork, unsigned long delay)
+{
+	struct timer_list *timer = &dwork->timer;
+	struct work_struct *work = &dwork->work;
+
+	if (!test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work)))
+		return queue_delayed_work_on(-1, wq, dwork, delay);
+
+	BUG_ON(!timer_pending(timer));
+
+	mod_timer(timer, jiffies + delay);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mod_delayed_work);
+
+/**
  * queue_delayed_work - queue work on a workqueue after delay
  * @wq: workqueue to use
  * @dwork: delayable work to queue
-- 
1.7.5.1

Re: [RFC 1/1] workqueue: Add mod_delayed_work()

[Tejun Heo]

Hello,

On Fri, Jun 24, 2011 at 07:27:37PM -0300, Gustavo F. Padovan wrote:
> mod_delayed_work() updates a timer if the work is pending otherwise calls
> queue_delayed_work_on() to queue the work with the specified delay.
> 
> Call cancel_delayed_work_sync() and then queue_delayed_work() again to
> change a timer's delays is too expensive (and requires process context).
> Istead we call mod_delayed_work() to only modify the timer's timeout.

Yes, this part of the interface is lacking.  It might be best to
modify queue_delayed_work() to adjust the timer according to the new
timeout but we would need to audit the current users to make sure
nothing breaks and I agree introducing a new function probably makes
sense.

> +int mod_delayed_work(struct workqueue_struct *wq,
> +			struct delayed_work *dwork, unsigned long delay)
> +{
> +	struct timer_list *timer = &dwork->timer;
> +	struct work_struct *work = &dwork->work;
> +
> +	if (!test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work)))
> +		return queue_delayed_work_on(-1, wq, dwork, delay);
> +
> +	BUG_ON(!timer_pending(timer));
> +
> +	mod_timer(timer, jiffies + delay);
> +
> +	return 0;
> +}

But I think the current implementation is as it is because modifying
delayed work safely wasn't very simple.  The above code is broken in
multiple ways - a delayed work could be pending without timer pending,
and timer may expire after test_bit() but before the rest of the code.

I haven't thought about it too hard but think it would require the
timer sync part of __cancel_work_timer() (sans wait_on_work()) to get
it correctly.  Care to delve into it?

Thanks.

-- 
tejun

Re: [RFC 1/1] workqueue: Add mod_delayed_work()

[Gustavo F. Padovan]

Hi Tejun,

* Tejun Heo <tj@kernel.org> [2011-06-25 13:43:31 +0200]:

> Hello,
> 
> On Fri, Jun 24, 2011 at 07:27:37PM -0300, Gustavo F. Padovan wrote:
> > mod_delayed_work() updates a timer if the work is pending otherwise calls
> > queue_delayed_work_on() to queue the work with the specified delay.
> > 
> > Call cancel_delayed_work_sync() and then queue_delayed_work() again to
> > change a timer's delays is too expensive (and requires process context).
> > Istead we call mod_delayed_work() to only modify the timer's timeout.
> 
> Yes, this part of the interface is lacking.  It might be best to
> modify queue_delayed_work() to adjust the timer according to the new
> timeout but we would need to audit the current users to make sure
> nothing breaks and I agree introducing a new function probably makes
> sense.
> 
> > +int mod_delayed_work(struct workqueue_struct *wq,
> > +			struct delayed_work *dwork, unsigned long delay)
> > +{
> > +	struct timer_list *timer = &dwork->timer;
> > +	struct work_struct *work = &dwork->work;
> > +
> > +	if (!test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work)))
> > +		return queue_delayed_work_on(-1, wq, dwork, delay);
> > +
> > +	BUG_ON(!timer_pending(timer));
> > +
> > +	mod_timer(timer, jiffies + delay);
> > +
> > +	return 0;
> > +}
> 
> But I think the current implementation is as it is because modifying
> delayed work safely wasn't very simple.  The above code is broken in
> multiple ways - a delayed work could be pending without timer pending,
> and timer may expire after test_bit() but before the rest of the code.
> 
> I haven't thought about it too hard but think it would require the
> timer sync part of __cancel_work_timer() (sans wait_on_work()) to get
> it correctly.  Care to delve into it?

Sure, I can do that. I'm kind of stuck in the Bluetooth changes I need to do
due to this workqueue patch. I'll send a -v2 soon.

	Gustavo