[PATCH] hvc_console polling mode timer backoff

[PATCH] hvc_console polling mode timer backoff

[Will Schmidt]

Add a backoff mechanism to hvc_console's polling logic.   This change
drops the timers/second ratio from ~90 to ~1 while the console is idle.
This is very noticable when watching /proc/timer_stats output.

This only affects when the hvc_console is running in poll mode, i.e. power4
and cell systems.

I've tested on Power4, Michael Ellerman has tested on cell.

Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
cc: Michael Ellerman <michael@ellerman.id.au>

---

 drivers/char/hvc_console.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index a0a88aa..8ac8066 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -47,8 +47,6 @@ #include "hvc_console.h"
 #define HVC_MAJOR	229
 #define HVC_MINOR	0
 
-#define TIMEOUT		(10)
-
 /*
  * Wait this long per iteration while trying to push buffered data to the
  * hypervisor before allowing the tty to complete a close operation.
@@ -550,6 +548,19 @@ static int hvc_chars_in_buffer(struct tt
 	return hp->n_outbuf;
 }
 
+/*
+ * timeout will vary between the MIN and MAX values defined here.  By default
+ * and during console activity we will use a default MIN_TIMEOUT of 10.  When
+ * the console is idle, we increase the timeout value on each pass through
+ * msleep until we reach the max.  This may be noticeable as a brief (average
+ * one second) delay on the console before the console responds to input when
+ * there has been no input for some time.
+ */
+#define MIN_TIMEOUT		(10)
+#define MAX_TIMEOUT		(2000)
+static u32 timeout = MIN_TIMEOUT;
+
+
 #define HVC_POLL_READ	0x00000001
 #define HVC_POLL_WRITE	0x00000002
 
@@ -642,9 +653,14 @@ #endif /* CONFIG_MAGIC_SYSRQ */
  bail:
 	spin_unlock_irqrestore(&hp->lock, flags);
 
-	if (read_total)
+	if (read_total) {
+		/* Activity is occurring, so reset the polling backoff value to
+		   a minimum for performance. */
+		timeout = MIN_TIMEOUT;
+
 		tty_flip_buffer_push(tty);
-	
+	}
+
 	return poll_mask;
 }
 
@@ -688,8 +704,11 @@ int khvcd(void *unused)
 		if (!hvc_kicked) {
 			if (poll_mask == 0)
 				schedule();
-			else
-				msleep_interruptible(TIMEOUT);
+			else {
+				if (timeout < MAX_TIMEOUT)
+					timeout++;
+				msleep_interruptible(timeout);
+			}
 		}
 		__set_current_state(TASK_RUNNING);
 	} while (!kthread_should_stop());

Re: [PATCH] hvc_console typo corrections

[Will Schmidt]

Fix some typos in hvc_console code.   Comments only, no code changes.

Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>

---

 drivers/char/hvc_console.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index 8ac8066..b6403db 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -102,12 +102,12 @@ static DEFINE_SPINLOCK(hvc_structs_lock)
 /*
  * This value is used to assign a tty->index value to a hvc_struct based
  * upon order of exposure via hvc_probe(), when we can not match it to
- * a console canidate registered with hvc_instantiate().
+ * a console candidate registered with hvc_instantiate().
  */
 static int last_hvc = -1;
 
 /*
- * Do not call this function with either the hvc_strucst_lock or the hvc_struct
+ * Do not call this function with either the hvc_structs_lock or the hvc_struct
  * lock held.  If successful, this function increments the kobject reference
  * count against the target hvc_struct so it should be released when finished.
  */
