| Message ID | 20170619093644.16054-1-willn@resin.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Just adding a few people to CC. I'd love to get some feedback as to whether this patch makes sense or not. Cheers, On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> wrote: > Reduce max_segs to a value that allows allocation of an entire > descriptor list within a single page. This avoids doing a > higher order GFP_ATOMIC allocation when setting up a transfer > which can potentially fail and lead to I/O failures. > > Signed-off-by: Will Newton <willn@resin.io> > --- > drivers/mmc/host/omap_hsmmc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 1438a72..d5f42d9 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -2864,9 +2864,9 @@ static int omap_hsmmc_probe(struct platform_device *pdev) > host->use_adma = true; > } > > - /* Since we do only SG emulation, we can have as many segs > - * as we want. */ > - mmc->max_segs = 1024; > + /* Set this to a value that allows allocating an entire descriptor > + list within a page (zero order allocation). */ > + mmc->max_segs = 64; > > mmc->max_blk_size = 512; /* Block Length at max can be 1024 */ > mmc->max_blk_count = 0xFFFF; /* No. of Blocks is 16 bits */ > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Will Newton <will.newton@gmail.com> [170620 05:39]: > Just adding a few people to CC. I'd love to get some feedback as to > whether this patch makes sense or not. > > On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> wrote: > > Reduce max_segs to a value that allows allocation of an entire > > descriptor list within a single page. This avoids doing a > > higher order GFP_ATOMIC allocation when setting up a transfer > > which can potentially fail and lead to I/O failures. I recall we only ever have few SG entries so if there is no performance impact I see no reason to lower it to save memory. Care to check if that's the case still? Regards, Tony > > Signed-off-by: Will Newton <willn@resin.io> > > --- > > drivers/mmc/host/omap_hsmmc.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > > index 1438a72..d5f42d9 100644 > > --- a/drivers/mmc/host/omap_hsmmc.c > > +++ b/drivers/mmc/host/omap_hsmmc.c > > @@ -2864,9 +2864,9 @@ static int omap_hsmmc_probe(struct platform_device *pdev) > > host->use_adma = true; > > } > > > > - /* Since we do only SG emulation, we can have as many segs > > - * as we want. */ > > - mmc->max_segs = 1024; > > + /* Set this to a value that allows allocating an entire descriptor > > + list within a page (zero order allocation). */ > > + mmc->max_segs = 64; > > > > mmc->max_blk_size = 512; /* Block Length at max can be 1024 */ > > mmc->max_blk_count = 0xFFFF; /* No. of Blocks is 16 bits */ > > -- > > 2.7.4 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 21, 2017 at 7:24 AM, Tony Lindgren <tony@atomide.com> wrote: Hi Tony, > * Will Newton <will.newton@gmail.com> [170620 05:39]: >> Just adding a few people to CC. I'd love to get some feedback as to >> whether this patch makes sense or not. >> >> On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> wrote: >> > Reduce max_segs to a value that allows allocation of an entire >> > descriptor list within a single page. This avoids doing a >> > higher order GFP_ATOMIC allocation when setting up a transfer >> > which can potentially fail and lead to I/O failures. > > I recall we only ever have few SG entries so if there is no performance > impact I see no reason to lower it to save memory. Care to check > if that's the case still? I have been seeing allocation failures in edma_prep_slave_sg allocating the descriptor list of order 3, which from my rough calculations is an sg_len of ~800. The memory usage itself doesn't seem like a problem but doing a GFP_ATOMIC allocation of higher order under I/O load seems like it is always going to cause problems. I'm not an expert on this code but it looks like the scatterlist is created from the queue so if the queue gets full we can get a very large scatterlist. The current value of 1024 seems to be something of an outlier: drivers/mmc/host/android-goldfish.c: mmc->max_segs = 32; drivers/mmc/host/atmel-mci.c: mmc->max_segs = 256; drivers/mmc/host/atmel-mci.c: mmc->max_segs = 64; drivers/mmc/host/au1xmmc.c: mmc->max_segs = AU1XMMC_DESCRIPTOR_COUNT; drivers/mmc/host/bcm2835.c: mmc->max_segs = 128; drivers/mmc/host/bfin_sdh.c: mmc->max_segs = 1; drivers/mmc/host/bfin_sdh.c: mmc->max_segs = PAGE_SIZE / sizeof(struct dma_desc_array); drivers/mmc/host/cavium.c: mmc->max_segs = 16; drivers/mmc/host/cavium.c: mmc->max_segs = 1; drivers/mmc/host/dw_mmc.c: mmc->max_segs = host->ring_size; drivers/mmc/host/dw_mmc.c: mmc->max_segs = 64; drivers/mmc/host/dw_mmc.c: mmc->max_segs = 64; drivers/mmc/host/jz4740_mmc.c: mmc->max_segs = 128; drivers/mmc/host/meson-gx-mmc.c: mmc->max_segs = SD_EMMC_DESC_BUF_LEN / sizeof(struct sd_emmc_desc); drivers/mmc/host/mmc_spi.c: mmc->max_segs = MMC_SPI_BLOCKSATONCE; drivers/mmc/host/mmci.c: mmc->max_segs = NR_SG; drivers/mmc/host/mtk-sd.c: mmc->max_segs = MAX_BD_NUM; drivers/mmc/host/mvsdio.c: mmc->max_segs = 1; drivers/mmc/host/mxcmmc.c: mmc->max_segs = 64; drivers/mmc/host/mxs-mmc.c: mmc->max_segs = 52; drivers/mmc/host/omap.c: mmc->max_segs = 32; drivers/mmc/host/omap_hsmmc.c: mmc->max_segs = 1024; drivers/mmc/host/pxamci.c: mmc->max_segs = NR_SG; drivers/mmc/host/rtsx_pci_sdmmc.c: mmc->max_segs = 256; drivers/mmc/host/rtsx_usb_sdmmc.c: mmc->max_segs = 256; drivers/mmc/host/sdhci.c: mmc->max_segs = SDHCI_MAX_SEGS; drivers/mmc/host/sdhci.c: mmc->max_segs = 1; drivers/mmc/host/sdhci.c: mmc->max_segs = SDHCI_MAX_SEGS; drivers/mmc/host/sh_mmcif.c: mmc->max_segs = 32; drivers/mmc/host/tifm_sd.c: mmc->max_segs = mmc->max_blk_count; drivers/mmc/host/tmio_mmc_pio.c: mmc->max_segs = 32; drivers/mmc/host/usdhi6rol0.c: mmc->max_segs = 32; drivers/mmc/host/ushc.c: mmc->max_segs = 1; drivers/mmc/host/via-sdmmc.c: mmc->max_segs = 1; drivers/mmc/host/vub300.c: mmc->max_segs = 128; drivers/mmc/host/wbsd.c: mmc->max_segs = 128; drivers/mmc/host/wmt-sdmmc.c: mmc->max_segs = wmt_caps->max_segs; > > >> > Signed-off-by: Will Newton <willn@resin.io> >> > --- >> > drivers/mmc/host/omap_hsmmc.c | 6 +++--- >> > 1 file changed, 3 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c >> > index 1438a72..d5f42d9 100644 >> > --- a/drivers/mmc/host/omap_hsmmc.c >> > +++ b/drivers/mmc/host/omap_hsmmc.c >> > @@ -2864,9 +2864,9 @@ static int omap_hsmmc_probe(struct platform_device *pdev) >> > host->use_adma = true; >> > } >> > >> > - /* Since we do only SG emulation, we can have as many segs >> > - * as we want. */ >> > - mmc->max_segs = 1024; >> > + /* Set this to a value that allows allocating an entire descriptor >> > + list within a page (zero order allocation). */ >> > + mmc->max_segs = 64; >> > >> > mmc->max_blk_size = 512; /* Block Length at max can be 1024 */ >> > mmc->max_blk_count = 0xFFFF; /* No. of Blocks is 16 bits */ >> > -- >> > 2.7.4 >> > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Will Newton <will.newton@gmail.com> [170621 01:18]: > On Wed, Jun 21, 2017 at 7:24 AM, Tony Lindgren <tony@atomide.com> wrote: > > Hi Tony, > > > * Will Newton <will.newton@gmail.com> [170620 05:39]: > >> Just adding a few people to CC. I'd love to get some feedback as to > >> whether this patch makes sense or not. > >> > >> On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> wrote: > >> > Reduce max_segs to a value that allows allocation of an entire > >> > descriptor list within a single page. This avoids doing a > >> > higher order GFP_ATOMIC allocation when setting up a transfer > >> > which can potentially fail and lead to I/O failures. > > > > I recall we only ever have few SG entries so if there is no performance > > impact I see no reason to lower it to save memory. Care to check > > if that's the case still? > > I have been seeing allocation failures in edma_prep_slave_sg > allocating the descriptor list of order 3, which from my rough > calculations is an sg_len of ~800. The memory usage itself doesn't > seem like a problem but doing a GFP_ATOMIC allocation of higher order > under I/O load seems like it is always going to cause problems. OK > I'm not an expert on this code but it looks like the scatterlist is > created from the queue so if the queue gets full we can get a very > large scatterlist. The current value of 1024 seems to be something of > an outlier: > > drivers/mmc/host/android-goldfish.c: mmc->max_segs = 32; > drivers/mmc/host/atmel-mci.c: mmc->max_segs = 256; > drivers/mmc/host/atmel-mci.c: mmc->max_segs = 64; > drivers/mmc/host/au1xmmc.c: mmc->max_segs = AU1XMMC_DESCRIPTOR_COUNT; > drivers/mmc/host/bcm2835.c: mmc->max_segs = 128; > drivers/mmc/host/bfin_sdh.c: mmc->max_segs = 1; > drivers/mmc/host/bfin_sdh.c: mmc->max_segs = PAGE_SIZE / > sizeof(struct dma_desc_array); > drivers/mmc/host/cavium.c: mmc->max_segs = 16; > drivers/mmc/host/cavium.c: mmc->max_segs = 1; > drivers/mmc/host/dw_mmc.c: mmc->max_segs = host->ring_size; > drivers/mmc/host/dw_mmc.c: mmc->max_segs = 64; > drivers/mmc/host/dw_mmc.c: mmc->max_segs = 64; > drivers/mmc/host/jz4740_mmc.c: mmc->max_segs = 128; > drivers/mmc/host/meson-gx-mmc.c: mmc->max_segs = > SD_EMMC_DESC_BUF_LEN / sizeof(struct sd_emmc_desc); > drivers/mmc/host/mmc_spi.c: mmc->max_segs = MMC_SPI_BLOCKSATONCE; > drivers/mmc/host/mmci.c: mmc->max_segs = NR_SG; > drivers/mmc/host/mtk-sd.c: mmc->max_segs = MAX_BD_NUM; > drivers/mmc/host/mvsdio.c: mmc->max_segs = 1; > drivers/mmc/host/mxcmmc.c: mmc->max_segs = 64; > drivers/mmc/host/mxs-mmc.c: mmc->max_segs = 52; > drivers/mmc/host/omap.c: mmc->max_segs = 32; > drivers/mmc/host/omap_hsmmc.c: mmc->max_segs = 1024; > drivers/mmc/host/pxamci.c: mmc->max_segs = NR_SG; > drivers/mmc/host/rtsx_pci_sdmmc.c: mmc->max_segs = 256; > drivers/mmc/host/rtsx_usb_sdmmc.c: mmc->max_segs = 256; > drivers/mmc/host/sdhci.c: mmc->max_segs = SDHCI_MAX_SEGS; > drivers/mmc/host/sdhci.c: mmc->max_segs = 1; > drivers/mmc/host/sdhci.c: mmc->max_segs = SDHCI_MAX_SEGS; > drivers/mmc/host/sh_mmcif.c: mmc->max_segs = 32; > drivers/mmc/host/tifm_sd.c: mmc->max_segs = mmc->max_blk_count; > drivers/mmc/host/tmio_mmc_pio.c: mmc->max_segs = 32; > drivers/mmc/host/usdhi6rol0.c: mmc->max_segs = 32; > drivers/mmc/host/ushc.c: mmc->max_segs = 1; > drivers/mmc/host/via-sdmmc.c: mmc->max_segs = 1; > drivers/mmc/host/vub300.c: mmc->max_segs = 128; > drivers/mmc/host/wbsd.c: mmc->max_segs = 128; > drivers/mmc/host/wmt-sdmmc.c: mmc->max_segs = wmt_caps->max_segs; OK maybe update the patch description a bit more with that info above :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 21 June 2017 02:02 PM, Tony Lindgren wrote: > * Will Newton <will.newton@gmail.com> [170621 01:18]: >> On Wed, Jun 21, 2017 at 7:24 AM, Tony Lindgren <tony@atomide.com> wrote: >> >> Hi Tony, >> >>> * Will Newton <will.newton@gmail.com> [170620 05:39]: >>>> Just adding a few people to CC. I'd love to get some feedback as to >>>> whether this patch makes sense or not. >>>> >>>> On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> wrote: >>>>> Reduce max_segs to a value that allows allocation of an entire >>>>> descriptor list within a single page. This avoids doing a >>>>> higher order GFP_ATOMIC allocation when setting up a transfer >>>>> which can potentially fail and lead to I/O failures. >>> I recall we only ever have few SG entries so if there is no performance >>> impact I see no reason to lower it to save memory. Care to check >>> if that's the case still? >> I have been seeing allocation failures in edma_prep_slave_sg >> allocating the descriptor list of order 3, which from my rough >> calculations is an sg_len of ~800. The memory usage itself doesn't >> seem like a problem but doing a GFP_ATOMIC allocation of higher order >> under I/O load seems like it is always going to cause problems. max_segs= 64 looks like a sensible value to me for edma/sdma, with each param set ~40Bytes - this will ensure to fit in a single page. IIRC, edma splits the list at 20. MAX_NR_SEGS set to 20 in edma.c. So, having a bigger number *may not* guarantee a proportional increase in throughput. > OK > >> I'm not an expert on this code but it looks like the scatterlist is >> created from the queue so if the queue gets full we can get a very >> large scatterlist. The current value of 1024 seems to be something of >> an outlier: When using ADMA, since you allocate the table only once during probe, there'll be no alloc calls at a later time. And increase in number of sg entries would increase efficiency. I've seen as many as 290-300 sg entries being used during file-system init/mount. So, for ADMA, I would say at least max_segs = 512 is a better option since descriptor size is 8B, would still fit in a page. You can over-wirte it under "if (host->adma)" >> >> drivers/mmc/host/android-goldfish.c: mmc->max_segs = 32; >> drivers/mmc/host/atmel-mci.c: mmc->max_segs = 256; >> drivers/mmc/host/atmel-mci.c: mmc->max_segs = 64; >> drivers/mmc/host/au1xmmc.c: mmc->max_segs = AU1XMMC_DESCRIPTOR_COUNT; >> drivers/mmc/host/bcm2835.c: mmc->max_segs = 128; >> drivers/mmc/host/bfin_sdh.c: mmc->max_segs = 1; >> drivers/mmc/host/bfin_sdh.c: mmc->max_segs = PAGE_SIZE / >> sizeof(struct dma_desc_array); >> drivers/mmc/host/cavium.c: mmc->max_segs = 16; >> drivers/mmc/host/cavium.c: mmc->max_segs = 1; >> drivers/mmc/host/dw_mmc.c: mmc->max_segs = host->ring_size; >> drivers/mmc/host/dw_mmc.c: mmc->max_segs = 64; >> drivers/mmc/host/dw_mmc.c: mmc->max_segs = 64; >> drivers/mmc/host/jz4740_mmc.c: mmc->max_segs = 128; >> drivers/mmc/host/meson-gx-mmc.c: mmc->max_segs = >> SD_EMMC_DESC_BUF_LEN / sizeof(struct sd_emmc_desc); >> drivers/mmc/host/mmc_spi.c: mmc->max_segs = MMC_SPI_BLOCKSATONCE; >> drivers/mmc/host/mmci.c: mmc->max_segs = NR_SG; >> drivers/mmc/host/mtk-sd.c: mmc->max_segs = MAX_BD_NUM; >> drivers/mmc/host/mvsdio.c: mmc->max_segs = 1; >> drivers/mmc/host/mxcmmc.c: mmc->max_segs = 64; >> drivers/mmc/host/mxs-mmc.c: mmc->max_segs = 52; >> drivers/mmc/host/omap.c: mmc->max_segs = 32; >> drivers/mmc/host/omap_hsmmc.c: mmc->max_segs = 1024; >> drivers/mmc/host/pxamci.c: mmc->max_segs = NR_SG; >> drivers/mmc/host/rtsx_pci_sdmmc.c: mmc->max_segs = 256; >> drivers/mmc/host/rtsx_usb_sdmmc.c: mmc->max_segs = 256; >> drivers/mmc/host/sdhci.c: mmc->max_segs = SDHCI_MAX_SEGS; >> drivers/mmc/host/sdhci.c: mmc->max_segs = 1; >> drivers/mmc/host/sdhci.c: mmc->max_segs = SDHCI_MAX_SEGS; >> drivers/mmc/host/sh_mmcif.c: mmc->max_segs = 32; >> drivers/mmc/host/tifm_sd.c: mmc->max_segs = mmc->max_blk_count; >> drivers/mmc/host/tmio_mmc_pio.c: mmc->max_segs = 32; >> drivers/mmc/host/usdhi6rol0.c: mmc->max_segs = 32; >> drivers/mmc/host/ushc.c: mmc->max_segs = 1; >> drivers/mmc/host/via-sdmmc.c: mmc->max_segs = 1; >> drivers/mmc/host/vub300.c: mmc->max_segs = 128; >> drivers/mmc/host/wbsd.c: mmc->max_segs = 128; >> drivers/mmc/host/wmt-sdmmc.c: mmc->max_segs = wmt_caps->max_segs; > OK maybe update the patch description a bit more with that > info above :) > > Regards, > > Tony Regards, RK -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 21, 2017 at 10:16 AM, Ravikumar <rk@ti.com> wrote: Hi Ravikumar, > On Wednesday 21 June 2017 02:02 PM, Tony Lindgren wrote: >> >> * Will Newton <will.newton@gmail.com> [170621 01:18]: >>> >>> On Wed, Jun 21, 2017 at 7:24 AM, Tony Lindgren <tony@atomide.com> wrote: >>> >>> Hi Tony, >>> >>>> * Will Newton <will.newton@gmail.com> [170620 05:39]: >>>>> >>>>> Just adding a few people to CC. I'd love to get some feedback as to >>>>> whether this patch makes sense or not. >>>>> >>>>> On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> >>>>> wrote: >>>>>> >>>>>> Reduce max_segs to a value that allows allocation of an entire >>>>>> descriptor list within a single page. This avoids doing a >>>>>> higher order GFP_ATOMIC allocation when setting up a transfer >>>>>> which can potentially fail and lead to I/O failures. >>>> >>>> I recall we only ever have few SG entries so if there is no performance >>>> impact I see no reason to lower it to save memory. Care to check >>>> if that's the case still? >>> >>> I have been seeing allocation failures in edma_prep_slave_sg >>> allocating the descriptor list of order 3, which from my rough >>> calculations is an sg_len of ~800. The memory usage itself doesn't >>> seem like a problem but doing a GFP_ATOMIC allocation of higher order >>> under I/O load seems like it is always going to cause problems. > > max_segs= 64 looks like a sensible value to me for edma/sdma, with each > param set ~40Bytes - this will ensure to fit in a single page. > IIRC, edma splits the list at 20. MAX_NR_SEGS set to 20 in edma.c. So, > having a bigger number *may not* guarantee a proportional increase > in throughput. > >> OK >> >>> I'm not an expert on this code but it looks like the scatterlist is >>> created from the queue so if the queue gets full we can get a very >>> large scatterlist. The current value of 1024 seems to be something of >>> an outlier: > > When using ADMA, since you allocate the table only once during probe, > there'll be no alloc calls at a later time. > And increase in number of sg entries would increase efficiency. I've seen > as many as 290-300 sg entries being used during file-system init/mount. So, > for ADMA, I would say at least max_segs = 512 is a better option since > descriptor size is 8B, > would still fit in a page. > You can over-wirte it under "if (host->adma)" I can't find this member in struct mmc_host on master, is there somewhere else I should be looking? Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 21 June 2017 08:39 PM, Will Newton wrote: > On Wed, Jun 21, 2017 at 10:16 AM, Ravikumar <rk@ti.com> wrote: > > Hi Ravikumar, > >> On Wednesday 21 June 2017 02:02 PM, Tony Lindgren wrote: >>> * Will Newton <will.newton@gmail.com> [170621 01:18]: >>>> On Wed, Jun 21, 2017 at 7:24 AM, Tony Lindgren <tony@atomide.com> wrote: >>>> >>>> Hi Tony, >>>> >>>>> * Will Newton <will.newton@gmail.com> [170620 05:39]: >>>>>> Just adding a few people to CC. I'd love to get some feedback as to >>>>>> whether this patch makes sense or not. >>>>>> >>>>>> On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> >>>>>> wrote: >>>>>>> Reduce max_segs to a value that allows allocation of an entire >>>>>>> descriptor list within a single page. This avoids doing a >>>>>>> higher order GFP_ATOMIC allocation when setting up a transfer >>>>>>> which can potentially fail and lead to I/O failures. >>>>> I recall we only ever have few SG entries so if there is no performance >>>>> impact I see no reason to lower it to save memory. Care to check >>>>> if that's the case still? >>>> I have been seeing allocation failures in edma_prep_slave_sg >>>> allocating the descriptor list of order 3, which from my rough >>>> calculations is an sg_len of ~800. The memory usage itself doesn't >>>> seem like a problem but doing a GFP_ATOMIC allocation of higher order >>>> under I/O load seems like it is always going to cause problems. >> max_segs= 64 looks like a sensible value to me for edma/sdma, with each >> param set ~40Bytes - this will ensure to fit in a single page. >> IIRC, edma splits the list at 20. MAX_NR_SEGS set to 20 in edma.c. So, >> having a bigger number *may not* guarantee a proportional increase >> in throughput. >> >>> OK >>> >>>> I'm not an expert on this code but it looks like the scatterlist is >>>> created from the queue so if the queue gets full we can get a very >>>> large scatterlist. The current value of 1024 seems to be something of >>>> an outlier: >> When using ADMA, since you allocate the table only once during probe, >> there'll be no alloc calls at a later time. >> And increase in number of sg entries would increase efficiency. I've seen >> as many as 290-300 sg entries being used during file-system init/mount. So, >> for ADMA, I would say at least max_segs = 512 is a better option since >> descriptor size is 8B, >> would still fit in a page. >> You can over-wirte it under "if (host->adma)" > I can't find this member in struct mmc_host on master, is there > somewhere else I should be looking? The exact member is "host->use_adma". > Thanks, RK -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 22 June 2017 11:11 AM, Ravikumar wrote: > > > On Wednesday 21 June 2017 08:39 PM, Will Newton wrote: >> On Wed, Jun 21, 2017 at 10:16 AM, Ravikumar <rk@ti.com> wrote: >> >> Hi Ravikumar, >> >>> On Wednesday 21 June 2017 02:02 PM, Tony Lindgren wrote: >>>> * Will Newton <will.newton@gmail.com> [170621 01:18]: >>>>> On Wed, Jun 21, 2017 at 7:24 AM, Tony Lindgren <tony@atomide.com> wrote: >>>>> >>>>> Hi Tony, >>>>> >>>>>> * Will Newton <will.newton@gmail.com> [170620 05:39]: >>>>>>> Just adding a few people to CC. I'd love to get some feedback as to >>>>>>> whether this patch makes sense or not. >>>>>>> >>>>>>> On Mon, Jun 19, 2017 at 10:36 AM, Will Newton <will.newton@gmail.com> >>>>>>> wrote: >>>>>>>> Reduce max_segs to a value that allows allocation of an entire >>>>>>>> descriptor list within a single page. This avoids doing a >>>>>>>> higher order GFP_ATOMIC allocation when setting up a transfer >>>>>>>> which can potentially fail and lead to I/O failures. >>>>>> I recall we only ever have few SG entries so if there is no performance >>>>>> impact I see no reason to lower it to save memory. Care to check >>>>>> if that's the case still? >>>>> I have been seeing allocation failures in edma_prep_slave_sg >>>>> allocating the descriptor list of order 3, which from my rough >>>>> calculations is an sg_len of ~800. The memory usage itself doesn't >>>>> seem like a problem but doing a GFP_ATOMIC allocation of higher order >>>>> under I/O load seems like it is always going to cause problems. >>> max_segs= 64 looks like a sensible value to me for edma/sdma, with each >>> param set ~40Bytes - this will ensure to fit in a single page. >>> IIRC, edma splits the list at 20. MAX_NR_SEGS set to 20 in edma.c. So, >>> having a bigger number *may not* guarantee a proportional increase >>> in throughput. >>> >>>> OK >>>> >>>>> I'm not an expert on this code but it looks like the scatterlist is >>>>> created from the queue so if the queue gets full we can get a very >>>>> large scatterlist. The current value of 1024 seems to be something of >>>>> an outlier: >>> When using ADMA, since you allocate the table only once during probe, >>> there'll be no alloc calls at a later time. >>> And increase in number of sg entries would increase efficiency. I've seen >>> as many as 290-300 sg entries being used during file-system init/mount. So, >>> for ADMA, I would say at least max_segs = 512 is a better option since >>> descriptor size is 8B, >>> would still fit in a page. >>> You can over-wirte it under "if (host->adma)" >> I can't find this member in struct mmc_host on master, is there >> somewhere else I should be looking? > The exact member is "host->use_adma". ADMA support patch is not yet merged. https://patchwork.kernel.org/patch/9791407/ -Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 1438a72..d5f42d9 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -2864,9 +2864,9 @@ static int omap_hsmmc_probe(struct platform_device *pdev) host->use_adma = true; } - /* Since we do only SG emulation, we can have as many segs - * as we want. */ - mmc->max_segs = 1024; + /* Set this to a value that allows allocating an entire descriptor + list within a page (zero order allocation). */ + mmc->max_segs = 64; mmc->max_blk_size = 512; /* Block Length at max can be 1024 */ mmc->max_blk_count = 0xFFFF; /* No. of Blocks is 16 bits */
Reduce max_segs to a value that allows allocation of an entire descriptor list within a single page. This avoids doing a higher order GFP_ATOMIC allocation when setting up a transfer which can potentially fail and lead to I/O failures. Signed-off-by: Will Newton <willn@resin.io> --- drivers/mmc/host/omap_hsmmc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)