cdrom changes in test13-pre2 slow down cdrom access by 70%

cdrom changes in test13-pre2 slow down cdrom access by 70%

[David Mansfield]

Jens, 

The cdrom changes that went into test13-pre2 really kill the performance
of my cdrom.  I'm using cdparanoia to read audio data, and it normally
reads at 2-3x.  Since test13-pre2 it's  down to .6 - .7x.  I've reverted
the following files to the ones from test13-pre1 and it's back to
normal:

drivers/cdrom/cdrom.c
drivers/ide/ide-cd.c
drivers/ide/ide-cd.h
drivers/scsi/sr.c
drivers/scsi/sr.h
drivers/scsi/sr-ioctl.c
drivers/scsi/sr-vendor.c
include/linux/cdrom.h

My hardware is:

Uniform Multi-Platform E-IDE driver Revision: 6.31
ide: Assuming 33MHz system bus speed for PIO modes; override with
idebus=xx
AMD7409: IDE controller on PCI bus 00 dev 39
AMD7409: chipset revision 3
AMD7409: not 100% native mode: will probe irqs later
AMD7409: disabling single-word DMA support (revision < C4)
    ide0: BM-DMA at 0xf000-0xf007, BIOS settings: hda:DMA, hdb:pio
    ide1: BM-DMA at 0xf008-0xf00f, BIOS settings: hdc:pio, hdd:pio
hda: CREATIVE CD5230E, ATAPI CDROM drive
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
hda: ATAPI 52X CD-ROM drive, 128kB Cache, DMA
Uniform CD-ROM driver Revision: 3.11

The only IDE device (as you can see) is the cdrom drive.

This is a huge patch, is there some way I could break it apart to see
what the relevant changes are?

David

-- 
David Mansfield                                           (718) 963-2020
david@ultramaster.com
Ultramaster Group, LLC                               www.ultramaster.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: cdrom changes in test13-pre2 slow down cdrom access by 70%

[Jens Axboe]

On Fri, Dec 22 2000, David Mansfield wrote:
> Jens, 
> 
> The cdrom changes that went into test13-pre2 really kill the performance
> of my cdrom.  I'm using cdparanoia to read audio data, and it normally
> reads at 2-3x.  Since test13-pre2 it's  down to .6 - .7x.  I've reverted
> the following files to the ones from test13-pre1 and it's back to
> normal:

Humm, interesting.

> This is a huge patch, is there some way I could break it apart to see
> what the relevant changes are?

The change affecting you is most likely the CDROMREADAUDIO change,
where we now just read a single cdda frame at the time. This gives
us less data per interrupt, and apparently this is more than a
theoretical slowdown for you. Please try with attached patch. If
this solves it (as it should), then we should probably try and do
persistent allocation of a bigger buffer for things like this.
Grabbing > 1 frame was disabled because multi-page allocation is not
reliable.

--- drivers/cdrom/cdrom.c~	Sat Dec 23 13:27:33 2000
+++ drivers/cdrom/cdrom.c	Sat Dec 23 13:30:39 2000
@@ -1985,7 +1985,7 @@
 		}
 	case CDROMREADAUDIO: {
 		struct cdrom_read_audio ra;
-		int lba;
+		int lba, frames;
 
 		IOCTL_IN(arg, struct cdrom_read_audio, ra);
 
@@ -2002,7 +2002,9 @@
 		if (lba < 0 || ra.nframes <= 0)
 			return -EINVAL;
 
-		if ((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW, GFP_KERNEL)) == NULL)
+		frames = ra.nframes > 8 ? 8 : ra.nframes;
+
+		if ((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW * frames, GFP_KERNEL)) == NULL)
 			return -ENOMEM;
 
 		if (!access_ok(VERIFY_WRITE, ra.buf, ra.nframes*CD_FRAMESIZE_RAW)) {
@@ -2011,12 +2013,14 @@
 		}
 		cgc.data_direction = CGC_DATA_READ;
 		while (ra.nframes > 0) {
-			ret = cdrom_read_block(cdi, &cgc, lba, 1, 1, CD_FRAMESIZE_RAW);
-			if (ret) break;
-			__copy_to_user(ra.buf, cgc.buffer, CD_FRAMESIZE_RAW);
-			ra.buf += CD_FRAMESIZE_RAW;
-			ra.nframes--;
-			lba++;
+			ret = cdrom_read_block(cdi, &cgc, lba, frames, 1, CD_FRAMESIZE_RAW);
+			if (ret)
+				break;
+			__copy_to_user(ra.buf, cgc.buffer,
+				       CD_FRAMESIZE_RAW * frames);
+			ra.buf += (CD_FRAMESIZE_RAW * frames);
+			ra.nframes -= frames;
+			lba += frames;
 		}
 		kfree(cgc.buffer);
 		return ret;