@@ -160,7 +160,7 @@ void hvc_console_print(struct console *c
 	if (index >= MAX_NR_HVC_CONSOLES)
 		return;
 
-	/* This console adapter was removed so it is not useable. */
+	/* This console adapter was removed so it is not usable. */
 	if (vtermnos[index] < 0)
 		return;
 
@@ -218,7 +218,7 @@ struct console hvc_con_driver = {
 };
 
 /*
- * Early console initialization.  Preceeds driver initialization.
+ * Early console initialization.  Precedes driver initialization.
  *
  * (1) we are first, and the user specified another driver
  * -- index will remain -1
@@ -255,7 +255,7 @@ int hvc_instantiate(uint32_t vtermno, in
 	if (vtermnos[index] != -1)
 		return -1;
 
-	/* make sure no no tty has been registerd in this index */
+	/* make sure no no tty has been registered in this index */
 	hp = hvc_get_by_index(index);
 	if (hp) {
 		kobject_put(&hp->kobj);
@@ -265,7 +265,7 @@ int hvc_instantiate(uint32_t vtermno, in
 	vtermnos[index] = vtermno;
 	cons_ops[index] = ops;
 
-	/* reserve all indices upto and including this index */
+	/* reserve all indices up to and including this index */
 	if (last_hvc < index)
 		last_hvc = index;
 
@@ -526,7 +526,7 @@ static int hvc_write(struct tty_struct *
 
 /*
  * This is actually a contract between the driver and the tty layer outlining
- * how much write room the driver can guarentee will be sent OR BUFFERED.  This
+ * how much write room the driver can guarantee will be sent OR BUFFERED.  This
  * driver MUST honor the return value.
  */
 static int hvc_write_room(struct tty_struct *tty)
@@ -813,7 +813,7 @@ int __devexit hvc_remove(struct hvc_stru
 
 	/*
 	 * We 'put' the instance that was grabbed when the kobject instance
-	 * was intialized using kobject_init().  Let the last holder of this
+	 * was initialized using kobject_init().  Let the last holder of this
 	 * kobject cause it to be removed, which will probably be the tty_hangup
 	 * below.
 	 */
@@ -869,7 +869,7 @@ int __init hvc_init(void)
 }
 module_init(hvc_init);
 
-/* This isn't particularily necessary due to this being a console driver
+/* This isn't particularly necessary due to this being a console driver
  * but it is nice to be thorough.
  */
 static void __exit hvc_exit(void)

Re: [PATCH] hvc_console polling mode timer backoff

[Linas Vepstas]

On Thu, Apr 12, 2007 at 11:14:36AM -0500, Will Schmidt wrote:
> +/*
> + * timeout will vary between the MIN and MAX values defined here.  By default
> + * and during console activity we will use a default MIN_TIMEOUT of 10.  When
> + * the console is idle, we increase the timeout value on each pass through
> + * msleep until we reach the max.  This may be noticeable as a brief (average
> + * one second) delay on the console before the console responds to input when
> + * there has been no input for some time.
> + */
> +#define MIN_TIMEOUT		(10)
> +#define MAX_TIMEOUT		(2000)

[...]
> +				msleep_interruptible(timeout);


These values are milliseconds (that's what the m in msleep stands for
or at least it did last time I looked). This 10 is 1/100 of a second,
which makes a responsive keyboard for even a very very fast typist.
That's fine.  But 2000 is two seconds, which is longer than the amount 
of time that I wait before I start panicking that something is broken. 

I'd suggest that maybe 1000 or 750 or 500 is more apropriate.

--linas

Re: [PATCH] hvc_console polling mode timer backoff

[Will Schmidt]

On Thu, 2007-12-04 at 12:33 -0500, Linas Vepstas wrote:
> On Thu, Apr 12, 2007 at 11:14:36AM -0500, Will Schmidt wrote:
> > +/*
> > + * timeout will vary between the MIN and MAX values defined here.  By default
> > + * and during console activity we will use a default MIN_TIMEOUT of 10.  When
> > + * the console is idle, we increase the timeout value on each pass through
> > + * msleep until we reach the max.  This may be noticeable as a brief (average
> > + * one second) delay on the console before the console responds to input when
> > + * there has been no input for some time.
> > + */
> > +#define MIN_TIMEOUT		(10)
> > +#define MAX_TIMEOUT		(2000)
> 
> [...]
> > +				msleep_interruptible(timeout);
> 
> 
> These values are milliseconds (that's what the m in msleep stands for
> or at least it did last time I looked). This 10 is 1/100 of a second,
> which makes a responsive keyboard for even a very very fast typist.
> That's fine.  But 2000 is two seconds, which is longer than the amount 
> of time that I wait before I start panicking that something is broken. 

Well, I usually give my systems at least ten seconds before I start to
worry. :-) 

I did ponder the max value to use.   Though when I sit down at &random&
console in the lab, It often takes a moment for the terminal to warm up,
or for the screen to unblank.   With that in mind, a couple seconds max
for the console does not seem unreasonable to me.

Will see if other folks have an opinion either way.. 

> 
> I'd suggest that maybe 1000 or 750 or 500 is more apropriate.
> 
> --linas

Re: [PATCH] hvc_console polling mode timer backoff

[Olof Johansson]

On Thu, Apr 12, 2007 at 01:43:30PM -0500, Will Schmidt wrote:
> On Thu, 2007-12-04 at 12:33 -0500, Linas Vepstas wrote:
> > On Thu, Apr 12, 2007 at 11:14:36AM -0500, Will Schmidt wrote:
> > > +/*
> > > + * timeout will vary between the MIN and MAX values defined here.  By default
> > > + * and during console activity we will use a default MIN_TIMEOUT of 10.  When
> > > + * the console is idle, we increase the timeout value on each pass through
> > > + * msleep until we reach the max.  This may be noticeable as a brief (average
> > > + * one second) delay on the console before the console responds to input when
> > > + * there has been no input for some time.
> > > + */
> > > +#define MIN_TIMEOUT		(10)
> > > +#define MAX_TIMEOUT		(2000)
> > 
> > [...]
> > > +				msleep_interruptible(timeout);
> > 
> > 
> > These values are milliseconds (that's what the m in msleep stands for
> > or at least it did last time I looked). This 10 is 1/100 of a second,
> > which makes a responsive keyboard for even a very very fast typist.
> > That's fine.  But 2000 is two seconds, which is longer than the amount 
> > of time that I wait before I start panicking that something is broken. 
> 
> Well, I usually give my systems at least ten seconds before I start to
> worry. :-) 
> 
> I did ponder the max value to use.   Though when I sit down at &random&
> console in the lab, It often takes a moment for the terminal to warm up,
> or for the screen to unblank.   With that in mind, a couple seconds max
> for the console does not seem unreasonable to me.
> 
> Will see if other folks have an opinion either way.. 

I wasn't going to bring it up since I think it's a good approach as is,
but another thing that could be worth considering is to not just go
from whatever value to min on every actual input, instead go down in
delay a bit (say half the delay). Likewise you might be able to back off
more aggressively than just one millisecond per iteration.   Of course,
both of these should take min/max values into consideration.

It's the kind of change that's useful to get in early in the .22 cycle
no matter what, so if there are some interactivity hickups people have
a chance to tune it.

Besides that i think it's a great approach, there's no use in polling an
idle console often.


-Olof

Re: [PATCH] hvc_console polling mode timer backoff

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2894 bytes --]

On Thu, 2007-04-12 at 14:09 -0500, Olof Johansson wrote:
> On Thu, Apr 12, 2007 at 01:43:30PM -0500, Will Schmidt wrote:
> > On Thu, 2007-12-04 at 12:33 -0500, Linas Vepstas wrote:
> > > On Thu, Apr 12, 2007 at 11:14:36AM -0500, Will Schmidt wrote:
> > > > +/*
> > > > + * timeout will vary between the MIN and MAX values defined here.  By default
> > > > + * and during console activity we will use a default MIN_TIMEOUT of 10.  When
> > > > + * the console is idle, we increase the timeout value on each pass through
> > > > + * msleep until we reach the max.  This may be noticeable as a brief (average
> > > > + * one second) delay on the console before the console responds to input when
> > > > + * there has been no input for some time.
> > > > + */
> > > > +#define MIN_TIMEOUT		(10)
> > > > +#define MAX_TIMEOUT		(2000)
> > > 
> > > [...]
> > > > +				msleep_interruptible(timeout);
> > > 
> > > 
> > > These values are milliseconds (that's what the m in msleep stands for
> > > or at least it did last time I looked). This 10 is 1/100 of a second,
> > > which makes a responsive keyboard for even a very very fast typist.
> > > That's fine.  But 2000 is two seconds, which is longer than the amount 
> > > of time that I wait before I start panicking that something is broken. 
> > 
> > Well, I usually give my systems at least ten seconds before I start to
> > worry. :-) 
> > 
> > I did ponder the max value to use.   Though when I sit down at &random&
> > console in the lab, It often takes a moment for the terminal to warm up,
> > or for the screen to unblank.   With that in mind, a couple seconds max
> > for the console does not seem unreasonable to me.
> > 
> > Will see if other folks have an opinion either way.. 

