[PATCH] CDROM: replace jiffies busyloop with msleep

[PATCH] CDROM: replace jiffies busyloop with msleep

[Thomas Gleixner]

From: Ingo Molnar <mingo@elte.hu>

The SJCD driver uses a jiffies busy loop. Replace it with msleep.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/cdrom/sjcd.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/cdrom/sjcd.c
===================================================================
--- linux-2.6.orig/drivers/cdrom/sjcd.c
+++ linux-2.6/drivers/cdrom/sjcd.c
@@ -69,6 +69,7 @@
 #include <linux/string.h>
 #include <linux/major.h>
 #include <linux/init.h>
+#include <linux/delay.h>
 
 #include <asm/system.h>
 #include <asm/io.h>
@@ -1709,12 +1710,11 @@ static int __init sjcd_init(void)
 	printk(KERN_INFO "SJCD: Resetting: ");
 	sjcd_send_cmd(SCMD_RESET);
 	for (i = 1000; i > 0 && !sjcd_status_valid; --i) {
-		unsigned long timer;
-
 		/*
 		 * Wait 10ms approx.
 		 */
-		for (timer = jiffies; time_before_eq(jiffies, timer););
+		msleep(10);
+
 		if ((i % 100) == 0)
 			printk(".");
 		(void) sjcd_check_status();

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Jeff Garzik]

Thomas Gleixner wrote:
> From: Ingo Molnar <mingo@elte.hu>
> 
> The SJCD driver uses a jiffies busy loop. Replace it with msleep.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> 
> ---
>  drivers/cdrom/sjcd.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/cdrom/sjcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/cdrom/sjcd.c
> +++ linux-2.6/drivers/cdrom/sjcd.c
> @@ -69,6 +69,7 @@
>  #include <linux/string.h>
>  #include <linux/major.h>
>  #include <linux/init.h>
> +#include <linux/delay.h>
>  
>  #include <asm/system.h>
>  #include <asm/io.h>
> @@ -1709,12 +1710,11 @@ static int __init sjcd_init(void)
>  	printk(KERN_INFO "SJCD: Resetting: ");
>  	sjcd_send_cmd(SCMD_RESET);
>  	for (i = 1000; i > 0 && !sjcd_status_valid; --i) {
> -		unsigned long timer;
> -
>  		/*
>  		 * Wait 10ms approx.
>  		 */
> -		for (timer = jiffies; time_before_eq(jiffies, timer););
> +		msleep(10);
> +

I always worry when I see code like this, wondering if there is some 
arcane reason that I cannot fathom, that is being removed.  You gotta 
wonder how long it has been since this driver was used, by anybody.

Oh well, I cannot find fault with the patch, paranoia worries aside.

ACK.

	Jeff

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Jens Axboe]

On Tue, May 22 2007, Ingo Molnar wrote:
> 
> * Jeff Garzik <jeff@garzik.org> wrote:
> 
> > >@@ -1709,12 +1710,11 @@ static int __init sjcd_init(void)
> > > 	printk(KERN_INFO "SJCD: Resetting: ");
> > > 	sjcd_send_cmd(SCMD_RESET);
> > > 	for (i = 1000; i > 0 && !sjcd_status_valid; --i) {
> > >-		unsigned long timer;
> > >-
> > > 		/*
> > > 		 * Wait 10ms approx.
> > > 		 */
> > >-		for (timer = jiffies; time_before_eq(jiffies, timer););
> > >+		msleep(10);
> > >+
> > 
> > I always worry when I see code like this, wondering if there is some 
> > arcane reason that I cannot fathom, that is being removed.  You gotta 
> > wonder how long it has been since this driver was used, by anybody.
> > 
> > Oh well, I cannot find fault with the patch, paranoia worries aside.
> 
> well, while i dont have that hardware, i found this by booting an 
> allyesconfig bzImage which runs the code above, which locks up without 
> this change. And then it booted fine with this change. So arcane issues 
> aside, it does visibly improve things ;-)

But if you looked at the driver, you'd see 2 other identical busy loops
in the very same function :-)

-- 
Jens Axboe

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Ingo Molnar]

* Jeff Garzik <jeff@garzik.org> wrote:

> >@@ -1709,12 +1710,11 @@ static int __init sjcd_init(void)
> > 	printk(KERN_INFO "SJCD: Resetting: ");
> > 	sjcd_send_cmd(SCMD_RESET);
> > 	for (i = 1000; i > 0 && !sjcd_status_valid; --i) {
> >-		unsigned long timer;
> >-
> > 		/*
> > 		 * Wait 10ms approx.
> > 		 */
> >-		for (timer = jiffies; time_before_eq(jiffies, timer););
> >+		msleep(10);
> >+
> 
> I always worry when I see code like this, wondering if there is some 
> arcane reason that I cannot fathom, that is being removed.  You gotta 
> wonder how long it has been since this driver was used, by anybody.
> 
> Oh well, I cannot find fault with the patch, paranoia worries aside.

well, while i dont have that hardware, i found this by booting an 
allyesconfig bzImage which runs the code above, which locks up without 
this change. And then it booted fine with this change. So arcane issues 
aside, it does visibly improve things ;-)

	Ingo

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Rene Herman]

