[PATCH] AHCI: Remove an unnecessary flush from ahci_qc_issue

[PATCH] AHCI: Remove an unnecessary flush from ahci_qc_issue

[Matthew Wilcox]

In an I/O heavy workload (IOZone), ahci_qc_issue is the second-highest
consumer of CPU cycles.  Removing the flush gets us approximately 10%
bandwidth improvement.  I believe this to be because the CPU can start
queueing the next request instead of waiting for the readl() to flush the
writes to the device.  The flush isn't necessary because we're using a
'queue' metaphor; we don't guarantee the command has got to the device,
nor do we need to guarantee the command has got to the controller.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/ata/ahci.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 6a7a70a..58915bd 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1846,7 +1846,6 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 	if (qc->tf.protocol == ATA_PROT_NCQ)
 		writel(1 << qc->tag, port_mmio + PORT_SCR_ACT);
 	writel(1 << qc->tag, port_mmio + PORT_CMD_ISSUE);
-	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
 
 	return 0;
 }
-- 
1.5.5.4

Re: [PATCH] AHCI: Remove an unnecessary flush from ahci_qc_issue

[Jeff Garzik]

Matthew Wilcox wrote:
> In an I/O heavy workload (IOZone), ahci_qc_issue is the second-highest
> consumer of CPU cycles.  Removing the flush gets us approximately 10%
> bandwidth improvement.  I believe this to be because the CPU can start
> queueing the next request instead of waiting for the readl() to flush the
> writes to the device.  The flush isn't necessary because we're using a
> 'queue' metaphor; we don't guarantee the command has got to the device,
> nor do we need to guarantee the command has got to the controller.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/ata/ahci.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 6a7a70a..58915bd 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1846,7 +1846,6 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
>  	if (qc->tf.protocol == ATA_PROT_NCQ)
>  		writel(1 << qc->tag, port_mmio + PORT_SCR_ACT);
>  	writel(1 << qc->tag, port_mmio + PORT_CMD_ISSUE);
> -	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
>  

(LKML CC added for wider review)

As I noted in IRC, I've queued this and am planning to apply this, as 
I've been thinking along the same lines for quite a while now...  not 
just in this driver but other drivers too.

A couple places in libata arguably need additional flushing, but some 
places could actually stand to use /less/

Re: [PATCH] AHCI: Remove an unnecessary flush from ahci_qc_issue

[Jeff Garzik]

Matthew Wilcox wrote:
> In an I/O heavy workload (IOZone), ahci_qc_issue is the second-highest
> consumer of CPU cycles.  Removing the flush gets us approximately 10%
> bandwidth improvement.  I believe this to be because the CPU can start
> queueing the next request instead of waiting for the readl() to flush the
> writes to the device.  The flush isn't necessary because we're using a
> 'queue' metaphor; we don't guarantee the command has got to the device,
> nor do we need to guarantee the command has got to the controller.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/ata/ahci.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 6a7a70a..58915bd 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1846,7 +1846,6 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
>  	if (qc->tf.protocol == ATA_PROT_NCQ)
>  		writel(1 << qc->tag, port_mmio + PORT_SCR_ACT);
>  	writel(1 << qc->tag, port_mmio + PORT_CMD_ISSUE);
> -	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
>  

applied