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

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

On Sat, Jan 02, 2016 at 12:29:19PM +0000, 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.

Looking at this closer, it would not be appropriately aligned to place
the alignment buffer after the adma table, but placing the alignment
buffer in the allocation first would give appropriate alignment.

The buffer sizes are:

Table		32-bit	64-bit
alignment	512	1024		(entry sz * 128)
adma		2056	3084		(desc sz * (128 * 2 + 1))

Allocating the two together gives advantages and disadvantages:

* for 32-bit address sizes, instead of allocating two order-0 pages,
  we end up allocating one order-0 page, so halve the coherent DMA
  allocation of the driver.

* for 64-bit address sizes, we allocate one order-1 page instead,
  which may be more prone to failure with a 4k page size, and it
  also means the ADMA table will overlap a page boundary.  I've no
  idea whether that is an issue for any SDHCI controllers or not.

I could add additional complexity to do different allocations in the
32-bit and 64-bit paths, but that results in a _far_ more complicated
cleanup path, and much more prone to errors.

In any case, such a patch should be entirely separate from _this_
patch given that such a change may cause breakage and could need
to be reworked as a result.  Indeed, it's a _separate_ change in
itself, for a different purpose from _this_ patch.

-- 
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.