[patch 21/25] hvc_console: replace schedule_timeout() with msleep()

[patch 21/25] hvc_console: replace schedule_timeout() with msleep()

[janitor]

I would appreciate any comments from the janitor@sternweltens list. This is one (of
many) cases where I made a decision about replacing

set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(some_time);

with

msleep(jiffies_to_msecs(some_time));

msleep() is not exactly the same as the previous code, but I only did
this replacement where I thought long delays were *desired*. If this is
not the case here, then just disregard this patch.

Thanks,
Nish



Description: Uses msleep() instead of schedule_timeout() to guarantee
the task delays at least the desired time amount.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Signed-off-by: Maximilian Attems <janitor@sternwelten.at>



---

 linux-2.6.9-rc1-bk7-max/drivers/char/hvc_console.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff -puN drivers/char/hvc_console.c~msleep-drivers_char_hvc_console drivers/char/hvc_console.c
--- linux-2.6.9-rc1-bk7/drivers/char/hvc_console.c~msleep-drivers_char_hvc_console	2004-09-01 21:01:32.000000000 +0200
+++ linux-2.6.9-rc1-bk7-max/drivers/char/hvc_console.c	2004-09-01 21:02:32.000000000 +0200
@@ -26,6 +26,7 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/sched.h>
+#include <linux/delay.h>
 #include <linux/kbd_kern.h>
 #include <asm/uaccess.h>
 #include <linux/spinlock.h>
@@ -40,7 +41,7 @@ extern int hvc_put_chars(int index, cons
 
 #define MAX_NR_HVC_CONSOLES	4
 
-#define TIMEOUT		((HZ + 99) / 100)
+#define TIMEOUT		10
 
 static struct tty_driver *hvc_driver;
 static int hvc_offset;
@@ -276,8 +277,7 @@ int khvcd(void *unused)
 			for (i = 0; i < MAX_NR_HVC_CONSOLES; ++i)
 				hvc_poll(i);
 		}
-		set_current_state(TASK_INTERRUPTIBLE);
-		schedule_timeout(TIMEOUT);
+		msleep(TIMEOUT);
 	}
 }
 

_

Re: [patch 21/25] hvc_console: replace schedule_timeout() with msleep()

[Andrew Morton]

janitor@sternwelten.at wrote:
>
> -#define TIMEOUT		((HZ + 99) / 100)
> +#define TIMEOUT		10
>  
>  static struct tty_driver *hvc_driver;
>  static int hvc_offset;
> @@ -276,8 +277,7 @@ int khvcd(void *unused)
>  			for (i = 0; i < MAX_NR_HVC_CONSOLES; ++i)
>  				hvc_poll(i);
>  		}
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		schedule_timeout(TIMEOUT);
> +		msleep(TIMEOUT);

This one is wrong: we need to sleep in interruptible state here, otherwise
this kernel thread will contribute to the system load average.

Several other of your msleep conversion patches actually fix bugs.  You've
found drivers which want to sleep for a fixed period, but they do that with
TASK_INTERRUPTIBLE.  If someone sends the calling process a signal, these
drivers will end up not sleeping at all and may fail.

I'll going through these patches and shall apply the ones which look right.
Please consider them all to have been handled, thanks.

Re: [patch 21/25] hvc_console: replace schedule_timeout() with msleep()

[maximilian attems]

On Wed, 01 Sep 2004, Andrew Morton wrote:

> janitor@sternwelten.at wrote:
> >
> > -#define TIMEOUT		((HZ + 99) / 100)
> > +#define TIMEOUT		10
> >  
> >  static struct tty_driver *hvc_driver;
> >  static int hvc_offset;
> > @@ -276,8 +277,7 @@ int khvcd(void *unused)
> >  			for (i = 0; i < MAX_NR_HVC_CONSOLES; ++i)
> >  				hvc_poll(i);
> >  		}
> > -		set_current_state(TASK_INTERRUPTIBLE);
> > -		schedule_timeout(TIMEOUT);
> > +		msleep(TIMEOUT);
> 
> This one is wrong: we need to sleep in interruptible state here, otherwise
> this kernel thread will contribute to the system load average.

could we add for such cases msleep_interruptible()?
patch for that function was sent separately.
 
> Several other of your msleep conversion patches actually fix bugs.  You've
> found drivers which want to sleep for a fixed period, but they do that with
> TASK_INTERRUPTIBLE.  If someone sends the calling process a signal, these
> drivers will end up not sleeping at all and may fail.

i'll instantly queue some more msleep conversions up.
 
> I'll going through these patches and shall apply the ones which look right.
> Please consider them all to have been handled, thanks.

thanks cool :)
 
--
maks
kernel janitor  	http://janitor.kernelnewbies.org/

Re: [patch 21/25] hvc_console: replace schedule_timeout() with msleep()

[Andrew Morton]

maximilian attems <janitor@sternwelten.at> wrote:
>
> > janitor@sternwelten.at wrote:
> > >
> > > -#define TIMEOUT		((HZ + 99) / 100)
> > > +#define TIMEOUT		10
> > >  
> > >  static struct tty_driver *hvc_driver;
> > >  static int hvc_offset;
> > > @@ -276,8 +277,7 @@ int khvcd(void *unused)
> > >  			for (i = 0; i < MAX_NR_HVC_CONSOLES; ++i)
> > >  				hvc_poll(i);
> > >  		}
> > > -		set_current_state(TASK_INTERRUPTIBLE);
> > > -		schedule_timeout(TIMEOUT);
> > > +		msleep(TIMEOUT);
> > 
> > This one is wrong: we need to sleep in interruptible state here, otherwise
> > this kernel thread will contribute to the system load average.
> 
> could we add for such cases msleep_interruptible()?

Sure.

Note that you're raising patches against some ancient kernel and that this
particular function has changed.