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
