* [RFC] Add mod_delayed_work()
@ 2011-06-24 22:27 Gustavo F. Padovan
2011-06-24 22:27 ` [RFC 1/1] workqueue: " Gustavo F. Padovan
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo F. Padovan @ 2011-06-24 22:27 UTC (permalink / raw)
To: tj; +Cc: linux-kernel, padovan, marcel, a.p.zijlstra, akpm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [RFC 1/1] workqueue: Add mod_delayed_work()
2011-06-24 22:27 [RFC] Add mod_delayed_work() Gustavo F. Padovan
@ 2011-06-24 22:27 ` Gustavo F. Padovan
2011-06-25 11:43 ` Tejun Heo
0 siblings, 1 reply; 4+ messages in thread
From: Gustavo F. Padovan @ 2011-06-24 22:27 UTC (permalink / raw)
To: tj; +Cc: linux-kernel, padovan, marcel, a.p.zijlstra, akpm
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC 1/1] workqueue: Add mod_delayed_work()
2011-06-24 22:27 ` [RFC 1/1] workqueue: " Gustavo F. Padovan
@ 2011-06-25 11:43 ` Tejun Heo
2011-06-28 18:16 ` Gustavo F. Padovan
0 siblings, 1 reply; 4+ messages in thread
From: Tejun Heo @ 2011-06-25 11:43 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-kernel, marcel, a.p.zijlstra, akpm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC 1/1] workqueue: Add mod_delayed_work()
2011-06-25 11:43 ` Tejun Heo
@ 2011-06-28 18:16 ` Gustavo F. Padovan
0 siblings, 0 replies; 4+ messages in thread
From: Gustavo F. Padovan @ 2011-06-28 18:16 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, marcel, a.p.zijlstra, akpm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-06-28 18:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-24 22:27 [RFC] Add mod_delayed_work() Gustavo F. Padovan
2011-06-24 22:27 ` [RFC 1/1] workqueue: " Gustavo F. Padovan
2011-06-25 11:43 ` Tejun Heo
2011-06-28 18:16 ` Gustavo F. Padovan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.