Re: [PATCH v3 1/4] mm/zswap: Make shrink_worker writeback cursor per-memcg

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

On 2026/6/3 07:19, Yosry Ahmed wrote:
>>>>>> Proactive writeback also wants a similar per-memcg cursor that is
>>>>>> scoped to the specified memcg, so that repeated invocations against
>>>>>> the same memcg make forward progress across its descendant memcgs
>>>>>> instead of restarting from the first child memcg each time.
>>>>>
>>>>> Is this a problem in practice?
>>>>>
>>>>> Is the concern the overhead of scanning memcgs repeatedly, or lack of
>>>>> fairness? I wonder if we should just do writeback in batches from all
>>>>> memcgs, similar to how reclaim does it, then evaluate at the end if we
>>>>> need to start over?
>>>>>
>>>>
>>>> Not using a per-cgroup cursor will cause issues for "repeated small-budget
>>>> calls" cases. For example, repeatedly triggering a 2MB writeback might
>>>> result in only writing back pages from the first few child memcgs every
>>>> time. In the worst-case scenario (where the writeback amount is less than
>>>> WB_BATCH), it might only ever write back from the first child memcg.
>>>
>>> Right, so a fairness concern?
>>>
>>> I wonder if we should just reclaim a batch from each memcg, then check
>>> if we reached the goal, otherwise start over. If the batch size is small
>>> enough that should work?
>>
>> Even with a small batch size, for small writeback requests triggered by
>> user-space (e.g., 2MB, which is batch size * N), it might still repeatedly
>> write back from only the first N child memcgs.
> 
> Yes, I understand, I am asking if this is a problem in practice. For
> this to be a problem we'd need to trigger small writeback requests and
> have many memcgs.
> 
>> This could cause the user-space agent to prematurely give up on zswap
>> writeback.
> 
> Why? The kernel should not return before trying to writeback from all
> memcgs. If we scan the first N child memcgs and did not writeback
> enough, we should keep going, right?
> 

Yes, this issue is not caused by the kernel, but rather by our 
user-space agent itself.

For instance, suppose a parent memcg has two children, memcg1 and 
memcg2, each with 200MB of zswap (100MB inactive). Triggering proactive 
writeback on the parent memcg will exhaust memcg1's inactive zswap 
pages. After that, even though memcg2 still has plenty of inactive zswap 
pages, it will continue to write back memcg1's active zswap pages. 
Writing back active zswap pages causes the user-space agent to 
prematurely abort the writeback because it detects that certain memcg 
metrics have exceeded predefined thresholds.

Of course, real-world scenarios are much more complex, and this kind of 
case is extremely rare in our environment.

That being said, your suggestion of using the global lock for the 
per-memcg cursors makes the writeback fairer and would resolve these 
corner cases.

>>> What if we do something like this (for the global cursor):
>>>
>>> 	do {
>>> 		memcg = xchg(zswap_next_shrink, NULL);
>>> 		memcg = mem_cgroup_iter(NULL, memcg, NULL);
>>> 		/* If the cursor was advanced from under us, try again */
>>> 		if (!try_cmpxchg(zswap_next_shrink, NULL, memcg))
>>> 			continue;
>>> 	} while (..);
>>> 			
>>>
>>
>> Regarding the code above, IIRC, both the global and per-cgroup cursors
>> suffer from race conditions. This race can cause mem_cgroup_iter(NULL, NULL,
>> NULL) to return the root memcg or its descendants, leading zswap to write
>> back pages from the wrong memcg.
> 
> Not the wrong memcg, it will just go back to the first memcg again,
> which should be fine as I mentioned below.
> 
>>
>> Additionally, since mem_cgroup_iter() puts the prev memcg ref and gets the
>> next memcg ref, a try_cmpxchg() failure on CPU1 might also lead to a ref
>> leak for memcg1.
>>
>>
>> 	CPU1                                       CPU2
>> memcg1 = xchg(pos, NULL)
>>                                 memcg2 = xchg(pos, NULL) memcg2 = NULL;
>>
>> memcg1 = mem_cgroup_iter()
>>                         mem_cgroup_iter(NULL, **NULL**, NULL) error memcg
>>                                  try_cmpxchg(pos，NULL，memcg2） succeed
>> try_cmpxchg(pos，NULL，memcg1） **fail**
> 
> Yes, we can probably just take a ref on the memcg before calling
> mem_cgroup_iter(). That being said, I think we can just keep the lock,
> see below.
> 
>>
>> I took a stab at implementing a cmpxchg()-based zswap_mem_cgroup_iter()
>> modeled after mem_cgroup_iter(), and it actually doesn't look that complex
>> after all :)
> 
> I don't think we should re-implement mem_cgroup_iter() here.
> 
> [..]
>>> There is a window where a racing shrinker will see the cursor as NULL
>>> and start over, but that should be fine. We can generalize this for the
>>> per-memcg cursor.
>>>
>>> That being said..
>>>
>>>>
>>>> Currently, this lock is only used in shrink_memcg(), proactive writeback,
>>>> and mem_cgroup_css_offline(). Note that shrink_memcg() only acquires the
>>>> lock of the root cgroup, and mem_cgroup_css_offline() is unlikely to be a
>>>> hot path.
>>>
>>> ..this made me realize it's probably fine to just use a global lock for
>>> now?
>>>
>>> IIUC the only additional contention to the existing lock will be from
>>> userspace proactive writeback, and that shouldn't be a big deal
>>> especially with the critical section being short?
>>>
>>
>> In the current patch implementation, this lock protects the cgroup's own
>> cursor variable. During each writeback, we only acquire the spin_lock of the
>> target cgroup itself; we do not attempt to **spin on any child cgroup's lock
>> while iterating through the descendants**.
> 
> Oh, I did not say anything about the current patch adding contention. I
> am suggesting we just keep using the global lock for the per-memcg
> cursors, if we keep them.
> 
> Right now, without this series, the global lock protects against
> concurrent changes to the global cursor from concurrent shrinkers. After
> the series, the only added contenders are userspace proactive writeback
> threads. Unless you have 10s or 100s of those, it should be fine to keep
> a single global lock, right?
> 

Ah yes, sorry about that, I misunderstood what you meant. Thanks a lot 
for the suggestion and for taking the time to explain it so patiently. 
I'll switch to using the global lock in v4 patch.


> Yes, userspace can affect writeback efficiency, but we can split the
> lock when it actually causes a problem.

Agreed.

> 
>>
>>
>>>>
>>>> So, should we keep the spin_lock or go with the cmpxchg() approach?
>>>> Yosry and Nhat, what are your thoughts on this?
>>>
>>> I think we should experiment with the global lock first. See if you
>>> observe any regressions with workloads that put a lot of pressure on the
>>> lock (a lot of threads in reclaim doing writeback + a few userspace
>>> threads doing proactive writeback). See if the userspace threads
>>> actually cause a meaningful regression.
>>
>> Sorry, it seems there are some implementation issues with the global lock
>> approach.
>>
>> In practice, our user-space agent mostly operates in the following two
>> scenarios:
>>   - Triggering proactive writeback on the same cgroup at different times
>> (sequentially).
>>   - Triggering proactive writeback on different cgroups at the same time
>> (concurrently).
>>
>> In both cases, there is no lock contention. So, the current lock works
>> perfectly fine for us.
> 
> Would using the existing global lock work for your use case? How many
> different cgroups can you end up reclaiming from concurrently?


It should work fine. We typically only have a dozen or so user-space 
agents triggering zswap writeback, and the critical section is very 
short anyway. I will implement this next.


Thanks,
Hao