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

[Arnd Bergmann]

On Tue, Aug 27, 2024, at 03:28, Sam Protsenko wrote:
> On Thu, Mar 7, 2024 at 1:52 AM Arnd Bergmann <arnd@xxxxxxxx> 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.

     Arnd