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

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

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