I found the 2 second value was fine. If you're really paying attention
you might notice the lag, but if you're not expecting it I don't think
it's enough to really scare you.

> I wasn't going to bring it up since I think it's a good approach as is,
> but another thing that could be worth considering is to not just go
> from whatever value to min on every actual input, instead go down in
> delay a bit (say half the delay). Likewise you might be able to back off
> more aggressively than just one millisecond per iteration.   Of course,
> both of these should take min/max values into consideration.

I experimented with increasing the delay faster (100ms per iteration)
and found it effected the feel of the console too much when you're
actually typing. Though perhaps increasing by 10%-20% of the current
delay every iteration might work OK.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] hvc_console polling mode timer backoff

[Olof Johansson]

On Fri, Apr 13, 2007 at 10:45:43AM +1000, Michael Ellerman wrote:
> On Thu, 2007-04-12 at 14:09 -0500, Olof Johansson wrote:
> > I wasn't going to bring it up since I think it's a good approach as is,
> > but another thing that could be worth considering is to not just go
> > from whatever value to min on every actual input, instead go down in
> > delay a bit (say half the delay). Likewise you might be able to back off
> > more aggressively than just one millisecond per iteration.   Of course,
> > both of these should take min/max values into consideration.
> 
> I experimented with increasing the delay faster (100ms per iteration)
> and found it effected the feel of the console too much when you're
> actually typing. Though perhaps increasing by 10%-20% of the current
> delay every iteration might work OK.

