Re: [PATCH] omap_hsmmc: Reduce max_segs

Re: [PATCH] omap_hsmmc: Reduce max_segs

[Will Newton]

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
>

Re: [PATCH] omap_hsmmc: Reduce max_segs

[Tony Lindgren]

* 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

Re: [PATCH] omap_hsmmc: Reduce max_segs

[Will Newton]

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

Re: [PATCH] omap_hsmmc: Reduce max_segs

[Tony Lindgren]

* 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

Re: [PATCH] omap_hsmmc: Reduce max_segs

[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)"
>>
>> 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

Re: [PATCH] omap_hsmmc: Reduce max_segs

[Will Newton]

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,

Re: [PATCH] omap_hsmmc: Reduce max_segs

[Ravikumar]

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

Re: [PATCH] omap_hsmmc: Reduce max_segs

[Kishon Vijay Abraham I]

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