Re: [PATCH v5 4/6] mm/zswap: Implement proactive writeback

[Hao Jia]

On 2026/7/1 00:10, Yosry Ahmed wrote:
> > > Before going through more versions we need to figure out if this will
> > > pivot to be a proactive demotion interfcae for swap tiering.
> >
> > Yes. Should I drop patches 4-6 in the next version and wait for swap
> > tiering to be finalized?
> > We can try to get the non-memcg parts (patches 1-3) merged upstream
> > first. This would also give them plenty of time to bake and catch any
> > potential regressions. Thoughts?
>
> Patches 1-2 can be sent and merged separately, yes. For patch 2,
> please include some numbers for the writeback performance before and
> after batching.
>
> Patch 3 does refactoring in preparation for patch 4, so I don't think
> it makes sense on its own.

Will do.

> > > > +int zswap_proactive_writeback(struct mem_cgroup *memcg, u64 bytes_to_writeback)
> > > > +{
> > > > +    struct zswap_shrink_state s = {};
> > > > +    struct mem_cgroup *iter = NULL;
> > > > +    u64 bytes_written = 0;
> > > > +    int ret = 0;
> > > > +
> > > > +    if (!memcg)
> > > > +            return -EINVAL;
> > >
> > > Can this ever happen? It would be a bug in the caller.
> >
> > IIRC，Writing the following to the NUMA node sysfs entry triggers this
> > check:
> > echo "10M source=zswap" > /sys/devices/system/node/nodeN/reclaim
>
> Oh yeah, I forgot about that one :)
>
> If we keep this, probably combine the !memcg and writeback check below.

Will do.
> > > > +    if (!mem_cgroup_zswap_writeback_enabled(memcg))
> > > > +            return -EINVAL;
> > > > +    if (!bytes_to_writeback)
> > > > +            return 0;
> > >
> > > Do we need this? I think the loop will just never enter and
> > > mem_cgroup_iter_break() will do nothing.
> >
> > Will do.
> > > > +
> > > > +    while (bytes_written < bytes_to_writeback) {
> > > > +            long shrunk;
> > > > +
> > > > +            cond_resched();
> > > > +
> > > > +            if (signal_pending(current)) {
> > > > +                    ret = -EINTR;
> > > > +                    break;
> > > > +            }
> > > > +
> > > > +            /*
> > > > +             * Use a local iterator to walk the memcg and its online descendants
> > > > +             * in a round-robin manner. Upon exiting the loop, mem_cgroup_iter_break()
> > > > +             * must be called to drop the iterator reference.
> > > > +             */
> > > > +            do {
> > > > +                    iter = mem_cgroup_iter(memcg, iter, NULL);
> > > > +            } while (iter && !mem_cgroup_tryget_online(iter));
> > > > +
> > > > +            shrunk = zswap_shrink_one_memcg(iter, &s);
> > > > +            if (shrunk > 0)
> > > > +                    bytes_written += shrunk;
> > > > +
> > > > +            /* drop the extra reference taken by mem_cgroup_tryget_online() */
> > > > +            mem_cgroup_put(iter);
> > >
> > >
> > > Can we just use mem_cgroup_online() instead since mem_cgroup_iter()
> > > already graps a ref?
> > Will do.
>
> If you're looking for another cleanup to do, shrink_worker() should
> probably also use mem_cgroup_online() and avoid taking/dropping an
> extra ref :)

IIRC, this might not work because zswap_next_shrink is a global variable 
and is accessed outside the lock during reclamation.

Consider the following race condition between shrink_worker on CPU0 and 
zswap_memcg_offline_cleanup on CPU1:


          CPU0                            CPU1
spin_lock(zswap_shrink_lock)
memcg1 = mem_cgroup_iter()
  memcg1.ref = 1
  zswap_next_shrink = memcg1
spin_unlock(zswap_shrink_lock)
                              zswap_memcg_offline_cleanup()
                              spin_lock(zswap_shrink_lock)
                              css_put(zswap_next_shrink)
                              memcg1.ref = 0  <--

shrink_memcg(memcg1) *maybe UAF*

Thanks,
Hao