linux-kernel - Re: [PATCH v2 3/5] mm: memcg: merge multiple page

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] [day] [month] [year] [list]

Message-ID: <20240726233123.GG1702603@cmpxchg.org>
Date: Fri, 26 Jul 2024 19:31:23 -0400
From: Johannes Weiner <hannes@...xchg.org>
To: Roman Gushchin <roman.gushchin@...ux.dev>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Michal Hocko <mhocko@...nel.org>,
	Shakeel Butt <shakeel.butt@...ux.dev>,
	Muchun Song <muchun.song@...ux.dev>
Subject: Re: [PATCH v2 3/5] mm: memcg: merge multiple page_counters into a
 single structure

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!