Message ID | 20100217130847.16338.55586.stgit@localhost.localdomain |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
On 02/17/10 08:10, Alan Cox wrote: > Avoid the device select overhead on every qc_issue (> 10uS) by caching the > currently selected device. This shows up on profiles under load. Best case > this costs us 10uS for the delay, worst case with a dumb interface it's > costing us about *1mS* a command. > > I believe the logic here is sufficient, but would welcome some second reviews > as its not something you want to get wrong ! > > Signed-off-by: Alan Cox<alan@linux.intel.com> .. I totally agree with this patch, but question the timings used to justify it. Surely the overhead is only 1-2usec for the case where the device is the one that was already selected (on a "smart" interface) ? And for the case where the currently selected device is different than the desired device (the 1msec case), this patch makes little/no difference? Cheers > --- > > drivers/ata/libata-sff.c | 8 ++++++-- > include/linux/libata.h | 1 + > 2 files changed, 7 insertions(+), 2 deletions(-) > > > diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c > index 63d9c6a..cf0332a 100644 > --- a/drivers/ata/libata-sff.c > +++ b/drivers/ata/libata-sff.c > @@ -469,6 +469,7 @@ void ata_sff_dev_select(struct ata_port *ap, unsigned int device) > > iowrite8(tmp, ap->ioaddr.device_addr); > ata_sff_pause(ap); /* needed; also flushes, for mmio */ > + ap->sff_selected = device; > } > EXPORT_SYMBOL_GPL(ata_sff_dev_select); > > @@ -1538,7 +1539,8 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc) > } > > /* select the device */ > - ata_dev_select(ap, qc->dev->devno, 1, 0); > + if (qc->dev->devno != ap->sff_selected) > + ata_dev_select(ap, qc->dev->devno, 1, 0); > > /* start the command */ > switch (qc->tf.protocol) { > @@ -1925,6 +1927,8 @@ int ata_sff_prereset(struct ata_link *link, unsigned long deadline) > struct ata_eh_context *ehc =&link->eh_context; > int rc; > > + link->ap->sff_selected = -1; /* Unknown */ > + > rc = ata_std_prereset(link, deadline); > if (rc) > return rc; > @@ -2687,7 +2691,7 @@ void ata_bus_reset(struct ata_port *ap) > iowrite8(ap->ctl, ioaddr->ctl_addr); > ap->last_ctl = ap->ctl; > } > - > + ap->sff_selected = -1; > DPRINTK("EXIT\n"); > return; > > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 5fb8884..a85adc2 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -725,6 +725,7 @@ struct ata_port { > > #ifdef CONFIG_ATA_SFF > struct ata_ioports ioaddr; /* ATA cmd/ctl/dma register blocks */ > + int sff_selected; /* Cache of selected device */ > #endif /* CONFIG_ATA_SFF */ > > u8 ctl; /* cache of ATA control register */ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
O> I totally agree with this patch, but question the timings used to justify it. > Surely the overhead is only 1-2usec for the case where the device > is the one that was already selected (on a "smart" interface) ? IFF you have a smart interface. A lot of the controllers in the PCI space don't appear to be that clever. > And for the case where the currently selected device is different > than the desired device (the 1msec case), this patch makes little/no difference? Correct, but even with two devices per cable (which is not the most common setup) you win. Worst case (which I've never seen) you draw. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/17/2010 08:10 AM, Alan Cox wrote: > Avoid the device select overhead on every qc_issue (> 10uS) by caching the > currently selected device. This shows up on profiles under load. Best case > this costs us 10uS for the delay, worst case with a dumb interface it's > costing us about *1mS* a command. > > I believe the logic here is sufficient, but would welcome some second reviews > as its not something you want to get wrong ! > > > Signed-off-by: Alan Cox<alan@linux.intel.com> > --- > > drivers/ata/libata-sff.c | 8 ++++++-- > include/linux/libata.h | 1 + > 2 files changed, 7 insertions(+), 2 deletions(-) > > > diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c > index 63d9c6a..cf0332a 100644 > --- a/drivers/ata/libata-sff.c > +++ b/drivers/ata/libata-sff.c > @@ -469,6 +469,7 @@ void ata_sff_dev_select(struct ata_port *ap, unsigned int device) > > iowrite8(tmp, ap->ioaddr.device_addr); > ata_sff_pause(ap); /* needed; also flushes, for mmio */ > + ap->sff_selected = device; > } > EXPORT_SYMBOL_GPL(ata_sff_dev_select); > > @@ -1538,7 +1539,8 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc) > } > > /* select the device */ > - ata_dev_select(ap, qc->dev->devno, 1, 0); > + if (qc->dev->devno != ap->sff_selected) > + ata_dev_select(ap, qc->dev->devno, 1, 0); > > /* start the command */ > switch (qc->tf.protocol) { My main worry here is that this logic excises the 150ms wait in ata_dev_select() that has been used effectively to allow ATAPI devices to "collect themselves" after waiting for idle, prior to command issuance. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > - ata_dev_select(ap, qc->dev->devno, 1, 0); > > + if (qc->dev->devno != ap->sff_selected) > > + ata_dev_select(ap, qc->dev->devno, 1, 0); > > > > /* start the command */ > > switch (qc->tf.protocol) { > > My main worry here is that this logic excises the 150ms wait in > ata_dev_select() that has been used effectively to allow ATAPI devices > to "collect themselves" after waiting for idle, prior to command issuance. It doesn't. You call it with wait = 1, can_sleep = 0 so it will never do the 150ms magic delay here anyway (good job or it would kill us for performance ;)) It does mean we don't do the device idle wait in that situation but there are no code paths where we try to overlap commands by spinning on the drive busy bit (again for obvious reasons) Alan -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index 63d9c6a..cf0332a 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -469,6 +469,7 @@ void ata_sff_dev_select(struct ata_port *ap, unsigned int device) iowrite8(tmp, ap->ioaddr.device_addr); ata_sff_pause(ap); /* needed; also flushes, for mmio */ + ap->sff_selected = device; } EXPORT_SYMBOL_GPL(ata_sff_dev_select); @@ -1538,7 +1539,8 @@ unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc) } /* select the device */ - ata_dev_select(ap, qc->dev->devno, 1, 0); + if (qc->dev->devno != ap->sff_selected) + ata_dev_select(ap, qc->dev->devno, 1, 0); /* start the command */ switch (qc->tf.protocol) { @@ -1925,6 +1927,8 @@ int ata_sff_prereset(struct ata_link *link, unsigned long deadline) struct ata_eh_context *ehc = &link->eh_context; int rc; + link->ap->sff_selected = -1; /* Unknown */ + rc = ata_std_prereset(link, deadline); if (rc) return rc; @@ -2687,7 +2691,7 @@ void ata_bus_reset(struct ata_port *ap) iowrite8(ap->ctl, ioaddr->ctl_addr); ap->last_ctl = ap->ctl; } - + ap->sff_selected = -1; DPRINTK("EXIT\n"); return; diff --git a/include/linux/libata.h b/include/linux/libata.h index 5fb8884..a85adc2 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -725,6 +725,7 @@ struct ata_port { #ifdef CONFIG_ATA_SFF struct ata_ioports ioaddr; /* ATA cmd/ctl/dma register blocks */ + int sff_selected; /* Cache of selected device */ #endif /* CONFIG_ATA_SFF */ u8 ctl; /* cache of ATA control register */
Avoid the device select overhead on every qc_issue (> 10uS) by caching the currently selected device. This shows up on profiles under load. Best case this costs us 10uS for the delay, worst case with a dumb interface it's costing us about *1mS* a command. I believe the logic here is sufficient, but would welcome some second reviews as its not something you want to get wrong ! Signed-off-by: Alan Cox <alan@linux.intel.com> --- drivers/ata/libata-sff.c | 8 ++++++-- include/linux/libata.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html