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 12:40:33, Joshua Hahn wrote:
> > > > > 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.
> >
> > Yes, you are right. We do actually cache memsw through the charging path
> > which charges both mem and memsw with the batch.
> 
> Thanks, Michal! So it seems like we are in alignment on everything
> except the cgroup v1 implications for fuzziness. FWIW I think nothing
> really changes from the userspace POV. memsw and memory usage has
> always been fuzzy due to stocking, so all this series does is to
> change "why" and "when" the stocking becomes fuzzy.

Right. The discrepancy was not so obvious but I agree that this change
doesn't fundamentally change the picture.

> What do you think about this? The other alternative would be to remove
> stocking for cgroup v1 users completely. But I am not sure that is a
> good solution either. For the record, I tried making the stock tied
> together so that they refill together, but that was a nightmare in
> terms of added complexity.

Nah, that seems overreaching without much added value. While we do not
support new features in v1 we shouldn't cripple performance for those
users unless there is no other way around.

-- 
Michal Hocko
SUSE Labs