On 05/22/2007 12:25 PM, Thomas Gleixner wrote:

> From: Ingo Molnar <mingo@elte.hu>
> 
> The SJCD driver uses a jiffies busy loop. Replace it with msleep.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>

Okay, that's just waiting for a reset to complete, which seems perfectly 
fine with a sleeping loop like that, but...

I re-started working on a rewrite of the mitsumi legacy cdrom driver that 
Pekka Enberg and I have been doing this afternoon again and I ran into not 
being able to use sleeping loops there an hour ago!

The trouble there is that unless you poll the bloody thing like mad too much 
of the Q subchannels passes below you and you need a huge number of retries 
to get anything out of it. I noticed when I started adding audio bits that 
the driver took full seconds to complete some audio requests while the old 
driver was snappy in that regard. When I replaced our sleeping loop with a 
busy-wait same as the original the snappyness returned and moreover, reading 
the TOC from the CD went from something close to a minute to approximately a 
second. Thought that minute was just because I was dealing with an old junk 
1-speed drive...

Now, as said, this looks to be fine since it's just waiting for a reset to 
complete, but unless you have the hardware to actually test, be careful in 
there.

Or in fact, maybe even decide there's not much point. The current plan is to 
do mitsumi, sony and panasonic and then throw the rest away (that trio is 
special in so far that controllers for them are still available on a lot of 
old ISA soundcards). Or do you actually have the hardware?

Rene.

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Ingo Molnar]

* Rene Herman <rene.herman@gmail.com> wrote:

> The trouble there is that unless you poll the bloody thing like mad 
> too much of the Q subchannels passes below you and you need a huge 
> number of retries to get anything out of it. I noticed when I started 
> adding audio bits that the driver took full seconds to complete some 
> audio requests while the old driver was snappy in that regard. When I 
> replaced our sleeping loop with a busy-wait same as the original the 
> snappyness returned and moreover, reading the TOC from the CD went 
> from something close to a minute to approximately a second. Thought 
> that minute was just because I was dealing with an old junk 1-speed 
> drive...

ouch ...

then i guess the right approach is to not use jiffies but to use a 
calculated number of udelay(10) calls or so.

	Ingo

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Alan Cox]

> driver was snappy in that regard. When I replaced our sleeping loop with a 
> busy-wait same as the original the snappyness returned and moreover, reading 
> the TOC from the CD went from something close to a minute to approximately a 
> second. Thought that minute was just because I was dealing with an old junk 
> 1-speed drive...

It happens. If you want to be fairer you can check need_resched in the
loop so that you still share the CPU fairly with other users.

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Rene Herman]

On 05/23/2007 09:28 PM, Ingo Molnar wrote:

> * Rene Herman <rene.herman@gmail.com> wrote:
> 
>> The trouble there is that unless you poll the bloody thing like mad 
>> too much of the Q subchannels passes below you and you need a huge 
>> number of retries to get anything out of it. I noticed when I started 
>> adding audio bits that the driver took full seconds to complete some 
>> audio requests while the old driver was snappy in that regard. When I 
>> replaced our sleeping loop with a busy-wait same as the original the 
>> snappyness returned and moreover, reading the TOC from the CD went 
>> from something close to a minute to approximately a second. Thought 
>> that minute was just because I was dealing with an old junk 1-speed 
>> drive...
> 
> ouch ...

Indeed...

> then i guess the right approach is to not use jiffies but to use a 
> calculated number of udelay(10) calls or so.

I'm not sure that will do -- the disc is still spinning below us while we're 
delaying/sleeping. Alan just now suggested checking need_resched which 
sounds like okay advice. I'll be trying what still works...

(given that noone uses these drives for anything serious anymore I'm in fact 
also not overly worried about being a busy-looping pig and just saying "this 
hardware sucks, why are you using it?").

Rene.

Re: [PATCH] CDROM: replace jiffies busyloop with msleep

[Rene Herman]

On 05/23/2007 09:34 PM, Alan Cox wrote:

>> driver was snappy in that regard. When I replaced our sleeping loop with a 
>> busy-wait same as the original the snappyness returned and moreover, reading 
>> the TOC from the CD went from something close to a minute to approximately a 
>> second. Thought that minute was just because I was dealing with an old junk 
>> 1-speed drive...
> 
> It happens. If you want to be fairer you can check need_resched in the
> loop so that you still share the CPU fairly with other users.

Thanks for the tip -- I'll do this.

Rene.