Re: [PATCH] mmc: dw_mmc: Fix IDMAC operation with pages bigger than 4K

Sam Protsenko
2024-08-27T17:00:32Z

Hi Ulf,

On Tue, Aug 27, 2024 at 2:27 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Aug 27, 2024, at 03:28, Sam Protsenko wrote:
> > On Thu, Mar 7, 2024 at 1:52 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> >>
> >> The change looks good to me.
> >>
> >> I see that the host->ring_size depends on PAGE_SIZE as well:
> >>
> >> #define DESC_RING_BUF_SZ PAGE_SIZE
> >> host->ring_size = DESC_RING_BUF_SZ / sizeof(struct idmac_desc_64addr);
> >> host->sg_cpu = dmam_alloc_coherent(host->dev,
> >> DESC_RING_BUF_SZ, &host->sg_dma, GFP_KERNEL);
> >>
> >> I don't see any reason for the ring buffer size to be tied to
> >> PAGE_SIZE at all, it was probably picked as a reasonable
> >> default in the initial driver but isn't necessarily ideal.
> >>
> >> From what I can see, the number of 4KB elements in the
> >> ring can be as small as 128 (4KB pages, 64-bit addresses)
> >> or as big as 4096 (64KB pages, 32-bit addresses), which is
> >> quite a difference. If you are still motivated to drill
> >> down into this, could you try changing DESC_RING_BUF_SZ
> >> to a fixed size of either 4KB or 64KB and test again
> >> with the opposite page size, to see if that changes the
> >> throughput?
> >>
> >
> > Sorry for the huge delay. Just ran the tests:
> >
> > - 4K pages, DESC_RING_BUF_SZ = 4K: 97 MB/s
> > - 4K pages, DESC_RING_BUF_SZ = 16K: 98 MB/s
> > - 4K pages, DESC_RING_BUF_SZ = 64K: 97 MB/s
> > - 16K pages, DESC_RING_BUF_SZ = 4K: 123 MB/s
> > - 16K pages, DESC_RING_BUF_SZ = 16K: 125 MB/s
> > - 16K pages, DESC_RING_BUF_SZ = 64K: 124 MB/s
> > - 64K pages, DESC_RING_BUF_SZ = 4K: 137 MB/s
> > - 64K pages, DESC_RING_BUF_SZ = 16K: 135 MB/s
> > - 64K pages, DESC_RING_BUF_SZE = 64K: 138 MB/s
>
> Thanks!
>
> > From what you said, it looks like it may make a sense to reduce
> > DESC_RING_BUF_SZ down to 4 KiB? If so, I'd suggest we do that in a
> > separate patch, as this one actually fixes kernel panic when 16k/64k
> > pages are enabled. Please let me know what you think.
>
> Agreed, sounds good to me.
>

Can you please take this patch? It should apply fine without the need
to rebase it. It fixes a kernel panic when big pages are enabled, so
it's an important fix for us. As discussed with Arnd, the
DESC_RING_BUF_SZ change can be made later as a separate patch.

Thanks!