On 21/12/15 13:41, Russell King wrote:
> Unnecessarily mapping and unmapping the align buffer for SD cards is
> expensive: performance measurements on iMX6 show that this gives a hit
> of 10% on hdparm buffered disk reads.
>
> MMC/SD card IO comes from the mm/vfs which gives us page based IO, so
> for this case, the align buffer is not going to be used. However, we
> still map and unmap this buffer.
>
> Eliminate this by switching the align buffer to be a DMA coherent
> buffer, which needs no DMA maintenance to access the buffer.
Did you consider putting the align buffer in the same allocation
as the adma_table?
>
> Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
> ---
> drivers/mmc/host/sdhci.c | 53 ++++++++++++++++--------------------------------
> 1 file changed, 18 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 3e718e465a1b..8a4eb1c41f3e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -465,8 +465,6 @@ static void sdhci_adma_mark_end(void *desc)
> static int sdhci_adma_table_pre(struct sdhci_host *host,
> struct mmc_data *data)
> {
> - int direction;
> -
> void *desc;
> void *align;
> dma_addr_t addr;
> @@ -483,20 +481,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
> * We currently guess that it is LE.
> */
>
> - if (data->flags & MMC_DATA_READ)
> - direction = DMA_FROM_DEVICE;
> - else
> - direction = DMA_TO_DEVICE;
> -
> - host->align_addr = dma_map_single(mmc_dev(host->mmc),
> - host->align_buffer, host->align_buffer_sz, direction);
> - if (dma_mapping_error(mmc_dev(host->mmc), host->align_addr))
> - goto fail;
> - BUG_ON(host->align_addr & host->align_mask);
> -
> host->sg_count = sdhci_pre_dma_transfer(host, data);
> if (host->sg_count < 0)
> - goto unmap_align;
> + return -EINVAL;
>
> desc = host->adma_table;
> align = host->align_buffer;
> @@ -567,22 +554,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
> /* nop, end, valid */
> sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
> }
> -
> - /*
> - * Resync align buffer as we might have changed it.
> - */
> - if (data->flags & MMC_DATA_WRITE) {
> - dma_sync_single_for_device(mmc_dev(host->mmc),
> - host->align_addr, host->align_buffer_sz, direction);
> - }
> -
> return 0;
> -
> -unmap_align:
> - dma_unmap_single(mmc_dev(host->mmc), host->align_addr,
> - host->align_buffer_sz, direction);
> -fail:
> - return -EINVAL;
> }
>
> static void sdhci_adma_table_post(struct sdhci_host *host,
> @@ -602,9 +574,6 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
> else
> direction = DMA_TO_DEVICE;
>
> - dma_unmap_single(mmc_dev(host->mmc), host->align_addr,
> - host->align_buffer_sz, direction);
> -
> /* Do a quick scan of the SG list for any unaligned mappings */
> has_unaligned = false;
> for_each_sg(data->sg, sg, host->sg_count, i)
> @@ -3003,6 +2972,10 @@ int sdhci_add_host(struct sdhci_host *host)
> host->adma_table_sz,
> &host->adma_addr,
> GFP_KERNEL);
> + host->align_buffer = dma_alloc_coherent(mmc_dev(mmc),
> + host->align_buffer_sz,
> + &host->align_addr,
> + GFP_KERNEL);
> host->align_buffer = kmalloc(host->align_buffer_sz, GFP_KERNEL);
kmalloc line is still there
> if (!host->adma_table || !host->align_buffer) {
> if (host->adma_table)
> @@ -3010,7 +2983,11 @@ int sdhci_add_host(struct sdhci_host *host)
> host->adma_table_sz,
> host->adma_table,
> host->adma_addr);
> - kfree(host->align_buffer);
> + if (host->align_buffer)
> + dma_free_coherent(mmc_dev(mmc),
> + host->align_buffer_sz,
> + host->align_buffer,
> + host->align_addr);
> pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
> mmc_hostname(mmc));
> host->flags &= ~SDHCI_USE_ADMA;
> @@ -3022,10 +2999,14 @@ int sdhci_add_host(struct sdhci_host *host)
> host->flags &= ~SDHCI_USE_ADMA;
> dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
> host->adma_table, host->adma_addr);
> - kfree(host->align_buffer);
> + dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
> + host->align_buffer, host->align_addr);
> host->adma_table = NULL;
> host->align_buffer = NULL;
> }
> +
> + /* dma_alloc_coherent returns page aligned and sized buffers */
> + BUG_ON(host->align_addr & host->align_mask);
It would be nicer not to have any BUG_ON()
> }
>
> /*
> @@ -3489,7 +3470,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> if (host->adma_table)
> dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
> host->adma_table, host->adma_addr);
> - kfree(host->align_buffer);
> + if (host->align_buffer)
> + dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
> + host->align_buffer, host->align_addr);
>
> host->adma_table = NULL;
> host->align_buffer = NULL;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
