Re: [PATCH v7 0/5] mm/page_counter: move stock from mem_cgroup to page_counter

[Michal Hocko <mhocko@suse.com>]

On Mon 02-11-26 07:20:17, Joshua Hahn wrote:
> On Mon, 2 Nov 2026 13:25:28 +0100 Michal Hocko <mhocko@suse.com> wrote:
> 
> > On Mon 26-10-26 08:59:37, Joshua Hahn wrote:
> > I guess I still do not understand what you mean by opting-in to the
> > caching.
> 
> Reading my original cover letter again, yeah. It does feel a bit strange
> to call it "opt-in"... I guess I just meant that not all page_coutners
> will be getting it, it would only really be the memory page_counter
> (and memsw page_counter for cgroup v2). I'll change the wording in the
> next version.

OK, that makes it clear to me now.

[...]
> > > > > We get an added benefit of simplifying try_charge_memcg(), which now
> > > > > has all the stock management handled transparently within the
> > > > > page_counter layer.
> > > > 
> > > > This is quite a large change and I believe it will be really benefitial
> > > > to have some high level design overview in the cover rather than trying to
> > > > chase it in respective patches.
> > > 
> > > Sounds good with me. I can add the call traces above which should
> > > explain my thought process a bit more. Hopefully the problem I am
> > > trying to solve is evident there.
> > 
> > I believe I follow where you are heading. I am not sure this is the
> > right path but that is a separate discussion. I would focus on whether
> > moving cache to the counter makes sense on its own (without that future
> > work) at this stage.
> 
> If I'm being honest, most of the generic benefits that exist beyond that
> work were in the other part of this series that I got rid of to scale
> down the scope of this version. But to explicitly name the generic
> benefits, it would be unbounded memcg stock caching per-cpu, eliminating
> random evictions, (like you mentioned above as well) limiting stocks
> to leaf memcgs only, etc.

limiting cache to leaf memcgs makes a lot of sense and addresses a big
concern I had. How exactly does this work?
 
> I think there was a proposal to make the number of memcg stocks per cpu
> even lower than it already is, because the author wanted to include
> additional fields into the stock that would make 7 no longer fit into
> a single cacheline. I feel like we are reaching some limits with that
> side as well, so the "unbounded" portion seems to be quite opportune.
> 
> But I believe those benefits that we would like to see are made better
> by this series, which makes the stocking transparent such that the new
> optimizations don't make the existing memcg charging code any more
> complex than it is. By abstracting away this mechanism, we can make sure
> that memcg just handles the logical charging semantics, and page_counter
> handles the implementation-level details of stock management.

Yes, these are all good arguments. Especially when clarified that this
leaf only feature. I have to say I was struggling to see how caching on
all levels of the hierarchy would work in practice.

> Personally I am a big fan of how try_charge_memcg looks after this
> series, what do you think? 
> 
> > Quite honestly I am still not convinced this move
> > is the right thing but I would like to fully understand the mechanics
> > first. Having discrepancies among different counters just sounds like a
> > potential source of future problems.
> 
> I hope that I can present a good argument for why this is not as
> concern as it may seem initially.
> 
> My first point is that the memory usage counters were always fuzzy, as
> specified in the memcg v1 documentation too: 
> 
> usage_in_bytes ... doesn't show 'exact' value of memory (and swap)
> usage, it's a fuzz value for efficient access.
> 
> Secondly, the raw value of the usage is actually always accurate. It is
> just the stock portion that overcharge the memory at times, making it
> seem like there is a discrepancy. Flushing the memcg stock immediately
> bring the counters to their real values (like what happens right now).
> 
> Finally, this is a change that is only limited to cgroup v1 users.

Right but is there any actual point in adding caching for memsw charges
now? If you have a way to opt-in to caching then you can simply disabled
that for memsw counter altogether. Fewer problems to deal with, explain
etc...

> This entire series actually has no effect at all on cgroup v2 users,
> which I think is a nice perk. Obviously I'm not out here trying to
> make cgroup v1 unusable, but I wonder if anyone is still using the
> legacy feature.
> 
> I hope this wasn't too much of a brain dump on you. Thanks for your
> feedback Michal. Please let me know if there is anything I can do to
> clarify my intent.
> 
> Would you prefer if we continud the discussion here, or if I sent out
> a new series with just the cover letter changed? (I don't anticipate
> any code changes, based on sashiko review or anything else that I see).

Let's just iron out things first.
-- 
Michal Hocko
SUSE Labs