Re: [PATCH] mm/list_lru: Account node removals in isolation helpers

[Dave Chinner <dgc@kernel.org>]

On Tue, Nov 03, 2026 at 12:50:53PM +0800, Kaitao Cheng wrote:
> From: Kaitao Cheng <chengkaitao@kylinos.cn>
> 
> Successful writeback calls zswap_entry_free(), which removes the entry
> from the LRU and decrements both the sublist and node item counts.
> shrink_memcg_cb() then returns LRU_REMOVED_RETRY, causing the LRU walker
> to decrement the node count again.
> 
> The affected call path before this change is:
> 
>   __list_lru_walk_one()
>     shrink_memcg_cb()
>       zswap_writeback_entry()
>         zswap_entry_free()
>           zswap_lru_del()
>             list_lru_del()
>               __list_lru_del()
>                 atomic_long_dec()  /* first node count decrement */
>       return LRU_REMOVED_RETRY
>     atomic_long_dec()              /* second node count decrement */

That's a bug in the zswap code. You shouldn't be calling
list_lru_del() from __list_lru_walk_one() context.

Walker callbacks are supposed to use list_lru_isolate() or
list_lru_isolate_move() to remove the item from the LRU under the
lru lock. i.e. the walk and remove are a single atomic operation,
accounted by the walker infrastructure.

LRU_REMOVED_RETRY is for walkers that need to do work -after- the
item has been removed from the LRU (e.g. reclaim work that can't run
under the lru lock). i.e. the pattern for the callback code is
remove-unlock-reclaim-LRU_REMOVED_RETRY.

What the zswap code is doing is
unlock-reclaim-lock-remove-unlock-LRU_REMOVED_RETRY. That's
problematic, as we can have concurrent walkers, and so it's possible
that two shrinker invocations try to remove the same item at the
same time....

.... oh, what a nasty hack.

The zswap code is doing a list_move_tail() on the LRU object to
rotate it to the end of the LRU list before dropping the LRU lock,
in the hope that a concurrent scanner won't trip over it. That 
doesn't fix anything...

> During the mailing list discussion, Johannes Weiner suggested making
> users of the isolation interface responsible for maintaining the item
> counts, rather than having the walker decrement the node count based
> on the callback's return value.

urk. How is smearing the LRU NUMA abstraction into all the isolation
interfaces an improvement?

It seems to me that that this LRU hacking is all because of this:

         * 2. Usually, objects are taken off the LRU for reclaim. In
         *    this case this isn't possible, because if reclaim fails
         *    for whatever reason, we have no means of knowing if the
         *    entry is alive to put it back on the LRU.

And so to work around this inability of zswap to handle failure
sanely, the LRU list the reclaim code is being abused. And now the
abuse of the lru list code is triggering problems with the LRU
infrastructure, the suggestion is to push internal LRU structures
into all LRU list walkers to allow racy (and IMO broken) walker
behaviour?

I'd much prefer the zswap code get fixed than to have to modify a
dozen other subsystems to enable zswap to work around broken reclaim
code...

-Dave.
-- 
Dave Chinner
dgc@kernel.org