relay: use plain timer instead of delayed work

PATCH In review Tom Zanussi 3 participants 6 messages May 6, 2007 – May 7, 2007 Relay

lore

Share & tools

lore

Tom Zanussi

author

2007-05-06 05:35 UTC #0 lore raw

PATCH 1/1 relay: use plain timer instead of delayed work

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(-)

include/linux/relay.h +1 -1

index 759a0f9..cac0732 100644
@@ -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 */

kernel/relay.c +20 ~~-17~~

index 577f251..6b252be 100644
@@ -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);
+			}
 		}
 	}

Satyam Sharma

2007-05-06 10:10 UTC #1 lore raw

Hi Tom,

Quoted 11 lines · Tom Zanussi

[...]
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 +0 -0
index 759a0f9..cac0732 100644

include/linux/relay.h struct rchan_buf · line 44

Quoted 8 lines

include/linux/relay.h +1 -1

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

Tom Zanussi

author

2007-05-07 03:55 UTC #2 lore raw

Quoted 19 lines · Satyam Sharma

Hi Tom,

Quoted 16 lines · Tom Zanussi

[...]
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 +1 -1

index 759a0f9..cac0732 100644
@@ -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 */

Quoted 9 lines

include/linux/relay.h struct rchan_buf · line 44
Quoted 3 lines
include/linux/relay.h +0 -0
        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

Satyam Sharma

2007-05-07 06:35 UTC #3 lore raw

Quoted 4 lines · Tom Zanussi

[...]
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

Oleg Nesterov

2007-05-06 12:47 UTC #4 lore raw

In reply to Tom Zanussi

Quoted 3 lines · Tom Zanussi

This patch makes relay use timers instead of workqueues for reader waking.

A couple of very minor nits,

kernel/relay.c static void __relay_reset(struct rchan_buf *buf, unsigned int init) · line 339

Quoted 12 lines

kernel/relay.c +3 -5

@@ -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);

Quoted 7 lines

kernel/relay.c +0 -3

@@ -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);

kernel/relay.c size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length) · line 617

Quoted 10 lines

kernel/relay.c +10 -0

+			/*
+			 * 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.

Tom Zanussi

author

2007-05-07 03:53 UTC #5 lore raw

Quoted 43 lines · Oleg Nesterov

Quoted 3 lines · Tom Zanussi
This patch makes relay use timers instead of workqueues for reader waking.
A couple of very minor nits,
kernel/relay.c static void __relay_reset(struct rchan_buf *buf, unsigned int init) · line 339
Quoted 12 lines
kernel/relay.c +3 -5
@@ -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);
Quoted 7 lines
kernel/relay.c +0 -3
@@ -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);
kernel/relay.c size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length) · line 617
Quoted 10 lines
kernel/relay.c +10 -0
+			/*
+			 * 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?

Quoted 5 lines

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