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

[Michal Hocko]

On Mon 26-10-26 08:59:37, Joshua Hahn wrote:
> On Mon, 26 Oct 2026 10:03:58 +0100 Michal Hocko <mhocko@xxxxxxxx> wrote:
> 
> > [Sorry for coming to this really late]
> 
> Hi Michal, no worries at all : -) Thanks for taking a look!
> 
> > On Wed 21-10-26 15:55:09, Joshua Hahn wrote:
> > > v6 --> v7
> > > =========
> > > - Preserve two cgroup v1 proprerties that previously followed implicitly
> > >   from sharing a stock:
> > >   - memory.usage_in_bytes <= memory.memsw.usage_in_bytes
> > >   - Reclaim does not swap when memory and memsw have equal limits.
> > > - Wordsmithing commit messages for 4/5 and 5/5 to document the
> > >   cgroupv1 divergence (bounded) between memory and memsw caused by
> > >   independent stock.
> > > 
> > > INTRODUCTION
> > > ============
> > > Memcg keeps a per-CPU stock of precharged pages so that small, frequent
> > > allocations do not walk the page_counter hierarchy every time.
> > > Today, the stock implementation is within memcontrol code, even though
> > > the operation it caches is a page_counter charge. This makes it
> > > difficult to add new page_counters to a memcg and preserve the fast
> > > path behavior.
> > > 
> > > This matters for future work like my tiered memcg limits series [1]
> > > which introduces multiple new page_counters to memcg. Without making
> > > stock a page_counter-level property, it means that every memcg charge
> > > now goes through multiple page_counter hierarchy walks, instead of
> > > being able to cache these charges.
> > > 
> > > To make future page_counters scalable and performant, move stock from
> > > mem_cgroup to page_counter so that each page_counter can opt into its
> > > own per-CPU cache of pre-charged pages.
> > 
> > What does it mean that each counter can opt into its own per-cpu
> > caching?
> 
> So right now for cgroupv2, there's kind of a 1-1 mapping between
> page_counter and memcg. I say "kind of" because there still is a
> page_counter for swap, but it takes a completely different charging
> path (i.e. not try_charge_memcg).
> 
> This means that currently stocking charge for memcg is equivalent to
> stocking charge for the memcg->memory page_counter.

correct

> However, one of the series that I'm currently working on [1] adds new
> page_counters to struct memcg to track the usage of memory
> per-memcg, per-tier. The goal is to (1) track how much memory of a
> cgroup lives on which tier, and (2) to limit the usage so that a single
> cgroup doesn't use up all the fast tier memory alone, and shares it
> fairly with all the other cgroups on the host.

Right. I guess you have layed down your motivation quite well in your
original cover letter.

> When adding a new per-tier page_counter to struct mem_cgroup that gets
> charged for every memcg_charge, I cannot use the stock fastpath since
> the stock does not distinguish where the memory resides. It would
> essentialy boil down to this pattern:
> 
> try_charge_memcg
>    \__ page_couner_try_charge(memcg->tier[0])
>    \__ consume_stock()
> 
> Where every path still has to do a hierarchy charge to the tier it's
> charging to, whereas this patch gets us:
> 
> try_charge_memcg
>    \__ page_counter_try_charge(memcg->tier[0])
>           \__ consume_stock(memcg->tier[0])
>    \__ page_countertry_charge(memcg->memory)
>           \__ consume_stock(memcg->memory)
> 
> So that we can avoid the hierarchy walk for most charges.
> 
> I'd be happy to submit a new cover letter which better explains this
> idea!

I guess I still do not understand what you mean by opting-in to the
caching. I believe you should focus less about what you plan to do and
rather explain what _this_ patchset achieves and how it works.
Also focus on describing why do we need to keep caches on all layers of
the hierarchy.

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

-- 
Michal Hocko
SUSE Labs