Ok, no need to do it differently then really. I mostly wondered if
someone had already experimented.


-Olof

Re: [PATCH] hvc_console polling mode timer backoff

[Michael Ellerman]

On Thu, 2007-04-12 at 11:14 -0500, Will Schmidt wrote:
> Add a backoff mechanism to hvc_console's polling logic.   This change
> drops the timers/second ratio from ~90 to ~1 while the console is idle.
> This is very noticable when watching /proc/timer_stats output.
> 
> This only affects when the hvc_console is running in poll mode, i.e. power4
> and cell systems.
> 
> I've tested on Power4, Michael Ellerman has tested on cell.

I played with this a bit more, and a ~2% increment seems to work nicely.
It stays low for the first 10-20 seconds, but then approaches the max
much faster once there's no activity for a while.

A few examples:
http://michael.ellerman.id.au/files/cell-backoff.svg

As you can (sort of) see on the graph, after 30 seconds the delay is
still around 1/2 a second which is basically unnoticable IMHO.

Because this is a super tight performance critical inner loop I've
changed it to use a shift instead of divide. So now it's more like 1.5%,
but still the point is it increases faster as time passes.

It'd be good if some more people can bang on this and see what they
think of the interactivity.

Index: cell/drivers/char/hvc_console.c
===================================================================
--- cell.orig/drivers/char/hvc_console.c
+++ cell/drivers/char/hvc_console.c
@@ -47,8 +47,6 @@
 #define HVC_MAJOR	229
 #define HVC_MINOR	0
 
-#define TIMEOUT		(10)
-
 /*
  * Wait this long per iteration while trying to push buffered data to the
  * hypervisor before allowing the tty to complete a close operation.
@@ -550,6 +548,26 @@ static int hvc_chars_in_buffer(struct tt
 	return hp->n_outbuf;
 }
 
+/*
+ * timeout will vary between the MIN and MAX values defined here.  By default
+ * and during console activity we will use a default MIN_TIMEOUT of 10.  When
+ * the console is idle, we increase the timeout value on each pass through
+ * msleep until we reach the max.  This may be noticable as a brief (average
+ * one second) delay on the console before the console responds to input when
+ * there has been no input for sometime.
+ */
+#define MIN_TIMEOUT		(10)
+#define MAX_TIMEOUT		(2000)
+static u32 timeout = MIN_TIMEOUT;
+
+#include <linux/debugfs.h>
+static int hvc_debug_init(void)
+{
+	debugfs_create_u32("hvc_timeout", 0400, NULL, &timeout);
+	return 0;
+}
+__initcall(hvc_debug_init);
+
 #define HVC_POLL_READ	0x00000001
 #define HVC_POLL_WRITE	0x00000002
 
