Re: [PATCH v2 2/2] mm/zswap: Support batch writeback in shrink_memcg()

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

On 2026/7/24 00:39, Yosry Ahmed wrote:
> On Thu, Jul 23, 2026 at 6:55 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>>
>> On Wed, Jul 22, 2026 at 09:52:18PM -0700, Yosry Ahmed wrote:
>>> On Wed, Jul 22, 2026 at 7:27 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>>>>
>>>> On Fri, Jul 17, 2026 at 04:51:51PM +0800, Hao Jia wrote:
>>>>> @@ -1369,7 +1402,7 @@ static void shrink_worker(struct work_struct *w)
>>>>>                        goto resched;
>>>>>                }
>>>>>
>>>>> -             ret = shrink_memcg(memcg);
>>>>> +             ret = shrink_memcg(memcg, NR_ZSWAP_WB_BATCH);
>>>>>                /* drop the extra reference */
>>>>>                mem_cgroup_put(memcg);
>>>>>
>>>>> @@ -1493,7 +1526,7 @@ bool zswap_store(struct folio *folio)
>>>>>        objcg = get_obj_cgroup_from_folio(folio);
>>>>>        if (objcg && !obj_cgroup_may_zswap(objcg)) {
>>>>>                memcg = get_mem_cgroup_from_objcg(objcg);
>>>>> -             if (shrink_memcg(memcg)) {
>>>>> +             if (shrink_memcg(memcg, 1)) {
>>>>
>>>> Why 64 for the global limit but only 1 for the cgroup limit? That
>>>> seems arbitrary in multiple ways.
>>>
>>> I suggested that we keep the writeback here without batching and do
>>> that change separately, mainly out of abundance of caution as
>>> writeback is done synchronously here so the extra latency could be
>>> problematic. I think we probably want to measure the performance
>>> impact of that separately.
>>>
>>> That being said, this path is potentially too expensive anyway due to
>>> the flush, but I would rather we do some basic measurements before
>>> batching here.
>>>
>>> What do you think?
>>
>> It's not an unknown, right? We know this works for direct reclaimers,
>> cgroup limit reclaim e.g., and what the latency implications are.
>>
>> Because of how reclaim works, we also know it'll call zswap_store() in
>> batches of SWAP_CLUSTER_MAX. If we don't batch here, they're likely to
>> each call shrink_memcg() once we're at the limit - while still risking
>> rejections due to compressibility differences.
>>
>> My worry is that if we start with an inconsistency, we'll be stuck
>> with it for a long time.
>>
>> I'd rather start with the clean, consistent version. Dial it back only
>> if we have data to justfiy the complication that we can put into a
>> comment and the changelog that outlines why exactly it's different.
> 
> I am fine with doing that and basically always using NR_ZSWAP_WB_BATCH
> as the batch size in shrink_memcg(), but I would be more comfortable
> if we did some sanity testing.
> 
> Hao, would you be able to do some smoke testing with NR_ZSWAP_WB_BATCH
> used for all paths, and memory.zswap.max set in a way that induces
> writeback? You can probably set memory.zswap.max to 1% of total memory
> instead of the global pool limit and rerun the same test.

Building on Test Case 2, I set zswap.max=320M (~1% of total system 
memory) and updated both invocation paths of shrink_memcg() to process 
batches of 32 or 64. The resulting benchmark data is shown below.
(Note: Test Case 2 also sets max_pool_percent=1.)

                       baseline-cgroup   batch-all-32-cgroup 
batch-all-64-cgroup
shrink_worker wakeups        7,238             766               367
shrink_memcg calls      12,059,142       1,961,194           983,878
written_back                28,277         301,157           327,997
zswap_store calls         1,349,572       1,168,190         1,114,549
   store succeeded            492,861         521,315           459,246
   store rejected             856,712         646,875           655,303
   store reject rate            ~63%           ~55%              ~58%
pool_limit_hit              510,130          50,096            57,715
pswpout                     884,989         948,032           983,300
pswpin                    1,251,268       1,638,668         1,878,453

Thanks,
Hao