block/xd: replace schedule_timeout() with msleep()

PATCH In review [email protected] 3 participants 7 messages Jun 21, 2005 Block layer

lore

Share & tools

lore

[email protected]

author

2005-06-21 06:23 UTC #0 lore raw

PATCH 4/12 block/xd: replace schedule_timeout() with msleep()

From: Nishanth Aravamudan <[email protected]>

Use msleep() instead of schedule_timeout() to guarantee the task delays as expected. The current code wishes to sleep for 1 jiffy, but I am not sure if this is actually intended, as with the change to HZ=1000, the time equivalent of 1 jiffy changed from 10ms to 1ms. I have assumed the former in this case; however the patch can be easily changed to assume the latter.

Signed-off-by Nishanth Aravamudan <[email protected]>

Signed-off-by Domen Puncer <[email protected]>

---
 xd.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

drivers/block/xd.c +3 -4

Index: quilt/drivers/block/xd.c
===================================================================
@@ -47,6 +47,7 @@
 #include <linux/wait.h>
 #include <linux/blkdev.h>
 #include <linux/blkpg.h>
+#include <linux/delay.h>
 
 #include <asm/system.h>
 #include <asm/io.h>

@@ -529,10 +530,8 @@ static inline u_char xd_waitport (u_shor
 	int success;
 
 	xdc_busy = 1;
-	while ((success = ((inb(port) & mask) != flags)) && time_before(jiffies, expiry)) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(1);
-	}
+	while ((success = ((inb(port) & mask) != flags)) && time_before(jiffies, expiry))
+		msleep(10);
 	xdc_busy = 0;
 	return (success);
 }

Maciej W. Rozycki

2005-06-21 11:43 UTC #1 lore raw

Quoted 9 lines · On Mon, 20 Jun 2005 [email protected] wrote:

From: Nishanth Aravamudan <[email protected]>
Use msleep() instead of schedule_timeout() to guarantee the task delays as expected. The current code wishes to sleep for 1 jiffy, but I am not sure if this is actually intended, as with the change to HZ=1000, the time equivalent of 1 jiffy changed from 10ms to 1ms. I have assumed the former in this case; however the patch can be easily changed to assume the latter.

[...]

drivers/block/xd.c static inline u_char xd_waitport (u_shor · line 537

Quoted 13 lines

drivers/block/xd.c +2 -4

@@ -529,10 +530,8 @@ static inline u_char xd_waitport (u_shor
 	int success;
 
 	xdc_busy = 1;
-	while ((success = ((inb(port) & mask) != flags)) && time_before(jiffies, expiry)) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(1);
-	}
+	while ((success = ((inb(port) & mask) != flags)) && time_before(jiffies, expiry))
+		msleep(10);
 	xdc_busy = 0;
 	return (success);
 }

I think it's obvious what this code intends and this makes the function
to busy-loop instead of giving up the CPU friendly. The change makes no sense -- the code is correct as is.

Maciej

Domen Puncer

author

2005-06-21 13:35 UTC #2 lore raw

Quoted 15 lines · Maciej W. Rozycki

Quoted 9 lines · On Mon, 20 Jun 2005 [email protected] wrote:
From: Nishanth Aravamudan <[email protected]>
Use msleep() instead of schedule_timeout() to guarantee the task delays as expected. The current code wishes to sleep for 1 jiffy, but I am not sure if this is actually intended, as with the change to HZ=1000, the time equivalent of 1 jiffy changed from 10ms to 1ms. I have assumed the former in this case; however the patch can be easily changed to assume the latter.
[...]
Quoted 3 lines
drivers/block/xd.c +0 -0
@@ -529,10 +530,8 @@ static inline u_char xd_waitport (u_shor
 	int success;
 

Quoted 14 lines

drivers/block/xd.c static inline u_char xd_waitport (u_shor · line 537
Quoted 10 lines
drivers/block/xd.c +2 -4
 	xdc_busy = 1;
-	while ((success = ((inb(port) & mask) != flags)) && time_before(jiffies, expiry)) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(1);
-	}
+	while ((success = ((inb(port) & mask) != flags)) && time_before(jiffies, expiry))
+		msleep(10);
 	xdc_busy = 0;
 	return (success);
 }
I think it's obvious what this code intends and this makes the function
to busy-loop instead of giving up the CPU friendly. The change makes no sense -- the code is correct as is.

mdelay - busy loop
msleep - schedule

Quoted 2 lines

Maciej

Maciej W. Rozycki

2005-06-21 13:56 UTC #3 lore raw

Quoted 2 lines · Domen Puncer

