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

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Mon, 2 Nov 2026 17:16:12 +0100 Michal Hocko <mhocko@suse.com> wrote:

> > > > > > 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?

It's not a big change, actually. This was the implementation from the
origianl RFC of the series:

https://lore.kernel.org/all/20260410210742.550489-8-joshua.hahnjy@gmail.com/

The idea is that you treat a cgroup as a leaf until it becomes a parent,
at which point you drain the stock. Stock can land on the parent due
to hierarchy walks, but I can also write another part to this that
prevents stock from getting cached when a cgroup has children.

> > 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.

Sounds good. If it makes sense to you then, I can fold in the
leaf-only caching into this series to maintain more parity with the
existing code. How does that sound? 

> > 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...

I think the problem actually still exists, unfortunately. In fact I
think the issue becomes bigger if you remove stocking for memsw.

Let's say only memory gets stock. Then on a single page charge for a
new cgroup:

memory page_counter charges 64 pages to refill the stock, memsw only
charges 1 page to fulfill the current allocation. Then you have
memory: 64 pages
memsw: 1 page

And the invariant that memory <= memsw is immediately broken.

Unfortunately only stocking for memsw is not a solution as well since
successful memsw charges still charge memory and will therefore go
through a hierarchy walk.

Actually maybe then, the solution is just to remove stocking for
cgroup v1 users? Does that sound like a fair tradeoff to you?

> > 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.

Sounds good to me. Thanks!