@@ -642,9 +660,14 @@ static int hvc_poll(struct hvc_struct *h
  bail:
 	spin_unlock_irqrestore(&hp->lock, flags);
 
-	if (read_total)
+	if (read_total) {
+		/* Activity is occurring, so reset the polling backoff value to
+		   a minimum for performance. */
+		timeout = MIN_TIMEOUT;
+
 		tty_flip_buffer_push(tty);
-	
+	}
+
 	return poll_mask;
 }
 
@@ -688,8 +711,12 @@ int khvcd(void *unused)
 		if (!hvc_kicked) {
 			if (poll_mask == 0)
 				schedule();
-			else
-				msleep_interruptible(TIMEOUT);
+			else {
+				if (timeout < MAX_TIMEOUT)
+					timeout += (timeout >> 6) + 1;
+
+				msleep_interruptible(timeout);
+			}
 		}
 		__set_current_state(TASK_RUNNING);
 	} while (!kthread_should_stop());

Re: [PATCH] hvc_console polling mode timer backoff

[Linas Vepstas]

On Fri, Apr 13, 2007 at 05:47:39PM +1000, Michael Ellerman wrote:
> 
> I played with this a bit more, and a ~2% increment seems to work nicely.
> It stays low for the first 10-20 seconds, but then approaches the max
> much faster once there's no activity for a while.
> 
> A few examples:
> http://michael.ellerman.id.au/files/cell-backoff.svg

Looks good to me. 

> changed it to use a shift instead of divide. So now it's more like 1.5%,
> but still the point is it increases faster as time passes.

Looks god to me. I suppose

Acked-by: Linas Vepstas <linas@austin.ibm.com> 

is the right thing to say here.

--linas

Re: [PATCH] hvc_console polling mode timer backoff

[Will Schmidt]

On Fri, 2007-13-04 at 17:47 +1000, Michael Ellerman wrote:
> Because this is a super tight performance critical inner loop I've
> changed it to use a shift instead of divide. So now it's more like 1.5%,
> but still the point is it increases faster as time passes.

Using a shift here is a good idea.  I had considered accelerating the
backoff value, but it got more complex than I wanted quickly.. I didnt
consider a shift. :-) 

> It'd be good if some more people can bang on this and see what they
> think of the interactivity.

Still works OK on my power4.   I can see the delay on my console, but
I'm specifically looking for it, I don't think it's an issue. 

> +#include <linux/debugfs.h>
> +static int hvc_debug_init(void)
> +{
> +	debugfs_create_u32("hvc_timeout", 0400, NULL, &timeout);
> +	return 0;
> +}
> +__initcall(hvc_debug_init);
> +

Is this part temporary for your graphing, or think it's something that
should go in? 
I used /proc/timer_stats output for my graphing, just dont have a good
public spot to upload the pics.. 

-will

Re: [PATCH] hvc_console polling mode timer backoff

[Milton Miller]

Michael Ellerman wrote:

> + * msleep until we reach the max.  This may be noticable as a brief 
> (average
> + * one second) delay on the console before the console responds to 
> input when
> + * there has been no input for sometime.
> + */
> +#define MIN_TIMEOUT            (10)
> +#define MAX_TIMEOUT            (2000)
> +static u32 timeout = MIN_TIMEOUT;

Did you consider making MAX_TIMEOUT a module parameter?   It could then
be changed at runtime through /sys/modules/.

> +
> +                               if (timeout < MAX_TIMEOUT)
> +                                       timeout += (timeout >> 6) + 1;

Keeping MIN_TIMEOUT a define is okay with me, and the code will
naturally ignore max < MIN.  It may overshoot timeout by 1/64th.

I doubt we would worry about the overflow case, because by then
the timeout would have been quite long, and the user would like
it to be short again.

milton