mdelay - busy loop
msleep - schedule

Right -- that's my mistake. But what's the point of the change in the
first place anyway? The original code is correct.

Maciej

Nishanth Aravamudan

2005-06-21 16:17 UTC #4 lore raw

Quoted 7 lines · Maciej W. Rozycki

Quoted 2 lines · Domen Puncer
mdelay - busy loop
msleep - schedule
Right -- that's my mistake. But what's the point of the change in the
first place anyway? The original code is correct.

Please refer to the comment in the description:

schedule_timeout(1) is ambiguous in older/unchanged code since 2.4, as it indicated a 10 millisecond sleep then. Now, in 2.6, it indicates a 1 millisecond sleep (HZ==1000). I am trying to prevent issues like this coming up in the future (CONFIG_HZ has hit -mm, e.g.) and msleep() is a good way to do so.

If you are trying to sleep for the shortest amount of time possible (a tick), though, then the code is fine, I guess. A comment may be useful, though.

Thanks,
Nish

Maciej W. Rozycki

2005-06-21 16:47 UTC #5 lore raw

Quoted 5 lines · Nishanth Aravamudan

schedule_timeout(1) is ambiguous in older/unchanged code since 2.4, as it indicated a 10 millisecond sleep then. Now, in 2.6, it indicates a 1 millisecond sleep (HZ==1000). I am trying to prevent issues like this coming up in the future (CONFIG_HZ has hit -mm, e.g.) and msleep() is a good way to do so.

Well, HZ has never been consistent across platforms, e.g. 1024 for
the Alpha or even within certain platforms, e.g. 128 vs 100 for MIPS for different configurations, so relying on jiffies to provide any absolute time measurement has always been a misconception. But assuming all code authors have failed to observe it is probably going a little bit too far, so I'd always assume a given piece of code is correct unless I had reasons to decide it does something silly.

Quoted 3 lines

If you are trying to sleep for the shortest amount of time possible (a tick), though, then the code is fine, I guess. A comment may be useful, though.

This is obviously the case -- the code waits for a condition of an I/O
device to change and does not want to hog the CPU for the duration as the device is slooow. I don't think any comment is needed -- it speaks for
itself: "I'm giving up now, but let me proceed at the next opportunity."

Maciej

Nishanth Aravamudan

2005-06-21 18:30 UTC #6 lore raw

Quoted 8 lines · Maciej W. Rozycki

Quoted 5 lines · Nishanth Aravamudan
schedule_timeout(1) is ambiguous in older/unchanged code since 2.4, as it indicated a 10 millisecond sleep then. Now, in 2.6, it indicates a 1 millisecond sleep (HZ==1000). I am trying to prevent issues like this coming up in the future (CONFIG_HZ has hit -mm, e.g.) and msleep() is a good way to do so.

Quoted 7 lines

Well, HZ has never been consistent across platforms, e.g. 1024 for
the Alpha or even within certain platforms, e.g. 128 vs 100 for MIPS for different configurations, so relying on jiffies to provide any absolute time measurement has always been a misconception. But assuming all code authors have failed to observe it is probably going a little bit too far, so I'd always assume a given piece of code is correct unless I had reasons to decide it does something silly.

Right, I agree HZ is inconsistent. That is why I am trying to get users to use human-time interfaces, that way when HZ becomes dynamic (think tickless!) or variable at compile-time (already in -mm with efforts to get it merged to mainline via CONFIG_HZ). I know code authors have failed to observe the brokenness of ticks as time and am trying my hardest to fix up as many callers as possible. If nothing else, use

schedule_timeout({msecs,usecs,nsecs}_to_jiffies(some_time_value));

I'm not saying your code is incorrect, by the way, I am just trying to make it maintainable and work on a kerneljanitor TODO.

Quoted 8 lines

Quoted 3 lines
If you are trying to sleep for the shortest amount of time possible (a tick), though, then the code is fine, I guess. A comment may be useful, though.
This is obviously the case -- the code waits for a condition of an I/O
device to change and does not want to hog the CPU for the duration as the device is slooow. I don't think any comment is needed -- it speaks for
itself: "I'm giving up now, but let me proceed at the next opportunity."

Ok, there isn't much point in arguing this further then. My point was simply this: there is *a lot* of code that has not been updated since 2.4 wrt. to sleep times. Thus, I cannot tell by reading the code, necessarily, whether a schedule_timeout(1) is referring to 1 millisecond or 1 tick. But that just may be me being confused.

I appreciate all your feedback, Maciej.

Thanks,
Nish