Re: [PATCH v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Mon, Jan 04, 2016 at 01:50:32PM +0200, Adrian Hunter wrote:
> On 02/01/16 14:29, Russell King - ARM Linux wrote:
> > On Tue, Dec 29, 2015 at 03:44:38PM +0200, Adrian Hunter wrote:
> >> 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?
> > 
> > It's not clear whether host->adma_table_sz would be appropriately
> > aligned.
> > 
> >>> @@ -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
> > 
> > Good catch, thanks.
> > 
> >>> +
> >>> +		/* 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()
> > 
> > This is a situation that should _never_ occur (if it does, then the
> > dma_alloc_coherent() implementation is violating the API requirements,
> > which are to return a page-sized page-aligned buffer.)  I guess it
> > could be a WARN_ON(), but if it fails we're likely to cause data
> > corruption.  So, a WARN_ON() and failing the probe seems more
> > appropriate - but then that means yet more messy cleanup in an already
> > messy part of the driver.
> 
> Isn't there already a cleanup path? i.e.
> 
> 		} else if (host->adma_addr & host->align_mask) {
> 			pr_warn("%s: unable to allocate aligned ADMA descriptor\n",
> 				mmc_hostname(mmc));
> 			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);
> 			host->adma_table = NULL;
> 			host->align_buffer = NULL;
> 		}

It means duplicating this, because the resulting warning would not be
suitable for the alignment buffer.  I'm no fan of misleading printk()s
or duplicating cleanup, especially when the situation should never
occur.

In any case, with the two allocations combined, the BUG_ON() gets
removed again.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.