Re: [PATCH] hvc_console polling mode timer backoff

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Fri, 2007-04-13 at 14:11 -0500, Will Schmidt wrote:
> On Fri, 2007-13-04 at 17:47 +1000, Michael Ellerman wrote:
> > Because this is a super tight performance critical inner loop I've
> > changed it to use a shift instead of divide. So now it's more like 1.5%,
> > but still the point is it increases faster as time passes.
> 
> Using a shift here is a good idea.  I had considered accelerating the
> backoff value, but it got more complex than I wanted quickly.. I didnt
> consider a shift. :-) 
> 
> > It'd be good if some more people can bang on this and see what they
> > think of the interactivity.
> 
> Still works OK on my power4.   I can see the delay on my console, but
> I'm specifically looking for it, I don't think it's an issue. 

Cool. Yeah I think people will hardly notice it unless they're trying to
t notice it.

> > +#include <linux/debugfs.h>
> > +static int hvc_debug_init(void)
> > +{
> > +	debugfs_create_u32("hvc_timeout", 0400, NULL, &timeout);
> > +	return 0;
> > +}
> > +__initcall(hvc_debug_init);
> > +
> 
> Is this part temporary for your graphing, or think it's something that
> should go in? 

No that's just debug foobar I used for the graphs, I'll send a cleaned
up version during the week if you don't beat me to it.

> I used /proc/timer_stats output for my graphing, just dont have a good
> public spot to upload the pics.. 

Time to get a blag! ;)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] hvc_console polling mode timer backoff

[Linas Vepstas]

On Sat, Apr 14, 2007 at 02:42:02PM -0500, Milton Miller wrote:
> 
> Did you consider making MAX_TIMEOUT a module parameter?   It could then
> be changed at runtime through /sys/modules/.

There's a philosophy (that I have warmed up to over time) that 
says that too much kernel configurability is a bad thing.  
Less configurability makes problems easier to debug: you don't
have to worry about:

-- bugs that occur for only certain parameter settings
-- some dang-fool user/sysadmin setting the timout to 
   1000 seconds (mistaking seconds for milliseconds) and
   then reporting a bug.
-- the latest fedora core having some udev script that
   finds the hvc console and slaps some crazy values into it.

Surely, we have all known that bright-eyed, bushy-tailed 
developer who, when they ask you for help with a bug, 
your first questions are "did you mess with it?" and 
"which part did you mess with?"

--linas

Re: [PATCH] hvc_console polling mode timer backoff

[Will Schmidt]

On Sun, 2007-15-04 at 23:33 +1000, Michael Ellerman wrote:
> On Fri, 2007-04-13 at 14:11 -0500, Will Schmidt wrote:
> > On Fri, 2007-13-04 at 17:47 +1000, Michael Ellerman wrote:
> > 
> > Is this part temporary for your graphing, or think it's something that
> > should go in? 
> 
> No that's just debug foobar I used for the graphs, I'll send a cleaned
> up version during the week if you don't beat me to it.

I'll do it, just waiting to see if I should rework it to include a sys
interface or not..

> Time to get a blag! ;)

yup. 

> cheers
> 
-Will

Re: [PATCH] hvc_console polling mode timer backoff

[Will Schmidt]

On Sat, 2007-14-04 at 14:42 -0500, Milton Miller wrote:
> Michael Ellerman wrote:

> Did you consider making MAX_TIMEOUT a module parameter?   It could then
> be changed at runtime through /sys/modules/.

Yup, considered that.

Decided against it, for no reason other than "keeping it simple".  Not
sure that extra function does more good or bad. 

Who really wants the ability to tune their console timeout value
anyway? :-)    hrm, maybe thats a silly question.

For discussion, I'll attach a version of the patch that adds a sysfs
parm for setting max_timeout.   I'm not 100% on the S_IR* parms in the
param call, but verified that it works and it's sufficient to illustrate
what we're discussing.   
A downside is that it does no input checking, so if a useradmin wishes
to wait a very long time for their console, they can do so.  In this
implementation, it is worth noting that the current timeout value (say
100000), will not automatically decrease if a smaller max_timeout value
(say 500) is entered.  The timeout only gets reset (to MIN_TIMEOUT)
during console input.

