Re: [rfc/patch] libata -- port configurable delays
From: Jeff Garzik <hidden>
Date: 2005-05-13 19:44:52
Also in:
linux-ide
Benjamin LaHaise wrote:
Hello Jeff et al, The patch below makes the delays in ata_pause() and ata_busy_wait() configurable on a per-port basis, and enables the no delay flag on the one chipset I've tested on. Getting rid of the delays is worth quite a bit: doing sequential 512 byte O_DIRECT AIO reads results in a drop from 35.743s to 29.205s using simple-aio-min_nr 20480 10 (a copy is available at http://www.kvack.org/~bcrl/simple-aio-min_nr.c). Before this patch __delay() is the number one entry in oprofile results for this workload. Does this look like a reasonable approach for chipsets that aren't completely braindead? Cheers,
Well, there are several things going on here.
quoted hunk ↗ jump to hunk
@@ -469,7 +470,8 @@ static inline u8 ata_chk_status(struct a static inline void ata_pause(struct ata_port *ap) { ata_altstatus(ap); - ndelay(400); + if (!(ap->flags & ATA_FLAG_NO_UDELAY)) + ndelay(400);
This delay is required per spec. So, this specific change is vetoed.
quoted hunk ↗ jump to hunk
@@ -478,7 +480,8 @@ static inline u8 ata_busy_wait(struct at u8 status; do { - udelay(10); + if (!(ap->flags & ATA_FLAG_NO_UDELAY)) + udelay(10); status = ata_chk_status(ap); max--; } while ((status & bits) && (max > 0));
This delay is based on field experience, rather than spec. I'm open to making this optional, as you have done. Some issues related to this delay, to consider: 1) Nothing in life is free. This delay is useful on some platforms, because banging away at the Status register for extended periods of time can cause an insane amount of PCI IO traffic. Removing the delay just moves the punishment from one area to another. 2) In a few controllers, the SATA<->FIS emulation can go kerflooey if you bang the Status register 'too hard'. 3) IIRC some rare PATA devices don't like having their Status register banged "too hard". No data, just a vague memory. 4) It may be worthwhile to rewrite the loop to check the Status register _first_, then delay. Finally, simply disabling the delay is IMO _far_ too dangerous on such a popular driver (ata_piix). I would be conservative, and create a module option for libata (not ata_piix) which allows a user to globally disable the delay. And make sure that option defaults to 'delay', the current behavior. Creative suggestions welcomed... Jeff