Hi,
This patch makes relay use timers instead of workqueues for reader
waking.
Tom
---
relay doesn't need to use schedule_delayed_work() for waking readers
when a simple timer will do.
Signed-off-by: Tom Zanussi <[email protected]>
---
include/linux/relay.h | 2 +-
kernel/relay.c | 37 ++++++++++++++++++++-----------------
2 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/include/linux/relay.h b/include/linux/relay.h
index 759a0f9..cac0732 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -38,7 +38,7 @@ struct rchan_buf
size_t subbufs_consumed; /* count of sub-buffers consumed */
struct rchan *chan; /* associated channel */
wait_queue_head_t read_wait; /* reader wait queue */
- struct delayed_work wake_readers; /* reader wake-up work struct */
+ struct timer_list timer; /* reader wake-up timer */
struct dentry *dentry; /* channel file dentry */
struct kref kref; /* channel buffer refcount */
struct page **page_array; /* array of current buffer pages */
diff --git a/kernel/relay.c b/kernel/relay.c
index 577f251..6b252be 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -310,16 +310,13 @@ static struct rchan_callbacks default_channel_callbacks = {
/**
* wakeup_readers - wake up readers waiting on a channel
- * @work: work struct that contains the the channel buffer
+ * @data: contains the the channel buffer
*
- * This is the work function used to defer reader waking. The
- * reason waking is deferred is that calling directly from write
- * causes problems if you're writing from say the scheduler.
+ * This is the timer function used to defer reader waking.
*/
-static void wakeup_readers(struct work_struct *work)
+static void wakeup_readers(unsigned long data)
{
- struct rchan_buf *buf =
- container_of(work, struct rchan_buf, wake_readers.work);
+ struct rchan_buf *buf = (struct rchan_buf *)data;
wake_up_interruptible(&buf->read_wait);
}
@@ -337,11 +334,11 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
if (init) {
init_waitqueue_head(&buf->read_wait);
kref_init(&buf->kref);
- INIT_DELAYED_WORK(&buf->wake_readers, NULL);
- } else {
- cancel_delayed_work(&buf->wake_readers);
- flush_scheduled_work();
- }
+ init_timer(&buf->timer);
+ buf->timer.data = (unsigned long)buf;
+ buf->timer.function = wakeup_readers;
+ } else
+ del_timer_sync(&buf->timer);
buf->subbufs_produced = 0;
buf->subbufs_consumed = 0;
@@ -447,8 +444,7 @@ end:
static void relay_close_buf(struct rchan_buf *buf)
{
buf->finalized = 1;
- cancel_delayed_work(&buf->wake_readers);
- flush_scheduled_work();
+ del_timer_sync(&buf->timer);
kref_put(&buf->kref, relay_remove_buf);
}
@@ -609,9 +605,16 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
buf->padding[old_subbuf];
smp_mb();
if (waitqueue_active(&buf->read_wait)) {
- PREPARE_DELAYED_WORK(&buf->wake_readers,
- wakeup_readers);
- schedule_delayed_work(&buf->wake_readers, 1);
+ /*
+ * Calling wake_up_interruptible() from here
+ * will deadlock if we happen to be logging
+ * from the scheduler (trying to re-grab
+ * rq->lock), so defer it.
+ */
+ if (!timer_pending(&buf->timer)) {
+ buf->timer.expires = jiffies + 1;
+ add_timer(&buf->timer);
+ }
}
}
Hi Tom,
On 5/6/07, Tom Zanussi <[email protected]> wrote:
> [...]
> relay doesn't need to use schedule_delayed_work() for waking readers
> when a simple timer will do.
>
> Signed-off-by: Tom Zanussi <[email protected]>
> [...]
> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index 759a0f9..cac0732 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -38,7 +38,7 @@ struct rchan_buf
> size_t subbufs_consumed; /* count of sub-buffers consumed */
> struct rchan *chan; /* associated channel */
> wait_queue_head_t read_wait; /* reader wait queue */
> - struct delayed_work wake_readers; /* reader wake-up work struct */
> + struct timer_list timer; /* reader wake-up timer */
> struct dentry *dentry; /* channel file dentry */
> struct kref kref; /* channel buffer refcount */
> struct page **page_array; /* array of current buffer pages */
I suspect you could now safely get rid of the "#include
<linux/wait.h>" at the top of relay.h too. And although timer.h comes
via #include <linux/sched.h>, you might want to do that explicitly
too.
Satyam
On 05/05, Tom Zanussi wrote:
>
> This patch makes relay use timers instead of workqueues for reader
> waking.
A couple of very minor nits,
> @@ -337,11 +334,11 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
> if (init) {
> init_waitqueue_head(&buf->read_wait);
> kref_init(&buf->kref);
> - INIT_DELAYED_WORK(&buf->wake_readers, NULL);
> - } else {
> - cancel_delayed_work(&buf->wake_readers);
> - flush_scheduled_work();
> - }
> + init_timer(&buf->timer);
> + buf->timer.data = (unsigned long)buf;
> + buf->timer.function = wakeup_readers;
I'd suggest to use setup_timer(&buf->timer, wakeup_readers, buf);
> @@ -609,9 +605,16 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
> buf->padding[old_subbuf];
> smp_mb();
> if (waitqueue_active(&buf->read_wait)) {
> - PREPARE_DELAYED_WORK(&buf->wake_readers,
> - wakeup_readers);
> - schedule_delayed_work(&buf->wake_readers, 1);
> + /*
> + * Calling wake_up_interruptible() from here
> + * will deadlock if we happen to be logging
> + * from the scheduler (trying to re-grab
> + * rq->lock), so defer it.
> + */
> + if (!timer_pending(&buf->timer)) {
> + buf->timer.expires = jiffies + 1;
> + add_timer(&buf->timer);
> + }
I think it is better to use __mod_timer(&buf->timer, jiffies + 1). In that
case this "if (!timer_pending(&buf->timer))" is not strictly needed, yes?
Imho, add_timer() is almost never should be used. The only valid usage is when
timer->expires was already set by somebody else.
Btw, thanks for your explanation about deferred wakeup.
Oleg.
On Sun, 2007-05-06 at 16:47 +0400, Oleg Nesterov wrote:
> On 05/05, Tom Zanussi wrote:
> >
> > This patch makes relay use timers instead of workqueues for reader
> > waking.
>
> A couple of very minor nits,
>
> > @@ -337,11 +334,11 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
> > if (init) {
> > init_waitqueue_head(&buf->read_wait);
> > kref_init(&buf->kref);
> > - INIT_DELAYED_WORK(&buf->wake_readers, NULL);
> > - } else {
> > - cancel_delayed_work(&buf->wake_readers);
> > - flush_scheduled_work();
> > - }
> > + init_timer(&buf->timer);
> > + buf->timer.data = (unsigned long)buf;
> > + buf->timer.function = wakeup_readers;
>
> I'd suggest to use setup_timer(&buf->timer, wakeup_readers, buf);
>
> > @@ -609,9 +605,16 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
> > buf->padding[old_subbuf];
> > smp_mb();
> > if (waitqueue_active(&buf->read_wait)) {
> > - PREPARE_DELAYED_WORK(&buf->wake_readers,
> > - wakeup_readers);
> > - schedule_delayed_work(&buf->wake_readers, 1);
> > + /*
> > + * Calling wake_up_interruptible() from here
> > + * will deadlock if we happen to be logging
> > + * from the scheduler (trying to re-grab
> > + * rq->lock), so defer it.
> > + */
> > + if (!timer_pending(&buf->timer)) {
> > + buf->timer.expires = jiffies + 1;
> > + add_timer(&buf->timer);
> > + }
>
> I think it is better to use __mod_timer(&buf->timer, jiffies + 1). In that
> case this "if (!timer_pending(&buf->timer))" is not strictly needed, yes?
>
> Imho, add_timer() is almost never should be used. The only valid usage is when
> timer->expires was already set by somebody else.
>
> Btw, thanks for your explanation about deferred wakeup.
>
OK, and thanks for your helpful comments. Corrected patch to follow...
Tom
On Sun, 2007-05-06 at 15:40 +0530, Satyam Sharma wrote:
> Hi Tom,
>
> On 5/6/07, Tom Zanussi <[email protected]> wrote:
> > [...]
> > relay doesn't need to use schedule_delayed_work() for waking readers
> > when a simple timer will do.
> >
> > Signed-off-by: Tom Zanussi <[email protected]>
> > [...]
> > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > index 759a0f9..cac0732 100644
> > --- a/include/linux/relay.h
> > +++ b/include/linux/relay.h
> > @@ -38,7 +38,7 @@ struct rchan_buf
> > size_t subbufs_consumed; /* count of sub-buffers consumed */
> > struct rchan *chan; /* associated channel */
> > wait_queue_head_t read_wait; /* reader wait queue */
> > - struct delayed_work wake_readers; /* reader wake-up work struct */
> > + struct timer_list timer; /* reader wake-up timer */
> > struct dentry *dentry; /* channel file dentry */
> > struct kref kref; /* channel buffer refcount */
> > struct page **page_array; /* array of current buffer pages */
>
> I suspect you could now safely get rid of the "#include
> <linux/wait.h>" at the top of relay.h too. And although timer.h comes
> via #include <linux/sched.h>, you might want to do that explicitly
> too.
>
Hi Satyam,
Thanks for your comments - I've updated the patch to include timer.h but
wait.h would still be needed for wait_queue_head_t, so will leave that
in.
Tom
On 5/7/07, Tom Zanussi <[email protected]> wrote:
> [...]
> Thanks for your comments - I've updated the patch to include timer.h but
> wait.h would still be needed for wait_queue_head_t, so will leave that
> in.
Whoops ... I'd actually meant to say workqueue.h there, but perhaps
must've got confused on not finding any workqueue.h in relay.h at all
and so accidentally mentioned wait.h instead.
Thanks,
Satyam