> Keeping MIN_TIMEOUT a define is okay with me, and the code will
> naturally ignore max < MIN.  It may overshoot timeout by 1/64th.

> I doubt we would worry about the overflow case, because by then
> the timeout would have been quite long, and the user would like
> it to be short again.

You mean like ending up with a timeout of 2016 when max_timeout is 2000?
I think we're OK with that. :-)

> milton
> 

(Not sure I prefer this over the other one, but will see what other
commentary this generates...)
-Will

--
diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index a0a88aa..ba4c3cc 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -47,8 +47,6 @@ #include "hvc_console.h"
 #define HVC_MAJOR	229
 #define HVC_MINOR	0
 
-#define TIMEOUT		(10)
-
 /*
  * Wait this long per iteration while trying to push buffered data to the
  * hypervisor before allowing the tty to complete a close operation.
@@ -550,6 +548,23 @@ static int hvc_chars_in_buffer(struct tt
 	return hp->n_outbuf;
 }
 
+/*
+ * timeout will vary between the MIN and max_timeout values here.  By default
+ * and during console activity we will use a default MIN_TIMEOUT of 10.  When
+ * the console is idle, we increase the timeout value on each pass through
+ * msleep until we reach the max.  This may be noticeable as a brief (average
+ * one second) delay on the console before the console responds to input when
+ * there has been no input for some time.  The max_timeout defaults to 2000, but
+ * can be set via /sys/module/hvc_console/parameters/max_timeout.
+ */
+#define MIN_TIMEOUT		(10)
+static u32 max_timeout = 2000;
+static u32 timeout = MIN_TIMEOUT;
+
+module_param_named(max_timeout,max_timeout,uint,S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(max_timeout,
+	"Max for timer delay used by hvc_console in polling mode. (default: 2000)");
+
 #define HVC_POLL_READ	0x00000001
 #define HVC_POLL_WRITE	0x00000002
 
@@ -642,9 +657,14 @@ #endif /* CONFIG_MAGIC_SYSRQ */
  bail:
 	spin_unlock_irqrestore(&hp->lock, flags);
 
-	if (read_total)
+	if (read_total) {
+		/* Activity is occurring, so reset the polling backoff value to
+		   a minimum for performance. */
+		timeout = MIN_TIMEOUT;
+
 		tty_flip_buffer_push(tty);
-	
+	}
+
 	return poll_mask;
 }
 
@@ -688,8 +708,12 @@ int khvcd(void *unused)
 		if (!hvc_kicked) {
 			if (poll_mask == 0)
 				schedule();
-			else
-				msleep_interruptible(TIMEOUT);
+			else {
+				if (timeout < max_timeout)
+					timeout += (timeout >> 6) + 1;
+
+				msleep_interruptible(timeout);
+			}
 		}
 		__set_current_state(TASK_RUNNING);
 	} while (!kthread_should_stop());

Re: [PATCH] hvc_console polling mode timer backoff

[Linas Vepstas]

On Mon, Apr 16, 2007 at 03:22:23PM -0500, Will Schmidt wrote:
> 
> (Not sure I prefer this over the other one, but will see what other
> commentary this generates...)

Its making me think about the color of the bicycle shed.

--linas

Re: [PATCH] hvc_console polling mode timer backoff

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

On Mon, 2007-04-16 at 15:22 -0500, Will Schmidt wrote:
> On Sat, 2007-14-04 at 14:42 -0500, Milton Miller wrote:
> > Michael Ellerman wrote:
> 
> > Did you consider making MAX_TIMEOUT a module parameter?   It could then
> > be changed at runtime through /sys/modules/.
> 
> Yup, considered that.
> 
> Decided against it, for no reason other than "keeping it simple".  Not
> sure that extra function does more good or bad. 
> 
> Who really wants the ability to tune their console timeout value
> anyway? :-)    hrm, maybe thats a silly question.

Patch looks good. But I think it's overkill to have a parameter.

The range of useful values for MAX_TIMEOUT is 0 to perhaps 5000, it's
not like there's a vast array of possible values that people might
sensibly want to use. This is one of those places where we should just
pick a good value and stick with it.

If people really think we've got the wrong value, patches welcome.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]