* [PATCH] cgroup: Migrate tasks to the root css when a controller is rebound
From: Tejun Heo @ 2026-06-01 19:02 UTC (permalink / raw)
To: cgroups, linux-kernel
Cc: Mark Brown, Bert Karwatzki, Johannes Weiner, Michal Koutný,
Sebastian Andrzej Siewior, Petr Malat, kernel test robot,
Martin Pitt, Aishwarya.TCV, Tejun Heo
In-Reply-To: <a9f6c0bcd262e764453b95eb7397871825e11559.camel@web.de>
cgroup_apply_control_disable() defers kill_css_finish() while a css is
still populated, relying on css_update_populated() to fire the deferred
kill once the populated count reaches zero.
This deadlocks when a controller is rebound out of a hierarchy. Mounting
an implicit_on_dfl controller such as perf_event as a v1 hierarchy steals
it off the default hierarchy, and rebind_subsystems() kills its
per-cgroup csses while they are still populated. The migration run in the
same step keeps the old css for a controller no longer in the hierarchy's
mask, so no task is migrated off the dying csses. Their populated count
never reaches zero, the deferred kill_css_finish() never fires, and the
next cgroup_lock_and_drain_offline() hangs forever under cgroup_mutex.
That migration is already a no-op pass over the rebound subtree. Add
cgroup_rebind_ss_mask so find_existing_css_set() resolves the leaving
controllers to the root css. Their tasks are migrated there, the
per-cgroup csses depopulate, and cgroup_apply_control_disable() kills
them synchronously. The deferral stays correct for the rmdir and
controller-disable paths it was meant for.
Fixes: 1dffd95575eb ("cgroup: Defer kill_css_finish() in cgroup_apply_control_disable()")
Reported-by: Mark Brown <broonie@kernel.org>
Closes: https://lore.kernel.org/all/41cd159c-54e5-45e0-81df-eaf36a6c028e@sirena.org.uk/
Reported-by: Bert Karwatzki <spasswolf@web.de>
Closes: https://lore.kernel.org/all/4e986b4ed7e16547805d54b6e67d09120bc4d2f2.camel@web.de/
Signed-off-by: Tejun Heo <tj@kernel.org>
---
Hello, and thanks a lot for all the reproduction information. It made this
much easier to track down.
Bert, Mark, would you mind giving this a try on your setups?
kernel/cgroup/cgroup.c | 35 +++++++++++++++++++++++++++++++----
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index bdc8deedb4f7..7f4861109e48 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -197,6 +197,14 @@ static u32 cgrp_dfl_implicit_ss_mask;
/* some controllers can be threaded on the default hierarchy */
static u32 cgrp_dfl_threaded_ss_mask;
+/*
+ * Set across rebind_subsystems() to the controllers leaving a hierarchy.
+ * Guarded by cgroup_mutex. Makes find_existing_css_set() resolve them to the
+ * root css so the affected tasks are migrated there before
+ * cgroup_apply_control_disable() kills the per-cgroup csses.
+ */
+static u32 cgroup_rebind_ss_mask;
+
/* The list of hierarchy roots */
LIST_HEAD(cgroup_roots);
static int cgroup_root_count;
@@ -1083,7 +1091,15 @@ static struct css_set *find_existing_css_set(struct css_set *old_cset,
* won't change, so no need for locking.
*/
for_each_subsys(ss, i) {
- if (root->subsys_mask & (1UL << i)) {
+ if (unlikely(cgroup_rebind_ss_mask & (1UL << i))) {
+ /*
+ * @ss is leaving this hierarchy and its per-cgroup
+ * csses are about to be killed. Resolve to the
+ * surviving root css so the tasks are migrated there.
+ */
+ template[i] = cgroup_css(&root->cgrp, ss);
+ WARN_ON_ONCE(!template[i]);
+ } else if (root->subsys_mask & (1UL << i)) {
/*
* @ss is in this hierarchy, so we want the
* effective css from @cgrp.
@@ -1853,11 +1869,17 @@ int rebind_subsystems(struct cgroup_root *dst_root, u32 ss_mask)
struct cgroup *scgrp = &cgrp_dfl_root.cgrp;
/*
- * Controllers from default hierarchy that need to be rebound
- * are all disabled together in one go.
+ * Controllers leaving the default hierarchy are disabled
+ * together. cgroup_rebind_ss_mask makes cgroup_apply_control()
+ * migrate their tasks to the root css, so the per-cgroup csses
+ * are unpopulated when cgroup_finalize_control() kills them.
+ * Clear it before cgroup_finalize_control(), which does no
+ * css_set lookup.
*/
cgrp_dfl_root.subsys_mask &= ~dfl_disable_ss_mask;
+ cgroup_rebind_ss_mask = dfl_disable_ss_mask;
WARN_ON(cgroup_apply_control(scgrp));
+ cgroup_rebind_ss_mask = 0;
cgroup_finalize_control(scgrp, 0);
}
@@ -1871,9 +1893,14 @@ int rebind_subsystems(struct cgroup_root *dst_root, u32 ss_mask)
WARN_ON(!css || cgroup_css(dcgrp, ss));
if (src_root != &cgrp_dfl_root) {
- /* disable from the source */
+ /*
+ * Disable from the source, migrating its tasks to the
+ * root css first (see cgroup_rebind_ss_mask).
+ */
src_root->subsys_mask &= ~(1 << ssid);
+ cgroup_rebind_ss_mask = 1 << ssid;
WARN_ON(cgroup_apply_control(scgrp));
+ cgroup_rebind_ss_mask = 0;
cgroup_finalize_control(scgrp, 0);
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v5 9/9] mm: switch deferred split shrinker to list_lru
From: Johannes Weiner @ 2026-06-01 18:17 UTC (permalink / raw)
To: Lance Yang
Cc: baolin.wang, akpm, david, ljs, shakeel.butt, mhocko, david,
roman.gushchin, muchun.song, qi.zheng, yosry.ahmed, ziy, liam,
usama.arif, kas, vbabka, ryncsn, zaslonko, gor, baohua, dev.jain,
npache, ryan.roberts, cgroups, linux-mm, linux-kernel
In-Reply-To: <20260601132135.14272-1-lance.yang@linux.dev>
On Mon, Jun 01, 2026 at 09:21:35PM +0800, Lance Yang wrote:
>
> On Wed, May 27, 2026 at 04:45:16PM -0400, Johannes Weiner wrote:
> [...]
> >diff --git a/mm/swap_state.c b/mm/swap_state.c
> >index 04f5ce992401..9c3a5cf99778 100644
> >--- a/mm/swap_state.c
> >+++ b/mm/swap_state.c
> >@@ -465,6 +465,16 @@ static struct folio *__swap_cache_alloc(struct swap_cluster_info *ci,
> > return ERR_PTR(-ENOMEM);
> > }
> >
>
> Shouldn't this be limited to anon swapin?
>
> e.g. vmf && vma_is_anonymous(vmf->vma)
>
> >+ if (order > 1 && folio_memcg_alloc_deferred(folio)) {
>
> __swap_cache_alloc() is also used by shmem direct swapin, so shmem can
> get here too when handling a large swap entry:
>
> shmem_get_folio_gfp()
> shmem_swapin_folio()
> shmem_swap_alloc_folio()
> swapin_sync()
> swap_cache_alloc_folio()
> __swap_cache_alloc()
> folio_memcg_alloc_deferred()
Good catch, I think you're right. I shouldn't have dismissed that
branch due to "/* Direct swapin skipping swap cache & readahead */"
> @Baolin please correct me if I got it wrong :)
>
> folio_memcg_alloc_deferred() itself doesn't filter shmem out either; it
> only allocates the memcg list_lru metadata for deferred_split_lru:
>
> int folio_memcg_alloc_deferred(struct folio *folio)
> {
> if (mem_cgroup_disabled())
> return 0;
> return folio_memcg_list_lru_alloc(folio, &deferred_split_lru, GFP_KERNEL);
> }
>
> Since deferred_split_lru only queues anon large folios, doing this for
> shmem swapin doesn't buy us anything :)
Yes, agreed. I don't think it's a big deal / show stopper in terms of
user-visible effect, but of course still worth fixing.
I'll send a follow-up patch.
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Nhat Pham @ 2026-06-01 18:06 UTC (permalink / raw)
To: Kairui Song
Cc: Liam.Howlett, akpm, apopple, axelrasmussen, baohua, baolin.wang,
bhe, byungchul, cgroups, chengming.zhou, chrisl, corbet, david,
dev.jain, gourry, hannes, hughd, jannh, joshua.hahnjy, lance.yang,
lenb, linux-doc, linux-kernel, linux-mm, linux-pm,
lorenzo.stoakes, matthew.brost, mhocko, muchun.song, npache,
pavel, peterx, peterz, pfalcato, rafael, rakie.kim,
roman.gushchin, rppt, ryan.roberts, shakeel.butt, shikemeng,
surenb, tglx, vbabka, weixugc, ying.huang, yosry.ahmed, yuanchu,
zhengqi.arch, ziy, kernel-team, riel, haowenchao22
In-Reply-To: <CAMgjq7BhOn48xEyC=2j837R7qddfjeBVHMiRqdx8no4ZEBpBLg@mail.gmail.com>
On Mon, Jun 1, 2026 at 10:45 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Mon, Jun 1, 2026 at 11:57 PM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > On Mon, Jun 1, 2026 at 12:34 AM Kairui Song <ryncsn@gmail.com> wrote:
> > >
> > > For the format part, PHYS don't need that much bits I think,
> > > so by slightly adjust the format vswap device could be share
> > > mostly the same format with ordinary device.
> > >
> > > For example typical modern system don't have a address space larger
> > > than 52 bit. (Even with full 64 bits used for addressing, shift it
> > > by 12 we get 52). Plus 5 for type, you get 57, so you can have a
> > > marker that should work as long as it shorter than 1000000 for PHYS,
> > > and shared for all table format since it's not in conflict with
> > > anything. You have also use a few extra bits so a single swap space
> > > can be 8 times larger than RAM space, and since we can help
> > > multiple swap type I think that should be far than enough?
> > >
> > > Then you have Shadow back at 001, and zero bit in shadow. The only
> > > special one is Zswap, which will be 100 now, and that's exactly the
> > > reserved pointer format in current swap table format, on seeing
> > > si->flags & VSWAP && is_pointer(swp_tb) you know that's zswap :)
> >
> > Are you suggesting we merge the virtual table with main swap table?
> >
> > Man, I'd love to do this. There is a problem though - we have a case
> > where we occupy both backing physical swap AND swap cache. Do you
> > think we can fit both the physical swap slot handle and the swap cache
> > PFN into the same slot in virtual table? Maybe with some expanding...?
>
> I don't really get why we would need to do that? If you put the PFN
> info in the virtual / upper layer, then the count info, locking, and
> all swap IO synchronization (via folio lock), dup (current protected
> by ci lock / folio lock), and allocation (folio_alloc_swap), are all
> handled in this layer.
>
> The physical / lower layer will just hold a reverse entry on
> folio_realloc_swap, or no entry at all (no physical layer used, zswap,
> or after swap allocation but before IO) right?
>
> Looking up the actual folio from the physical layer will be a bit
> slower since it needs to resolve the reverse entry, but the only place
> we need to do that is things like migrate, compaction (none of them
> exist yet) which seems totally fine?
All of this is correct, but consider swaping in a vswap entry backed
by pswap. There are cases where you still want to maintain the pswap
slots around backing vswap entry, while having the swap cache folio as
well.
For e.g, at swap in time, we add the folio into the swap cache. First
of all, we need to hold on to the physical swap slot for IO step. But
even after IO succeeds, there are cases where you would still like to
keep physical swap slots around (for e.g, to avoid swapping out again
if the folio is only speculatively fetched).
So you have to make sure we have space for both the physical swap
slot, and the swap cache folio's PFN at the same time for each vswap
entry. So we still need the vtable extension (well maybe the other
approach I mentioned could work, but I'm not 100% sure).
>
> > > This could be imporved by per-si percpu cluster. Both YoungJun's
> > > tiering and Baoquan's previous swap ops mentioned this is needed,
> > > and now vswap also need that. If the vswap is also a si, then it will
> > > make use of this too.
> >
> > Yeah I made the same recommendation when I review swap tier last week:
> >
> > https://lore.kernel.org/all/CAKEwX=N2XcMHN1jatppOk6wnmz-Shab5XMtTtzgYOzRvU_6YFw@mail.gmail.com/
> >
> > I like it, but yeah it will be complicated. That said, I think not
> > fixing the fast path for tiering/vswap will seriously restrict their
> > usefulness. We don't want to go back to the old swap allocator days :)
>
> Thanks. Not too complicated, actually our internal kernel
> implementation still using si->percpu cluster, and use a counter for
> the rotation and each order have a counter :P, it's a bit ugly but
> works fine. It still serves pretty well just like the global percpu
> cluster, YoungJun's previous per ci percpu cluster also still provides
> the fast path, many ways to do that.
Sounds like something that should be upstreamed? ;)
I was concerned that you might deem the overhead too high (so I came
up with the per-tier percpu caching), but honestly I like it a lot.
>
> >
> > >
> > > YoungJun posted this a few month before:
> > > https://lore.kernel.org/linux-mm/20260131125454.3187546-5-youngjun.park@lge.com/
> > >
> > > The concern is that some locking contention could be heavier, or maybe
> > > that's just a hypothetical problem though.
> >
> > I don't think it's hypothetical. At least with vswap, it's very easy
> > to get into a state where the shared per-cpu cache gets invalidated
> > constantly if phys swap and vswap allocation alternates (which is
> > actually very possible under heavy memory pressure), hammering the
> > slow paths...
>
> I mean if the per-cpu cache is moved to si level, then whoever enters
> the allocation path of a si will almost always get a stable percpu
> cache to use, even if the last used si changes.
>
> We might better have a more explicit per si fast path for that. The
> plist rotation should better be done in a different (will be even
> better if lockless) way.
Exactly! That's what I'm thinking too. Basically I'm special-casing
for vswap here, but if you're happy with generalizing this for every
si I'm happy with it.
>
> >
> > >
> > > >
> > > > - Runtime enable/disable of the vswap device. To be honest, I don't
> > > > know if there is a value in this. My preference is vswap can be
> > > > optimized to the point that any overhead is negligible. Failing that,
> > > > maybe we can come up with some simple heuristics that automatically
> > > > decides for users?
> > > >
> > > > In this RFC, CONFIG_VSWAP=y means the vswap device is always created at
> > > > boot, and CONFIG_VSWAP=n means the vswap device is never created. This
> > > > *might* be enough just on its own.
> > > >
> > > > Is a runtime knob (sysfs or sysctl) worth the complexity beyond
> > > > these heuristics? I'm not sure yet. Maintaining both cases
> > >
> > > I checked the code and I think it's not hard to do, patch 1 already
> > > handling the meta data dynamically, everything will still just work
> > > even if you remove vswap at runtime. The rest of patches need adaption
> > > but might not end up being complex, it other comments here
> > > are considered.
> >
> > Yeah, it's not terribily hard to do. I'm more wondering if it's worth
> > the effort, both for the implementer and the user :)
> >
> > As I said here, if we want vswap, just enable it at boot time and get
> > a vast (but dynamic) device. We can make it optional per-cgroup
> > through Youngjun's interface, and that would be good enough?
> >
> > >
> > > For patch 2, a few routines like vswap_can_swapin_thp seems not
> > > needed or should be moved to __swap_cache_alloc? VSWAP_FOLIO is
> > > same as swap cache folio check, which is already covered. Same for
> > > zero checking, and VSWAP_NONE which is same as swap count check
> > > I think. That way we not only save a lot of code, we also no
> > > longer need to treat vswap specially.
> >
> > Unfortunately, I think a lot of this complexity is still needed. Vswap
> > adds a new layer, which means new complications :)
> >
> > For instance, I think you still need vswap_can_swapin_thp. It
> > basically enforces that the backend must be something
> > swap_read_folio() can handle. That means:
> >
> > 1. No zswap.
> >
> > 2. No mixed backend.
>
> If mixed backend means phys vs zero vs zswap, then we already have
> part of that covered with the current swap cache except for the phys
> part (zswap part seems very doable with fujunjie's work).
> swap_cache_alloc_folio will ensure there is no mixed zerobit, it can
> be easily extended to ensure there is no mixed zswap as well
> (according to what I've learned from fujunjie's code). Similar logic
> for phys detection I think.
Yeah it's basically generalizing that check, and handle the case where
we can have indirection.
I mean I can open-code it, but it has to be there :) And I figure it
might be useful to check this opportunistically (at swap_pte_batch,
even if it's not guaranteed to be correct down the line) before we
even attempt to allocate a large folio etc. to avoid large folio
allocation.
>
> > > For memcg table allocation, on demand seems a good idea, and actually
> > > we are not far from there, I tried to generalize the
> > > alloc-then-retry-sleep-alloc in swap_alloc_table but still not generic
> > > enough I guess.. Good new is the allocation of the table is already
> > > kind of ondemand, just need to split the detection of these two kind
> > > of table.
> >
> > I have a prototype of this, but I have not tested, so I do not want to
> > send it out. :)
> >
> > TLDR is - I still want to record the memcg for vswap (just not charge
> > it towards the counter). So we still need memcg_table at both level,
> > generally - just not allocating until needed (basically if a physical
> > swap slot in the cluster is directly mapped into PTE). You can kinda
> > tell, since you pass the folio into the allocation path - with some
> > care you can distinguish between:
> >
> > 1. Virtual swap, or directly maped physical swap -> need memcg_table
> >
> > 2. Physical swap, backing vswap -> does not memcg_table.
> >
> > Another alternative is you can defer this allocation until the point
> > where you have to do the charging action. But then you have to be
> > careful with failure handling, and need to backoff ya di da di da.
> > Funsies.
> >
> > I think I did a mixed of these 2 strategies. Anyway, I'll include the
> > patch in v2 (if folks like this approach).
> >
> > >
> > > Mean while I also remember we once discussed about splitting the
> > > accounting for vswap / physical swap? If we went that approach we
> > > don't need to treat memcg_table specially.
> >
> > For the charging behavior, I already have a patch for it actually in
> > this series (just not the dynamic allocation of the memcg_table field
> > yet).
> >
> > Basically:
> >
> > 1. For vswap entry, not backed by phys swap: record swap memcg, hold
> > reference to pin the memcg, but not charging towards swap.current.
>
> Maybe you don't need to record memcg here since folio->memcg already
> have that info?
>
> I previously had a patch:
> https://lore.kernel.org/linux-mm/20260220-swap-table-p4-v1-7-104795d19815@tencent.com/
>
> The defers the recording of memcg, the behavior is almost identical to
> before, but charging & recording should be cleaner and you don't need
> to record memcg at allocation time hence maybe reduce the possibility
> of pinning a memcg. I didn't include that in P4 just to reduce LOC,
> maybe can be resent or included.
That works-ish when the folio is sitll in swap cache, but say if it's
vswap backed by zswap (and the swap cache folio has been reclaimed),
you need a place to store the memcg, no?
Just seems cleaner to centralize this info at vswap layer when it is
presented, for now anyway, rather than juggling this on a per-backend
basis.
>
> > 2. For phys swap backing vswap: charging towards swap.current, but
> > does not record the memcg in its memcg_table, nor does it hold
> > reference to memcg (its vswap entry holds the reference already)
> >
> > 2. For phys swap directly mapped to PTE: charges, records, and holds reference.
> >
> > The motivation here is I do not want vswap entry to shares the same
> > limit as phys swap counter. If we think of it as "infinite" or
> > "dynamic", it should not be capped at all, but even if it is charged,
> > it should be something separate.
>
> Good to know, it's not too messy to make everything dynamic, but if
> our ultimate goal is to account for them separately, maybe we can save
> some effort here.
It's not terrible, TBH.
^ permalink raw reply
* Re: [PATCH v2] mm/list_lru: drain before clearing xarray entry on reparent
From: Kairui Song @ 2026-06-01 17:50 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Johannes Weiner, Dave Chinner, Roman Gushchin,
Muchun Song, Qi Zheng, Meta kernel team, linux-mm, cgroups,
linux-kernel, Chris Mason, stable
In-Reply-To: <20260601161501.1444829-1-shakeel.butt@linux.dev>
On Tue, Jun 2, 2026 at 12:28 AM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> memcg_reparent_list_lrus() clears the dying memcg's xarray entry with
> xas_store(&xas, NULL) before reparenting its per-node lists into the
> parent. This opens a window where a concurrent list_lru_del() arriving
> for the dying memcg sees xa_load() == NULL, walks to the parent in
> lock_list_lru_of_memcg(), takes the parent's per-node lock, and calls
> list_del_init() on an item still physically linked on the dying
> memcg's list.
>
> If another in-flight thread holds the dying memcg's per-node lock at
> the same moment (another list_lru_del, or a list_lru_walk_one running
> an isolate callback), both threads modify ->next/->prev pointers on the
> same physical list under different locks. Adjacent items can corrupt
> each other's links.
>
> Fix it by reversing the order: reparent each per-node list and mark the
> child's list lru dead and then clear the xarray entry. Any concurrent
> list_lru op that finds the still-set xarray entry either takes the dying
> memcg's per-node lock (synchronizing with the drain) or sees LONG_MIN
> and walks to the parent, where the items now live.
>
> Fixes: fb56fdf8b9a2 ("mm/list_lru: split the lock to per-cgroup scope")
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reported-by: Chris Mason <clm@fb.com>
> Cc: stable@vger.kernel.org
> ---
> Changes since v1:
> - Use xa_erase_irq() instead of xa_erase() (Sashiko & Claude).
> - Added comment on CSS_DYING check in memcg_list_lru_alloc avoiding a new mlru
> allocation.
>
> mm/list_lru.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index dd29bcf8eb5f..d454bce9a78e 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -473,26 +473,29 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
> mutex_lock(&list_lrus_mutex);
> list_for_each_entry(lru, &memcg_list_lrus, list) {
> struct list_lru_memcg *mlru;
> - XA_STATE(xas, &lru->xa, memcg->kmemcg_id);
>
> /*
> - * Lock the Xarray to ensure no on going list_lru_memcg
> - * allocation and further allocation will see css_is_dying().
> + * css_is_dying() check in memcg_list_lru_alloc() avoids
> + * allocating a new mlru since CSS_DYING is already set for this
> + * memcg a rcu grace period ago.
> */
> - xas_lock_irq(&xas);
> - mlru = xas_store(&xas, NULL);
> - xas_unlock_irq(&xas);
> + mlru = xa_load(&lru->xa, memcg->kmemcg_id);
> if (!mlru)
> continue;
>
> /*
> - * With Xarray value set to NULL, holding the lru lock below
> - * prevents list_lru_{add,del,isolate} from touching the lru,
> - * safe to reparent.
> + * Reparent each per-node list and mark the child dead
> + * (LONG_MIN) before clearing xarray entry otherwise a
> + * concurrent list_lru_del() may corrupt the list if it arrives
> + * after xarray clear but before reparenting as
> + * lock_list_lru_of_memcg will acquire parent's lock while the
> + * item is still on child's list.
> */
> for_each_node(i)
> memcg_reparent_list_lru_one(lru, i, &mlru->node[i], parent);
>
> + xa_erase_irq(&lru->xa, memcg->kmemcg_id);
> +
> /*
> * Here all list_lrus corresponding to the cgroup are guaranteed
> * to remain empty, we can safely free this lru, any further
> --
> 2.53.0-Meta
>
>
Nice catch! Thanks a lot!
Reviewed-by: Kairui Song <kasong@tencent.com>
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Kairui Song @ 2026-06-01 17:49 UTC (permalink / raw)
To: Nhat Pham
Cc: Liam.Howlett, akpm, apopple, axelrasmussen, baohua, baolin.wang,
bhe, byungchul, cgroups, chengming.zhou, chrisl, corbet, david,
dev.jain, gourry, hannes, hughd, jannh, joshua.hahnjy, lance.yang,
lenb, linux-doc, linux-kernel, linux-mm, linux-pm,
lorenzo.stoakes, matthew.brost, mhocko, muchun.song, npache,
pavel, peterx, peterz, pfalcato, rafael, rakie.kim,
roman.gushchin, rppt, ryan.roberts, shakeel.butt, shikemeng,
surenb, tglx, vbabka, weixugc, ying.huang, yosry.ahmed, yuanchu,
zhengqi.arch, ziy, kernel-team, riel, haowenchao22
In-Reply-To: <CAKEwX=NNNf0KCZC0ph7VRW0gjnbXd4W5NKEaHM4XzPdN03Ek3A@mail.gmail.com>
On Tue, Jun 2, 2026 at 12:22 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Mon, Jun 1, 2026 at 8:56 AM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > On Mon, Jun 1, 2026 at 12:34 AM Kairui Song <ryncsn@gmail.com> wrote:
> > >
> > > On Thu, May 28, 2026 at 02:29:24PM +0800, Nhat Pham wrote:
> > > > III. Follow-ups:
> > > >
> > > > In no particular order (and most of which can be done as follow-up
> > > > patch series rather than shoving everything in the initial landing):
> > > >
> > > > - More thorough stress testing is very much needed.
> > > >
> > > > - Performance benchmarks to make sure I don't accidentally regress
> > > > the vswap-less case, and that the vswap's case performance is
> > > > good. I suspect I will have to port a lot of the
> > > > optimizations I implemented in v6 over here - some of the
> > > > inefficiencies are inherent in any swap virtualization, and
> > > > would require the same fix (for e.g the MRU cluster caching
> > > > for faster cluster lookup - see [8] and [9]).
> > >
> > > This could be imporved by per-si percpu cluster. Both YoungJun's
> > > tiering and Baoquan's previous swap ops mentioned this is needed,
> > > and now vswap also need that. If the vswap is also a si, then it will
> > > make use of this too.
>
> Oh and the MRU cluster caching I mentioned here is not the allocation
> caching. It's the lookup caching, basically to avoid doing the
> xa_load() to look up clusters for consecutive swap operations on the
> same vswap cluster (which is the common case with vswap). For v6, it
> massively reduces this indirection lookup overhead. Performance-wise
> it's an absolute winner, just more complexity (because I need to
> handle reference counting carefully).
Ah alright, that's interesting. And I think we can keep things simple
to start, since sensitive users is stil able tol use plain device this
way.
BTW maintaining MRU is also an overhead, I'm not sure if the lookup
pattern always follows that?
> I also just realized we'll induce the indirection overhead on
> allocation here too, even if the cached cluster still have slots for
> allocation, because we look up the cluster (which is basically free
> for static swap device, but not free for vswap devices). Might need to
> take care of that to maintain vswap performance (but it will then
> diverge from your existing code...).
That part should be indeed coverable by the si->percpu cluster though, I think.
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Kairui Song @ 2026-06-01 17:44 UTC (permalink / raw)
To: Nhat Pham
Cc: Liam.Howlett, akpm, apopple, axelrasmussen, baohua, baolin.wang,
bhe, byungchul, cgroups, chengming.zhou, chrisl, corbet, david,
dev.jain, gourry, hannes, hughd, jannh, joshua.hahnjy, lance.yang,
lenb, linux-doc, linux-kernel, linux-mm, linux-pm,
lorenzo.stoakes, matthew.brost, mhocko, muchun.song, npache,
pavel, peterx, peterz, pfalcato, rafael, rakie.kim,
roman.gushchin, rppt, ryan.roberts, shakeel.butt, shikemeng,
surenb, tglx, vbabka, weixugc, ying.huang, yosry.ahmed, yuanchu,
zhengqi.arch, ziy, kernel-team, riel, haowenchao22
In-Reply-To: <CAKEwX=PzMwXXgq=ULAkFD9UqMz+ewLqhKt+xdGxkV7OmA2QG6w@mail.gmail.com>
On Mon, Jun 1, 2026 at 11:57 PM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Mon, Jun 1, 2026 at 12:34 AM Kairui Song <ryncsn@gmail.com> wrote:
> >
> > For the format part, PHYS don't need that much bits I think,
> > so by slightly adjust the format vswap device could be share
> > mostly the same format with ordinary device.
> >
> > For example typical modern system don't have a address space larger
> > than 52 bit. (Even with full 64 bits used for addressing, shift it
> > by 12 we get 52). Plus 5 for type, you get 57, so you can have a
> > marker that should work as long as it shorter than 1000000 for PHYS,
> > and shared for all table format since it's not in conflict with
> > anything. You have also use a few extra bits so a single swap space
> > can be 8 times larger than RAM space, and since we can help
> > multiple swap type I think that should be far than enough?
> >
> > Then you have Shadow back at 001, and zero bit in shadow. The only
> > special one is Zswap, which will be 100 now, and that's exactly the
> > reserved pointer format in current swap table format, on seeing
> > si->flags & VSWAP && is_pointer(swp_tb) you know that's zswap :)
>
> Are you suggesting we merge the virtual table with main swap table?
>
> Man, I'd love to do this. There is a problem though - we have a case
> where we occupy both backing physical swap AND swap cache. Do you
> think we can fit both the physical swap slot handle and the swap cache
> PFN into the same slot in virtual table? Maybe with some expanding...?
I don't really get why we would need to do that? If you put the PFN
info in the virtual / upper layer, then the count info, locking, and
all swap IO synchronization (via folio lock), dup (current protected
by ci lock / folio lock), and allocation (folio_alloc_swap), are all
handled in this layer.
The physical / lower layer will just hold a reverse entry on
folio_realloc_swap, or no entry at all (no physical layer used, zswap,
or after swap allocation but before IO) right?
Looking up the actual folio from the physical layer will be a bit
slower since it needs to resolve the reverse entry, but the only place
we need to do that is things like migrate, compaction (none of them
exist yet) which seems totally fine?
> > This could be imporved by per-si percpu cluster. Both YoungJun's
> > tiering and Baoquan's previous swap ops mentioned this is needed,
> > and now vswap also need that. If the vswap is also a si, then it will
> > make use of this too.
>
> Yeah I made the same recommendation when I review swap tier last week:
>
> https://lore.kernel.org/all/CAKEwX=N2XcMHN1jatppOk6wnmz-Shab5XMtTtzgYOzRvU_6YFw@mail.gmail.com/
>
> I like it, but yeah it will be complicated. That said, I think not
> fixing the fast path for tiering/vswap will seriously restrict their
> usefulness. We don't want to go back to the old swap allocator days :)
Thanks. Not too complicated, actually our internal kernel
implementation still using si->percpu cluster, and use a counter for
the rotation and each order have a counter :P, it's a bit ugly but
works fine. It still serves pretty well just like the global percpu
cluster, YoungJun's previous per ci percpu cluster also still provides
the fast path, many ways to do that.
>
> >
> > YoungJun posted this a few month before:
> > https://lore.kernel.org/linux-mm/20260131125454.3187546-5-youngjun.park@lge.com/
> >
> > The concern is that some locking contention could be heavier, or maybe
> > that's just a hypothetical problem though.
>
> I don't think it's hypothetical. At least with vswap, it's very easy
> to get into a state where the shared per-cpu cache gets invalidated
> constantly if phys swap and vswap allocation alternates (which is
> actually very possible under heavy memory pressure), hammering the
> slow paths...
I mean if the per-cpu cache is moved to si level, then whoever enters
the allocation path of a si will almost always get a stable percpu
cache to use, even if the last used si changes.
We might better have a more explicit per si fast path for that. The
plist rotation should better be done in a different (will be even
better if lockless) way.
>
> >
> > >
> > > - Runtime enable/disable of the vswap device. To be honest, I don't
> > > know if there is a value in this. My preference is vswap can be
> > > optimized to the point that any overhead is negligible. Failing that,
> > > maybe we can come up with some simple heuristics that automatically
> > > decides for users?
> > >
> > > In this RFC, CONFIG_VSWAP=y means the vswap device is always created at
> > > boot, and CONFIG_VSWAP=n means the vswap device is never created. This
> > > *might* be enough just on its own.
> > >
> > > Is a runtime knob (sysfs or sysctl) worth the complexity beyond
> > > these heuristics? I'm not sure yet. Maintaining both cases
> >
> > I checked the code and I think it's not hard to do, patch 1 already
> > handling the meta data dynamically, everything will still just work
> > even if you remove vswap at runtime. The rest of patches need adaption
> > but might not end up being complex, it other comments here
> > are considered.
>
> Yeah, it's not terribily hard to do. I'm more wondering if it's worth
> the effort, both for the implementer and the user :)
>
> As I said here, if we want vswap, just enable it at boot time and get
> a vast (but dynamic) device. We can make it optional per-cgroup
> through Youngjun's interface, and that would be good enough?
>
> >
> > For patch 2, a few routines like vswap_can_swapin_thp seems not
> > needed or should be moved to __swap_cache_alloc? VSWAP_FOLIO is
> > same as swap cache folio check, which is already covered. Same for
> > zero checking, and VSWAP_NONE which is same as swap count check
> > I think. That way we not only save a lot of code, we also no
> > longer need to treat vswap specially.
>
> Unfortunately, I think a lot of this complexity is still needed. Vswap
> adds a new layer, which means new complications :)
>
> For instance, I think you still need vswap_can_swapin_thp. It
> basically enforces that the backend must be something
> swap_read_folio() can handle. That means:
>
> 1. No zswap.
>
> 2. No mixed backend.
If mixed backend means phys vs zero vs zswap, then we already have
part of that covered with the current swap cache except for the phys
part (zswap part seems very doable with fujunjie's work).
swap_cache_alloc_folio will ensure there is no mixed zerobit, it can
be easily extended to ensure there is no mixed zswap as well
(according to what I've learned from fujunjie's code). Similar logic
for phys detection I think.
> > For memcg table allocation, on demand seems a good idea, and actually
> > we are not far from there, I tried to generalize the
> > alloc-then-retry-sleep-alloc in swap_alloc_table but still not generic
> > enough I guess.. Good new is the allocation of the table is already
> > kind of ondemand, just need to split the detection of these two kind
> > of table.
>
> I have a prototype of this, but I have not tested, so I do not want to
> send it out. :)
>
> TLDR is - I still want to record the memcg for vswap (just not charge
> it towards the counter). So we still need memcg_table at both level,
> generally - just not allocating until needed (basically if a physical
> swap slot in the cluster is directly mapped into PTE). You can kinda
> tell, since you pass the folio into the allocation path - with some
> care you can distinguish between:
>
> 1. Virtual swap, or directly maped physical swap -> need memcg_table
>
> 2. Physical swap, backing vswap -> does not memcg_table.
>
> Another alternative is you can defer this allocation until the point
> where you have to do the charging action. But then you have to be
> careful with failure handling, and need to backoff ya di da di da.
> Funsies.
>
> I think I did a mixed of these 2 strategies. Anyway, I'll include the
> patch in v2 (if folks like this approach).
>
> >
> > Mean while I also remember we once discussed about splitting the
> > accounting for vswap / physical swap? If we went that approach we
> > don't need to treat memcg_table specially.
>
> For the charging behavior, I already have a patch for it actually in
> this series (just not the dynamic allocation of the memcg_table field
> yet).
>
> Basically:
>
> 1. For vswap entry, not backed by phys swap: record swap memcg, hold
> reference to pin the memcg, but not charging towards swap.current.
Maybe you don't need to record memcg here since folio->memcg already
have that info?
I previously had a patch:
https://lore.kernel.org/linux-mm/20260220-swap-table-p4-v1-7-104795d19815@tencent.com/
The defers the recording of memcg, the behavior is almost identical to
before, but charging & recording should be cleaner and you don't need
to record memcg at allocation time hence maybe reduce the possibility
of pinning a memcg. I didn't include that in P4 just to reduce LOC,
maybe can be resent or included.
> 2. For phys swap backing vswap: charging towards swap.current, but
> does not record the memcg in its memcg_table, nor does it hold
> reference to memcg (its vswap entry holds the reference already)
>
> 2. For phys swap directly mapped to PTE: charges, records, and holds reference.
>
> The motivation here is I do not want vswap entry to shares the same
> limit as phys swap counter. If we think of it as "infinite" or
> "dynamic", it should not be capped at all, but even if it is charged,
> it should be something separate.
Good to know, it's not too messy to make everything dynamic, but if
our ultimate goal is to account for them separately, maybe we can save
some effort here.
>
> >
> > > - Widen swap_info_struct->max to unsigned long. The vswap device's
> > > max is currently clamped to ALIGN_DOWN(UINT_MAX, SWAPFILE_CLUSTER)
> > > (~16 TiB) to fit in unsigned int. 16 TiB is small for vswap,
> > > especially when we're getting increasingly big machines memory-wise.
> >
> > This should be very easy to do, just replace unsigned int with
> > unsigned long, a lot of place to touch though :)
>
> Agree. I'm just lazy, and this sounds like a simple patch as a
> follow-up. This RFC is already 2000 LoCs - I do not want to burden
> reviewers with extra useless details :)
No problem, good to see progress here :)
^ permalink raw reply
* Re: [PATCH v3 1/4] mm/zswap: Make shrink_worker writeback cursor per-memcg
From: Nhat Pham @ 2026-06-01 17:08 UTC (permalink / raw)
To: Hao Jia
Cc: Yosry Ahmed, akpm, tj, hannes, shakeel.butt, mhocko, mkoutny,
chengming.zhou, muchun.song, roman.gushchin, cgroups, linux-mm,
linux-kernel, linux-doc, Hao Jia
In-Reply-To: <8c0e60e1-5713-69f0-a687-088c87e75764@gmail.com>
On Mon, Jun 1, 2026 at 4:07 AM Hao Jia <jiahao.kernel@gmail.com> wrote:
>
>
>
> On 2026/5/30 09:24, Yosry Ahmed wrote:
> > On Tue, May 26, 2026 at 07:45:58PM +0800, Hao Jia wrote:
> >> From: Hao Jia <jiahao1@lixiang.com>
> >>
> >> The zswap background writeback worker shrink_worker() uses a global
> >> cursor zswap_next_shrink, protected by zswap_shrink_lock, to round-robin
> >> across the online memcgs under root_mem_cgroup.
> >>
> >> 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.
>
> Similar to how memory reclaim uses mem_cgroup_iter() (via struct
> mem_cgroup_reclaim_iter) and the old shrink_worker() used
> zswap_next_shrink, we need a shared cursor here.
I think each proactive reclaim invocation just walk the entire subtree
for page reclaim right (see shrink_node_memcgs())? Would that be
acceptable for you?
I also wonder if we can at least make this structure dynamically
allocated... In a system, you only really invoke proactive reclaim
against a few target cgroups, no?
^ permalink raw reply
* Re: [PATCH v3 1/4] mm/zswap: Make shrink_worker writeback cursor per-memcg
From: Nhat Pham @ 2026-06-01 16:47 UTC (permalink / raw)
To: Hao Jia
Cc: Yosry Ahmed, akpm, tj, hannes, shakeel.butt, mhocko, mkoutny,
chengming.zhou, muchun.song, roman.gushchin, cgroups, linux-mm,
linux-kernel, linux-doc, Hao Jia
In-Reply-To: <CAKEwX=M5KiWc8ZZTEXCXtxeBrQho3Gs-JnKmBB=YNUkp=WXaKA@mail.gmail.com>
On Mon, Jun 1, 2026 at 9:44 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
>
> TBH, I think the spinlock is simpler at this point if we need to do
> all of this explanation to justify correctness of cmpxchg :)
>
> That said, if memcg folks feel like an extra spinlock per cgroup is a
> bit much, we can go with the cmpxchg() approach. Please include a FAT
> comment explains the compxchg() approach's nuance in the code though.
> Speaking from experience, I will forget why it is correct 2 months
> after the patch lands :)
Another alternative is - can we repurpose any lock here? Locks seem to
be per-lruvec or per-node, unfortunately, and we need something
per-cgroup hmmmm.
^ permalink raw reply
* Re: [PATCH v3 1/4] mm/zswap: Make shrink_worker writeback cursor per-memcg
From: Nhat Pham @ 2026-06-01 16:44 UTC (permalink / raw)
To: Hao Jia
Cc: Yosry Ahmed, akpm, tj, hannes, shakeel.butt, mhocko, mkoutny,
chengming.zhou, muchun.song, roman.gushchin, cgroups, linux-mm,
linux-kernel, linux-doc, Hao Jia
In-Reply-To: <8c0e60e1-5713-69f0-a687-088c87e75764@gmail.com>
On Mon, Jun 1, 2026 at 4:07 AM Hao Jia <jiahao.kernel@gmail.com> wrote:
>
>
>
> On 2026/5/30 09:24, Yosry Ahmed wrote:
> > On Tue, May 26, 2026 at 07:45:58PM +0800, Hao Jia wrote:
> >> From: Hao Jia <jiahao1@lixiang.com>
> >>
> >> The zswap background writeback worker shrink_worker() uses a global
> >> cursor zswap_next_shrink, protected by zswap_shrink_lock, to round-robin
> >> across the online memcgs under root_mem_cgroup.
> >>
> >> 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.
>
> Similar to how memory reclaim uses mem_cgroup_iter() (via struct
> mem_cgroup_reclaim_iter) and the old shrink_worker() used
> zswap_next_shrink, we need a shared cursor here.
>
>
> >>
> >> Naturally, group the cursor and its protecting spinlock into a
> >> zswap_wb_iter struct, and make it a member of struct mem_cgroup to
> >> realize per-memcg cursor management. Accordingly, shrink_worker() now
> >> uses the lock and cursor in root_mem_cgroup->zswap_wb_iter.
> >
> > If we really need to have per-memcg cursors (I am not a big fan), I
> > think we can minimize the overhead by making the cursor updates use
> > atomic cmpxchg instead of having a per-memcg lock.
> >
>
> Because mem_cgroup_iter() always calls css_put(&prev->css), we cannot
> simply update zswap_wb_iter.pos via cmpxchg() after calling it. Doing so
> could lead to a double css_put() issue on prev->css.
>
> Therefore, if we switch to the cmpxchg() approach, we wouldn't be able
> to reuse the existing mem_cgroup_iter() logic. We would have to write a
> new function similar to cgroup_iter(), and its implementation might end
> up looking a bit obscure/complex.
>
> 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.
>
> So, should we keep the spin_lock or go with the cmpxchg() approach?
> Yosry and Nhat, what are your thoughts on this?
TBH, I think the spinlock is simpler at this point if we need to do
all of this explanation to justify correctness of cmpxchg :)
That said, if memcg folks feel like an extra spinlock per cgroup is a
bit much, we can go with the cmpxchg() approach. Please include a FAT
comment explains the compxchg() approach's nuance in the code though.
Speaking from experience, I will forget why it is correct 2 months
after the patch lands :)
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Nhat Pham @ 2026-06-01 16:22 UTC (permalink / raw)
To: Kairui Song
Cc: kasong, Liam.Howlett, akpm, apopple, axelrasmussen, baohua,
baolin.wang, bhe, byungchul, cgroups, chengming.zhou, chrisl,
corbet, david, dev.jain, gourry, hannes, hughd, jannh,
joshua.hahnjy, lance.yang, lenb, linux-doc, linux-kernel,
linux-mm, linux-pm, lorenzo.stoakes, matthew.brost, mhocko,
muchun.song, npache, pavel, peterx, peterz, pfalcato, rafael,
rakie.kim, roman.gushchin, rppt, ryan.roberts, shakeel.butt,
shikemeng, surenb, tglx, vbabka, weixugc, ying.huang, yosry.ahmed,
yuanchu, zhengqi.arch, ziy, kernel-team, riel, haowenchao22
In-Reply-To: <CAKEwX=PzMwXXgq=ULAkFD9UqMz+ewLqhKt+xdGxkV7OmA2QG6w@mail.gmail.com>
On Mon, Jun 1, 2026 at 8:56 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Mon, Jun 1, 2026 at 12:34 AM Kairui Song <ryncsn@gmail.com> wrote:
> >
> > On Thu, May 28, 2026 at 02:29:24PM +0800, Nhat Pham wrote:
> > > Based on: mm-unstable @ 444fc9435e57 + swap-table phase IV v5 [2].
> > >
> > > I manually adapted Kairui's ghost device implementation (from [4])
> > > for my vswap device. I've credited him as Co-developed-by on Patch I
> > > since a substantial portion of the dynamic-cluster infrastructure is
> > > his (I did propose the idea of using xarray/radix tree for dynamic
> > > swap clusters allocation and management though :P).
> > >
> > > >From here on out, for simplicity, I will refer to swap table phase IV
> > > as "P4", and the older v6 virtual swap space implementation as "v6".
> > >
> >
> > ...
> >
> > >
> > > This series reimplements the virtual swap space concept (see [1])
> > > on top of Kairui Song's swap table infrastructure, on top of [2]
> > > and in accordance with his proposal in [3]. The proposal's idea
> > > is interesting, so I decided to give it a shot myself. I'm still not
> > > 100% sure that this is bug-proof, but hey, it compiles, and has
> > > not crashed in my simple stress testing :)
> > >
> > > The prototype here is feature-complete relative to the swap-table P4
> > > baseline — swapout, swapin, freeing, swapoff, zswap writeback, zswap
> > > shrinker, memcg charging, and THP swapin all work for
> > > both vswap and direct-physical entries — and satisfies all three
> > > requirements above: no backend coupling (zswap/zero entries hold no
> > > physical slot), dynamic swap space (clusters allocated on demand via
> > > xarray, no static provisioning), and efficient backend transfer
> > > (in-place vtable updates, no PTE/rmap walking).
> > >
> > > II. Design
> > >
> > > With vswap, pages are assigned virtual swap entries on a ghost device
> > > with no backing storage. These entries are backed by zswap, zero pages,
> > > or (lazily) physical swap slots. Physical backing is allocated only
> > > when needed — on zswap writeback or reclaim writeout, after the rmap
> > > step.
> > >
> > > Compared to the standalone v6 implementation [1], which introduces a
> > > 24-byte per-entry swap descriptor and its own cluster allocator, this
> > > edition uses swap_table infrastructure, and share a lot of the allocator
> > > logic. Per-slot metadata is stored in a tag-encoded virtual_table
> > > (atomic_long_t, 8 bytes per slot), and physical clusters store
> > > Pointer-tagged rmap entries in the swap_table for reverse lookup back to
> > > the virtual cluster.
> > >
> > > Here are some data layout diagrams:
> > >
> > > Case 1: vswap entry (virtualized)
> > >
> > > PTE swap_cluster_info_dynamic
> > > vswap_entry +-------------------------+
> > > (swp_entry_t) ------>| swap_cluster_info (ci) |
> > > | +--------------------+ |
> > > | | swap_table | |
> > > | | PFN / Shadow | |
> > > | | memcg_table | |
> > > | | count,flags,order | |
> > > | | lock, list | |
> > > | +--------------------+ |
> > > | |
> > > | virtual_table |
> > > | +--------------------+ |
> > > | | NONE | |
> > > | | PHYS | |
> > > | | ZERO | |
> > > | | ZSWAP(entry*) | |
> > > | | FOLIO(folio*) | |
> > > | +--------------------+ |
> > > +-------------------------+
> > > |
> > > | PHYS resolves to
> > > v
> > > PHYSICAL CLUSTER (swap_cluster_info)
> > > +--------------------------+
> > > | swap_table per-slot: |
> > > | NULL - free |
> > > | PFN - cached folio |
> > > | Shadow - swapped out |
> > > | Pointer- vswap rmap |
> > > | Bad - unusable |
> > > | |
> > > | Vswap-backing slot: |
> > > | Pointer(C|swp_entry_t) |
> > > | rmap back to vswap |
> > > +--------------------------+
> > >
> > > Case 2: direct-mapped physical entry (no vswap)
> > >
> > > PTE PHYSICAL CLUSTER (swap_cluster_info)
> > > phys_entry +--------------------------+
> > > (swp_entry_t) ------>| swap_table per-slot: |
> > > | NULL - free |
> > > | PFN - cached folio |
> > > | Shadow - swapped out |
> > > | Bad - unusable |
> > > +--------------------------+
> > >
> > > struct swap_cluster_info_dynamic {
> > > struct swap_cluster_info ci; /* swap_table, lock, etc. */
> > > unsigned int index; /* position in xarray */
> > > struct rcu_head rcu; /* kfree_rcu deferred free */
> > > atomic_long_t *virtual_table; /* backend info, 8 B/slot */
> > > };
> > >
> > > Each vswap cluster (swap_cluster_info_dynamic) extends the classic
> > > swap_cluster_info struct with a virtual_table array that stores the
> > > backend information for each virtual swap entry in the cluster. Each
> > > entry is tag-encoded in the low 3 bits to indicate backend types:
> > >
> > > NONE: |----- 0000 ------|000| free / unbacked
> > > PHYS: |-- (type:5,off:N)|001| on a physical swapfile (shifted)
> > > ZERO: |----- 0000 ------|010| zero-filled page
> > > ZSWAP: |--- zswap_entry* |011| compressed in zswap
> > > FOLIO: |--- folio* ------|100| in-memory folio
> >
> > Thanks for trying this approach!
>
> Thanks for the suggestions. I hope going forward we have sth concrete
> to tinker with, rather than abstractions :P
>
> >
> > For the format part, PHYS don't need that much bits I think,
> > so by slightly adjust the format vswap device could be share
> > mostly the same format with ordinary device.
> >
> > For example typical modern system don't have a address space larger
> > than 52 bit. (Even with full 64 bits used for addressing, shift it
> > by 12 we get 52). Plus 5 for type, you get 57, so you can have a
> > marker that should work as long as it shorter than 1000000 for PHYS,
> > and shared for all table format since it's not in conflict with
> > anything. You have also use a few extra bits so a single swap space
> > can be 8 times larger than RAM space, and since we can help
> > multiple swap type I think that should be far than enough?
> >
> > Then you have Shadow back at 001, and zero bit in shadow. The only
> > special one is Zswap, which will be 100 now, and that's exactly the
> > reserved pointer format in current swap table format, on seeing
> > si->flags & VSWAP && is_pointer(swp_tb) you know that's zswap :)
>
> Are you suggesting we merge the virtual table with main swap table?
>
> Man, I'd love to do this. There is a problem though - we have a case
> where we occupy both backing physical swap AND swap cache. Do you
> think we can fit both the physical swap slot handle and the swap cache
> PFN into the same slot in virtual table? Maybe with some expanding...?
>
> Another option is we can be a bit smart about it - if a virtual swap
> entry is in swap cache AND occupies physical swap slot, then put the
> folio at the physical swap's table, use folio->swap as the rmap.
>
> (I think you recommend this approach somewhere but for the life of me
> I can't find the reference - apologies if I'm putting words into your
> mouth :))
>
> But this is a bit more complicated - extra care is needed for rmap
> handling at the physical swap layer, and swap cache handling at the
> virtual swap layer. Maybe a follow-up? :)
>
> >
> > Folio / PFN can still be 010 as in the current swap table format.
> >
> > Then everything seems clean and aligned, no more special handling
> > for vswap needed, there are detailed to sort out, but it should work.
> >
> > > - Pointer-tagged swap_table on physical clusters for rmap (physical
> > > -> virtual) lookup.
> >
> > Or reuse the PHYS format (rename it maybe) since point back to vswap
> > is also pointing to a si.
>
> Noted. I'm just doing the simplest thing right now - working
> prototype. I mean, we have enough bits :)
>
> >
> > > III. Follow-ups:
> > >
> > > In no particular order (and most of which can be done as follow-up
> > > patch series rather than shoving everything in the initial landing):
> > >
> > > - More thorough stress testing is very much needed.
> > >
> > > - Performance benchmarks to make sure I don't accidentally regress
> > > the vswap-less case, and that the vswap's case performance is
> > > good. I suspect I will have to port a lot of the
> > > optimizations I implemented in v6 over here - some of the
> > > inefficiencies are inherent in any swap virtualization, and
> > > would require the same fix (for e.g the MRU cluster caching
> > > for faster cluster lookup - see [8] and [9]).
> >
> > This could be imporved by per-si percpu cluster. Both YoungJun's
> > tiering and Baoquan's previous swap ops mentioned this is needed,
> > and now vswap also need that. If the vswap is also a si, then it will
> > make use of this too.
Oh and the MRU cluster caching I mentioned here is not the allocation
caching. It's the lookup caching, basically to avoid doing the
xa_load() to look up clusters for consecutive swap operations on the
same vswap cluster (which is the common case with vswap). For v6, it
massively reduces this indirection lookup overhead. Performance-wise
it's an absolute winner, just more complexity (because I need to
handle reference counting carefully).
I also just realized we'll induce the indirection overhead on
allocation here too, even if the cached cluster still have slots for
allocation, because we look up the cluster (which is basically free
for static swap device, but not free for vswap devices). Might need to
take care of that to maintain vswap performance (but it will then
diverge from your existing code...).
^ permalink raw reply
* [PATCH v2] mm/list_lru: drain before clearing xarray entry on reparent
From: Shakeel Butt @ 2026-06-01 16:15 UTC (permalink / raw)
To: Andrew Morton
Cc: Johannes Weiner, Dave Chinner, Roman Gushchin, Muchun Song,
Qi Zheng, Kairui Song, Meta kernel team, linux-mm, cgroups,
linux-kernel, Chris Mason, stable
memcg_reparent_list_lrus() clears the dying memcg's xarray entry with
xas_store(&xas, NULL) before reparenting its per-node lists into the
parent. This opens a window where a concurrent list_lru_del() arriving
for the dying memcg sees xa_load() == NULL, walks to the parent in
lock_list_lru_of_memcg(), takes the parent's per-node lock, and calls
list_del_init() on an item still physically linked on the dying
memcg's list.
If another in-flight thread holds the dying memcg's per-node lock at
the same moment (another list_lru_del, or a list_lru_walk_one running
an isolate callback), both threads modify ->next/->prev pointers on the
same physical list under different locks. Adjacent items can corrupt
each other's links.
Fix it by reversing the order: reparent each per-node list and mark the
child's list lru dead and then clear the xarray entry. Any concurrent
list_lru op that finds the still-set xarray entry either takes the dying
memcg's per-node lock (synchronizing with the drain) or sees LONG_MIN
and walks to the parent, where the items now live.
Fixes: fb56fdf8b9a2 ("mm/list_lru: split the lock to per-cgroup scope")
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Reported-by: Chris Mason <clm@fb.com>
Cc: stable@vger.kernel.org
---
Changes since v1:
- Use xa_erase_irq() instead of xa_erase() (Sashiko & Claude).
- Added comment on CSS_DYING check in memcg_list_lru_alloc avoiding a new mlru
allocation.
mm/list_lru.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index dd29bcf8eb5f..d454bce9a78e 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -473,26 +473,29 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
mutex_lock(&list_lrus_mutex);
list_for_each_entry(lru, &memcg_list_lrus, list) {
struct list_lru_memcg *mlru;
- XA_STATE(xas, &lru->xa, memcg->kmemcg_id);
/*
- * Lock the Xarray to ensure no on going list_lru_memcg
- * allocation and further allocation will see css_is_dying().
+ * css_is_dying() check in memcg_list_lru_alloc() avoids
+ * allocating a new mlru since CSS_DYING is already set for this
+ * memcg a rcu grace period ago.
*/
- xas_lock_irq(&xas);
- mlru = xas_store(&xas, NULL);
- xas_unlock_irq(&xas);
+ mlru = xa_load(&lru->xa, memcg->kmemcg_id);
if (!mlru)
continue;
/*
- * With Xarray value set to NULL, holding the lru lock below
- * prevents list_lru_{add,del,isolate} from touching the lru,
- * safe to reparent.
+ * Reparent each per-node list and mark the child dead
+ * (LONG_MIN) before clearing xarray entry otherwise a
+ * concurrent list_lru_del() may corrupt the list if it arrives
+ * after xarray clear but before reparenting as
+ * lock_list_lru_of_memcg will acquire parent's lock while the
+ * item is still on child's list.
*/
for_each_node(i)
memcg_reparent_list_lru_one(lru, i, &mlru->node[i], parent);
+ xa_erase_irq(&lru->xa, memcg->kmemcg_id);
+
/*
* Here all list_lrus corresponding to the cgroup are guaranteed
* to remain empty, we can safely free this lru, any further
--
2.53.0-Meta
^ permalink raw reply related
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Nhat Pham @ 2026-06-01 15:56 UTC (permalink / raw)
To: Kairui Song
Cc: kasong, Liam.Howlett, akpm, apopple, axelrasmussen, baohua,
baolin.wang, bhe, byungchul, cgroups, chengming.zhou, chrisl,
corbet, david, dev.jain, gourry, hannes, hughd, jannh,
joshua.hahnjy, lance.yang, lenb, linux-doc, linux-kernel,
linux-mm, linux-pm, lorenzo.stoakes, matthew.brost, mhocko,
muchun.song, npache, pavel, peterx, peterz, pfalcato, rafael,
rakie.kim, roman.gushchin, rppt, ryan.roberts, shakeel.butt,
shikemeng, surenb, tglx, vbabka, weixugc, ying.huang, yosry.ahmed,
yuanchu, zhengqi.arch, ziy, kernel-team, riel, haowenchao22
In-Reply-To: <ahz_iYG4lqWL4g-J@KASONG-MC4>
On Mon, Jun 1, 2026 at 12:34 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Thu, May 28, 2026 at 02:29:24PM +0800, Nhat Pham wrote:
> > Based on: mm-unstable @ 444fc9435e57 + swap-table phase IV v5 [2].
> >
> > I manually adapted Kairui's ghost device implementation (from [4])
> > for my vswap device. I've credited him as Co-developed-by on Patch I
> > since a substantial portion of the dynamic-cluster infrastructure is
> > his (I did propose the idea of using xarray/radix tree for dynamic
> > swap clusters allocation and management though :P).
> >
> > >From here on out, for simplicity, I will refer to swap table phase IV
> > as "P4", and the older v6 virtual swap space implementation as "v6".
> >
>
> ...
>
> >
> > This series reimplements the virtual swap space concept (see [1])
> > on top of Kairui Song's swap table infrastructure, on top of [2]
> > and in accordance with his proposal in [3]. The proposal's idea
> > is interesting, so I decided to give it a shot myself. I'm still not
> > 100% sure that this is bug-proof, but hey, it compiles, and has
> > not crashed in my simple stress testing :)
> >
> > The prototype here is feature-complete relative to the swap-table P4
> > baseline — swapout, swapin, freeing, swapoff, zswap writeback, zswap
> > shrinker, memcg charging, and THP swapin all work for
> > both vswap and direct-physical entries — and satisfies all three
> > requirements above: no backend coupling (zswap/zero entries hold no
> > physical slot), dynamic swap space (clusters allocated on demand via
> > xarray, no static provisioning), and efficient backend transfer
> > (in-place vtable updates, no PTE/rmap walking).
> >
> > II. Design
> >
> > With vswap, pages are assigned virtual swap entries on a ghost device
> > with no backing storage. These entries are backed by zswap, zero pages,
> > or (lazily) physical swap slots. Physical backing is allocated only
> > when needed — on zswap writeback or reclaim writeout, after the rmap
> > step.
> >
> > Compared to the standalone v6 implementation [1], which introduces a
> > 24-byte per-entry swap descriptor and its own cluster allocator, this
> > edition uses swap_table infrastructure, and share a lot of the allocator
> > logic. Per-slot metadata is stored in a tag-encoded virtual_table
> > (atomic_long_t, 8 bytes per slot), and physical clusters store
> > Pointer-tagged rmap entries in the swap_table for reverse lookup back to
> > the virtual cluster.
> >
> > Here are some data layout diagrams:
> >
> > Case 1: vswap entry (virtualized)
> >
> > PTE swap_cluster_info_dynamic
> > vswap_entry +-------------------------+
> > (swp_entry_t) ------>| swap_cluster_info (ci) |
> > | +--------------------+ |
> > | | swap_table | |
> > | | PFN / Shadow | |
> > | | memcg_table | |
> > | | count,flags,order | |
> > | | lock, list | |
> > | +--------------------+ |
> > | |
> > | virtual_table |
> > | +--------------------+ |
> > | | NONE | |
> > | | PHYS | |
> > | | ZERO | |
> > | | ZSWAP(entry*) | |
> > | | FOLIO(folio*) | |
> > | +--------------------+ |
> > +-------------------------+
> > |
> > | PHYS resolves to
> > v
> > PHYSICAL CLUSTER (swap_cluster_info)
> > +--------------------------+
> > | swap_table per-slot: |
> > | NULL - free |
> > | PFN - cached folio |
> > | Shadow - swapped out |
> > | Pointer- vswap rmap |
> > | Bad - unusable |
> > | |
> > | Vswap-backing slot: |
> > | Pointer(C|swp_entry_t) |
> > | rmap back to vswap |
> > +--------------------------+
> >
> > Case 2: direct-mapped physical entry (no vswap)
> >
> > PTE PHYSICAL CLUSTER (swap_cluster_info)
> > phys_entry +--------------------------+
> > (swp_entry_t) ------>| swap_table per-slot: |
> > | NULL - free |
> > | PFN - cached folio |
> > | Shadow - swapped out |
> > | Bad - unusable |
> > +--------------------------+
> >
> > struct swap_cluster_info_dynamic {
> > struct swap_cluster_info ci; /* swap_table, lock, etc. */
> > unsigned int index; /* position in xarray */
> > struct rcu_head rcu; /* kfree_rcu deferred free */
> > atomic_long_t *virtual_table; /* backend info, 8 B/slot */
> > };
> >
> > Each vswap cluster (swap_cluster_info_dynamic) extends the classic
> > swap_cluster_info struct with a virtual_table array that stores the
> > backend information for each virtual swap entry in the cluster. Each
> > entry is tag-encoded in the low 3 bits to indicate backend types:
> >
> > NONE: |----- 0000 ------|000| free / unbacked
> > PHYS: |-- (type:5,off:N)|001| on a physical swapfile (shifted)
> > ZERO: |----- 0000 ------|010| zero-filled page
> > ZSWAP: |--- zswap_entry* |011| compressed in zswap
> > FOLIO: |--- folio* ------|100| in-memory folio
>
> Thanks for trying this approach!
Thanks for the suggestions. I hope going forward we have sth concrete
to tinker with, rather than abstractions :P
>
> For the format part, PHYS don't need that much bits I think,
> so by slightly adjust the format vswap device could be share
> mostly the same format with ordinary device.
>
> For example typical modern system don't have a address space larger
> than 52 bit. (Even with full 64 bits used for addressing, shift it
> by 12 we get 52). Plus 5 for type, you get 57, so you can have a
> marker that should work as long as it shorter than 1000000 for PHYS,
> and shared for all table format since it's not in conflict with
> anything. You have also use a few extra bits so a single swap space
> can be 8 times larger than RAM space, and since we can help
> multiple swap type I think that should be far than enough?
>
> Then you have Shadow back at 001, and zero bit in shadow. The only
> special one is Zswap, which will be 100 now, and that's exactly the
> reserved pointer format in current swap table format, on seeing
> si->flags & VSWAP && is_pointer(swp_tb) you know that's zswap :)
Are you suggesting we merge the virtual table with main swap table?
Man, I'd love to do this. There is a problem though - we have a case
where we occupy both backing physical swap AND swap cache. Do you
think we can fit both the physical swap slot handle and the swap cache
PFN into the same slot in virtual table? Maybe with some expanding...?
Another option is we can be a bit smart about it - if a virtual swap
entry is in swap cache AND occupies physical swap slot, then put the
folio at the physical swap's table, use folio->swap as the rmap.
(I think you recommend this approach somewhere but for the life of me
I can't find the reference - apologies if I'm putting words into your
mouth :))
But this is a bit more complicated - extra care is needed for rmap
handling at the physical swap layer, and swap cache handling at the
virtual swap layer. Maybe a follow-up? :)
>
> Folio / PFN can still be 010 as in the current swap table format.
>
> Then everything seems clean and aligned, no more special handling
> for vswap needed, there are detailed to sort out, but it should work.
>
> > - Pointer-tagged swap_table on physical clusters for rmap (physical
> > -> virtual) lookup.
>
> Or reuse the PHYS format (rename it maybe) since point back to vswap
> is also pointing to a si.
Noted. I'm just doing the simplest thing right now - working
prototype. I mean, we have enough bits :)
>
> > III. Follow-ups:
> >
> > In no particular order (and most of which can be done as follow-up
> > patch series rather than shoving everything in the initial landing):
> >
> > - More thorough stress testing is very much needed.
> >
> > - Performance benchmarks to make sure I don't accidentally regress
> > the vswap-less case, and that the vswap's case performance is
> > good. I suspect I will have to port a lot of the
> > optimizations I implemented in v6 over here - some of the
> > inefficiencies are inherent in any swap virtualization, and
> > would require the same fix (for e.g the MRU cluster caching
> > for faster cluster lookup - see [8] and [9]).
>
> This could be imporved by per-si percpu cluster. Both YoungJun's
> tiering and Baoquan's previous swap ops mentioned this is needed,
> and now vswap also need that. If the vswap is also a si, then it will
> make use of this too.
Yeah I made the same recommendation when I review swap tier last week:
https://lore.kernel.org/all/CAKEwX=N2XcMHN1jatppOk6wnmz-Shab5XMtTtzgYOzRvU_6YFw@mail.gmail.com/
I like it, but yeah it will be complicated. That said, I think not
fixing the fast path for tiering/vswap will seriously restrict their
usefulness. We don't want to go back to the old swap allocator days :)
We can also revive the swap slot cache, but why do it if we can
repurpose your proven design, and just extend it a bit for multiple
tiers/swap devices? :)
>
> YoungJun posted this a few month before:
> https://lore.kernel.org/linux-mm/20260131125454.3187546-5-youngjun.park@lge.com/
>
> The concern is that some locking contention could be heavier, or maybe
> that's just a hypothetical problem though.
I don't think it's hypothetical. At least with vswap, it's very easy
to get into a state where the shared per-cpu cache gets invalidated
constantly if phys swap and vswap allocation alternates (which is
actually very possible under heavy memory pressure), hammering the
slow paths...
>
> >
> > - Runtime enable/disable of the vswap device. To be honest, I don't
> > know if there is a value in this. My preference is vswap can be
> > optimized to the point that any overhead is negligible. Failing that,
> > maybe we can come up with some simple heuristics that automatically
> > decides for users?
> >
> > In this RFC, CONFIG_VSWAP=y means the vswap device is always created at
> > boot, and CONFIG_VSWAP=n means the vswap device is never created. This
> > *might* be enough just on its own.
> >
> > Is a runtime knob (sysfs or sysctl) worth the complexity beyond
> > these heuristics? I'm not sure yet. Maintaining both cases
>
> I checked the code and I think it's not hard to do, patch 1 already
> handling the meta data dynamically, everything will still just work
> even if you remove vswap at runtime. The rest of patches need adaption
> but might not end up being complex, it other comments here
> are considered.
Yeah, it's not terribily hard to do. I'm more wondering if it's worth
the effort, both for the implementer and the user :)
As I said here, if we want vswap, just enable it at boot time and get
a vast (but dynamic) device. We can make it optional per-cgroup
through Youngjun's interface, and that would be good enough?
>
> For patch 2, a few routines like vswap_can_swapin_thp seems not
> needed or should be moved to __swap_cache_alloc? VSWAP_FOLIO is
> same as swap cache folio check, which is already covered. Same for
> zero checking, and VSWAP_NONE which is same as swap count check
> I think. That way we not only save a lot of code, we also no
> longer need to treat vswap specially.
Unfortunately, I think a lot of this complexity is still needed. Vswap
adds a new layer, which means new complications :)
For instance, I think you still need vswap_can_swapin_thp. It
basically enforces that the backend must be something
swap_read_folio() can handle. That means:
1. No zswap.
2. No mixed backend.
3. If it's phys swap, it must be contiguous in the same device.
The vswap entries might look contiguous in the virtual address space,
but completely unsuitable for the backend.... This is a new
complication that does not previously exist for non-virtualized swap,
so it needs more code to handle it...
I use vswap_can_swapin_thp() in two places - first when we try to find
a candidate range of PTEs for swap in (swap_pte_batch). And then
double check after we've added into swap cache, as the backend can
change arbitrarily before swap cache folio is locked.
Similar for some of the other helpers. For instance,
vswap_swapfile_backed() is needed in certain optimizations or for
correctness's sake, etc.
The VSWAP_FOLIO is redundant, I agree. It's just for convenient. It
represents the state of vswap where it's still in swap (swap cache),
but only backed by the swap cache folio, and not any other backends.
Technically, you can represent it with VSWAP_NONE + a check in the
swap cache field to see if there is a folio there.
If it's not too much code in v2 I can try to remove it (while leaving
behind a comment explains the state).
>
> If you keep the format compatible with what we already have
> as the earlier comment mentions, a large portion of this part
> might be unneeded.
>
> > at runtime also has overhead for checking as well, and some of the
> > checks are not cheap :)
>
> I also noticed the new introduced swap_read_folio_phys in patch 3, so
> this actually can be done using Baoquan's swapops idea which is now
> part of Christoph's swap batching:
I'm actually trying to have swap_read_folio() work for both vswap and
phys swpa just with a biiit of if-else. But yeah this might be cleaner
:)
>
> https://lore.kernel.org/linux-mm/20260528124559.2566481-9-hch@lst.de/#r
>
> That series is focusing on batching and better performance but swapops
> was also proposed as a way to solve the virtual layer, makes it possible
> to have vswap as one kind of swapops which is Chris talked a lot about:
>
> https://lore.kernel.org/linux-mm/aZiFvzlBJiYBUDre@MiWiFi-R3L-srv/
>
> Following this, we could have something like:
>
> const struct swap_ops swap_vswap_ops = {
> .submit_write = swap_vswap_submit_write,
> .submit_read = swap_vswap_submit_read,
> };
>
> The move the folio_realloc_swap in swap_vswap_submit_write.
>
> Merge of IO might be moved to lower phyiscal level for vswap.
>
> Another gain is that the memory usage and CPU overhead will be
> lower with only one layer. As I'm recently trying to offload swap
> dataplane off CPU so the CPU won't touch the data at all, the
> overhead will be purely by swap itself, plus some mm overhead.
> Things like that and IO optimization above and could make swap
> subsystem more and more performance sensitive so we have cases
> that needs only one layer.
Yeah I can take a look at this. This prototype purely based on your P4
and with just a bit of hackery to transfer my v6 implementation over.
Not very clean - just a PoC. If everyone is happy I can put more time
in :)
>
> >
> > - Defer per-cluster memcg_table and zeromap allocation on physical
> > clusters. A physical swap cluster backing vswap entries only do
> > not really need their memcg_table, but the current design forces
> > us to allocate it anyway. This is a waste of memory, and is an
> > overhead regression compared to my older design on the zswap-only
> > case, which Johannes has pointed out multiple times (see [6]),
> > and is one of the biggest reasons why I have not been satisfied
> > with this approach thus far. It honestly is a bit of a
> > deal-breaker...
> >
> > That said, I think I might be able to allocate them on demand, i.e
> > only when the first direct-mapped slot is allocated on that cluster.
> > That will give us the best of BOTH worlds, for both the vswap and
> > directly-mapped physical swap cases. No promises, but I will try
> > (if this approach is good enough for all parties).
>
> Zero map is not really a problem when it's just a inlined bit I think.
Yeah no problemo indeed. I saw a zeromap field in struct phys swap
cluster, so I put this in the plan as "to remove later", but I just
took a look and realized it's only for cases where you cant fit the
bit in swap table. I'm gating vswap for 64 bit for now, so should not
be a problem.
> For memcg table allocation, on demand seems a good idea, and actually
> we are not far from there, I tried to generalize the
> alloc-then-retry-sleep-alloc in swap_alloc_table but still not generic
> enough I guess.. Good new is the allocation of the table is already
> kind of ondemand, just need to split the detection of these two kind
> of table.
I have a prototype of this, but I have not tested, so I do not want to
send it out. :)
TLDR is - I still want to record the memcg for vswap (just not charge
it towards the counter). So we still need memcg_table at both level,
generally - just not allocating until needed (basically if a physical
swap slot in the cluster is directly mapped into PTE). You can kinda
tell, since you pass the folio into the allocation path - with some
care you can distinguish between:
1. Virtual swap, or directly maped physical swap -> need memcg_table
2. Physical swap, backing vswap -> does not memcg_table.
Another alternative is you can defer this allocation until the point
where you have to do the charging action. But then you have to be
careful with failure handling, and need to backoff ya di da di da.
Funsies.
I think I did a mixed of these 2 strategies. Anyway, I'll include the
patch in v2 (if folks like this approach).
>
> Mean while I also remember we once discussed about splitting the
> accounting for vswap / physical swap? If we went that approach we
> don't need to treat memcg_table specially.
For the charging behavior, I already have a patch for it actually in
this series (just not the dynamic allocation of the memcg_table field
yet).
Basically:
1. For vswap entry, not backed by phys swap: record swap memcg, hold
reference to pin the memcg, but not charging towards swap.current.
2. For phys swap backing vswap: charging towards swap.current, but
does not record the memcg in its memcg_table, nor does it hold
reference to memcg (its vswap entry holds the reference already)
2. For phys swap directly mapped to PTE: charges, records, and holds reference.
The motivation here is I do not want vswap entry to shares the same
limit as phys swap counter. If we think of it as "infinite" or
"dynamic", it should not be capped at all, but even if it is charged,
it should be something separate.
>
> > - Widen swap_info_struct->max to unsigned long. The vswap device's
> > max is currently clamped to ALIGN_DOWN(UINT_MAX, SWAPFILE_CLUSTER)
> > (~16 TiB) to fit in unsigned int. 16 TiB is small for vswap,
> > especially when we're getting increasingly big machines memory-wise.
>
> This should be very easy to do, just replace unsigned int with
> unsigned long, a lot of place to touch though :)
Agree. I'm just lazy, and this sounds like a simple patch as a
follow-up. This RFC is already 2000 LoCs - I do not want to burden
reviewers with extra useless details :)
^ permalink raw reply
* Re: [PATCH] mm/list_lru: drain before clearing xarray entry on reparent
From: Shakeel Butt @ 2026-06-01 15:38 UTC (permalink / raw)
To: Muchun Song
Cc: Andrew Morton, Johannes Weiner, Dave Chinner, Roman Gushchin,
Qi Zheng, Kairui Song, Meta kernel team, linux-mm, cgroups,
linux-kernel, Chris Mason
In-Reply-To: <79CD986A-2130-4FB8-804F-A543AF22342B@linux.dev>
Hi Muchun, thanks for taking a look.
On Mon, Jun 01, 2026 at 05:54:01PM +0800, Muchun Song wrote:
>
>
> > On Jun 1, 2026, at 14:34, Shakeel Butt <shakeel.butt@linux.dev> wrote:
> >
> > memcg_reparent_list_lrus() clears the dying memcg's xarray entry with
> > xas_store(&xas, NULL) before reparenting its per-node lists into the
> > parent. This opens a window where a concurrent list_lru_del() arriving
> > for the dying memcg sees xa_load() == NULL, walks to the parent in
> > lock_list_lru_of_memcg(), takes the parent's per-node lock, and calls
> > list_del_init() on an item still physically linked on the dying
> > memcg's list.
> >
> > If another in-flight thread holds the dying memcg's per-node lock at
> > the same moment (another list_lru_del, or a list_lru_walk_one running
> > an isolate callback), both threads modify ->next/->prev pointers on the
> > same physical list under different locks. Adjacent items can corrupt
> > each other's links.
> >
> > Fix it by reversing the order: reparent each per-node list and mark the
> > child's list lru dead and then clear the xarray entry. Any concurrent
> > list_lru op that finds the still-set xarray entry either takes the dying
> > memcg's per-node lock (synchronizing with the drain) or sees LONG_MIN
> > and walks to the parent, where the items now live.
> >
> > Fixes: fb56fdf8b9a2 ("mm/list_lru: split the lock to per-cgroup scope")
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> > Reported-by: Chris Mason <clm@fb.com>
> > ---
> > mm/list_lru.c | 20 +++++++++-----------
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/mm/list_lru.c b/mm/list_lru.c
> > index dd29bcf8eb5f..ae55a52307db 100644
> > --- a/mm/list_lru.c
> > +++ b/mm/list_lru.c
> > @@ -473,26 +473,24 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
> > mutex_lock(&list_lrus_mutex);
> > list_for_each_entry(lru, &memcg_list_lrus, list) {
> > struct list_lru_memcg *mlru;
> > - XA_STATE(xas, &lru->xa, memcg->kmemcg_id);
> >
> > - /*
> > - * Lock the Xarray to ensure no on going list_lru_memcg
> > - * allocation and further allocation will see css_is_dying().
> > - */
> > - xas_lock_irq(&xas);
> > - mlru = xas_store(&xas, NULL);
> > - xas_unlock_irq(&xas);
> > + mlru = xa_load(&lru->xa, memcg->kmemcg_id);
> > if (!mlru)
> > continue;
>
> Is it possible that concurrent threads running memcg_list_lru_alloc() could
> allocate a new mlru after this check passes? This could happen because the
> threads haven't noticed css_is_dying() yet. We would consequently miss the
> reparent operation for this list. So xas_lock_irq is necessary to serialize
> CSS_DYING setting here. Right?
Good question and it seems like Sashiko [1] raised a similar concern. However
please note that memcg_list_lru_alloc() uses CSS_DYING when it allocate a new
mlru but memcg_reparent_list_lrus() is called from offlice_css() callback and
the given css should already have CSS_DYING before calling offline_css(). There
is a rcu grace period between setting CSS_DYING and calling offline_css().
[1] https://sashiko.dev/#/patchset/20260601063408.2879011-1-shakeel.butt%40linux.dev
>
> Thanks.
> Muchun
>
> >
> > /*
> > - * With Xarray value set to NULL, holding the lru lock below
> > - * prevents list_lru_{add,del,isolate} from touching the lru,
> > - * safe to reparent.
> > + * Reparent each per-node list and mark the child dead
> > + * (LONG_MIN) before clearing xarray entry otherwisw a
> > + * concurrent list_lru_del() may corrupt the list if it arrives
> > + * after xarray clear but before reparenting as
> > + * lock_list_lru_of_memcg will acquire parent's lock while the
> > + * item is still on child's list.
> > */
> > for_each_node(i)
> > memcg_reparent_list_lru_one(lru, i, &mlru->node[i], parent);
> >
> > + xa_erase(&lru->xa, memcg->kmemcg_id);
This one is more tricky. Sashiko said:
" Is it safe to use xa_erase() here instead of xa_erase_irq()?
The list_lru xarray is initialized with XA_FLAGS_LOCK_IRQ, and elements are
added holding the lock via xas_lock_irqsave(), which establishes an IRQ-safe
lock class.
Since xa_erase() internally calls spin_lock() without disabling local
interrupts, an interrupt firing while the lock is held could attempt to
re-acquire the same lock in __memcg_list_lru_alloc(), leading to a deadlock.
This could also trigger a lockdep warning for an inconsistent lock state. "
Initially I though this is a false positive as I couldn't find irq callers for
kmem_cache_alloc_lru() but then claude came up with more concrete scenario which
is below:
"""
For the shadow_nodes lru this lock is also acquired nested under the page
cache i_pages lock, which is irq-safe. Adding a folio holds i_pages and
then allocates an xarray node through the shadow_nodes lru:
__filemap_add_folio()
mapping_set_update(&xas, mapping) // xas->xa_lru = &shadow_nodes
xas_lock_irq(&xas) // holds mapping->i_pages
xas_store() -> xas_alloc()
kmem_cache_alloc_lru(radix_tree_node_cachep, xas->xa_lru, gfp)
memcg_list_lru_alloc(memcg, &shadow_nodes, gfp)
xas_lock_irqsave(&shadow_nodes->xa) // shadow_nodes->xa under i_pages
and i_pages is taken from writeback completion in irq context:
__folio_end_writeback()
xa_lock_irqsave(&mapping->i_pages, flags);
So with xa_erase() taking shadow_nodes->xa with irqs enabled:
CPU0 memcg_reparent_list_lrus() CPU1 __filemap_add_folio()
xa_erase(&shadow_nodes->xa)
xa_lock(&shadow_nodes->xa)
xas_lock_irq(&i_pages) // holds i_pages
... memcg_list_lru_alloc()
xas_lock_irqsave(&shadow_nodes->xa) // waits
<io completion irq on CPU0>
__folio_end_writeback()
xa_lock_irqsave(&i_pages) // waits
Can this deadlock, and should this be xa_erase_irq() to keep the irq-safe
acquisition that the removed xas_lock_irq() had?
"""
This seems more plausible and I think simply using xa_erase_irq() is more safe.
I will send a v2 with this change.
^ permalink raw reply
* Re: [PATCH] mm: don't allow empty relative nodemask in mpol_relative_nodemask()
From: David Hildenbrand (Arm) @ 2026-06-01 14:32 UTC (permalink / raw)
To: Yury Norov, Joshua Hahn
Cc: Andrew Morton, Zi Yan, Matthew Brost, Rakie Kim, Byungchul Park,
Gregory Price, Ying Huang, Alistair Popple, linux-mm,
linux-kernel, Farhad Alemi, Waiman Long, Rasmus Villemoes,
cgroups
In-Reply-To: <ahnRIDBk4bQ3xX2q@yury>
>>
>> Thank you for taking a shot at fixing the bug report, please let me know what
>> you think! Have a great day : -)
>
> Hi Joshua.
>
> Indeed, quick and dirty shot.
>
> The problem is that nodes_fold() can't work with the sz == 0. In
> other words, folding to a 0-bit bitmap is an error. We don't check
> that on bitmaps level because it's an internal helper, and it's a
> caller's responsibility to validate the parameters.
>
> nodes_onto(), or more specifically bitmap_onto(), is a different
> story. In case of empty relmap, the function actually clears all the
> bits in dst and returns.
It's very weird that mpol_new_nodemask() (->create() callback) disallows empty
nodemasks, but mpol_rebind_nodemask() (->rebind() callback) would allow empty
nodemasks.
I guess mpol_set_nodemask() could trigger it after doing the
nodes_and(nsc->mask1, cpuset_current_mems_allowed,
node_states[N_MEMORY]);
And ending with an empty &nsc->mask1.
The later "mpol_ops[pol->mode].create(pol, &nsc->mask2);" would reject it, but
the division by zero could still happen.
>
> I see 2 options to move this forward.
>
> 1. Simply disallow empty relmap in mpol_relative_nodemask(). There's
> no valid cases for it, AFAIK, so the nodes_fold() limitation looks
> reasonable. We can consider it as a new policy.
>
> We've got 2 users for mpol_relative_nodemask(). In mpol_set_nodemask()
> we can simply propagate the error; and in mpol_rebind_nodemask() we
> can throw a warning and do nothing.
Throwing a warning is really bad. We'd still end up with an empty nodemask?
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH] mm: don't allow empty relative nodemask in mpol_relative_nodemask()
From: David Hildenbrand (Arm) @ 2026-06-01 14:06 UTC (permalink / raw)
To: Yury Norov, Andrew Morton, Zi Yan, Matthew Brost, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple, linux-mm, linux-kernel
Cc: Farhad Alemi, Waiman Long, Rasmus Villemoes, cgroups
In-Reply-To: <20260528190337.878027-1-ynorov@nvidia.com>
On 5/28/26 21:03, Yury Norov wrote:
> Reassigning nodes relative an empty user-provided nodemask is useless,
> and triggers divide-by-zero in the function.
>
> Reported-by: Farhad Alemi <farhad.alemi@berkeley.edu>
> Link: https://lore.kernel.org/all/CA+0ovCgxbZkXa+OU8w3s84R3KNPNxxRfmsNR-udh+afQBbGNmw@mail.gmail.com/
Likely this should be a
Closes:
And be accompanied by a Fixes: and Cc stable.
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
> ---
> mm/mempolicy.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 4e4421b22b59..cd961fa1eb33 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -370,8 +370,13 @@ static inline int mpol_store_user_nodemask(const struct mempolicy *pol)
> static void mpol_relative_nodemask(nodemask_t *ret, const nodemask_t *orig,
> const nodemask_t *rel)
> {
Continuing the discussion of the context in the other thread :)
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH v5 9/9] mm: switch deferred split shrinker to list_lru
From: Lance Yang @ 2026-06-01 13:21 UTC (permalink / raw)
To: hannes, baolin.wang
Cc: akpm, david, ljs, shakeel.butt, mhocko, david, roman.gushchin,
muchun.song, qi.zheng, yosry.ahmed, ziy, liam, usama.arif, kas,
vbabka, ryncsn, zaslonko, gor, baohua, dev.jain, lance.yang,
npache, ryan.roberts, cgroups, linux-mm, linux-kernel
In-Reply-To: <20260527204757.2544958-10-hannes@cmpxchg.org>
On Wed, May 27, 2026 at 04:45:16PM -0400, Johannes Weiner wrote:
[...]
>diff --git a/mm/swap_state.c b/mm/swap_state.c
>index 04f5ce992401..9c3a5cf99778 100644
>--- a/mm/swap_state.c
>+++ b/mm/swap_state.c
>@@ -465,6 +465,16 @@ static struct folio *__swap_cache_alloc(struct swap_cluster_info *ci,
> return ERR_PTR(-ENOMEM);
> }
>
Shouldn't this be limited to anon swapin?
e.g. vmf && vma_is_anonymous(vmf->vma)
>+ if (order > 1 && folio_memcg_alloc_deferred(folio)) {
__swap_cache_alloc() is also used by shmem direct swapin, so shmem can
get here too when handling a large swap entry:
shmem_get_folio_gfp()
shmem_swapin_folio()
shmem_swap_alloc_folio()
swapin_sync()
swap_cache_alloc_folio()
__swap_cache_alloc()
folio_memcg_alloc_deferred()
@Baolin please correct me if I got it wrong :)
folio_memcg_alloc_deferred() itself doesn't filter shmem out either; it
only allocates the memcg list_lru metadata for deferred_split_lru:
int folio_memcg_alloc_deferred(struct folio *folio)
{
if (mem_cgroup_disabled())
return 0;
return folio_memcg_list_lru_alloc(folio, &deferred_split_lru, GFP_KERNEL);
}
Since deferred_split_lru only queues anon large folios, doing this for
shmem swapin doesn't buy us anything :)
Maybe I'm missing something, just wanted to raise it.
>+ spin_lock(&ci->lock);
>+ __swap_cache_do_del_folio(ci, folio, entry, shadow);
>+ spin_unlock(&ci->lock);
>+ folio_unlock(folio);
>+ /* nr_pages refs from swap cache, 1 from allocation */
>+ folio_put_refs(folio, nr_pages + 1);
>+ return ERR_PTR(-ENOMEM);
>+ }
>+
> /* memsw uncharges swap when folio is added to swap cache */
> memcg1_swapin(folio);
> if (shadow)
>--
>2.54.0
>
>
^ permalink raw reply
* Re: [PATCH v5 9/9] mm: switch deferred split shrinker to list_lru
From: Lance Yang @ 2026-06-01 11:09 UTC (permalink / raw)
To: hannes
Cc: akpm, david, ljs, shakeel.butt, mhocko, david, roman.gushchin,
muchun.song, qi.zheng, yosry.ahmed, ziy, liam, usama.arif, kas,
vbabka, ryncsn, zaslonko, gor, baolin.wang, baohua, dev.jain,
npache, ryan.roberts, cgroups, linux-mm, linux-kernel
In-Reply-To: <20260601103947.63923-1-lance.yang@linux.dev>
On 2026/6/1 18:39, Lance Yang wrote:
>
> On Wed, May 27, 2026 at 04:45:16PM -0400, Johannes Weiner wrote:
> [...]
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 135f5c0f57bd..f22e61d8c8de 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5222,6 +5222,10 @@ static struct folio *alloc_anon_folio(struct vm_fault *vmf)
>> folio_put(folio);
>> goto next;
>> }
>> + if (order > 1 && folio_memcg_alloc_deferred(folio)) {
>> + folio_put(folio);
>
> Missing a MTHP_STAT_ANON_FAULT_FALLBACK bump here?
>
> Since we jump straight to fallback and end up at order-0 :)
Sorry for the noise, I missed the earlier discussion. Never mind this
one, please.
>
>> + goto fallback;
>> + }
>> folio_throttle_swaprate(folio, gfp);
>> /*
>> * When a folio is not zeroed during allocation
> [...]
>
> Cheers, Lance
^ permalink raw reply
* Re: [PATCH v3 1/4] mm/zswap: Make shrink_worker writeback cursor per-memcg
From: Hao Jia @ 2026-06-01 11:07 UTC (permalink / raw)
To: Yosry Ahmed
Cc: akpm, tj, hannes, shakeel.butt, mhocko, mkoutny, nphamcs,
chengming.zhou, muchun.song, roman.gushchin, cgroups, linux-mm,
linux-kernel, linux-doc, Hao Jia
In-Reply-To: <aho7nepN5jZtKmef@google.com>
On 2026/5/30 09:24, Yosry Ahmed wrote:
> On Tue, May 26, 2026 at 07:45:58PM +0800, Hao Jia wrote:
>> From: Hao Jia <jiahao1@lixiang.com>
>>
>> The zswap background writeback worker shrink_worker() uses a global
>> cursor zswap_next_shrink, protected by zswap_shrink_lock, to round-robin
>> across the online memcgs under root_mem_cgroup.
>>
>> 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.
Similar to how memory reclaim uses mem_cgroup_iter() (via struct
mem_cgroup_reclaim_iter) and the old shrink_worker() used
zswap_next_shrink, we need a shared cursor here.
>>
>> Naturally, group the cursor and its protecting spinlock into a
>> zswap_wb_iter struct, and make it a member of struct mem_cgroup to
>> realize per-memcg cursor management. Accordingly, shrink_worker() now
>> uses the lock and cursor in root_mem_cgroup->zswap_wb_iter.
>
> If we really need to have per-memcg cursors (I am not a big fan), I
> think we can minimize the overhead by making the cursor updates use
> atomic cmpxchg instead of having a per-memcg lock.
>
Because mem_cgroup_iter() always calls css_put(&prev->css), we cannot
simply update zswap_wb_iter.pos via cmpxchg() after calling it. Doing so
could lead to a double css_put() issue on prev->css.
Therefore, if we switch to the cmpxchg() approach, we wouldn't be able
to reuse the existing mem_cgroup_iter() logic. We would have to write a
new function similar to cgroup_iter(), and its implementation might end
up looking a bit obscure/complex.
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.
So, should we keep the spin_lock or go with the cmpxchg() approach?
Yosry and Nhat, what are your thoughts on this?
>>
>> Because the cursor is now per-memcg, the offline cleanup must visit
>> every ancestor that could be holding a reference to the dying memcg.
>> Factor out __zswap_memcg_offline_cleanup() and walk from dead_memcg up
>> to the root.
>
> Another reason why I don't like per-memcg cursors. There is too much
> complexity and I wonder if it's warranted. If we stick with per-memcg
> cursors please do the refactoring in separate patches to make the
> patches easier to review.
Sorry about that. I will try to keep each patch as simple as possible in
the next version.
Thanks,
Hao
^ permalink raw reply
* Re: [PATCH v5 9/9] mm: switch deferred split shrinker to list_lru
From: Lance Yang @ 2026-06-01 10:39 UTC (permalink / raw)
To: hannes
Cc: akpm, david, ljs, shakeel.butt, mhocko, david, roman.gushchin,
muchun.song, qi.zheng, yosry.ahmed, ziy, liam, usama.arif, kas,
vbabka, ryncsn, zaslonko, gor, baolin.wang, baohua, dev.jain,
lance.yang, npache, ryan.roberts, cgroups, linux-mm, linux-kernel
In-Reply-To: <20260527204757.2544958-10-hannes@cmpxchg.org>
On Wed, May 27, 2026 at 04:45:16PM -0400, Johannes Weiner wrote:
[...]
>diff --git a/mm/memory.c b/mm/memory.c
>index 135f5c0f57bd..f22e61d8c8de 100644
>--- a/mm/memory.c
>+++ b/mm/memory.c
>@@ -5222,6 +5222,10 @@ static struct folio *alloc_anon_folio(struct vm_fault *vmf)
> folio_put(folio);
> goto next;
> }
>+ if (order > 1 && folio_memcg_alloc_deferred(folio)) {
>+ folio_put(folio);
Missing a MTHP_STAT_ANON_FAULT_FALLBACK bump here?
Since we jump straight to fallback and end up at order-0 :)
>+ goto fallback;
>+ }
> folio_throttle_swaprate(folio, gfp);
> /*
> * When a folio is not zeroed during allocation
[...]
Cheers, Lance
^ permalink raw reply
* Re: [PATCH] mm/list_lru: drain before clearing xarray entry on reparent
From: Muchun Song @ 2026-06-01 9:54 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Johannes Weiner, Dave Chinner, Roman Gushchin,
Qi Zheng, Kairui Song, Meta kernel team, linux-mm, cgroups,
linux-kernel, Chris Mason
In-Reply-To: <20260601063408.2879011-1-shakeel.butt@linux.dev>
> On Jun 1, 2026, at 14:34, Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> memcg_reparent_list_lrus() clears the dying memcg's xarray entry with
> xas_store(&xas, NULL) before reparenting its per-node lists into the
> parent. This opens a window where a concurrent list_lru_del() arriving
> for the dying memcg sees xa_load() == NULL, walks to the parent in
> lock_list_lru_of_memcg(), takes the parent's per-node lock, and calls
> list_del_init() on an item still physically linked on the dying
> memcg's list.
>
> If another in-flight thread holds the dying memcg's per-node lock at
> the same moment (another list_lru_del, or a list_lru_walk_one running
> an isolate callback), both threads modify ->next/->prev pointers on the
> same physical list under different locks. Adjacent items can corrupt
> each other's links.
>
> Fix it by reversing the order: reparent each per-node list and mark the
> child's list lru dead and then clear the xarray entry. Any concurrent
> list_lru op that finds the still-set xarray entry either takes the dying
> memcg's per-node lock (synchronizing with the drain) or sees LONG_MIN
> and walks to the parent, where the items now live.
>
> Fixes: fb56fdf8b9a2 ("mm/list_lru: split the lock to per-cgroup scope")
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reported-by: Chris Mason <clm@fb.com>
> ---
> mm/list_lru.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index dd29bcf8eb5f..ae55a52307db 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -473,26 +473,24 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
> mutex_lock(&list_lrus_mutex);
> list_for_each_entry(lru, &memcg_list_lrus, list) {
> struct list_lru_memcg *mlru;
> - XA_STATE(xas, &lru->xa, memcg->kmemcg_id);
>
> - /*
> - * Lock the Xarray to ensure no on going list_lru_memcg
> - * allocation and further allocation will see css_is_dying().
> - */
> - xas_lock_irq(&xas);
> - mlru = xas_store(&xas, NULL);
> - xas_unlock_irq(&xas);
> + mlru = xa_load(&lru->xa, memcg->kmemcg_id);
> if (!mlru)
> continue;
Is it possible that concurrent threads running memcg_list_lru_alloc() could
allocate a new mlru after this check passes? This could happen because the
threads haven't noticed css_is_dying() yet. We would consequently miss the
reparent operation for this list. So xas_lock_irq is necessary to serialize
CSS_DYING setting here. Right?
Thanks.
Muchun
>
> /*
> - * With Xarray value set to NULL, holding the lru lock below
> - * prevents list_lru_{add,del,isolate} from touching the lru,
> - * safe to reparent.
> + * Reparent each per-node list and mark the child dead
> + * (LONG_MIN) before clearing xarray entry otherwisw a
> + * concurrent list_lru_del() may corrupt the list if it arrives
> + * after xarray clear but before reparenting as
> + * lock_list_lru_of_memcg will acquire parent's lock while the
> + * item is still on child's list.
> */
> for_each_node(i)
> memcg_reparent_list_lru_one(lru, i, &mlru->node[i], parent);
>
> + xa_erase(&lru->xa, memcg->kmemcg_id);
> +
> /*
> * Here all list_lrus corresponding to the cgroup are guaranteed
> * to remain empty, we can safely free this lru, any further
> --
> 2.52.0
>
^ permalink raw reply
* Re: [RFC PATCH v2 5/9] mm: add common locality admission for zswap large swapin
From: Fujunjie @ 2026-06-01 9:47 UTC (permalink / raw)
To: Nhat Pham
Cc: Andrew Morton, linux-mm, Alexandre Ghiti, Kairui Song, Usama Arif,
Chris Li, Johannes Weiner, Yosry Ahmed, David Hildenbrand,
Hugh Dickins, Roman Gushchin, Shakeel Butt, linux-kernel, cgroups
In-Reply-To: <CAKEwX=O_Jt3aCxocDoY1h5AY=-eOYnj_0saQ4rMbdfnLzPAFxw@mail.gmail.com>
On 5/30/2026 3:00 AM, Nhat Pham wrote:
> On Fri, May 29, 2026 at 5:19 AM fujunjie <fujunjie1@qq.com> wrote:
>>
>> Fully zswap-backed ranges are safe to load as a large folio only when
>> the caller has a reason to expect the neighbouring slots to be useful.
>> Otherwise a sparse refault can turn one 4K demand fault into a 64K
>> decompression and swapcache fill.
>>
>> Add a common admission gate for zswap-backed large swapin. The common
>> layer keeps backend checks, the 64K cap, recent-refault rejection, and
>> zswap reclaim-pressure rejection. It consumes a caller-provided locality
>> order mask instead of looking at anon or shmem state directly.
>
> Can you add more documentation about these policies, both in patch
> changelog and in code? I'm pretty confused by the
> zswap_pool_reclaim_pressure heuristics, for e.g
>
You're right, this should have been documented much better, and I think the
heuristic itself needs another look.
The intent was not to block demand swapin when zswap is under pressure. It was
only meant to block speculative large zswapin. Even after locality_orders
admits an order, that is still only evidence that the neighbouring slots are
likely useful; it does not prove that the whole large folio will be consumed
soon enough to justify the extra resident memory under pressure.
In this RFC, the cost is also higher because of my late mixed-backend fallback
design. I kept the zswap entries after filling the large swapcache folio, so
that the fresh large folio could still be dropped and retried as order-0 if a
late mixed-backend race was detected. With that design, a large zswapin does
not immediately reduce the zswap pool: the uncompressed large swapcache folio
and the compressed zswap entries can exist at the same time until the swap
slots are freed.
I used zswap_pool_reclaim_pressure() as a rough signal for avoiding that extra
speculative expansion. My assumption was that a zswap pool that has reached
its limit is often correlated with memory pressure. But that is not a strict
relationship, and I did not validate this heuristic rigorously. Sorry, this
was too under-explained and probably too ad hoc in this RFC.
Your earlier point also makes me think the design is over-defensive. Once the
large swapcache folio is installed, zswap writeback should not be able to turn
one slot in the range into disk-backed state, since it first has to allocate
an order-0 swapcache folio. For v3 I will revisit the completion rule first:
drop the late mixed-race -EAGAIN path, decide whether a successful large load
should consume zswap entries like the order-0 path, and then either remove
this pressure heuristic or keep it only with clearer documentation and
specific experiments.
>>
>> Callers pass no locality evidence for now, so this patch only installs
>> the common policy hook. Later patches add anon and shmem producers.
>>
>> Signed-off-by: fujunjie <fujunjie1@qq.com>
>> ---
>> mm/memory.c | 2 +-
>> mm/shmem.c | 2 +-
>> mm/swap.h | 8 ++--
>> mm/swap_state.c | 118 ++++++++++++++++++++++++++++++++++++++++++++----
>> 4 files changed, 117 insertions(+), 13 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index d73a19692dea..92a82008d583 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4849,7 +4849,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>> if (data_race(si->flags & SWP_SYNCHRONOUS_IO))
>> folio = swapin_sync(entry, GFP_HIGHUSER_MOVABLE,
>> thp_swapin_suitable_orders(vmf) | BIT(0),
>> - vmf, NULL, 0);
>> + 0, vmf, NULL, 0);
>> else
>> folio = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE, vmf);
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 56c23a7b15c7..fa99b48ed62b 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -2031,7 +2031,7 @@ static struct folio *shmem_swap_alloc_folio(struct inode *inode,
>>
>> again:
>> mpol = shmem_get_pgoff_policy(info, index, order, &ilx);
>> - folio = swapin_sync(entry, gfp, BIT(order), vmf, mpol, ilx);
>> + folio = swapin_sync(entry, gfp, BIT(order), 0, vmf, mpol, ilx);
>> mpol_cond_put(mpol);
>>
>> if (!IS_ERR(folio))
>> diff --git a/mm/swap.h b/mm/swap.h
>> index ea7e1f3c4410..dd35a310d06d 100644
>> --- a/mm/swap.h
>> +++ b/mm/swap.h
>> @@ -323,9 +323,10 @@ struct folio *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
>> struct folio *swap_cluster_readahead(swp_entry_t entry, gfp_t flag,
>> struct mempolicy *mpol, pgoff_t ilx);
>> struct folio *swapin_readahead(swp_entry_t entry, gfp_t flag,
>> - struct vm_fault *vmf);
>> + struct vm_fault *vmf);
>> struct folio *swapin_sync(swp_entry_t entry, gfp_t flag, unsigned long orders,
>> - struct vm_fault *vmf, struct mempolicy *mpol, pgoff_t ilx);
>> + unsigned long locality_orders, struct vm_fault *vmf,
>> + struct mempolicy *mpol, pgoff_t ilx);
>> void swap_update_readahead(struct folio *folio, struct vm_area_struct *vma,
>> unsigned long addr);
>>
>> @@ -418,7 +419,8 @@ static inline struct folio *swapin_readahead(swp_entry_t swp, gfp_t gfp_mask,
>>
>> static inline struct folio *swapin_sync(
>> swp_entry_t entry, gfp_t flag, unsigned long orders,
>> - struct vm_fault *vmf, struct mempolicy *mpol, pgoff_t ilx)
>> + unsigned long locality_orders, struct vm_fault *vmf,
>> + struct mempolicy *mpol, pgoff_t ilx)
>> {
>> return NULL;
>> }
>> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> index f03ad4832f16..5a4ca289009a 100644
>> --- a/mm/swap_state.c
>> +++ b/mm/swap_state.c
>> @@ -21,6 +21,7 @@
>> #include <linux/migrate.h>
>> #include <linux/vmalloc.h>
>> #include <linux/huge_mm.h>
>> +#include <linux/sizes.h>
>> #include <linux/zswap.h>
>> #include <linux/shmem_fs.h>
>> #include "internal.h"
>> @@ -556,6 +557,24 @@ static struct folio *swap_cache_alloc_speculative_folio(swp_entry_t targ_entry,
>> mpol, ilx, true);
>> }
>>
>> +/*
>> + * Initial conservative cap for speculative zswap large swapin. Locality
>> + * evidence is supplied by the caller or by generic VMA hints; the common
>> + * swapin layer keeps backend safety and pressure decisions here.
>> + */
>> +#define SWAPIN_ZSWAP_MAX_SIZE SZ_64K
>> +#if PAGE_SIZE < SWAPIN_ZSWAP_MAX_SIZE
>> +#define SWAPIN_ZSWAP_MAX_ORDER \
>> + ilog2(SWAPIN_ZSWAP_MAX_SIZE / PAGE_SIZE)
>> +#else
>> +#define SWAPIN_ZSWAP_MAX_ORDER 0
>> +#endif
>> +
>> +struct zswap_admit_ctx {
>> + bool pressure_checked;
>> + bool reclaim_pressure;
>> +};
>> +
>> static bool swapin_zeromap_same(swp_entry_t entry, unsigned int nr_pages)
>> {
>> unsigned int ci_start = swp_cluster_offset(entry);
>> @@ -586,11 +605,84 @@ static bool swapin_zeromap_same(swp_entry_t entry, unsigned int nr_pages)
>> return true;
>> }
>>
>> +static bool swapin_zswap_locality(struct vm_fault *vmf, unsigned int order,
>> + unsigned long locality_orders)
>> +{
>> + struct vm_area_struct *vma = vmf ? vmf->vma : NULL;
>> +
>> + if (!order || order > MAX_PAGE_ORDER)
>> + return false;
>> +
>> + if (vma && (vma->vm_flags & VM_RAND_READ))
>> + return false;
>
> what about VM_SEQ_READ?
This helper is meant to consume locality_orders, not to produce locality
evidence itself. VM_SEQ_READ is handled by the caller-side locality producer:
the anon producer returns all candidate orders for VM_SEQ_READ, and the shmem
producer does the same for now.
I kept the check in the common helper mostly as a common veto for the readahead path where ra_orders are passed as locality_orders directly.
I think the cleaner fix is to move this into the producer side as well.
>
>> +
>> + return locality_orders & BIT(order);
>> +}
>> +
>> +static bool swapin_zswap_refaulted(swp_entry_t entry, unsigned int nr_pages)
>
> nit: this does not seem zswap-specific. Just call it
> swapin_range_refaulted or sth like that, maybe?
>
>> + for (i = 0; i < nr_pages; i++) {
>> + bool workingset;
>> + void *shadow;
>> +
>> + shadow = swap_cache_get_shadow(swp_entry(type, offset + i));
>
> This seems inefficient. Can't we just lock the swap cluster once,
> check all the shadow in the range, instead of repeatedly getting then
> dropping the swap cluster lock?
both points make sense.
The refault check is not zswap-specific, so I will rename it to something like
swapin_range_refaulted().
And yes, the current implementation is too expensive. The range is already
bounded and contiguous, so I should check the swap table under one cluster lock
instead of calling swap_cache_get_shadow() for every slot. I will rework that
in v3.
>
>> + if (!shadow)
>> + continue;
>> + if (workingset_test_recent(shadow, false, &workingset, false) &&
>> + workingset)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static bool swapin_zswap_admit(swp_entry_t entry,
>> + unsigned int order, unsigned int nr_pages,
>> + struct vm_fault *vmf,
>> + unsigned long locality_orders,
>> + struct zswap_admit_ctx *ctx)
>> +{
>> + if (order > SWAPIN_ZSWAP_MAX_ORDER)
>> + return false;
>> +
>> + /*
>> + * Treat zswap-backed large swapin as speculative. The common layer
>> + * consumes caller-provided locality orders, but does not inspect
>> + * anon-specific PTE state or shmem-specific mapping state directly.
>> + */
>> + if (!swapin_zswap_locality(vmf, order, locality_orders))
>> + return false;
>> +
>> + /*
>> + * A recent workingset refault shadow in the target range means reclaim
>> + * already saw churn there. Keep the refault path narrow instead of
>> + * speculatively decompressing neighbouring slots.
>> + */
>> + if (swapin_zswap_refaulted(entry, nr_pages))
>> + return false;
>
> Hmm this depends. If it's just a refault coming from a speculative
> read (readhead or THP (z)swpin), which is then promptly discarded,
> then yeah we should backoff here. But maybe the refaulted page is
> workingset one?
>
> But yeah I guess it is better to be cautious when you are uncertain :)
>
>
>> +
>> + if (!ctx->pressure_checked) {
>> + ctx->reclaim_pressure = zswap_pool_reclaim_pressure();
>> + ctx->pressure_checked = true;
>> + }
>
> Why do we backoff if there is zswap_pool_reclaim_pressure (which only
> check if the pool is full ONCE in its lifetime)? What's the rationale
> here?
The ctx is only local to one swapin_admit_orders() call. It is initialized
before walking the candidate orders, so pressure_checked is not persistent
across faults and is not tied to the lifetime of the zswap pool.
It is also not there because zswap_pool_reclaim_pressure() is expensive. That
helper is cheap. The point was to use one consistent pressure snapshot while
evaluating all candidate orders for the same fault, instead of letting
different orders observe different pressure states.
The zswap state being read is not "full once forever". It is
hysteretic: zswap_check_limits() sets zswap_pool_reached_full when the pool
reaches the max limit, and clears it after the pool drops below the accept
threshold.
The rationale for backing off was conservative: I treated zswap pool pressure
as correlated with memory/reclaim pressure. With this RFC's current design,
large zswapin can temporarily keep both the compressed zswap entries and the
uncompressed large swapcache folio, so I wanted to avoid that speculative
expansion when the compressed pool already looks stressed.
>
>> + if (ctx->reclaim_pressure)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> static unsigned long swapin_admit_orders(swp_entry_t entry,
>> - unsigned long orders)
>> + unsigned long orders,
>> + struct vm_fault *vmf,
>> + unsigned long locality_orders)
>> {
>> unsigned long candidates = orders & ~BIT(0);
>> unsigned long admitted = orders & BIT(0);
>> + struct zswap_admit_ctx zswap_ctx = {};
>> int order;
>>
>> if (!candidates)
>> @@ -616,9 +708,14 @@ static unsigned long swapin_admit_orders(swp_entry_t entry,
>>
>> state = zswap_probe_range(range_entry, nr_pages);
>> switch (state) {
>> + case ZSWAP_RANGE_ALL_ZSWAP:
>> + admit = swapin_zswap_admit(range_entry, order,
>> + nr_pages, vmf,
>> + locality_orders,
>> + &zswap_ctx);
>> + break;
>> case ZSWAP_RANGE_MIXED:
>> break;
>> - case ZSWAP_RANGE_ALL_ZSWAP:
>> case ZSWAP_RANGE_NEVER_ENABLED:
>> case ZSWAP_RANGE_NO_ZSWAP:
>> admit = true;
>> @@ -769,8 +866,8 @@ static struct folio *swap_cache_read_folio(swp_entry_t entry, gfp_t gfp,
>> ret = swap_read_folio(folio, plug);
>> /*
>> * Swap readahead allocates order-0 folios. -EAGAIN is reserved for
>> - * retryable large zswap backend races and must be handled by the
>> - * synchronous common swapin path.
>> + * retryable large zswap backend races and should never escape to this
>> + * order-0 path.
>> */
>> VM_WARN_ON_ONCE(ret == -EAGAIN);
>> if (readahead) {
>> @@ -786,6 +883,7 @@ static struct folio *swap_cache_read_folio(swp_entry_t entry, gfp_t gfp,
>> * @entry: swap entry indicating the target slot
>> * @gfp: memory allocation flags
>> * @orders: allocation orders
>> + * @locality_orders: orders with caller-provided locality evidence
>> * @vmf: fault information
>> * @mpol: NUMA memory allocation policy to be applied
>> * @ilx: NUMA interleave index, for use only when MPOL_INTERLEAVE
>> @@ -794,16 +892,20 @@ static struct folio *swap_cache_read_folio(swp_entry_t entry, gfp_t gfp,
>> * existing folio in the swap cache for @entry. This initiates the IO, too,
>> * if needed. @entry is rounded down if @orders allow large allocation.
>> *
>> - * Context: Caller must ensure @entry is valid and pin the swap device with refcount.
>> + * Context: Caller must ensure @entry is valid and pin the swap device with
>> + * refcount.
>> * Return: Returns the folio on success, error code if failed.
>> */
>> -struct folio *swapin_sync(swp_entry_t entry, gfp_t gfp, unsigned long orders,
>> - struct vm_fault *vmf, struct mempolicy *mpol, pgoff_t ilx)
>> +struct folio *swapin_sync(swp_entry_t entry, gfp_t gfp,
>> + unsigned long orders,
>> + unsigned long locality_orders,
>> + struct vm_fault *vmf, struct mempolicy *mpol,
>> + pgoff_t ilx)
>> {
>> struct folio *folio;
>> int ret;
>>
>> - orders = swapin_admit_orders(entry, orders);
>> + orders = swapin_admit_orders(entry, orders, vmf, locality_orders);
>> again:
>> do {
>> folio = swap_cache_get_folio(entry);
>> --
>> 2.34.1
>>
^ permalink raw reply
* Re: [PATCH 5/5] cgroup: Defer kill_css_finish() in cgroup_apply_control_disable()
From: Bert Karwatzki @ 2026-06-01 9:22 UTC (permalink / raw)
To: Mark Brown, Tejun Heo
Cc: Johannes Weiner, spasswolf, Michal Koutný,
Sebastian Andrzej Siewior, Petr Malat, kernel test robot,
Martin Pitt, cgroups, linux-kernel, Aishwarya.TCV
In-Reply-To: <4e986b4ed7e16547805d54b6e67d09120bc4d2f2.camel@web.de>
Am Sonntag, dem 31.05.2026 um 20:45 +0200 schrieb Bert Karwatzki:
>
> The test that hang when running
> # LTPROOT=/home/bert/ltp-install/ ./kirk --run-suite controllers
> is always cgroup_fj_function_net_prio.
> Also when bisecting this I disabled (i.e. commented out) the
> memcg_stress test in ~/ltp-install/runtest/controllers as it takes a lot of
> time (30min) and succeeds even in the version where hangs occur.
>
> Bert Karwatzki
I've done more testing and found that running the
cgroup_fj_function_net_prio test alone gives no hang, the hang
only occurs when other tests are run before it:
Suite: controllers
──────────────────
cgroup_core01: pass (0.026s)
cgroup_core02: pass (0.004s)
cgroup_core03: pass (0.005s)
cgroup: fail (2m 41s)
memcg_regression: skip (3.558s)
memcg_test_3: pass (0.112s)
memcg_failcnt: skip (0.027s)
memcg_force_empty: skip (0.016s)
memcg_limit_in_bytes: skip (0.015s)
memcg_stat_rss: skip (0.015s)
memcg_subgroup_charge: skip (0.015s)
memcg_max_usage_in_bytes: skip (0.014s)
memcg_move_charge_at_immigrate: skip (0.015s)
memcg_memsw_limit_in_bytes: skip (0.015s)
memcg_stat: skip (0.014s)
memcg_use_hierarchy: skip (0.015s)
memcg_usage_in_bytes: skip (0.014s)
memcg_control: pass (6.046s)
memcontrol01: pass (0.004s)
memcontrol02: pass (0.628s)
memcontrol03: pass (16.009s)
memcontrol04: pass (0.926s)
cgroup_fj_function_debug: skip (0.012s)
cgroup_fj_function_cpuset: skip (0.037s)
cgroup_fj_function_cpu: skip (0.055s)
cgroup_fj_function_cpuacct: pass (0.046s)
cgroup_fj_function_memory: skip (0.035s)
cgroup_fj_function_freezer: pass (0.044s)
cgroup_fj_function_devices: pass (0.067s)
cgroup_fj_function_blkio: skip (0.010s)
cgroup_fj_function_net_cls: pass (0.055s)
cgroup_fj_function_perf_event: pass (0.063s)
cgroup_fj_function_net_prio: HANG
I tried to narrow down this list and found that a hang occurs
int the net_prio test only if the perf_event test is run before it:
cgroup_fj_function_perf_event: pass (0.063s)
cgroup_fj_function_net_prio: HANG
Bert Karwatzki
^ permalink raw reply
* Re: [PATCH v5 0/9] mm: switch THP shrinker to list_lru
From: Lance Yang @ 2026-06-01 8:36 UTC (permalink / raw)
To: hannes
Cc: akpm, david, ljs, shakeel.butt, mhocko, david, roman.gushchin,
muchun.song, qi.zheng, yosry.ahmed, ziy, liam, usama.arif, kas,
vbabka, ryncsn, zaslonko, gor, baolin.wang, baohua, dev.jain,
lance.yang, npache, ryan.roberts, cgroups, linux-mm, linux-kernel
In-Reply-To: <20260527204757.2544958-1-hannes@cmpxchg.org>
Hi Johannes,
On Wed, May 27, 2026 at 04:45:07PM -0400, Johannes Weiner wrote:
>This is version 5 of switching the THP shrinker to list_lru.
>
>Core of the new version is the list_lru/set_shrinker_bit fix up front,
>which minimally affects later patches; and a rebase onto the latest
>mm-unstable - replaced alloc_swap_folio() with __swap_cache_alloc().
>
>The changes seemed small enough that *I chose to keep the review tags
>from v4*. Please shout if you object to this!
>
>Changes in v5:
>- patch 1 is a new fix for a very old, pre-existing set_shrinker_bit()
> problem in list_lru, where the bit can be set on a dying child memcg
> instead of the ancestor that actually received the item. Pointed out
> by Usama Arif and Sashiko; fix it first to make it minimally
> backportable and so the conversion is safe.
>- patches 6 and 9 adapt to that fix's new memcg-by-reference
> lock_list_lru_of_memcg() signature
>- collapse_huge_page(): propagate folio_memcg_alloc_deferred() failure
> as SCAN_ALLOC_HUGE_PAGE_FAIL instead of leaking SCAN_SUCCEED and
> falsely reporting a successful MADV_COLLAPSE (Usama Arif, Sashiko)
>- deferred_split_isolate(): fix a UAF by reading folio state before
> list_lru_isolate(); once removed, a racing folio_put() frees the
> folio via the lockless list_empty() check while we still touch its
> flags and stats (Sashiko)
>- rebased to mm-unstable of 2026-05-27, which simplifies the flatten
> prep patch (now anon-only, as alloc_swap_folio() was folded into the
> new __swap_cache_alloc()) and moves the swap-side
> folio_memcg_alloc_deferred() hook into __swap_cache_alloc(). Kairui,
> I would appreciate an eyeball on that.
>
>Changes in v4:
>- guard folio_memcg_alloc_deferred() with mem_cgroup_disabled() to fix
> NULL deref in __memcg_list_lru_alloc() when booting with
> cgroup_disable=memory (e.g., kdump capture kernel) -- reported and
> tested by Mikhail Zaslonko on s390 and x86
>- flatten if (folio) branches in alloc_swap_folio() and alloc_anon_folio()
> in a prep patch so the list_lru allocation additions are a clean minimal
> diff (Lorenzo)
>- folio_memcg_alloc_deferred() moved out of alloc_charge_folio() into the
> anon-only collapse_huge_page() path; collapse_file() shares that helper
> but its pages don't go on the THP shrinker queue (David)
>- guard folio_memcg_alloc_deferred() with order > 1; mTHPs below order-2
> can't be queued on the deferred split list (David)
>- make deferred_split_lru static, hide behind folio_memcg_alloc_deferred()
> wrapper with GFP_KERNEL (Lorenzo)
>- rename l -> lru throughout huge_memory.c (Lorenzo)
>- kdoc for folio_memcg_list_lru_alloc() (Lorenzo)
>- list_lru_lock_irq()/unlock_irq()/add_irq() irq-disabling variants;
> use list_lru_add_irq() in deferred_split_scan() (Lorenzo)
>- reorder shrinker_free() before list_lru_destroy() (Lorenzo)
>
>Changes in v3:
>- dedicated lockdep_key for irqsafe deferred_split_lru.lock (syzbot)
>- conditional list_lru ops in __folio_freeze_and_split_unmapped() (syzbot)
>- annotate runs of inscrutable false, NULL, false function arguments (David)
>- rename to folio_memcg_list_lru_alloc() (David)
>
>Changes in v2:
>- explicit rcu_read_lock() in __folio_freeze_and_split_unmapped() (Usama)
>- split out list_lru prep bits (Dave)
>
>The open-coded deferred split queue has issues. It's not NUMA-aware
>(when cgroup is enabled), and it's more complicated in the callsites
>interacting with it. Switching to list_lru fixes the NUMA problem and
>streamlines things. It also simplifies planned shrinker work.
>
>Patch 1 fixes a pre-existing list_lru bug where the shrinker bit is
>set on the caller's memcg rather than the ancestor whose sublist the
>item actually lands on after a walk-up. Standalone, backportable; the
>rest of the series depends on it.
>
>Patches 2-5 are cleanups and small refactors in list_lru code. They're
>basically independent, but make the THP shrinker conversion easier.
>
>Patch 6 extends the list_lru API to allow the caller to control the
>locking scope. The THP shrinker has private state it needs to keep
>synchronized with the LRU state.
>
>Patch 7 extends the list_lru API with a convenience helper to do
>list_lru head allocation (memcg_list_lru_alloc) when coming from a
>folio. Anon THPs are instantiated in several places, and with the
>folio reparenting patches pending, folio_memcg() access is now a more
>delicate dance. This avoids having to replicate that dance everywhere.
>
>Patch 8 flattens the alloc_anon_folio() retry loop so the next patch's
>list_lru hook lands as a clean addition rather than nested deep inside
>an if (folio) block.
>
>Patch 9 finally switches the deferred_split_queue to list_lru.
As the changelog above says, the old queue is per-memcg only, rather
than per-memcg-per-node. So reclaim on one node can still walk the whole
memcg queue and split underused THPs from other nodes in the same memcg.
But I think the new one can lose reclaim in the cgroup.memory=nokmem
case ...
With nokmem, the deferred shrinker can still run from memcg reclaim,
because it is SHRINKER_NONSLAB. But the list_lru is no longer per-memcg:
__list_lru_init() clears memcg_aware,
if (mem_cgroup_kmem_disabled())
memcg_aware = false;
so list_lru_from_memcg_idx() falls back to the shared node list:
static inline struct list_lru_one *
list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
{
if (list_lru_memcg_aware(lru) && idx >= 0) {
[...]
}
return &lru->node[nid].lru;
}
That makes the shrinker bit unreliable. __list_lru_add() still sets the
bit on the memcg passed in, but only when the list goes from empty to
non-empty:
bool __list_lru_add(struct list_lru *lru, struct list_lru_one *l,
struct list_head *item, int nid,
struct mem_cgroup *memcg)
{
if (list_empty(item)) {
[...]
if (!l->nr_items++)
set_shrinker_bit(memcg, nid, lru_shrinker_id(lru));
[...]
return true;
}
return false;
}
If memcg A adds the first folio, A gets the bit. If memcg B later adds a
folio to the same shared list, B does not get a bit, because the list
was already non-empty.
So in the A-first/B-later case, reclaim from B may not call the deferred
shrinker at all. The shared list is scanned from memcg reclaim only if
reclaim runs from the memcg that has the bit, such as A here, or from
global reclaim :)
Anyway, only after the shared list is emptied does the next memcg to add
a folio get to be the one with the bit, IIUC :)
Hopefully I didn't miss somthing important ...
Cheers, Lance
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Kairui Song @ 2026-06-01 7:34 UTC (permalink / raw)
To: Nhat Pham
Cc: kasong, Liam.Howlett, akpm, apopple, axelrasmussen, baohua,
baolin.wang, bhe, byungchul, cgroups, chengming.zhou, chrisl,
corbet, david, dev.jain, gourry, hannes, hughd, jannh,
joshua.hahnjy, lance.yang, lenb, linux-doc, linux-kernel,
linux-mm, linux-pm, lorenzo.stoakes, matthew.brost, mhocko,
muchun.song, npache, pavel, peterx, peterz, pfalcato, rafael,
rakie.kim, roman.gushchin, rppt, ryan.roberts, shakeel.butt,
shikemeng, surenb, tglx, vbabka, weixugc, ying.huang, yosry.ahmed,
yuanchu, zhengqi.arch, ziy, kernel-team, riel, haowenchao22
In-Reply-To: <20260528212955.1912856-1-nphamcs@gmail.com>
On Thu, May 28, 2026 at 02:29:24PM +0800, Nhat Pham wrote:
> Based on: mm-unstable @ 444fc9435e57 + swap-table phase IV v5 [2].
>
> I manually adapted Kairui's ghost device implementation (from [4])
> for my vswap device. I've credited him as Co-developed-by on Patch I
> since a substantial portion of the dynamic-cluster infrastructure is
> his (I did propose the idea of using xarray/radix tree for dynamic
> swap clusters allocation and management though :P).
>
> >From here on out, for simplicity, I will refer to swap table phase IV
> as "P4", and the older v6 virtual swap space implementation as "v6".
>
...
>
> This series reimplements the virtual swap space concept (see [1])
> on top of Kairui Song's swap table infrastructure, on top of [2]
> and in accordance with his proposal in [3]. The proposal's idea
> is interesting, so I decided to give it a shot myself. I'm still not
> 100% sure that this is bug-proof, but hey, it compiles, and has
> not crashed in my simple stress testing :)
>
> The prototype here is feature-complete relative to the swap-table P4
> baseline — swapout, swapin, freeing, swapoff, zswap writeback, zswap
> shrinker, memcg charging, and THP swapin all work for
> both vswap and direct-physical entries — and satisfies all three
> requirements above: no backend coupling (zswap/zero entries hold no
> physical slot), dynamic swap space (clusters allocated on demand via
> xarray, no static provisioning), and efficient backend transfer
> (in-place vtable updates, no PTE/rmap walking).
>
> II. Design
>
> With vswap, pages are assigned virtual swap entries on a ghost device
> with no backing storage. These entries are backed by zswap, zero pages,
> or (lazily) physical swap slots. Physical backing is allocated only
> when needed — on zswap writeback or reclaim writeout, after the rmap
> step.
>
> Compared to the standalone v6 implementation [1], which introduces a
> 24-byte per-entry swap descriptor and its own cluster allocator, this
> edition uses swap_table infrastructure, and share a lot of the allocator
> logic. Per-slot metadata is stored in a tag-encoded virtual_table
> (atomic_long_t, 8 bytes per slot), and physical clusters store
> Pointer-tagged rmap entries in the swap_table for reverse lookup back to
> the virtual cluster.
>
> Here are some data layout diagrams:
>
> Case 1: vswap entry (virtualized)
>
> PTE swap_cluster_info_dynamic
> vswap_entry +-------------------------+
> (swp_entry_t) ------>| swap_cluster_info (ci) |
> | +--------------------+ |
> | | swap_table | |
> | | PFN / Shadow | |
> | | memcg_table | |
> | | count,flags,order | |
> | | lock, list | |
> | +--------------------+ |
> | |
> | virtual_table |
> | +--------------------+ |
> | | NONE | |
> | | PHYS | |
> | | ZERO | |
> | | ZSWAP(entry*) | |
> | | FOLIO(folio*) | |
> | +--------------------+ |
> +-------------------------+
> |
> | PHYS resolves to
> v
> PHYSICAL CLUSTER (swap_cluster_info)
> +--------------------------+
> | swap_table per-slot: |
> | NULL - free |
> | PFN - cached folio |
> | Shadow - swapped out |
> | Pointer- vswap rmap |
> | Bad - unusable |
> | |
> | Vswap-backing slot: |
> | Pointer(C|swp_entry_t) |
> | rmap back to vswap |
> +--------------------------+
>
> Case 2: direct-mapped physical entry (no vswap)
>
> PTE PHYSICAL CLUSTER (swap_cluster_info)
> phys_entry +--------------------------+
> (swp_entry_t) ------>| swap_table per-slot: |
> | NULL - free |
> | PFN - cached folio |
> | Shadow - swapped out |
> | Bad - unusable |
> +--------------------------+
>
> struct swap_cluster_info_dynamic {
> struct swap_cluster_info ci; /* swap_table, lock, etc. */
> unsigned int index; /* position in xarray */
> struct rcu_head rcu; /* kfree_rcu deferred free */
> atomic_long_t *virtual_table; /* backend info, 8 B/slot */
> };
>
> Each vswap cluster (swap_cluster_info_dynamic) extends the classic
> swap_cluster_info struct with a virtual_table array that stores the
> backend information for each virtual swap entry in the cluster. Each
> entry is tag-encoded in the low 3 bits to indicate backend types:
>
> NONE: |----- 0000 ------|000| free / unbacked
> PHYS: |-- (type:5,off:N)|001| on a physical swapfile (shifted)
> ZERO: |----- 0000 ------|010| zero-filled page
> ZSWAP: |--- zswap_entry* |011| compressed in zswap
> FOLIO: |--- folio* ------|100| in-memory folio
Thanks for trying this approach!
For the format part, PHYS don't need that much bits I think,
so by slightly adjust the format vswap device could be share
mostly the same format with ordinary device.
For example typical modern system don't have a address space larger
than 52 bit. (Even with full 64 bits used for addressing, shift it
by 12 we get 52). Plus 5 for type, you get 57, so you can have a
marker that should work as long as it shorter than 1000000 for PHYS,
and shared for all table format since it's not in conflict with
anything. You have also use a few extra bits so a single swap space
can be 8 times larger than RAM space, and since we can help
multiple swap type I think that should be far than enough?
Then you have Shadow back at 001, and zero bit in shadow. The only
special one is Zswap, which will be 100 now, and that's exactly the
reserved pointer format in current swap table format, on seeing
si->flags & VSWAP && is_pointer(swp_tb) you know that's zswap :)
Folio / PFN can still be 010 as in the current swap table format.
Then everything seems clean and aligned, no more special handling
for vswap needed, there are detailed to sort out, but it should work.
> - Pointer-tagged swap_table on physical clusters for rmap (physical
> -> virtual) lookup.
Or reuse the PHYS format (rename it maybe) since point back to vswap
is also pointing to a si.
> III. Follow-ups:
>
> In no particular order (and most of which can be done as follow-up
> patch series rather than shoving everything in the initial landing):
>
> - More thorough stress testing is very much needed.
>
> - Performance benchmarks to make sure I don't accidentally regress
> the vswap-less case, and that the vswap's case performance is
> good. I suspect I will have to port a lot of the
> optimizations I implemented in v6 over here - some of the
> inefficiencies are inherent in any swap virtualization, and
> would require the same fix (for e.g the MRU cluster caching
> for faster cluster lookup - see [8] and [9]).
This could be imporved by per-si percpu cluster. Both YoungJun's
tiering and Baoquan's previous swap ops mentioned this is needed,
and now vswap also need that. If the vswap is also a si, then it will
make use of this too.
YoungJun posted this a few month before:
https://lore.kernel.org/linux-mm/20260131125454.3187546-5-youngjun.park@lge.com/
The concern is that some locking contention could be heavier, or maybe
that's just a hypothetical problem though.
>
> - Runtime enable/disable of the vswap device. To be honest, I don't
> know if there is a value in this. My preference is vswap can be
> optimized to the point that any overhead is negligible. Failing that,
> maybe we can come up with some simple heuristics that automatically
> decides for users?
>
> In this RFC, CONFIG_VSWAP=y means the vswap device is always created at
> boot, and CONFIG_VSWAP=n means the vswap device is never created. This
> *might* be enough just on its own.
>
> Is a runtime knob (sysfs or sysctl) worth the complexity beyond
> these heuristics? I'm not sure yet. Maintaining both cases
I checked the code and I think it's not hard to do, patch 1 already
handling the meta data dynamically, everything will still just work
even if you remove vswap at runtime. The rest of patches need adaption
but might not end up being complex, it other comments here
are considered.
For patch 2, a few routines like vswap_can_swapin_thp seems not
needed or should be moved to __swap_cache_alloc? VSWAP_FOLIO is
same as swap cache folio check, which is already covered. Same for
zero checking, and VSWAP_NONE which is same as swap count check
I think. That way we not only save a lot of code, we also no
longer need to treat vswap specially.
If you keep the format compatible with what we already have
as the earlier comment mentions, a large portion of this part
might be unneeded.
> at runtime also has overhead for checking as well, and some of the
> checks are not cheap :)
I also noticed the new introduced swap_read_folio_phys in patch 3, so
this actually can be done using Baoquan's swapops idea which is now
part of Christoph's swap batching:
https://lore.kernel.org/linux-mm/20260528124559.2566481-9-hch@lst.de/#r
That series is focusing on batching and better performance but swapops
was also proposed as a way to solve the virtual layer, makes it possible
to have vswap as one kind of swapops which is Chris talked a lot about:
https://lore.kernel.org/linux-mm/aZiFvzlBJiYBUDre@MiWiFi-R3L-srv/
Following this, we could have something like:
const struct swap_ops swap_vswap_ops = {
.submit_write = swap_vswap_submit_write,
.submit_read = swap_vswap_submit_read,
};
The move the folio_realloc_swap in swap_vswap_submit_write.
Merge of IO might be moved to lower phyiscal level for vswap.
Another gain is that the memory usage and CPU overhead will be
lower with only one layer. As I'm recently trying to offload swap
dataplane off CPU so the CPU won't touch the data at all, the
overhead will be purely by swap itself, plus some mm overhead.
Things like that and IO optimization above and could make swap
subsystem more and more performance sensitive so we have cases
that needs only one layer.
>
> - Defer per-cluster memcg_table and zeromap allocation on physical
> clusters. A physical swap cluster backing vswap entries only do
> not really need their memcg_table, but the current design forces
> us to allocate it anyway. This is a waste of memory, and is an
> overhead regression compared to my older design on the zswap-only
> case, which Johannes has pointed out multiple times (see [6]),
> and is one of the biggest reasons why I have not been satisfied
> with this approach thus far. It honestly is a bit of a
> deal-breaker...
>
> That said, I think I might be able to allocate them on demand, i.e
> only when the first direct-mapped slot is allocated on that cluster.
> That will give us the best of BOTH worlds, for both the vswap and
> directly-mapped physical swap cases. No promises, but I will try
> (if this approach is good enough for all parties).
Zero map is not really a problem when it's just a inlined bit I think.
For memcg table allocation, on demand seems a good idea, and actually
we are not far from there, I tried to generalize the
alloc-then-retry-sleep-alloc in swap_alloc_table but still not generic
enough I guess.. Good new is the allocation of the table is already
kind of ondemand, just need to split the detection of these two kind
of table.
Mean while I also remember we once discussed about splitting the
accounting for vswap / physical swap? If we went that approach we
don't need to treat memcg_table specially.
> - Widen swap_info_struct->max to unsigned long. The vswap device's
> max is currently clamped to ALIGN_DOWN(UINT_MAX, SWAPFILE_CLUSTER)
> (~16 TiB) to fit in unsigned int. 16 TiB is small for vswap,
> especially when we're getting increasingly big machines memory-wise.
This should be very easy to do, just replace unsigned int with
unsigned long, a lot of place to touch though :)
^ permalink raw reply
* [PATCH] mm/list_lru: drain before clearing xarray entry on reparent
From: Shakeel Butt @ 2026-06-01 6:34 UTC (permalink / raw)
To: Andrew Morton
Cc: Johannes Weiner, Dave Chinner, Roman Gushchin, Muchun Song,
Qi Zheng, Kairui Song, Meta kernel team, linux-mm, cgroups,
linux-kernel, Chris Mason
memcg_reparent_list_lrus() clears the dying memcg's xarray entry with
xas_store(&xas, NULL) before reparenting its per-node lists into the
parent. This opens a window where a concurrent list_lru_del() arriving
for the dying memcg sees xa_load() == NULL, walks to the parent in
lock_list_lru_of_memcg(), takes the parent's per-node lock, and calls
list_del_init() on an item still physically linked on the dying
memcg's list.
If another in-flight thread holds the dying memcg's per-node lock at
the same moment (another list_lru_del, or a list_lru_walk_one running
an isolate callback), both threads modify ->next/->prev pointers on the
same physical list under different locks. Adjacent items can corrupt
each other's links.
Fix it by reversing the order: reparent each per-node list and mark the
child's list lru dead and then clear the xarray entry. Any concurrent
list_lru op that finds the still-set xarray entry either takes the dying
memcg's per-node lock (synchronizing with the drain) or sees LONG_MIN
and walks to the parent, where the items now live.
Fixes: fb56fdf8b9a2 ("mm/list_lru: split the lock to per-cgroup scope")
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Reported-by: Chris Mason <clm@fb.com>
---
mm/list_lru.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index dd29bcf8eb5f..ae55a52307db 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -473,26 +473,24 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
mutex_lock(&list_lrus_mutex);
list_for_each_entry(lru, &memcg_list_lrus, list) {
struct list_lru_memcg *mlru;
- XA_STATE(xas, &lru->xa, memcg->kmemcg_id);
- /*
- * Lock the Xarray to ensure no on going list_lru_memcg
- * allocation and further allocation will see css_is_dying().
- */
- xas_lock_irq(&xas);
- mlru = xas_store(&xas, NULL);
- xas_unlock_irq(&xas);
+ mlru = xa_load(&lru->xa, memcg->kmemcg_id);
if (!mlru)
continue;
/*
- * With Xarray value set to NULL, holding the lru lock below
- * prevents list_lru_{add,del,isolate} from touching the lru,
- * safe to reparent.
+ * Reparent each per-node list and mark the child dead
+ * (LONG_MIN) before clearing xarray entry otherwisw a
+ * concurrent list_lru_del() may corrupt the list if it arrives
+ * after xarray clear but before reparenting as
+ * lock_list_lru_of_memcg will acquire parent's lock while the
+ * item is still on child's list.
*/
for_each_node(i)
memcg_reparent_list_lru_one(lru, i, &mlru->node[i], parent);
+ xa_erase(&lru->xa, memcg->kmemcg_id);
+
/*
* Here all list_lrus corresponding to the cgroup are guaranteed
* to remain empty, we can safely free this lru, any further
--
2.52.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox