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

[Yosry Ahmed <yosry@kernel.org>]

> > > > > 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?

> > 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?

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

> 
> 
> > > 
> > > 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?

> 
> However, if we really hate zswap_wb_iter.lock, I can try replacing it with
> the cmpxchg() approach.
> 
> Thanks,
> Hao