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
[email protected]
Ultramaster Group, LLC http://www.ultramaster.com
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 <[email protected]>
* SuSE Labs
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
[email protected]
Ultramaster Group, LLC http://www.ultramaster.com
> > >
> > > 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
[email protected]
Ultramaster Group, LLC http://www.ultramaster.com
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 <[email protected]>
* SuSE Labs
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
[email protected]
Ultramaster Group, LLC http://www.ultramaster.com
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 <[email protected]>
* SuSE Labs