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

[Carlos Llamas <cmllamas@google.com>]

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@google.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@infradead.org>
> > Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> > Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> > Signed-off-by: Carlos Llamas <cmllamas@google.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