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 <[email protected]>
Signed-off-by: Maximilian Attems <[email protected]>
---
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);
}
}
_
[email protected] 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.
maximilian attems <[email protected]> wrote:
>
> > [email protected] 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.
On Wed, 01 Sep 2004, Andrew Morton wrote:
> [email protected] 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/