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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPLW+4=EJ4qw4u34bvJ2KPhkEObodB5y-YjnsQmLwHhKpfPvsw@mail.gmail.com>
Date: Tue, 27 Aug 2024 12:00:20 -0500
From: Sam Protsenko <semen.protsenko@...aro.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: "jh80.chung" <jh80.chung@...sung.com>, Christoph Hellwig <hch@....de>, Chris Ball <cjb@...top.org>, 
	Will Newton <will.newton@...il.com>, Matt Fleming <matt@...sole-pimps.org>, 
	Christian Brauner <brauner@...nel.org>, Jens Axboe <axboe@...nel.dk>, 
	Sumit Semwal <sumit.semwal@...aro.org>, Dan Carpenter <dan.carpenter@...aro.org>, 
	Anders Roxell <anders.roxell@...aro.org>, Naresh Kamboju <naresh.kamboju@...aro.org>, 
	"linux-mmc @ vger . kernel . org" <linux-mmc@...r.kernel.org>, linux-block <linux-block@...r.kernel.org>, 
	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] mmc: dw_mmc: Fix IDMAC operation with pages bigger than 4K

Hi Ulf,

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