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

[Kairui Song]

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.

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

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.