Re: [PATCH v4 1/6] mm/memcontrol: make lru_zone_size atomic and simplify sanity check

[Shakeel Butt]

On Tue, Sep 01, 2026 at 01:20:33PM +0800, Kairui Song wrote:
> On Tue, Sep 1, 2026 at 12:38 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
> >
> > On Mon, Aug 31, 2026 at 02:43:31AM +0800, Kairui Song via B4 Relay wrote:
> > > From: Kairui Song <kasong@xxxxxxxxxxx>
> > >
> > > commit ca707239e8a7 ("mm: update_lru_size warn and reset bad lru_size")
> > > introduced a sanity check to catch memcg counter underflow, which was
> > > more of a workaround for another bug: lru_zone_size is unsigned, so
> > > underflow wraps it around and returns an enormously large number, then
> > > the memcg shrinker loops almost forever as the calculated number of
> > > folios to shrink is huge. That commit also checked if a zero value
> > > matches the empty LRU list, so we have to hold the LRU lock, and
> > > handle the positive and negative deltas separately.
> > >
> > > But later commit b4536f0c829c ("mm, memcg: fix the active list aging
> > > for lowmem requests when memcg is enabled") already removed the LRU
> > > emptiness check, so handling the deltas separately is no longer
> > > needed. And if we just turn it into an atomic long, underflow isn't a
> > > big issue either,
> >
> > Why atomic long and not just long?
> 
> Long is fine too; I was just trying to be defensive. The original
> check was a defensive design for leaks or races, and atomic long is
> more immune to race conditions. See below for the other conditions.

Unless we really need it, let's stay with "long" for now. As you mentioned below
this is busy/hot path, no need to add overhead of atomic here.

> 
> > > So let's turn the counter into an atomic long and check at the reader
> > > side instead, which has a smaller overhead. The underflow correction
> > > is removed: a massive leak of the LRU size counter would indicate
> > > that something else has gone very wrong, and one should fix that
> > > leaking site instead. Besides, the updater-side sanity check is
> > > unlikely to catch the leaking site anyway: if a folio was removed
> > > without updating the counter while other folios remain on the LRU,
> > > the WARN only triggers much later, from a likely innocent callsite.
> >
> > Do you have any data to support your claim that updater-side sanity check is not
> > that useful? Also can you explain the motivation to move the check from the
> > update side to reader side?
> 
> I think I described that in the commit message. Commit ca707239e8a7
> introduced the original emptiness check. The goal was to fix an
> over-busy reclaim scenario: upon underflow, an unsigned long returns
> an extremely large value. The reclaimer calculates the page number to
> scan based on that value, and thus enters a busy loop trying to
> reclaim much more than the actual usage. It's really a defensive
> design or workaround.
> 
> And later commit b4536f0c829c just removed that emptiness check,
> leaving only the underflow check. So at this point we are just
> defending against underflow. Currently, I don't believe there are any
> known leaks or underflow issues in mainline, still purely a defensive
> design.
> 
> Reading these counters (mem_cgroup_get_zone_lru_size()) is much rare
> than updating them: the only readers are reclaimer - per loop, each
> loop will scan a large chunk of folios. Or during reparenting - per
> child.
> 
> Writers, however, operate on every LRU folio.
> 
> So checking it in the reader side is much lighter and cleaner code-wise.

So, one motivation is cleaner code?

> 
> Checking in the reader side also tolerates deferred accounting false
> positives: if a folio is added to / removed from LRU but have the
> accounting happens later, that's fine, but might cause a temporary
> underflow (there shouldn't be such usage right now, but cleanly the
> oldest commit I mentioned is trying to catch something). It should
> just proceed without performing extra operations on the counter or
> raising a warning on the updater side.
> 
> The underflow won't affect anything besides the two readers mentioned
> above (reclaimer and reparenting).
> 
> Besides, I don't think warning on the writer side will help catch the
> real leak either, it just adds overhead to a busy path. The folio
> triggering the underflow is very unlikely to be the one leaking the
> count. And if that leak is actually caused by a race, the atomic long
> change should cure it.

I think you are going and looking too much into the history of this warning. I
think you just need to explain that the current warning in the update side
already can't always catch the actual leak and is costly. Moving to read side
make the code cleaner and warnings will be at the location where the consumers
we care about are going to consume it. Also add couple of sentences on behavior
difference it will have i.e. warning on flipping into negative vs we will
continue to operate even in negative but only warn on negative consumption.

Another question: why is this patch part of this series? Just some orthogonal
cleanups or are they related?