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

[Hao Jia]

On 2026/7/24 00:39, Yosry Ahmed wrote:
> On Thu, Jul 23, 2026 at 6:55 AM Johannes Weiner <hannes@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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