Re: [PATCH] mm/zswap: fix double decrement of the LRU node count on writeback

[Kaitao Cheng <kaitao.cheng@linux.dev>]

在 2026/10/29 05:10, Johannes Weiner 写道:
> On Wed, Oct 28, 2026 at 12:22:43PM -0700, Yosry Ahmed wrote:
>> On Wed, Oct 28, 2026 at 4:36 AM Kaitao Cheng <kaitao.cheng@linux.dev> 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.
>>
>> I wonder if we have a valid use case for adding LRU_REMOVED_COUNTED or
>> LRU_DELETED instead to simply avoid the decrement.
> 
> It bugs me that it's the walker, not the callback, that adjusts the
> count. That composes poorly with the normal list manipulation API.
> 
> How about pushing the accounting into the isolation callback, which
> can then simply use __list_lru_del()?
> 
> The tradeoff is that more things need to be passed into the isolation
> callback, but that seems reasonable. It does make the change noisy,
> however, and we might want something like Kaitao's change as a
> backportable fix first regardless.

I tried three approaches.

The first was "pushing the accounting into the isolation callback", as
Johannes Weiner suggested. However, as Johannes's example below also shows,
this would require extensive changes across multiple subsystems. I doubted
that such a broad change would be accepted, so I dropped this approach.

The second was to add a new flag to enum lru_status, as Yosry Ahmed suggested,
to handle cases like zswap where the callback has already decremented the
count. However, the incorrect lru->node[nid].nr_items value in zswap is only
an accounting issue: I could not find any code that reads this counter for
zswap. Changing a generic interface to fix an accounting error with no
apparent functional impact did not seem worthwhile.

The third approach is the one implemented in this patch. It keeps the changes
within zswap and fixes the accounting issue, so I think it is appropriate.

> Thoughts?
> 
> ---
> 
> diff --git a/drivers/android/binder/page_range.rs b/drivers/android/binder/page_range.rs
> index b0c797523bc2..157256e52dd0 100644
> --- a/drivers/android/binder/page_range.rs
> +++ b/drivers/android/binder/page_range.rs
> @@ -24,7 +24,7 @@
>  use kernel::{
>      bindings,
>      error::Result,
> -    ffi::{c_ulong, c_void},
> +    ffi::{c_int, c_ulong, c_void},
>      mm::{virt, Mm, MmWithUser},
>      new_mutex, new_spinlock,
>      page::{Page, PAGE_SHIFT, PAGE_SIZE},
> @@ -684,8 +684,10 @@ fn drop(self: Pin<&mut Self>) {
>  /// Called by the shrinker.
>  #[no_mangle]
>  unsafe extern "C" fn rust_shrink_free_page(
> +    lru: *mut bindings::list_lru,
> +    l: *mut bindings::list_lru_one,
>      item: *mut bindings::list_head,
> -    lru: *mut bindings::list_lru_one,
> +    nid: c_int,
>      _cb_arg: *mut c_void,
>  ) -> bindings::lru_status {
>      // Fields that should survive after unlocking the lru lock.
> @@ -728,7 +730,7 @@ fn drop(self: Pin<&mut Self>) {
>          };
>  
>          // SAFETY: The item is in this lru list, so it's okay to remove it.
> -        unsafe { bindings::list_lru_isolate(lru, item) };
> +        unsafe { bindings::__list_lru_del(lru, l, item, nid) };
>  
>          // SAFETY: Both pointers are in bounds of the same allocation.
>          page_index = unsafe { info.offset_from(inner.pages) } as usize;
> @@ -746,7 +748,7 @@ fn drop(self: Pin<&mut Self>) {
>      }
>  
>      // SAFETY: The lru lock is locked when this method is called.
>  >  ........
>  ........

-- 
Thanks
Kaitao Cheng