[Kernel-janitors] RE: [PATCH] cciss: replace schedule_timeout()

[Kernel-janitors] RE: [PATCH] cciss: replace schedule_timeout()

[Miller, Mike (OS Dev)]

You'll have to explain why msleep will guarantee the timeout period whereas schedule_timeout may not. Maybe I'm missing something.

mikem

-----Original Message-----
From: Nishanth Aravamudan [mailto:nacc@us.ibm.com]
Sent: Monday, July 19, 2004 4:39 PM
To: Miller, Mike (OS Dev)
Cc: kernel-janitors@lists.osdl.org
Subject: [PATCH] cciss: replace schedule_timeout() with msleep()


I would appreciate any comments from the janitors 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



Applys-to: 2.6.7

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>

--- linux-vanilla/drivers/block/cciss.c	2004-06-16 05:20:04.000000000 +0000
+++ linux-dev/drivers/block/cciss.c	2004-07-10 18:19:33.000000000 +0000
@@ -2242,8 +2242,7 @@ static int cciss_pci_init(ctlr_info_t *c
 		scratchpad = readl(c->vaddr + SA5_SCRATCHPAD_OFFSET);
 		if (scratchpad = CCISS_FIRMWARE_READY)
 			break;
-		set_current_state(TASK_INTERRUPTIBLE);
-		schedule_timeout(HZ / 10); /* wait 100ms */
+		msleep(100);
 	}
 	if (scratchpad != CCISS_FIRMWARE_READY) {
 		printk(KERN_WARNING "cciss: Board not ready.  Timed out.\n");

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] RE: [PATCH] cciss: replace schedule_timeout()

[Nish Aravamudan]

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

uOn Fri, 23 Jul 2004 17:45:55 -0500, Miller, Mike (OS Dev)
<mike.miller@hp.com> wrote:
> You'll have to explain why msleep will guarantee the timeout period whereas 
> schedule_timeout may not. Maybe I'm missing something.

Well, there are two things to consider here:

As a reference, the way the code was written before: 
-               set_current_state(TASK_INTERRUPTIBLE);
-               schedule_timeout(HZ / 10); /* wait 100ms */

1) Since the state is set to TASK_INTERRUPTIBLE, although a 100ms
delay is requested, if a signal is received, then the delay ends
prematurely and the task will resume after the schedule_timeout()
call. Now, if this was the desired effect, then the the patch should
not be applied.

2) However, if a true delay was desired, then TASK_INTERRUPTIBLE
should not have been the set state, but TASK_UNINTERRUPTIBLE should
have been. This would have made the code:
                 set_current_state(TASK_UNINTERRUPTIBLE);
                 schedule_timeout(HZ / 10);
msleep() achieves this exact purpose, but also:
  a) allows the parameter time to be in msecs, which is far clearer IMO; and
  b) guarantees that the task will not continue execution until after
the timeout has expired. Confusingly (at least to me), there are
certain conditions -- the details of which I will have to defer to
Greg Kroah-Hartman -- in which TASK_UNINTERRUPTIBLE'd
schedule_timeout()s may still return before the timeout. The desired
delay is achieved by wrapping the schedule_timeout(timeout) with a
while(timeout) loop. Thus, as long as timeout is non-zero, the task is
deferred. (see msleep()s definition for more details).

Does that help some? Please mail again if you have more questions, or
if something I wrote was unclear.

-Nish

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

RE: [Kernel-janitors] RE: [PATCH] cciss: replace schedule_timeout()

[Miller, Mike (OS Dev)]

Nish,
Sorry I've taken so long to respond. I just figured out yesterday that my Linux mail was not going out :(

I agree about TASK_INTERRUPTIBLE, but in all the 2.6.x code I've looked at we are using TASK_UNINTERRUPTIBLE.
If Greg Kroah-Hartman would care to elaborate on the problems using that I am open to new ideas & suggestions.

Thanks,
mikem
-----Original Message-----
From: Nish Aravamudan [mailto:nish.aravamudan@gmail.com]
Sent: Friday, July 23, 2004 6:27 PM
To: Miller, Mike (OS Dev)
Cc: kernel-janitors@lists.osdl.org
Subject: Re: [Kernel-janitors] RE: [PATCH] cciss: replace
schedule_timeout() with msleep()


uOn Fri, 23 Jul 2004 17:45:55 -0500, Miller, Mike (OS Dev)
<mike.miller@hp.com> wrote:
> You'll have to explain why msleep will guarantee the timeout period whereas 
> schedule_timeout may not. Maybe I'm missing something.

Well, there are two things to consider here:

As a reference, the way the code was written before: 
-               set_current_state(TASK_INTERRUPTIBLE);
-               schedule_timeout(HZ / 10); /* wait 100ms */

1) Since the state is set to TASK_INTERRUPTIBLE, although a 100ms
delay is requested, if a signal is received, then the delay ends
prematurely and the task will resume after the schedule_timeout()
call. Now, if this was the desired effect, then the the patch should
not be applied.

2) However, if a true delay was desired, then TASK_INTERRUPTIBLE
should not have been the set state, but TASK_UNINTERRUPTIBLE should
have been. This would have made the code:
                 set_current_state(TASK_UNINTERRUPTIBLE);
                 schedule_timeout(HZ / 10);
msleep() achieves this exact purpose, but also:
  a) allows the parameter time to be in msecs, which is far clearer IMO; and
  b) guarantees that the task will not continue execution until after
the timeout has expired. Confusingly (at least to me), there are
certain conditions -- the details of which I will have to defer to
Greg Kroah-Hartman -- in which TASK_UNINTERRUPTIBLE'd
schedule_timeout()s may still return before the timeout. The desired
delay is achieved by wrapping the schedule_timeout(timeout) with a
while(timeout) loop. Thus, as long as timeout is non-zero, the task is
deferred. (see msleep()s definition for more details).

Does that help some? Please mail again if you have more questions, or
if something I wrote was unclear.

-Nish

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors