Re: [PATCH v2 3/5] mm: memcg: merge multiple page_counters into a single structure

[Johannes Weiner]

On Thu, Jul 25, 2024 at 11:33:51PM +0000, Roman Gushchin wrote:
> Hm, Idk, I do agree with what you're saying about the self-contained
> piece of abstraction (and I had very similar thoughts in the process),
> but there are also some complications.
> 
> First, funny enough, the protection calculation code was just moved to
> mm/page_counter.c by a8585ac68621 ("mm/page_counter: move calculating protection
> values to page_counter"). The commit log says that it's gonna be used by the drm
> cgroup controller, but the code is not (yet?) upstream apparently. I don't have
> any insights here. If there will be the second user for the protection
> functionality, moving it back to memcontrol.c is not feasible.

If it comes to that, I think it can be its own abstraction as well,
with a struct containing all the elow, low_usage, children_low_usage
stuff etc. and API functions to propagate and get effective protection.

Both memcg and drm can embed this into their css descriptors and use
those helpers as necessary.

> Second, I agree that it would be nice to get rid of the parent pointer in
> struct page_cgroup entirely and use one in css. But Idk how to do it
> without making the code way more messy or duplicate a lot of tree walks.
> In C++ (or another language with generics) we could make struct page_counter
> taking the number of counters and the set of features as template parameters.

It shouldn't be more than a for loop right + the unwind on failure,
right? *Somewhat* duplicative, but pretty simple code that's easy to
wrap in a controller function and have it out of the way.

Actually, we could keep a simple hierarchical version of the page
counter functions, but expose non-hiearchical ones for users that want
to do additional operations on each level.

IOW, e.g. page_counter_charge_one(), page_counter_try_charge_one()
etc, then implement page_counter_charge() et al on top of these.

swap, memsw, kmem, tcpmem, hugetlb etc. could remain unchanged. The
memory counter could have a memcg version of the charge function that
uses *charge_one() and handles protection propagation.

drm could do the same.

That would keep the page_counter->parent pointer, but would save a bit
of complexity for simple users at least.

> I feel like with memcg1 code being factored out the benefit of this reorg
> lowered, so how about me resending 2 first patches separately and spending
> more time on thinking on the best long-term strategy? I have some ideas
> here and I thought about this work as a preparatory step, but maybe you're
> right and it makes sense to approach it more comprehensively.

Sounds good to me.

Thanks!