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

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:

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

Quoted 7 lines · 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:

Humm, interesting.

Quoted 2 lines

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.

cdrom/cdrom.c Sat Dec 23 13:30:39 2000 renamed from cdrom/cdrom.c~ Sat Dec 23 13:27:33 2000 +5 -8

@@ -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);

Quoted 3 lines

Quoted 3 lines

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

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 5 lines

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

Quoted 5 lines

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