-- 
* Jens Axboe <axboe@suse.de>
* SuSE Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: cdrom changes in test13-pre2 slow down cdrom access by 70%

[David Mansfield]

Jens Axboe wrote:
> 
> On Fri, Dec 22 2000, David Mansfield wrote:
> > Jens,
> >
> > The cdrom changes that went into test13-pre2 really kill the performance
> > of my cdrom.  I'm using cdparanoia to read audio data, and it normally
> > reads at 2-3x.  Since test13-pre2 it's  down to .6 - .7x.  I've reverted
> > the following files to the ones from test13-pre1 and it's back to
> > normal:
> 
> Humm, interesting.
> 
> > This is a huge patch, is there some way I could break it apart to see
> > what the relevant changes are?
> 
> The change affecting you is most likely the CDROMREADAUDIO change,
> where we now just read a single cdda frame at the time. This gives
> us less data per interrupt, and apparently this is more than a
> theoretical slowdown for you. Please try with attached patch. If
> this solves it (as it should), then we should probably try and do
> persistent allocation of a bigger buffer for things like this.
> Grabbing > 1 frame was disabled because multi-page allocation is not
> reliable.
> 
> --- drivers/cdrom/cdrom.c~      Sat Dec 23 13:27:33 2000
> +++ drivers/cdrom/cdrom.c       Sat Dec 23 13:30:39 2000
> @@ -1985,7 +1985,7 @@
>                 }
>         case CDROMREADAUDIO: {
>                 struct cdrom_read_audio ra;
> -               int lba;
> +               int lba, frames;
> 
>                 IOCTL_IN(arg, struct cdrom_read_audio, ra);
> 
> @@ -2002,7 +2002,9 @@
>                 if (lba < 0 || ra.nframes <= 0)
>                         return -EINVAL;
> 
> -               if ((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW, GFP_KERNEL)) == NULL)
> +               frames = ra.nframes > 8 ? 8 : ra.nframes;
> +
> +               if ((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW * frames, GFP_KERNEL)) == NULL)
>                         return -ENOMEM;
> 
>                 if (!access_ok(VERIFY_WRITE, ra.buf, ra.nframes*CD_FRAMESIZE_RAW)) {
> @@ -2011,12 +2013,14 @@
>                 }
>                 cgc.data_direction = CGC_DATA_READ;
>                 while (ra.nframes > 0) {
> -                       ret = cdrom_read_block(cdi, &cgc, lba, 1, 1, CD_FRAMESIZE_RAW);
> -                       if (ret) break;
> -                       __copy_to_user(ra.buf, cgc.buffer, CD_FRAMESIZE_RAW);
> -                       ra.buf += CD_FRAMESIZE_RAW;
> -                       ra.nframes--;
> -                       lba++;
> +                       ret = cdrom_read_block(cdi, &cgc, lba, frames, 1, CD_FRAMESIZE_RAW);
> +                       if (ret)
> +                               break;
> +                       __copy_to_user(ra.buf, cgc.buffer,
> +                                      CD_FRAMESIZE_RAW * frames);
> +                       ra.buf += (CD_FRAMESIZE_RAW * frames);
> +                       ra.nframes -= frames;
> +                       lba += frames;
>                 }
>                 kfree(cgc.buffer);
>                 return ret;
> 

Yes.  test13-pre4 + the above patch is back to normal speed.  I had
spotted that too as a likely candidate, especially because when the
accesses were slow, the light on the cdrom was blinking at a much higher
rate than before (I suppose it was processing 8x the number of commands,
right?).

Anyway, do you think a 'try to allocate 8, if that fails, try to
allocate 1' solution would be a simple compromise?  That should be easy
to do, based on the above code (if kmalloc returns NULL && frames > 1,
frames = 1, retry...).

David

-- 
David Mansfield                                           (718) 963-2020
david@ultramaster.com
Ultramaster Group, LLC                               www.ultramaster.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: cdrom changes in test13-pre2 slow down cdrom access by 70%

[David Mansfield]

> > >
> > > The cdrom changes that went into test13-pre2 really kill the performance
> > > of my cdrom.  I'm using cdparanoia to read audio data, and it normally

... cut ...

> Anyway, do you think a 'try to allocate 8, if that fails, try to
> allocate 1' solution would be a simple compromise?  That should be easy
> to do, based on the above code (if kmalloc returns NULL && frames > 1,
> frames = 1, retry...).
> 

Jens, 

Here's a version of the above idea, ontop of the patch you sent.  It's
cut and pasted, but I don't think it's whitespace mangled...  I haven't
actually run with this patch, but it does compile :-)

