Re: [PATCH 2/2] vmalloc: Account memcg per vmalloc

[Shakeel Butt <shakeel.butt@linux.dev>]

On Wed, Dec 11, 2024 at 08:20:36PM +0000, Matthew Wilcox wrote:
> On Wed, Dec 11, 2024 at 11:32:13AM -0800, Shakeel Butt wrote:
> > On Wed, Dec 11, 2024 at 04:50:39PM +0000, Matthew Wilcox wrote:
> > > Perhaps you'd be more persuaded by:
> > > 
> > > (a) If we clear __GFP_ACCOUNT then alloc_pages_bulk() will work, and
> > > that's a pretty significant performance win over calling alloc_pages()
> > > in a loop.
> > > 
> > > (b) Once we get to memdescs, calling alloc_pages() with __GFP_ACCOUNT
> > > set is going to require allocating a memdesc to store the obj_cgroup
> > > in, so in the future we'll save an allocation.
> > > 
> > > Your proposed alternative will work and is way less churn.  But it's
> > > not preparing us for memdescs ;-)
> > 
> > We can make alloc_pages_bulk() work with __GFP_ACCOUNT but your second
> > argument is more compelling.
> > 
> > I am trying to think of what will we miss if we remove this per-page
> > memcg metadata. One thing I can think of is debugging a live system
> > or kdump where I need to track where a given page came from. I think
> 
> Umm, I don't think you know which vmalloc allocation a page came from
> today?  I've sent patches to add that information before, but they were
> rejected. 

Do you have a link handy for that discussion?

> In fact, I don't think we know even _that_ a page belongs to
> vmalloc today, do we?  Yes, we know that the page is accounted, and
> which memcg it belongs to ... but nothing more.

Yes you are correct. At the moment it is a guesswork and exhaustive
search into multiple sources.

> 
> I actually want to improve this, without adding additional overhead.
> What I'm working on right now (before I got waylaid by this bug) is:
> 
> +struct choir {
> +       struct kref refcount;
> +       unsigned int nr;
> +       struct page *pages[] __counted_by(nr);
> +};
> 
> and rewriting vmalloc to be based on choirs instead of its own pages.
> One thing I've come to realise today is that the obj_cgroup pointer
> needs to be in the choir and not in the vm_struct so that we uncharge the
> allocation when the choir refcount drops to 0, not when the allocation
> is unmapped.

What/who else can take a reference on a choir?

> 
> A regular choir allocation will (today) mark the pages in it as being
> allocated to a choir (and thus not having their own refcount / mapcount),
> but I'll give vmalloc a way to mark the pages as specifically being
> from vmalloc.

This sounds good. Thanks for the awesome work.