| Message ID | cc8694f6-4955-51d3-6342-107cfbb50ed2@cavium.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 19, 2017 at 9:30 AM, Steven J. Hill <Steven.Hill@cavium.com> wrote: > Remove MMC bounce buffer config option and associated code. This > is proposed in addition to Linus' changes to remove the config > option. I have tested this on our Octeon hardware platforms. > > Signed-off-by: Steven J. Hill <Steven.Hill@cavium.com> This would have to be rebased as Ulf merged my patch making this a per-host runtime config option. (The Kconfig is gone for example.) Bounce buffers were added by Pierre Ossman for kernel 2.6.23 in commit 98ccf14909ba02a41c5925b0b2c92aeeef23d3b9 "mmc: bounce requests for simple hosts" Quote: Some hosts cannot do scatter/gather in hardware. Since not doing sg is such a big performance hit, we (optionally) bounce the requests to a simple linear buffer that we hand over to the driver. Signed-off-by: Pierre Ossman <drzeus@drzeus.cx> So this runs the risk on reducing performance on simple MMC/SD controllers. Notice: simple, not old. We need to know if people are deploying simple controllers still and if this is something that really affects their performance. That said: this was put in place because the kernel was sending SG lists that the host DMA could not manage. Nowadays we have two mechanisms: - DMA engine and DMA-API that help out in managing bounce buffers when used. This means this only is useful for hardware that does autonomous DMA, without any separate DMA engine. - CMA that can actually allocate a big chunk of memory: I think this original code is restricted to a 64KB segment because kmalloc() will only guarantee contigous physical memory up to 64-128KiB or so. Now we could actually allocate a big badass CMA buffer if that improves the performance, and that would be a per-host setting. It would be good to hear from people seeing benefits from bounce buffers about this. What hardware is there that acually sees a significant improvement with bounce buffers? Pierre, what host were you developing this for? Maybe I can try to get the same and test it. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 11:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, May 19, 2017 at 9:30 AM, Steven J. Hill <Steven.Hill@cavium.com> wrote: > >> Remove MMC bounce buffer config option and associated code. This >> is proposed in addition to Linus' changes to remove the config >> option. I have tested this on our Octeon hardware platforms. >> >> Signed-off-by: Steven J. Hill <Steven.Hill@cavium.com> > > This would have to be rebased as Ulf merged my patch making this > a per-host runtime config option. (The Kconfig is gone for example.) > > Bounce buffers were added by Pierre Ossman for kernel 2.6.23 in > commit 98ccf14909ba02a41c5925b0b2c92aeeef23d3b9 > "mmc: bounce requests for simple hosts" > > Quote: > > Some hosts cannot do scatter/gather in hardware. Since not doing sg > is such a big performance hit, we (optionally) bounce the requests > to a simple linear buffer that we hand over to the driver. > > Signed-off-by: Pierre Ossman <drzeus@drzeus.cx> > > So this runs the risk on reducing performance on simple MMC/SD > controllers. Notice: simple, not old. Right, we definitely need to have something that allows us to send larger commands to the device for scattered requests. It wouldn't hurt to reconsider whether the current method is the most efficient way of doing this, but that doesn't seem to be a priority to me. If the MMC bounce buffers get in the way of the blk-mq conversion, we could try to come up with a different implementation as part of that conversion. > We need to know if people are deploying simple controllers still > and if this is something that really affects their performance. > > That said: this was put in place because the kernel was sending > SG lists that the host DMA could not manage. > > Nowadays we have two mechanisms: > > - DMA engine and DMA-API that help out in managing bounce > buffers when used. This means this only is useful for hardware > that does autonomous DMA, without any separate DMA engine. I think the bounce buffers in the DMA-API (swiotlb) should be kept separate since it has a completely different purpose of bouncing pages within the dma mask, rather than linearizing a sglist. If I read this right, we used to use blk-bounce for highmem pages and mmc-bounce for lowmem until 2ff1fa679115 ("mmc_block: bounce buffer highmem support") in 2008, now we don't use blk-bounce at all if mmc-bounce is in use. > - CMA that can actually allocate a big chunk of memory: I think > this original code is restricted to a 64KB segment because > kmalloc() will only guarantee contigous physical memory up to > 64-128KiB or so. Now we could actually allocate a big badass > CMA buffer if that improves the performance, and that would be > a per-host setting. > > It would be good to hear from people seeing benefits from bounce > buffers about this. What hardware is there that acually sees a > significant improvement with bounce buffers? The mmc-bounce code is active on hosts that have 'max_segs=1' (the default unless they override it). These one set it to '1' explicitly: drivers/mmc/host/au1xmmc.c: mmc->max_segs = AU1XMMC_DESCRIPTOR_COUNT; drivers/mmc/host/bfin_sdh.c: mmc->max_segs = 1; /* only BF51x */ drivers/mmc/host/sdhci.c: mmc->max_segs = 1; /* with SDHCI_USE_SDMA */ drivers/mmc/host/ushc.c: mmc->max_segs = 1; drivers/mmc/host/via-sdmmc.c: mmc->max_segs = 1; These have max_segs=1 but ask for bounce buffers to be disabled, which will severely limit their performance: drivers/mmc/host/cavium.c: mmc->max_segs = 1; /* cavium,octeon-6130-mmc */ drivers/mmc/host/pxamci.c: mmc->max_segs = NR_SG; These ones don't seem to set it at all, defaulting to '1': drivers/mmc/host/cb710-mmc.c drivers/mmc/host/moxart-mmc.c drivers/mmc/host/sdricoh_cs.c drivers/mmc/host/toshsd.c Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/05/17 11:05, Linus Walleij wrote: > > Pierre, what host were you developing this for? Maybe I can try > to get the same and test it. > I think the work primarily was done with the Texas Instruments SDHCI host that HP used in their laptops. Ricoh controllers were also popular, but they had some proprietary mode that you had to get them out of. If I recall correctly, all early SDHCI controllers needed this feature. I think scatter/gather was a later addition to the spec. I take it updates to the SD specification hasn't fixed this performance issue? Regards
diff --git a/arch/arm/configs/colibri_pxa300_defconfig b/arch/arm/configs/colibri_pxa300_defconfig index be02fe2..10c940c 100644 --- a/arch/arm/configs/colibri_pxa300_defconfig +++ b/arch/arm/configs/colibri_pxa300_defconfig @@ -51,7 +51,6 @@ CONFIG_USB_ANNOUNCE_NEW_DEVICES=y CONFIG_USB_MON=y CONFIG_USB_STORAGE=y CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_PXA=y CONFIG_EXT3_FS=y CONFIG_INOTIFY=y diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig index 67db829..8a8f086 100644 --- a/arch/arm/configs/davinci_all_defconfig +++ b/arch/arm/configs/davinci_all_defconfig @@ -188,7 +188,6 @@ CONFIG_USB_G_SERIAL=m CONFIG_USB_G_PRINTER=m CONFIG_USB_CDC_COMPOSITE=m CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_DAVINCI=y CONFIG_NEW_LEDS=y CONFIG_LEDS_CLASS=m diff --git a/arch/arm/configs/lpc32xx_defconfig b/arch/arm/configs/lpc32xx_defconfig index 6ba430d..655edfd 100644 --- a/arch/arm/configs/lpc32xx_defconfig +++ b/arch/arm/configs/lpc32xx_defconfig @@ -146,7 +146,6 @@ CONFIG_USB_LPC32XX=y CONFIG_USB_MASS_STORAGE=m CONFIG_USB_G_SERIAL=m CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_ARMMMCI=y CONFIG_MMC_SPI=y CONFIG_NEW_LEDS=y diff --git a/arch/arm/configs/nhk8815_defconfig b/arch/arm/configs/nhk8815_defconfig index 7d2ad30..e113d4a 100644 --- a/arch/arm/configs/nhk8815_defconfig +++ b/arch/arm/configs/nhk8815_defconfig @@ -95,7 +95,6 @@ CONFIG_GPIO_STMPE=y CONFIG_MFD_STMPE=y CONFIG_REGULATOR=y CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_ARMMMCI=y CONFIG_NEW_LEDS=y CONFIG_LEDS_CLASS=y diff --git a/arch/arm/configs/sama5_defconfig b/arch/arm/configs/sama5_defconfig index 777c9e9..c330071 100644 --- a/arch/arm/configs/sama5_defconfig +++ b/arch/arm/configs/sama5_defconfig @@ -180,7 +180,6 @@ CONFIG_USB_GADGET=y CONFIG_USB_ATMEL_USBA=y CONFIG_USB_G_SERIAL=y CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_SDHCI=y CONFIG_MMC_SDHCI_PLTFM=y CONFIG_MMC_SDHCI_OF_AT91=y diff --git a/arch/arm/configs/u300_defconfig b/arch/arm/configs/u300_defconfig index aaa95ab..d6eb9fe 100644 --- a/arch/arm/configs/u300_defconfig +++ b/arch/arm/configs/u300_defconfig @@ -50,7 +50,6 @@ CONFIG_BACKLIGHT_CLASS_DEVICE=y # CONFIG_USB_SUPPORT is not set CONFIG_MMC=y CONFIG_MMC_UNSAFE_RESUME=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_ARMMMCI=y CONFIG_RTC_CLASS=y # CONFIG_RTC_HCTOSYS is not set diff --git a/arch/arm/configs/zeus_defconfig b/arch/arm/configs/zeus_defconfig index cd11da8..a126f7b 100644 --- a/arch/arm/configs/zeus_defconfig +++ b/arch/arm/configs/zeus_defconfig @@ -146,7 +146,6 @@ CONFIG_USB_MASS_STORAGE=m CONFIG_USB_G_SERIAL=m CONFIG_USB_G_PRINTER=m CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_PXA=y CONFIG_NEW_LEDS=y CONFIG_LEDS_CLASS=m diff --git a/arch/blackfin/configs/CM-BF533_defconfig b/arch/blackfin/configs/CM-BF533_defconfig index 9a5716d..7a40109 100644 --- a/arch/blackfin/configs/CM-BF533_defconfig +++ b/arch/blackfin/configs/CM-BF533_defconfig @@ -59,7 +59,6 @@ CONFIG_SPI_BFIN5XX=y # CONFIG_HWMON is not set # CONFIG_USB_SUPPORT is not set CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_SPI=m # CONFIG_DNOTIFY is not set CONFIG_VFAT_FS=y diff --git a/arch/blackfin/configs/CM-BF537E_defconfig b/arch/blackfin/configs/CM-BF537E_defconfig index 6845928..b47ced8 100644 --- a/arch/blackfin/configs/CM-BF537E_defconfig +++ b/arch/blackfin/configs/CM-BF537E_defconfig @@ -83,7 +83,6 @@ CONFIG_GPIO_SYSFS=y CONFIG_USB_GADGET=m CONFIG_USB_ETH=m CONFIG_MMC=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_SPI=m CONFIG_EXT2_FS=y CONFIG_EXT2_FS_XATTR=y diff --git a/arch/mips/configs/cavium_octeon_defconfig b/arch/mips/configs/cavium_octeon_defconfig index d4fda41..7f337cf 100644 --- a/arch/mips/configs/cavium_octeon_defconfig +++ b/arch/mips/configs/cavium_octeon_defconfig @@ -130,7 +130,6 @@ CONFIG_USB_OHCI_HCD_PLATFORM=m CONFIG_MMC=y # CONFIG_PWRSEQ_EMMC is not set # CONFIG_PWRSEQ_SIMPLE is not set -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_CAVIUM_OCTEON=y CONFIG_RTC_CLASS=y CONFIG_RTC_DRV_DS1307=y diff --git a/arch/mips/configs/qi_lb60_defconfig b/arch/mips/configs/qi_lb60_defconfig index d7bb8cc..df067bc 100644 --- a/arch/mips/configs/qi_lb60_defconfig +++ b/arch/mips/configs/qi_lb60_defconfig @@ -110,7 +110,6 @@ CONFIG_USB_ETH=y # CONFIG_USB_ETH_RNDIS is not set CONFIG_MMC=y CONFIG_MMC_UNSAFE_RESUME=y -# CONFIG_MMC_BLOCK_BOUNCE is not set CONFIG_MMC_JZ4740=y CONFIG_RTC_CLASS=y CONFIG_RTC_DRV_JZ4740=y diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig index fc1ecda..42e8906 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 5c37b6b..0dcf31d 100644 --- a/drivers/mmc/core/queue.c +++ b/drivers/mmc/core/queue.c @@ -23,8 +23,6 @@ #include "core.h" #include "card.h" -#define MMC_QUEUE_BOUNCESZ 65536 - /* * Prepare a MMC request. This just filters out odd stuff. */ @@ -219,73 +217,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) -{ - int i; - - for (i = 0; i < qdepth; i++) { - mqrq[i].bounce_buf = kmalloc(bouncesz, GFP_KERNEL); - if (!mqrq[i].bounce_buf) - return -ENOMEM; - - mqrq[i].sg = mmc_alloc_sg(1); - if (!mqrq[i].sg) - return -ENOMEM; - - mqrq[i].bounce_sg = mmc_alloc_sg(bouncesz / 512); - if (!mqrq[i].bounce_sg) - return -ENOMEM; - } - - return 0; -} - -static bool mmc_queue_alloc_bounce(struct mmc_queue_req *mqrq, int qdepth, - unsigned int bouncesz) -{ - int ret; - - ret = mmc_queue_alloc_bounce_bufs(mqrq, qdepth, bouncesz); - if (ret) - mmc_queue_reqs_free_bufs(mqrq, qdepth); - - return !ret; -} - -static unsigned int mmc_queue_calc_bouncesz(struct mmc_host *host) -{ - unsigned int bouncesz = MMC_QUEUE_BOUNCESZ; - - if (host->max_segs != 1) - return 0; - - if (bouncesz > host->max_req_size) - bouncesz = host->max_req_size; - if (bouncesz > host->max_seg_size) - bouncesz = host->max_seg_size; - if (bouncesz > host->max_blk_count * 512) - bouncesz = host->max_blk_count * 512; - - if (bouncesz <= 512) - return 0; - - 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) { @@ -312,7 +243,6 @@ static int __mmc_queue_alloc_shared_queue(struct mmc_card *card, int qdepth) { struct mmc_host *host = card->host; struct mmc_queue_req *mqrq; - unsigned int bouncesz; int ret = 0; if (card->mqrq) @@ -325,26 +255,10 @@ static int __mmc_queue_alloc_shared_queue(struct mmc_card *card, int qdepth) card->mqrq = mqrq; card->qdepth = qdepth; - bouncesz = mmc_queue_calc_bouncesz(host); - - if (bouncesz && !mmc_queue_alloc_bounce(mqrq, qdepth, bouncesz)) { - bouncesz = 0; - pr_warn("%s: unable to allocate bounce buffers\n", - mmc_card_name(card)); - } - - card->bouncesz = bouncesz; - - if (!bouncesz) { - ret = mmc_queue_alloc_sgs(mqrq, qdepth, host->max_segs); - if (ret) - goto out_err; - } - - return ret; + ret = mmc_queue_alloc_sgs(mqrq, qdepth, host->max_segs); + if (ret) + mmc_queue_free_shared_queue(card); -out_err: - mmc_queue_free_shared_queue(card); return ret; } @@ -387,18 +301,11 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card, if (mmc_can_erase(card)) mmc_queue_setup_discard(mq->queue, card); - if (card->bouncesz) { - blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_ANY); - blk_queue_max_hw_sectors(mq->queue, card->bouncesz / 512); - blk_queue_max_segments(mq->queue, card->bouncesz / 512); - blk_queue_max_segment_size(mq->queue, card->bouncesz); - } else { - blk_queue_bounce_limit(mq->queue, limit); - blk_queue_max_hw_sectors(mq->queue, - min(host->max_blk_count, host->max_req_size / 512)); - blk_queue_max_segments(mq->queue, host->max_segs); - blk_queue_max_segment_size(mq->queue, host->max_seg_size); - } + blk_queue_bounce_limit(mq->queue, limit); + blk_queue_max_hw_sectors(mq->queue, + min(host->max_blk_count, host->max_req_size / 512)); + blk_queue_max_segments(mq->queue, host->max_segs); + blk_queue_max_segment_size(mq->queue, host->max_seg_size); sema_init(&mq->thread_sem, 1); diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c index 621ce47..2c6d131 100644 --- a/drivers/mmc/host/davinci_mmc.c +++ b/drivers/mmc/host/davinci_mmc.c @@ -149,11 +149,7 @@ /* * One scatterlist dma "segment" is at most MAX_CCNT rw_threshold units, - * and we handle up to MAX_NR_SG segments. MMC_BLOCK_BOUNCE kicks in only - * for drivers with max_segs == 1, making the segments bigger (64KB) - * than the page or two that's otherwise typical. nr_sg (passed from - * platform data) == 16 gives at least the same throughput boost, using - * EDMA transfer linkage instead of spending CPU time copying pages. + * and we handle up to MAX_NR_SG segments. */ #define MAX_CCNT ((1 << 16) - 1) diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index aad015e..23653c7 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -306,7 +306,6 @@ struct mmc_card { unsigned int nr_parts; struct mmc_queue_req *mqrq; /* Shared queue structure */ - unsigned int bouncesz; /* Bounce buffer size */ int qdepth; /* Shared queue depth */ };
Remove MMC bounce buffer config option and associated code. This is proposed in addition to Linus' changes to remove the config option. I have tested this on our Octeon hardware platforms. Signed-off-by: Steven J. Hill <Steven.Hill@cavium.com> --- arch/arm/configs/colibri_pxa300_defconfig | 1 - arch/arm/configs/davinci_all_defconfig | 1 - arch/arm/configs/lpc32xx_defconfig | 1 - arch/arm/configs/nhk8815_defconfig | 1 - arch/arm/configs/sama5_defconfig | 1 - arch/arm/configs/u300_defconfig | 1 - arch/arm/configs/zeus_defconfig | 1 - arch/blackfin/configs/CM-BF533_defconfig | 1 - arch/blackfin/configs/CM-BF537E_defconfig | 1 - arch/mips/configs/cavium_octeon_defconfig | 1 - arch/mips/configs/qi_lb60_defconfig | 1 - drivers/mmc/core/Kconfig | 18 ----- drivers/mmc/core/queue.c | 109 +++--------------------------- drivers/mmc/host/davinci_mmc.c | 6 +- include/linux/mmc/card.h | 1 - 15 files changed, 9 insertions(+), 136 deletions(-)