--- drivers/cdrom//cdrom.c	2000/12/26 17:53:14	1.6.4.2
+++ drivers/cdrom//cdrom.c	2000/12/26 21:39:42
@@ -2004,8 +2004,15 @@
 
 		frames = ra.nframes > 8 ? 8 : ra.nframes;
 
-		if ((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW * frames,
GFP_KERNEL)) == NULL)
-			return -ENOMEM;
+	retry:
+		if ((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW * frames,
GFP_KERNEL)) == NULL) {
+			if (frames > 1) {
+				frames = 1;
+				goto retry;
+			} else {
+				return -ENOMEM;
+			}
+		}
 
 		if (!access_ok(VERIFY_WRITE, ra.buf, ra.nframes*CD_FRAMESIZE_RAW)) {
 			kfree(cgc.buffer);

David


-- 
David Mansfield                                           (718) 963-2020
david@ultramaster.com
Ultramaster Group, LLC                               www.ultramaster.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: cdrom changes in test13-pre2 slow down cdrom access by 70%

[Jens Axboe]

On Tue, Dec 26 2000, David Mansfield wrote:
> > > > The cdrom changes that went into test13-pre2 really kill the performance
> > > > of my cdrom.  I'm using cdparanoia to read audio data, and it normally
> 
> ... cut ...
> 
> > Anyway, do you think a 'try to allocate 8, if that fails, try to
> > allocate 1' solution would be a simple compromise?  That should be easy
> > to do, based on the above code (if kmalloc returns NULL && frames > 1,
> > frames = 1, retry...).
> > 
> 
> Jens, 
> 
> Here's a version of the above idea, ontop of the patch you sent.  It's
> cut and pasted, but I don't think it's whitespace mangled...  I haven't
> actually run with this patch, but it does compile :-)

In principle it looks ok, but after some time we are bound to fail 8
frame allocations anyway and this patch won't help. For the modular
case, preallocation of a bigger chunk at init time is no good either.
Builtin would be fine of course. This almost screams sg to me :-)

-- 
* Jens Axboe <axboe@suse.de>
* SuSE Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: cdrom changes in test13-pre2 slow down cdrom access by 70%

[David Mansfield]

Jens Axboe wrote:
> 
> In principle it looks ok, but after some time we are bound to fail 8
> frame allocations anyway and this patch won't help. For the modular
> case, preallocation of a bigger chunk at init time is no good either.
> Builtin would be fine of course. This almost screams sg to me :-)
> 

Nonetheless, with your first patch and my patch, the system starts off
using the old method of trying to allocate 8 frames buffer (which is
essential for performance) and falls back to the current (as of
test13-pre2) way in low/fragmented memory situations. To me, that's
better than either the previous or the current method, with the slight
increased cost of the failed kmalloc every time in the low/fragmented
memory case.  BTW, have you gotten reports of that kmalloc failing for
people?  I've been ripping audio with every kernel since pre4 and have
never had a failure.  Granted, I put 'workstation' loads on my machine,
but I run some benchmarks from time-to-time, put memory pressure on
etc.  (H*ll, just netscape alone is memory pressure enough :-).

I just don't want to have to patch every kernel I run from here to
eternity.  Call me selfish.

David

-- 
David Mansfield                                           (718) 963-2020
david@ultramaster.com
Ultramaster Group, LLC                               www.ultramaster.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: cdrom changes in test13-pre2 slow down cdrom access by 70%

[Jens Axboe]

On Wed, Dec 27 2000, David Mansfield wrote:
> > In principle it looks ok, but after some time we are bound to fail 8
> > frame allocations anyway and this patch won't help. For the modular
> > case, preallocation of a bigger chunk at init time is no good either.
> > Builtin would be fine of course. This almost screams sg to me :-)
> > 
> 
> Nonetheless, with your first patch and my patch, the system starts off
> using the old method of trying to allocate 8 frames buffer (which is
> essential for performance) and falls back to the current (as of
> test13-pre2) way in low/fragmented memory situations. To me, that's
> better than either the previous or the current method, with the slight
> increased cost of the failed kmalloc every time in the low/fragmented
> memory case. [...]

Yes I agree, it's better than what is there. All I was saying is that
it could be better :-). I've already put something close to your patch
in my tree, will be sent off the next time

> BTW, have you gotten reports of that kmalloc failing for people?

Of course, otherwise I wouldn't have changed it.

> I've been ripping audio with every kernel since pre4 and have
> never had a failure.  Granted, I put 'workstation' loads on my machine,
> but I run some benchmarks from time-to-time, put memory pressure on
> etc.  (H*ll, just netscape alone is memory pressure enough :-).

Depends on how much RAM you have, and what you are doing.

-- 
* Jens Axboe <axboe@suse.de>
* SuSE Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/