Re: [PATCH v4 1/5] mm/zswap: Extend shrink_memcg() writeback capability

[Hao Jia <jiahao.kernel@gmail.com>]

On 2026/6/25 00:57, Yosry Ahmed wrote:
>>
>> /*
>>    * Scan up to @nr_to_scan pages across the per-node zswap LRUs of @memcg
>>    * and write back the reclaimable ones.
>>    *
>>    * Since the second-chance algorithm rotates referenced entries to the
>>    * LRU tail, the per-node scan is capped at the current LRU length so
>>    * each entry is scanned at most once per call. It is up to the caller
>>    * to handle retries, deciding whether to scan the next memcg to complete
> 
> Nit: "whether to scan another memcg to complete.."

Will fix in the next version.

> 
>>    * the full iteration, or to rescan the current memcg to drain its zswap
>>    * entries.
>>    *
>>    * Return: The number of compressed bytes written back (>= 0), or -ENOENT
>>    * if @memcg has writeback disabled, is a zombie cgroup, or has empty
>>    * zswap LRUs.
>>    */
>> static long shrink_memcg(struct mem_cgroup *memcg, unsigned long nr_to_scan)
>> {
>>       struct zswap_shrink_walk_arg walk_arg = {
>>           .bytes_written = 0,
>>           .encountered_page_in_swapcache = false,
>>       };
>>       unsigned long nr_remaining = nr_to_scan;
>>       int nid;
>>
>>       if (!mem_cgroup_zswap_writeback_enabled(memcg))
>>           return -ENOENT;
>>
>>       /*
>>        * Skip zombies because their LRUs are reparented and we would be
>>        * reclaiming from the parent instead of the dead memcg.
>>        */
>>       if (memcg && !mem_cgroup_online(memcg))
>>           return -ENOENT;
>>
>>       for_each_node_state(nid, N_NORMAL_MEMORY) {
>>           unsigned long nr_to_walk;
>>
>>           /*
>>            * Cap the walk at the current LRU length to ensure each entry is
>>            * scanned at most once per call. Referenced entries are rotated
>>            * to the tail for a second chance, and this bound prevents them
>>            * from being revisited within a single call. Retries are left to
>>            * the caller, which can choose to rescan the current memcg or
>>            * move on to the next one.
>>            */
> 
> Nit: Make this more concise since it's already explained above.
> 

Will fix in the next version. Thanks a lot for the review!

Thanks,
Hao

> Otherwise this looks good to me, thank you!
> 
>>           nr_to_walk = min(nr_remaining,
>>                    list_lru_count_one(&zswap_list_lru, nid, memcg));
>>           if (!nr_to_walk)
>>               continue;
>>
>>           nr_remaining -= nr_to_walk;
>>           list_lru_walk_one(&zswap_list_lru, nid, memcg, &shrink_memcg_cb,
>>                     &walk_arg, &nr_to_walk);
>>           /* Return the unused share of the budget to the pool. */
>>           nr_remaining += nr_to_walk;
>>
>>           if (!nr_remaining)
>>               break;
>>       }
>>
>>       /* Nothing was scanned: every LRU under @memcg was empty. */
>>       if (nr_remaining == nr_to_scan)
>>           return -ENOENT;
>>
>>       return walk_arg.bytes_written;
>> }
>>