[patch 04/12] block/xd: replace schedule_timeout() with msleep()

[patch 04/12] block/xd: replace schedule_timeout() with msleep()

[domen]

[-- Attachment #1: msleep-drivers_block_xd2.patch --]
[-- Type: text/plain, Size: 1323 bytes --]

From: Nishanth Aravamudan <nacc@us.ibm.com>



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 <nacc@us.ibm.com>
Signed-off-by: Domen Puncer <domen@coderock.org>
---
 xd.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

Index: quilt/drivers/block/xd.c
===================================================================
--- quilt.orig/drivers/block/xd.c
+++ 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);
 }

--

Re: [patch 04/12] block/xd: replace schedule_timeout() with msleep()

[Maciej W. Rozycki]

On Mon, 20 Jun 2005 domen@coderock.org wrote:

> From: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> 
> 
> 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.
[...]
> @@ -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

Re: [patch 04/12] block/xd: replace schedule_timeout() with msleep()

[Domen Puncer]

On 21/06/05 12:39 +0100, Maciej W. Rozycki wrote:
> On Mon, 20 Jun 2005 domen@coderock.org wrote:
> 
> > From: Nishanth Aravamudan <nacc@us.ibm.com>
> > 
> > 
> > 
> > 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.
> [...]
> > @@ -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.

mdelay - busy loop
msleep - schedule

> 
>  Maciej

Re: [patch 04/12] block/xd: replace schedule_timeout() with msleep()

[Maciej W. Rozycki]

On Tue, 21 Jun 2005, Domen Puncer wrote:

> 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

Re: [patch 04/12] block/xd: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On 21.06.2005 [14:53:49 +0100], Maciej W. Rozycki wrote:
> On Tue, 21 Jun 2005, Domen Puncer wrote:
> 
> > 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

Re: [patch 04/12] block/xd: replace schedule_timeout() with msleep()

[Maciej W. Rozycki]

On Tue, 21 Jun 2005, Nishanth Aravamudan wrote:

> 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.

> 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

Re: [patch 04/12] block/xd: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On 21.06.2005 [17:43:53 +0100], Maciej W. Rozycki wrote:
> On Tue, 21 Jun 2005, Nishanth Aravamudan wrote:
> 
> > 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.

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.

> > 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