linux-kernel - [PATCH v2] relay: Use irq_work instead of plain timer for deferred wakeup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <1472906487-1559-1-git-send-email-akash.goel@intel.com>
Date:   Sat,  3 Sep 2016 18:11:27 +0530
From:   akash.goel@...el.com
To:     akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Tom Zanussi <tzanussi@...il.com>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        Tvrtko Ursulin <tvrtko.ursulin@...el.com>,
        Akash Goel <akash.goel@...el.com>
Subject: [PATCH v2] relay: Use irq_work instead of plain timer for deferred wakeup

From: Peter Zijlstra <peterz@...radead.org>

Relay avoids calling wake_up_interruptible() for doing the wakeup of
readers/consumers, waiting for the generation of new data, from the
context of a process which produced the data. This is apparently done
to prevent the possibility of a deadlock in case Scheduler itself is
is generating data for the relay, after acquiring rq->lock.

The following patch used a timer (to be scheduled at next jiffy), for
delegating the wakeup to another context.
	commit 7c9cb38302e78d24e37f7d8a2ea7eed4ae5f2fa7
	Author: Tom Zanussi <zanussi@...cast.net>
	Date:   Wed May 9 02:34:01 2007 -0700

	relay: use plain timer instead of delayed work

	relay doesn't need to use schedule_delayed_work() for waking readers
	when a simple timer will do.

Scheduling a plain timer, at next jiffies boundary, to do the wakeup causes
a significant wakeup latency for the Userspace client, which makes relay less
suitable for the high-frequency low-payload use cases where the data gets
generated at a very high rate, like multiple sub buffers getting filled
within a milli second.
Moreover the timer is re-scheduled on every newly produced sub buffer so the
timer keeps getting pushed out if sub buffers are filled in a very quick
succession (less than a jiffy gap between filling of 2 sub buffers).
As a result relay runs out of sub buffers to store the new data.

By using irq_work it is ensured that wakeup of userspace client, blocked in
the poll call, is done at earliest (through self IPI or next timer tick)
enabling him to always consume the data in time.
Also this makes relay consistent with printk & ring buffers (trace), as they
too use irq_work for deferred wake up of readers.

v2: Fix the documentation related warning.

Cc: Tom Zanussi <tzanussi@...il.com>
Cc: Chris Wilson <chris@...is-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@...el.com>
Signed-off-by: Peter Zijlstra <peterz@...radead.org>
Signed-off-by: Akash Goel <akash.goel@...el.com>
---
 include/linux/relay.h |  3 ++-
 kernel/relay.c        | 22 ++++++++++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index eb295e3..54960a5 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -20,6 +20,7 @@
 #include <linux/poll.h>
 #include <linux/kref.h>
 #include <linux/percpu.h>
+#include <linux/irq_work.h>
 
 /*
  * Tracks changes to rchan/rchan_buf structs
@@ -38,7 +39,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 timer_list timer; 	/* reader wake-up timer */
+	struct irq_work wakeup_work;	/* reader wakeup */
 	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 f55ab82..946f976 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -328,13 +328,13 @@ static struct rchan_callbacks default_channel_callbacks = {
 
 /**
  *	wakeup_readers - wake up readers waiting on a channel
- *	@data: contains the channel buffer
+ *	@work: contains the channel buffer
  *
- *	This is the timer function used to defer reader waking.
+ *	This is the function used to defer reader waking
  */
-static void wakeup_readers(unsigned long data)
+static void wakeup_readers(struct irq_work *work)
 {
-	struct rchan_buf *buf = (struct rchan_buf *)data;
+	struct rchan_buf *buf = container_of(work, struct rchan_buf, wakeup_work);
 	wake_up_interruptible(&buf->read_wait);
 }
 
@@ -352,9 +352,10 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
 	if (init) {
 		init_waitqueue_head(&buf->read_wait);
 		kref_init(&buf->kref);
-		setup_timer(&buf->timer, wakeup_readers, (unsigned long)buf);
-	} else
-		del_timer_sync(&buf->timer);
+		init_irq_work(&buf->wakeup_work, wakeup_readers);
+	} else {
+		irq_work_sync(&buf->wakeup_work);
+	}
 
 	buf->subbufs_produced = 0;
 	buf->subbufs_consumed = 0;
@@ -487,7 +488,7 @@ free_buf:
 static void relay_close_buf(struct rchan_buf *buf)
 {
 	buf->finalized = 1;
-	del_timer_sync(&buf->timer);
+	irq_work_sync(&buf->wakeup_work);
 	buf->chan->cb->remove_buf_file(buf->dentry);
 	kref_put(&buf->kref, relay_remove_buf);
 }
@@ -777,14 +778,15 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
 			buf->early_bytes += buf->chan->subbuf_size -
 					    buf->padding[old_subbuf];
 		smp_mb();
-		if (waitqueue_active(&buf->read_wait))
+		if (waitqueue_active(&buf->read_wait)) {
 			/*
 			 * 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.
 			 */
-			mod_timer(&buf->timer, jiffies + 1);
+			irq_work_queue(&buf->wakeup_work);
+		}
 	}
 
 	old = buf->data;
-- 
1.9.2