linux-kernel - Re: [PATCH v6 4/9] binder: store shrinker metadata under page->private

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: <Z1BfQE6IMI9nqUIB@google.com>
Date: Wed, 4 Dec 2024 13:55:12 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Christian Brauner <brauner@...nel.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	linux-kernel@...r.kernel.org, kernel-team@...roid.com,
	Matthew Wilcox <willy@...radead.org>,
	"Liam R. Howlett" <Liam.Howlett@...cle.com>
Subject: Re: [PATCH v6 4/9] binder: store shrinker metadata under
 page->private

On Wed, Dec 04, 2024 at 10:39:58AM +0100, Alice Ryhl wrote:
> On Tue, Dec 3, 2024 at 10:56 PM Carlos Llamas <cmllamas@...gle.com> wrote:
> >
> > Instead of pre-allocating an entire array of struct binder_lru_page in
> > alloc->pages, install the shrinker metadata under page->private. This
> > ensures the memory is allocated and released as needed alongside pages.
> >
> > By converting the alloc->pages[] into an array of struct page pointers,
> > we can access these pages directly and only reference the shrinker
> > metadata where it's being used (e.g. inside the shrinker's callback).
> 
> Using many allocations instead of a single array will increase the
> number of allocations a lot. Is it worth it?

It's not a lot, is as needed. Yes, there will be some transactions that
need to allocate a page and the metadata here and there. However, the
vast majority will find an existing page.

Another way to think about this is how userspace defines the mmap size:
It makes sense to leave some slack beyond the expected usage. This patch
avoids preallocating all that memory for the "slack" which will end up
unused most of the time.

> 
> > Rename struct binder_lru_page to struct binder_shrinker_mdata to better
> > reflect its purpose. Add convenience functions that wrap the allocation
> > and freeing of pages along with their shrinker metadata.
> >
> > Note I've reworked this patch to avoid using page->lru and page->index
> > directly, as Matthew pointed out that these are being removed [1].
> >
> > Link: https://lore.kernel.org/all/ZzziucEm3np6e7a0@casper.infradead.org/ [1]
> > Cc: Matthew Wilcox <willy@...radead.org>
> > Cc: Liam R. Howlett <Liam.Howlett@...cle.com>
> > Reviewed-by: Suren Baghdasaryan <surenb@...gle.com>
> > Signed-off-by: Carlos Llamas <cmllamas@...gle.com>
> 
> [...]
> 
> > +static void binder_free_page(struct page *page)
> > +{
> > +       kfree((void *)page_private(page));
> > +       __free_page(page);
> 
> I would cast the page_private to a pointer of the right type here.
> There may be tools or future improvements to kfree that use the type
> information.

Ok, I'll change this. There is also us humans that might benefit from
using the explicit type for context.

--
Carlos Llamas