Hi Ulf, On Tue, Aug 27, 2024 at 2:27 AM Arnd Bergmann <arnd@xxxxxxxx> wrote: > > 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. > 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!
