Re: [PATCH 1/5] mmc: core: Delete bounce buffer Kconfig option

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

Hi,

[ I added Daniel, Marc & Steven to cc: ]

On Wednesday, May 10, 2017 10:24:14 AM Linus Walleij wrote:
> This option is activated by all multiplatform configs and what
> not so we almost always have it turned on, and the memory it
> saves is negligible, even more so moving forward. The actual
> bounce buffer only gets allocated only when used, the only
> thing the ifdefs are saving is a little bit of code.
> 
> It is highly improper to have this as a Kconfig option that
> get turned on by Kconfig, make this a pure runtime-thing and
> let the host decide whether we use bounce buffers. We add a
> new property "disable_bounce" to the host struct.
> 
> Notice that mmc_queue_calc_bouncesz() already disables the
> bounce buffers if host->max_segs != 1, so any arch that has a
> maximum number of segments higher than 1 will have bounce
> buffers disabled.
> 
> The option CONFIG_MMC_BLOCK_BOUNCE is default y so the
> majority of platforms in the kernel already have it on, and
> it then gets turned off at runtime since most of these have
> a host->max_segs > 1. The few exceptions that have
> host->max_segs == 1 and still turn off the bounce buffering
> are those that disable it in their defconfig.
> 
> Those are the following:
> 
> arch/arm/configs/colibri_pxa300_defconfig
> arch/arm/configs/zeus_defconfig
> - Uses MMC_PXA, drivers/mmc/host/pxamci.c
> - Sets host->max_segs = NR_SG, which is 1
> - This needs its bounce buffer deactivated so we set
>   host->disable_bounce to true in the host driver

[...]

> arch/mips/configs/cavium_octeon_defconfig
> - Uses MMC_CAVIUM_OCTEON, drivers/mmc/host/cavium.c
> - Sets host->max_segs to 16 or 1
> - Setting host->disable_bounce to be sure for the 1 case

>From looking at the code it seems that bounce buffering should
be always beneficial to MMC performance when host->max_segs == 1.

It would be useful to know why these specific defconfigs
disable bounce buffering (to save memory?). Maybe the defaults
should be changed nowadays?

[...]

>  drivers/mmc/core/Kconfig  | 18 ------------------
>  drivers/mmc/core/queue.c  | 15 +--------------
>  drivers/mmc/host/cavium.c |  3 +++
>  drivers/mmc/host/pxamci.c |  6 ++++++
>  include/linux/mmc/host.h  |  1 +
>  5 files changed, 11 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
> index fc1ecdaaa9ca..42e89060cd41 100644
> --- a/drivers/mmc/core/Kconfig
> +++ b/drivers/mmc/core/Kconfig
> @@ -61,24 +61,6 @@ config MMC_BLOCK_MINORS
>  
>  	  If unsure, say 8 here.
>  
> -config MMC_BLOCK_BOUNCE
> -	bool "Use bounce buffer for simple hosts"
> -	depends on MMC_BLOCK
> -	default y
> -	help
> -	  SD/MMC is a high latency protocol where it is crucial to
> -	  send large requests in order to get high performance. Many
> -	  controllers, however, are restricted to continuous memory
> -	  (i.e. they can't do scatter-gather), something the kernel
> -	  rarely can provide.
> -
> -	  Say Y here to help these restricted hosts by bouncing
> -	  requests back and forth from a large buffer. You will get
> -	  a big performance gain at the cost of up to 64 KiB of
> -	  physical memory.
> -
> -	  If unsure, say Y here.
> -
>  config SDIO_UART
>  	tristate "SDIO UART/GPS class support"
>  	depends on TTY
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index 5c37b6be3e7b..545466342fb1 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -219,7 +219,6 @@ static struct mmc_queue_req *mmc_queue_alloc_mqrqs(int qdepth)
>  	return mqrq;
>  }
>  
> -#ifdef CONFIG_MMC_BLOCK_BOUNCE
>  static int mmc_queue_alloc_bounce_bufs(struct mmc_queue_req *mqrq, int qdepth,
>  				       unsigned int bouncesz)
>  {
> @@ -258,7 +257,7 @@ static unsigned int mmc_queue_calc_bouncesz(struct mmc_host *host)
>  {
>  	unsigned int bouncesz = MMC_QUEUE_BOUNCESZ;
>  
> -	if (host->max_segs != 1)
> +	if (host->max_segs != 1 || host->disable_bounce)
>  		return 0;
>  
>  	if (bouncesz > host->max_req_size)
> @@ -273,18 +272,6 @@ static unsigned int mmc_queue_calc_bouncesz(struct mmc_host *host)
>  
>  	return bouncesz;
>  }
> -#else
> -static inline bool mmc_queue_alloc_bounce(struct mmc_queue_req *mqrq,
> -					  int qdepth, unsigned int bouncesz)
> -{
> -	return false;
> -}
> -
> -static unsigned int mmc_queue_calc_bouncesz(struct mmc_host *host)
> -{
> -	return 0;
> -}
> -#endif
>  
>  static int mmc_queue_alloc_sgs(struct mmc_queue_req *mqrq, int qdepth,
>  			       int max_segs)
> diff --git a/drivers/mmc/host/cavium.c b/drivers/mmc/host/cavium.c
> index 58b51ba6aabd..66066f73e477 100644
> --- a/drivers/mmc/host/cavium.c
> +++ b/drivers/mmc/host/cavium.c
> @@ -1050,6 +1050,9 @@ int cvm_mmc_of_slot_probe(struct device *dev, struct cvm_mmc_host *host)
>  	else
>  		mmc->max_segs = 1;
>  
> +	/* Disable bounce buffers for max_segs = 1 */
> +	mmc->disable_bounce = true;
> +
>  	/* DMA size field can address up to 8 MB */
>  	mmc->max_seg_size = 8 * 1024 * 1024;
>  	mmc->max_req_size = mmc->max_seg_size;
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index c763b404510f..d3b5e6376504 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -666,6 +666,12 @@ static int pxamci_probe(struct platform_device *pdev)
>  	mmc->max_segs = NR_SG;
>  
>  	/*
> +	 * This architecture used to disable bounce buffers through its
> +	 * defconfig, now it is done at runtime as a host property.
> +	 */
> +	mmc->disable_bounce = true;
> +
> +	/*
>  	 * Our hardware DMA can handle a maximum of one page per SG entry.
>  	 */
>  	mmc->max_seg_size = PAGE_SIZE;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 21385ac0c9b1..b53c0e18a33b 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -310,6 +310,7 @@ struct mmc_host {
>  	/* host specific block data */
>  	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
>  	unsigned short		max_segs;	/* see blk_queue_max_segments */
> +	bool			disable_bounce; /* disable bounce buffers */
>  	unsigned short		unused;
>  	unsigned int		max_req_size;	/* maximum number of bytes in one req */
>  	unsigned int		max_blk_size;	/* maximum size of one mmc block */

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics