* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Yosry Ahmed @ 2026-06-03 19:00 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: <CAKEwX=MZQJLHNNU0tUqnihdhdPdVd19KhC-HtJxfbQ_d8OezzQ@mail.gmail.com>
> > > I don't like that the code bifurcates for vswap vs. normal swap entries
> > > though. Not sure if this is an issue that can be fixed with proper
> > > abstractions to hide it, or if the design needs modifications. I was
> > > honestly really hoping we don't end up with this. I was hoping that the
> > > physical swap device no longer uses a full swap table and all, and
> > > everything goes through vswap.
> > >
> > > I hoping that if redirection isn't needed (e.g. zswap is disabled),
> > > vswap can directly encode the physical swap slot so that the reverse
> > > mapping isn't needed -- so we avoid the overhead without keeping the
> > > physical swap device using a fully-fledged swap table.
> >
> > Can you expand on "vswap can directly encode the physical swap slot"?
> > I'm not sure I follow here.
> >
> > >
> > > All that being said, perhaps I am too out of touch with the code to
> > > realize it's simply not possible.
> > >
> > > Honestly, if the main reason we can't have a single swap table for vswap
> > > is saving 8 bytes on the reverse mapping, it sounds like a weak-ish
> > > argument, even if we can't optimize the reverse mapping away. But maybe
> > > I am also out of touch with RAM prices :)
> >
> > In terms of the space overhead I do agree, FWIW :)
> >
> > I think the other concern is the indirection overhead with going
> > through the xarray for every swap operation, hence the per-CPU vswap
> > cluster lookup caching idea:
> >
> > https://lore.kernel.org/all/20260505153854.1612033-23-nphamcs@gmail.com/
> >
> > >
> > > I at least hope that, the current design is not painting us into a
> > > corner (e.g. through userspace interfaces), and we can still achieve a
> > > vswap-for-all implementation in the future (maybe that's what you have
> > > in mind already?).
> >
> > That's still my plan. Operationally speaking, I want to make this
> > completely transparent to users, with minimal to no performance
> > overhead.
>
> I do want to add that, even without achieving this, the current design
> already enables a lot of use cases. I think it is a good compromise to
> maintain both virtual and directly mapped physical swap entries for
> now, and revisit the conversation of whether we can afford a mandatory
> vswap layer once all the optimizations have been done :)
>
> We should strive to simplify the codebase, and it will naturally
> happen when the original overhead concern is no longer there. A
> swap-related example: a few years ago, everyone thought swap slot
> cache was needed. But then, Kairui optimized the swap allocator's lock
> contention issue away, and that swap slot cache is suddenly redundant.
> That finally allowed us to get rid of it. Similar thing happened (or
> is happening?) with the SWP_SYNCHRONOUS_IO swapcache-skipping
> heuristics.
I agree, I just want to make sure we have a line of sight (or at least
no blockers) to having a unified vswap layer in the future.
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Yosry Ahmed @ 2026-06-03 18:58 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: <CAKEwX=MWX9KkSFAoN4xEMg3b+gZUN9=yd7rirAWG5NOBf26eAg@mail.gmail.com>
> > I assume the main reason here is to avoid the extra overhead if
> > everything uses vswap, which would mainly be the reverse mapping
> > overhead? I guess there's also some simplicity that comes from reusing
> > the swap info infra as a whole, including the swap table.
>
> Yeah it helps a lot that we don't have to rewrite the whole allocator
> and swap entry reference counting logic again :)
I specifically meant using a full swap info thing for the physical swap
device even when it's behind vswap. That seems like an overkill, and we
don't need things like the swap entry reference coutning. We probably
just need a bitmap and a reverse mapping.
So I am assuming the main reason why we are not doing that (at least for
now) is simplicity?
> >
> > I don't like that the code bifurcates for vswap vs. normal swap entries
> > though. Not sure if this is an issue that can be fixed with proper
> > abstractions to hide it, or if the design needs modifications. I was
> > honestly really hoping we don't end up with this. I was hoping that the
> > physical swap device no longer uses a full swap table and all, and
> > everything goes through vswap.
> >
> > I hoping that if redirection isn't needed (e.g. zswap is disabled),
> > vswap can directly encode the physical swap slot so that the reverse
> > mapping isn't needed -- so we avoid the overhead without keeping the
> > physical swap device using a fully-fledged swap table.
>
> Can you expand on "vswap can directly encode the physical swap slot"?
> I'm not sure I follow here.
I meant that if redirection is not needed (e.g. zswap is disabled), then
instead of having a vswap device pointing at a physical swap device, we
can just the data (e.g. phyiscal swap slot) in the vswap device
directly. Then we don't need a full swap info thing and swap table for
the physical swap device.
This directly ties into my question above, about why we have a
fully-fledged swap info thing for the physical swap device when using
vswap.
> >
> > All that being said, perhaps I am too out of touch with the code to
> > realize it's simply not possible.
> >
> > Honestly, if the main reason we can't have a single swap table for vswap
> > is saving 8 bytes on the reverse mapping, it sounds like a weak-ish
> > argument, even if we can't optimize the reverse mapping away. But maybe
> > I am also out of touch with RAM prices :)
>
> In terms of the space overhead I do agree, FWIW :)
>
> I think the other concern is the indirection overhead with going
> through the xarray for every swap operation, hence the per-CPU vswap
> cluster lookup caching idea:
>
> https://lore.kernel.org/all/20260505153854.1612033-23-nphamcs@gmail.com/
Right, but we should already avoid the xarray with the swap table
design, right? We just have one swap table pointing to another
essentially?
> >
> > I at least hope that, the current design is not painting us into a
> > corner (e.g. through userspace interfaces), and we can still achieve a
> > vswap-for-all implementation in the future (maybe that's what you have
> > in mind already?).
>
> That's still my plan. Operationally speaking, I want to make this
> completely transparent to users, with minimal to no performance
> overhead.
So if CONFIG_VSWAP is set all swap devices are vswap by default, right?
Would it help with testing if it's controlled by a boot param?
>
> The next action item is to optimize for vswap-on-fast-swapfile case -
> that was Kairui's main concerns regarding performance. I spent a lot
> of time perfing and fixing issues for this case in v6. The issues with
> the most egregious effects and simplest fix (vswap-less
> swap-cache-only check for e.g) are already fixed in this new design,
> and eventually I will move the rest (lookup caching) and more to here.
So is the end goal to have vswap be the default rather than a special
swap device? It would certainly help to include some details about that.
> >
> > Aside from the swap code, the only sticking point for me is the logic
> > bifurcation in zswap. Why does zswap need to handle vswap vs. not vswap?
> > I thought the point of the design is to use vswap when zswap is used,
> > and otherwise use a normal swap table. In a way, one of the goals is to
> > make zswap a first class swap citizen, but it doesn't seem like we are
> > achieving that?
>
> We already have all the machinery to make zswap completely
> independent. Right now, if you use vswap, you'll skip the zswap's
> internal xarray entirely, and just store a zswap entry in the virtual
> swap cluster's vtable.
>
> I just haven't removed the old code for 2 reasons:
>
> 1. Reduce the delta on this RFC, to ease the burden for reviewers (and
> definitely not because I'm lazy :P)
>
> 2. The only other practical reason is so that we can let users compile
> with !CONFIG_VSWAP and still uses zswap on top of the old swapfile
> setup during the transition/experimentation period for now.
>
> But logically and conceptually speaking, there is no reason I can come
> up with to use zswap on without vswap. The CPU indirection overhead is
> already partially there (since zswap uses an xarray) and further
> optimized (cluster loopup caching etc.), as well as the space overhead
> (vswap replaces the zswap xarray). I actually wrote a whole paragraph
> about how we should always go for vswap if we're using zswap, but then
> decide to remove it since there's no code for it yet.
>
> If folks like it, what I can do is have CONFIG_ZSWAP depends on
> CONFIG_VSWAP, removes all the non-vswap logic, and call it a day? :)
> Then, on the swap allocation side, if vswap allocation fail and zswap
> writeback is disabled, we can error out early.
Hmm maybe we can keep it around for now and do that after vswap
stabilizes? It ultimately depend on how much complexity we maintain by
allowing both.
I think another problem is 32-bit, technically zswap can be used on
32-bit now, right? So vswap not supporitng 32-bit is a problem.
General question (for both zswap and general swap code), would a boot
param make implementation simpler? Right now we seem to key off the swap
device having the "vswap" flag, would it help if it was a runtime
constant?
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Yosry Ahmed @ 2026-06-03 18:54 UTC (permalink / raw)
To: Nhat Pham
Cc: Hao Jia, 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=OeFsFPdazWFf0tsKTOi3DneX3fCcreEkzwb=BOR_37hA@mail.gmail.com>
> > > I suspect not a lot of people invokes the shrink_memcg() synchronous
> > > path in zswap store though. Setting zswap.max is hard (as it involves
> > > guessing compression ratio ahead of time) and induces quite a bit of
> > > overhead (obj_cgroup_may_zswap() does a force flush for every store if
> > > you set zswap.max to a value other than 0 and max).
> >
> > Yeah I agree, but I just wanna make sure we don't completely kill
> > performance in case anyone is actually doing that.
> >
> > Regarding the force flush, it's unfortunate that we need to rely on a
> > stat for internal limit checking. It might make sense to use page
> > counters here, which already support hierarchical charging and
> > whatnot. We'll need something similar to the objcg approach to charge
> > sub-pages (or maybe just reuse that somehow?).
>
> Conceptually, I've moved towards not capping zswap.max, and using it
> only as a binary knob for enablement/disablement :)
>
> It's consuming the same resources it saves, so if we cap memory.max
> (and swap.max until we have vswap), then we already have isolation.
> The split between uncompressed/compressed should then be dynamically
> determined by workload characteristics (working set size, access
> pattern, compressibility) and service's SLO, and reclaimers
> (preferably proactive reclaimers) will drive us towards the optimal
> split/equilibrium.
Yeah I guess we can wait until someone actually has a use case for a
non-binary zswap.max and complains about performance.
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Nhat Pham @ 2026-06-03 18:51 UTC (permalink / raw)
To: Yosry Ahmed
Cc: Hao Jia, 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: <CAO9r8zO0GjTeY2+j_AjfTQ8qYOmHezZAV44f=aPgrjJcdtO9rQ@mail.gmail.com>
On Wed, Jun 3, 2026 at 11:43 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Wed, Jun 3, 2026 at 11:34 AM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > On Wed, Jun 3, 2026 at 11:26 AM Yosry Ahmed <yosry@kernel.org> wrote:
> > >
> > > > > > Is the main difference that we are scanning in batches here? I think we
> > > > > > can have shrink_memcg() do that too. If anything, it might make the
> > > > > > shrinker more efficient. Over-reclaim is ofc a concern, and especially
> > > > > > in the zswap_store() path as the overhead can be noticeable. Maybe we
> > > > > > can parameterize the batch size based on the code path.
> > > > > >
> > > > > > Nhat, what do you think?
> > > > >
> > > > > Nhat, since we now have the referenced-based second chance algorithm,
> > > > > should we consider doing batch writeback for shrink_memcg() as well?
> > > >
> > > > I just take a look at shrink_memcg() and realized it's reclaiming one
> > > > page at a time. Thanks for the reminder - I hated it.
> > > >
> > > > Please batchify it if it makes your life easier :) We don't reclaim
> > > > "just one page/object" anywhere else in the reclaim path, Sure, it
> > > > adds a bit of latency to zswap_store() if we reached cgroup limit, but
> > > > IMHO if we hit zswap.max limit at zswap_store() time, that is already
> > > > the slowest of slow path that you should have avoided with proactive
> > > > reclaim/zswap shrinker in the first place. And, setting zswap.max does
> > > > not make sense to me in the first place (I can write a whole essay
> > > > about it).
> > >
> > > Should we batchify shrink_memcg() from the shrinker and background
> > > writeback, but leave the synchronous zswap_store() path to reclaim one
> > > page for this series at least to avoid potential regressions?
> > >
> > > I think this change specifically needs more intensive testing (vs the
> > > other code paths).
> >
> > I'm fine with having shrink_memcg() takes a batch_size argument for now :)
> >
> > I suspect not a lot of people invokes the shrink_memcg() synchronous
> > path in zswap store though. Setting zswap.max is hard (as it involves
> > guessing compression ratio ahead of time) and induces quite a bit of
> > overhead (obj_cgroup_may_zswap() does a force flush for every store if
> > you set zswap.max to a value other than 0 and max).
>
> Yeah I agree, but I just wanna make sure we don't completely kill
> performance in case anyone is actually doing that.
>
> Regarding the force flush, it's unfortunate that we need to rely on a
> stat for internal limit checking. It might make sense to use page
> counters here, which already support hierarchical charging and
> whatnot. We'll need something similar to the objcg approach to charge
> sub-pages (or maybe just reuse that somehow?).
Conceptually, I've moved towards not capping zswap.max, and using it
only as a binary knob for enablement/disablement :)
It's consuming the same resources it saves, so if we cap memory.max
(and swap.max until we have vswap), then we already have isolation.
The split between uncompressed/compressed should then be dynamically
determined by workload characteristics (working set size, access
pattern, compressibility) and service's SLO, and reclaimers
(preferably proactive reclaimers) will drive us towards the optimal
split/equilibrium.
^ permalink raw reply
* Re: [PATCH] cgroup/cpuset: Support multiple source/destination cpusets using pids pattern
From: Waiman Long @ 2026-06-03 18:47 UTC (permalink / raw)
To: Ridong Chen; +Cc: cgroups, Tejun Heo, Johannes Weiner, linux-kernel
In-Reply-To: <20260603102604.177503-1-ridong.chen@linux.dev>
On 6/3/26 6:26 AM, Ridong Chen wrote:
> The current cpuset_can_attach() and cpuset_attach() functions assume task
> migration is from one source cpuset to one destination cpuset. This can be
> wrong in several scenarios:
> - Moving a multi-threaded process with threads in different cpusets
> - Disabling the cpuset controller (many children to one parent)
> - Enabling the cpuset controller (one parent to many children)
>
> Fix this by adopting the pids subsystem's per-task accounting pattern.
> In cpuset_can_attach(), use task_cs(task) to get the correct source cpuset
> for each task (like pids_can_attach uses task_css), adjust nr_deadline_tasks
> and reserve DL bandwidth per-task, and increment attach_in_progress per-task
> on the destination cpuset. In cpuset_attach(), handle destination cpuset
> changes within the task iteration loop.
>
> A shared helper cpuset_undo_attach() reverses the per-task operations for
> both partial rollback in cpuset_can_attach() and full reversal in
> cpuset_cancel_attach().
>
> When multiple source cpusets are detected in can_attach(), set
> attach_many_sources so that cpuset_attach() forces cpus_updated and
> mems_updated to true, ensuring all tasks get properly updated regardless
> of which source cpuset cpuset_attach_old_cs points to.
>
> This eliminates the need for nr_migrate_dl_tasks, sum_migrate_dl_bw, and
> dl_bw_cpu fields in struct cpuset.
>
> Fixes: 4ec22e9c5a90 ("cpuset: Enable cpuset controller in default hierarchy")
> Signed-off-by: Ridong Chen <ridong.chen@linux.dev>
It is not a problem doing per-task DL BW allocation and eliminating the
*dl_bw* fields. However, updating nr_deadline_tasks before it is
committed can be problematic.
nr_deadline_tasks is used in dl_rebuild_rd_accounting() which is called
by partition_sched_domains_locked(). After the release of cpuset_mutex
at the end of cpuset_can_attach() and before cpuset_attach() or
cpuset_cancel_attach() is called, it is possible
that partition_sched_domains_locked() can be called
and dl_rebuild_rd_accounting() is not getting the right DL BW accounting
information. So unless there is a way to confirm that this situation
cannot happen, we can't change nr_deadline_tasks before the attach is
commited.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Yosry Ahmed @ 2026-06-03 18:43 UTC (permalink / raw)
To: Nhat Pham
Cc: Hao Jia, 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=NQUqqrM9vdYE2KhWEZx-YwPc7YPhfz7xaBrGVDf824bA@mail.gmail.com>
On Wed, Jun 3, 2026 at 11:34 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Wed, Jun 3, 2026 at 11:26 AM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > > > > Is the main difference that we are scanning in batches here? I think we
> > > > > can have shrink_memcg() do that too. If anything, it might make the
> > > > > shrinker more efficient. Over-reclaim is ofc a concern, and especially
> > > > > in the zswap_store() path as the overhead can be noticeable. Maybe we
> > > > > can parameterize the batch size based on the code path.
> > > > >
> > > > > Nhat, what do you think?
> > > >
> > > > Nhat, since we now have the referenced-based second chance algorithm,
> > > > should we consider doing batch writeback for shrink_memcg() as well?
> > >
> > > I just take a look at shrink_memcg() and realized it's reclaiming one
> > > page at a time. Thanks for the reminder - I hated it.
> > >
> > > Please batchify it if it makes your life easier :) We don't reclaim
> > > "just one page/object" anywhere else in the reclaim path, Sure, it
> > > adds a bit of latency to zswap_store() if we reached cgroup limit, but
> > > IMHO if we hit zswap.max limit at zswap_store() time, that is already
> > > the slowest of slow path that you should have avoided with proactive
> > > reclaim/zswap shrinker in the first place. And, setting zswap.max does
> > > not make sense to me in the first place (I can write a whole essay
> > > about it).
> >
> > Should we batchify shrink_memcg() from the shrinker and background
> > writeback, but leave the synchronous zswap_store() path to reclaim one
> > page for this series at least to avoid potential regressions?
> >
> > I think this change specifically needs more intensive testing (vs the
> > other code paths).
>
> I'm fine with having shrink_memcg() takes a batch_size argument for now :)
>
> I suspect not a lot of people invokes the shrink_memcg() synchronous
> path in zswap store though. Setting zswap.max is hard (as it involves
> guessing compression ratio ahead of time) and induces quite a bit of
> overhead (obj_cgroup_may_zswap() does a force flush for every store if
> you set zswap.max to a value other than 0 and max).
Yeah I agree, but I just wanna make sure we don't completely kill
performance in case anyone is actually doing that.
Regarding the force flush, it's unfortunate that we need to rely on a
stat for internal limit checking. It might make sense to use page
counters here, which already support hierarchical charging and
whatnot. We'll need something similar to the objcg approach to charge
sub-pages (or maybe just reuse that somehow?).
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Nhat Pham @ 2026-06-03 18:34 UTC (permalink / raw)
To: Yosry Ahmed
Cc: Hao Jia, 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: <CAO9r8zMBUMXy_bkeT8z+M=dXayU=6VGEw+-HmfDWR2fyJy=z+A@mail.gmail.com>
On Wed, Jun 3, 2026 at 11:26 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > > > Is the main difference that we are scanning in batches here? I think we
> > > > can have shrink_memcg() do that too. If anything, it might make the
> > > > shrinker more efficient. Over-reclaim is ofc a concern, and especially
> > > > in the zswap_store() path as the overhead can be noticeable. Maybe we
> > > > can parameterize the batch size based on the code path.
> > > >
> > > > Nhat, what do you think?
> > >
> > > Nhat, since we now have the referenced-based second chance algorithm,
> > > should we consider doing batch writeback for shrink_memcg() as well?
> >
> > I just take a look at shrink_memcg() and realized it's reclaiming one
> > page at a time. Thanks for the reminder - I hated it.
> >
> > Please batchify it if it makes your life easier :) We don't reclaim
> > "just one page/object" anywhere else in the reclaim path, Sure, it
> > adds a bit of latency to zswap_store() if we reached cgroup limit, but
> > IMHO if we hit zswap.max limit at zswap_store() time, that is already
> > the slowest of slow path that you should have avoided with proactive
> > reclaim/zswap shrinker in the first place. And, setting zswap.max does
> > not make sense to me in the first place (I can write a whole essay
> > about it).
>
> Should we batchify shrink_memcg() from the shrinker and background
> writeback, but leave the synchronous zswap_store() path to reclaim one
> page for this series at least to avoid potential regressions?
>
> I think this change specifically needs more intensive testing (vs the
> other code paths).
I'm fine with having shrink_memcg() takes a batch_size argument for now :)
I suspect not a lot of people invokes the shrink_memcg() synchronous
path in zswap store though. Setting zswap.max is hard (as it involves
guessing compression ratio ahead of time) and induces quite a bit of
overhead (obj_cgroup_may_zswap() does a force flush for every store if
you set zswap.max to a value other than 0 and max).
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Yosry Ahmed @ 2026-06-03 18:26 UTC (permalink / raw)
To: Nhat Pham
Cc: Hao Jia, 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=PoBZ4ci30tKHQXs1o9=NDpPrtbe7RxxZTbnzVJf74ZYQ@mail.gmail.com>
> > > Is the main difference that we are scanning in batches here? I think we
> > > can have shrink_memcg() do that too. If anything, it might make the
> > > shrinker more efficient. Over-reclaim is ofc a concern, and especially
> > > in the zswap_store() path as the overhead can be noticeable. Maybe we
> > > can parameterize the batch size based on the code path.
> > >
> > > Nhat, what do you think?
> >
> > Nhat, since we now have the referenced-based second chance algorithm,
> > should we consider doing batch writeback for shrink_memcg() as well?
>
> I just take a look at shrink_memcg() and realized it's reclaiming one
> page at a time. Thanks for the reminder - I hated it.
>
> Please batchify it if it makes your life easier :) We don't reclaim
> "just one page/object" anywhere else in the reclaim path, Sure, it
> adds a bit of latency to zswap_store() if we reached cgroup limit, but
> IMHO if we hit zswap.max limit at zswap_store() time, that is already
> the slowest of slow path that you should have avoided with proactive
> reclaim/zswap shrinker in the first place. And, setting zswap.max does
> not make sense to me in the first place (I can write a whole essay
> about it).
Should we batchify shrink_memcg() from the shrinker and background
writeback, but leave the synchronous zswap_store() path to reclaim one
page for this series at least to avoid potential regressions?
I think this change specifically needs more intensive testing (vs the
other code paths).
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Nhat Pham @ 2026-06-03 18:23 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: <ea2c1323-1440-e927-f14a-0eac54a245bf@gmail.com>
On Wed, Jun 3, 2026 at 4:27 AM Hao Jia <jiahao.kernel@gmail.com> wrote:
>
>
>
> On 2026/5/30 09:37, Yosry Ahmed wrote:
> > On Tue, May 26, 2026 at 07:45:59PM +0800, Hao Jia wrote:
> >> From: Hao Jia <jiahao1@lixiang.com>
> >>
> >> Zswap currently writes back pages to backing swap reactively, triggered
> >> either by the shrinker or when the pool reaches its size limit. There is
> >> no mechanism to control the amount of writeback for a specific memory
> >> cgroup. However, users may want to proactively write back zswap pages,
> >> e.g., to free up memory for other applications or to prepare for
> >> memory-intensive workloads.
> >>
> >> Introduce a "zswap_writeback_only" key to the memory.reclaim cgroup
> >> interface. When specified, this key bypasses standard memory reclaim
> >> and exclusively performs proactive zswap writeback up to the requested
> >> budget. If omitted, the default reclaim behavior remains unchanged.
> >>
> >> Example usage:
> >> # Write back 100MB of pages from zswap to the backing swap
> >> echo "100M zswap_writeback_only" > memory.reclaim
> >>
> >> Note that the actual amount written back may be less than requested due
> >> to the zswap second-chance algorithm: referenced entries are rotated on
> >> the LRU on the first encounter and only written back on a second pass.
> >> If fewer bytes are written back than requested, -EAGAIN is returned,
> >> matching the existing memory.reclaim semantics.
> >>
> >> Internally, extend user_proactive_reclaim() to parse the new
> >> "zswap_writeback_only" token and invoke the dedicated handler. Add
> >> zswap_proactive_writeback() to walk the target memcg subtree via the
> >> per-memcg writeback cursor, draining per-node zswap LRUs through
> >> list_lru_walk_one() with the shrink_memcg_cb() callback.
> >>
> >> Suggested-by: Yosry Ahmed <yosry@kernel.org>
> >> Suggested-by: Nhat Pham <nphamcs@gmail.com>
> >> Signed-off-by: Hao Jia <jiahao1@lixiang.com>
> >> ---
> >> Documentation/admin-guide/cgroup-v2.rst | 18 +++-
> >> Documentation/admin-guide/mm/zswap.rst | 11 +-
> >> include/linux/zswap.h | 7 ++
> >> mm/vmscan.c | 14 +++
> >> mm/zswap.c | 138 ++++++++++++++++++++++++
> >> 5 files changed, 185 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> >> index 6efd0095ed99..6564abf0dec5 100644
> >> --- a/Documentation/admin-guide/cgroup-v2.rst
> >> +++ b/Documentation/admin-guide/cgroup-v2.rst
> >> @@ -1425,9 +1425,10 @@ PAGE_SIZE multiple when read back.
> >>
> >> The following nested keys are defined.
> >>
> >> - ========== ================================
> >> + ==================== ==================================================
> >> swappiness Swappiness value to reclaim with
> >> - ========== ================================
> >> + zswap_writeback_only Only perform proactive zswap writeback
> >> + ==================== ==================================================
> >>
> >> Specifying a swappiness value instructs the kernel to perform
> >> the reclaim with that swappiness value. Note that this has the
> >> @@ -1437,6 +1438,19 @@ The following nested keys are defined.
> >> The valid range for swappiness is [0-200, max], setting
> >> swappiness=max exclusively reclaims anonymous memory.
> >>
> >> + The zswap_writeback_only key skips ordinary memory reclaim and
> >> + writes back pages from zswap to the backing swap device until
> >> + the requested amount has been written or no further candidates
> >> + are found. This is useful to proactively offload cold pages from
> >> + the zswap pool to the swap device. It is only available if
> >> + zswap writeback is enabled. zswap_writeback_only cannot be combined
> >> + with swappiness; specifying both returns -EINVAL.
> >> +
> >> + Example::
> >> +
> >> + # Write back up to 100MB of pages from zswap to the backing swap
> >> + echo "100M zswap_writeback_only" > memory.reclaim
> >
> >
> > memcg folks need to chime in about the interface here. An alternative
> > would be a separate interface (e.g. memory.zswap.do_writeback or
> > memory.zswap.writeback.reclaim or sth).
> >
> >> diff --git a/mm/zswap.c b/mm/zswap.c
> >> index 73e64a635690..7bcbf788f634 100644
> >> --- a/mm/zswap.c
> >> +++ b/mm/zswap.c
> >> @@ -1679,6 +1679,144 @@ int zswap_load(struct folio *folio)
> >> return 0;
> >> }
> >>
> >> +/*
> >> + * Maximum LRU scan limit:
> >> + * number of entries to scan per page of remaining budget.
> >> + */
> >> +#define ZSWAP_PROACTIVE_WB_SCAN_RATIO 16UL
> >> +/*
> >> + * Batch size for proactive writeback:
> >> + * - As the per-memcg writeback target in the outer memcg loop.
> >> + * - As the per-walk budget passed to list_lru_walk_one().
> >> + */
> >> +#define ZSWAP_PROACTIVE_WB_BATCH 128UL
> >> +
> >> +/*
> >> + * Walk the per-node LRUs of @memcg to write back up to @nr_to_write pages.
> >> + * Returns the number of pages written back, or -ENOENT if @memcg is a
> >> + * zombie or has writeback disabled.
> >> + */
> >> +static long zswap_proactive_shrink_memcg(struct mem_cgroup *memcg,
> >> + unsigned long nr_to_write)
> >> +{
> >> + unsigned long nr_written = 0;
> >> + int nid;
> >> +
> >> + if (!mem_cgroup_zswap_writeback_enabled(memcg))
> >> + return -ENOENT;
> >> +
> >> + if (!mem_cgroup_online(memcg))
> >> + return -ENOENT;
> >> +
> >> + for_each_node_state(nid, N_NORMAL_MEMORY) {
> >> + bool encountered_page_in_swapcache = false;
> >> + unsigned long nr_to_scan, nr_scanned = 0;
> >> +
> >> + /*
> >> + * Cap by LRU length: bounds rewalks when referenced
> >> + * entries keep rotating to the tail.
> >> + */
> >> + nr_to_scan = list_lru_count_one(&zswap_list_lru, nid, memcg);
> >> + if (!nr_to_scan)
> >> + continue;
> >> +
> >> + /*
> >> + * Cap by SCAN_RATIO * remaining budget: bounds scan cost
> >> + * to the remaining writeback budget.
> >> + */
> >> + nr_to_scan = min(nr_to_scan,
> >> + (nr_to_write - nr_written) * ZSWAP_PROACTIVE_WB_SCAN_RATIO);
> >> +
> >> + while (nr_scanned < nr_to_scan) {
> >> + unsigned long nr_to_walk = min(ZSWAP_PROACTIVE_WB_BATCH,
> >> + nr_to_scan - nr_scanned);
> >> +
> >> + if (signal_pending(current))
> >> + return nr_written;
> >> +
> >> + /*
> >> + * Account for the committed budget rather than the walker's
> >> + * actual delta. If the list is emptied concurrently, the
> >> + * walker visits nothing and nr_scanned would never advance.
> >> + */
> >> + nr_scanned += nr_to_walk;
> >> +
> >> + nr_written += list_lru_walk_one(&zswap_list_lru, nid, memcg,
> >> + &shrink_memcg_cb,
> >> + &encountered_page_in_swapcache,
> >> + &nr_to_walk);
> >> +
> >> + if (nr_written >= nr_to_write)
> >> + return nr_written;
> >> + if (encountered_page_in_swapcache)
> >> + break;
> >> +
> >> + cond_resched();
> >> + }
> >> + }
> >> +
> >> + return nr_written;
> >> +}
> >> +
> >> +int zswap_proactive_writeback(struct mem_cgroup *memcg,
> >> + unsigned long nr_to_writeback)
> >> +{
> >> + struct mem_cgroup *iter_memcg;
> >> + unsigned long nr_written = 0;
> >> + int failures = 0, attempts = 0;
> >> +
> >> + if (!memcg)
> >> + return -EINVAL;
> >> + if (!nr_to_writeback)
> >> + return 0;
> >> +
> >> + /*
> >> + * Writeback will be aborted with -EAGAIN if we encounter
> >> + * the following MAX_RECLAIM_RETRIES times:
> >> + * - No writeback-candidate memcgs found in a subtree walk.
> >> + * - A writeback-candidate memcg wrote back zero pages.
> >> + */
> >> + while (nr_written < nr_to_writeback) {
> >> + unsigned long batch_size;
> >> + long shrunk;
> >> +
> >> + if (signal_pending(current))
> >> + return -EINTR;
> >> +
> >> + iter_memcg = zswap_mem_cgroup_iter(memcg);
> >> +
> >> + if (!iter_memcg) {
> >> + /*
> >> + * Continue without incrementing failures if we found
> >> + * candidate memcgs in the last subtree walk.
> >> + */
> >> + if (!attempts && ++failures == MAX_RECLAIM_RETRIES)
> >> + return -EAGAIN;
> >> + attempts = 0;
> >> + continue;
> >> + }
> >> +
> >> + batch_size = min(nr_to_writeback - nr_written,
> >> + ZSWAP_PROACTIVE_WB_BATCH);
> >> + shrunk = zswap_proactive_shrink_memcg(iter_memcg, batch_size);
> >> + mem_cgroup_put(iter_memcg);
> >> +
> >> + /* Writeback-disabled or offline: skip without counting. */
> >> + if (shrunk == -ENOENT)
> >> + continue;
> >> +
> >> + ++attempts;
> >> + if (shrunk > 0)
> >> + nr_written += shrunk;
> >> + else if (++failures == MAX_RECLAIM_RETRIES)
> >> + return -EAGAIN;
> >> +
> >> + cond_resched();
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > There is a lot of copy+paste from shrink_worker() and shrink_memcg()
> > here. We really should be able to reuse shrink_memcg().
> >
>
> I will do some consolidation and code reuse in the next version.
>
> > Is the main difference that we are scanning in batches here? I think we
> > can have shrink_memcg() do that too. If anything, it might make the
> > shrinker more efficient. Over-reclaim is ofc a concern, and especially
> > in the zswap_store() path as the overhead can be noticeable. Maybe we
> > can parameterize the batch size based on the code path.
> >
> > Nhat, what do you think?
>
> Nhat, since we now have the referenced-based second chance algorithm,
> should we consider doing batch writeback for shrink_memcg() as well?
I just take a look at shrink_memcg() and realized it's reclaiming one
page at a time. Thanks for the reminder - I hated it.
Please batchify it if it makes your life easier :) We don't reclaim
"just one page/object" anywhere else in the reclaim path, Sure, it
adds a bit of latency to zswap_store() if we reached cgroup limit, but
IMHO if we hit zswap.max limit at zswap_store() time, that is already
the slowest of slow path that you should have avoided with proactive
reclaim/zswap shrinker in the first place. And, setting zswap.max does
not make sense to me in the first place (I can write a whole essay
about it).
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Nhat Pham @ 2026-06-03 18:14 UTC (permalink / raw)
To: Yosry Ahmed
Cc: Hao Jia, Johannes Weiner, akpm, tj, shakeel.butt, mhocko, mkoutny,
chengming.zhou, muchun.song, roman.gushchin, cgroups, linux-mm,
linux-kernel, linux-doc, Hao Jia
In-Reply-To: <aiBqzOtEv5iAC_qC@google.com>
On Wed, Jun 3, 2026 at 10:58 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Wed, Jun 03, 2026 at 07:22:36PM +0800, Hao Jia wrote:
> >
> >
> > On 2026/5/30 09:40, Yosry Ahmed wrote:
> > > On Fri, May 29, 2026 at 12:58:09PM -0700, Nhat Pham wrote:
> > > > On Tue, May 26, 2026 at 4:46 AM Hao Jia <jiahao.kernel@gmail.com> wrote:
> > > > >
> > > > > From: Hao Jia <jiahao1@lixiang.com>
> > > > >
> > > > > Zswap currently writes back pages to backing swap reactively, triggered
> > > > > either by the shrinker or when the pool reaches its size limit. There is
> > > > > no mechanism to control the amount of writeback for a specific memory
> > > > > cgroup. However, users may want to proactively write back zswap pages,
> > > > > e.g., to free up memory for other applications or to prepare for
> > > > > memory-intensive workloads.
> > > > >
> > > > > Introduce a "zswap_writeback_only" key to the memory.reclaim cgroup
> > > > > interface. When specified, this key bypasses standard memory reclaim
> > > > > and exclusively performs proactive zswap writeback up to the requested
> > > > > budget. If omitted, the default reclaim behavior remains unchanged.
> > > > >
> > > > > Example usage:
> > > > > # Write back 100MB of pages from zswap to the backing swap
> > > > > echo "100M zswap_writeback_only" > memory.reclaim
> > > >
> > > > Hmmm, so this 100MB is the pre-compression size? i.e if this 100 MB
> > > > compresses to 25 MB, then you're only freeing 25 MB?
> > > >
> > > > I'm ok-ish with this, but can you document it?
> > >
> > > That's a good point. I think pre-compressed size doesn't make sense to
> > > be honest. We should care about how much memory we are actually trying
> > > to save by doing writeback here.
> > >
> > > The pre-compressed size is only useful in determining the blast radius,
> > > how many actual pages are going to have slower page faults now. But
> > > then, I don't think there's a reasonable way for userspace to decide
> > > that.
> > >
> > > I understand passing in the compressed size is tricky because we need to
> > > keep track of the size of the compressed pages we end up writing back,
> > > but it should be doable.
> >
> > Agreed. Using pre-compressed size is probably easier to implement. IIRC,
> > interfaces like ZRAM writeback_limit are also calculated using the
> > pre-compressed size.
> >
> > I'll clarify this in the documentation in the next version.
> >
> > >
> > > If we really want pre-compressed size here, then yes we need to make it
> > > very clear, and I vote that we use a separate interface in this case
> > > because memory.reclaim having different meanings for the amount of
> > > memory written to it is extremely counter-intuitive.
> > >
> > Agree. This would indeed break the semantics of memory.reclaim. I will use a
> > separate interface for proactive writeback in the next version.
>
> But doesn't it make more sense to specify the compressed size, which is
> ultimately the amount of memory you actually want to reclaim.
>
I personally prefer compressed size to pre-compressed size. That's
kinda what user cares about, no?
One thing we can do is let users prescribe a compressed size, but
internally, we can multiply that by the average compression ratio.
That gives us a guesstimate of how many pages we need to reclaim, and
you can follow the rest of your implementation as is (perhaps with
short-circuit when we reach the goal with fewer pages reclaimed).
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Yosry Ahmed @ 2026-06-03 17:58 UTC (permalink / raw)
To: Hao Jia, Nhat Pham, Johannes Weiner
Cc: akpm, tj, shakeel.butt, mhocko, mkoutny, chengming.zhou,
muchun.song, roman.gushchin, cgroups, linux-mm, linux-kernel,
linux-doc, Hao Jia
In-Reply-To: <6deeaea7-3cd1-4403-29fc-d2dc55c297f8@gmail.com>
On Wed, Jun 03, 2026 at 07:22:36PM +0800, Hao Jia wrote:
>
>
> On 2026/5/30 09:40, Yosry Ahmed wrote:
> > On Fri, May 29, 2026 at 12:58:09PM -0700, Nhat Pham wrote:
> > > On Tue, May 26, 2026 at 4:46 AM Hao Jia <jiahao.kernel@gmail.com> wrote:
> > > >
> > > > From: Hao Jia <jiahao1@lixiang.com>
> > > >
> > > > Zswap currently writes back pages to backing swap reactively, triggered
> > > > either by the shrinker or when the pool reaches its size limit. There is
> > > > no mechanism to control the amount of writeback for a specific memory
> > > > cgroup. However, users may want to proactively write back zswap pages,
> > > > e.g., to free up memory for other applications or to prepare for
> > > > memory-intensive workloads.
> > > >
> > > > Introduce a "zswap_writeback_only" key to the memory.reclaim cgroup
> > > > interface. When specified, this key bypasses standard memory reclaim
> > > > and exclusively performs proactive zswap writeback up to the requested
> > > > budget. If omitted, the default reclaim behavior remains unchanged.
> > > >
> > > > Example usage:
> > > > # Write back 100MB of pages from zswap to the backing swap
> > > > echo "100M zswap_writeback_only" > memory.reclaim
> > >
> > > Hmmm, so this 100MB is the pre-compression size? i.e if this 100 MB
> > > compresses to 25 MB, then you're only freeing 25 MB?
> > >
> > > I'm ok-ish with this, but can you document it?
> >
> > That's a good point. I think pre-compressed size doesn't make sense to
> > be honest. We should care about how much memory we are actually trying
> > to save by doing writeback here.
> >
> > The pre-compressed size is only useful in determining the blast radius,
> > how many actual pages are going to have slower page faults now. But
> > then, I don't think there's a reasonable way for userspace to decide
> > that.
> >
> > I understand passing in the compressed size is tricky because we need to
> > keep track of the size of the compressed pages we end up writing back,
> > but it should be doable.
>
> Agreed. Using pre-compressed size is probably easier to implement. IIRC,
> interfaces like ZRAM writeback_limit are also calculated using the
> pre-compressed size.
>
> I'll clarify this in the documentation in the next version.
>
> >
> > If we really want pre-compressed size here, then yes we need to make it
> > very clear, and I vote that we use a separate interface in this case
> > because memory.reclaim having different meanings for the amount of
> > memory written to it is extremely counter-intuitive.
> >
> Agree. This would indeed break the semantics of memory.reclaim. I will use a
> separate interface for proactive writeback in the next version.
But doesn't it make more sense to specify the compressed size, which is
ultimately the amount of memory you actually want to reclaim.
Johannes, Nhat, WDYT?
^ permalink raw reply
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Yosry Ahmed @ 2026-06-03 17:55 UTC (permalink / raw)
To: Hao Jia
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: <ea2c1323-1440-e927-f14a-0eac54a245bf@gmail.com>
> > Is the main difference that we are scanning in batches here? I think we
> > can have shrink_memcg() do that too. If anything, it might make the
> > shrinker more efficient. Over-reclaim is ofc a concern, and especially
> > in the zswap_store() path as the overhead can be noticeable. Maybe we
> > can parameterize the batch size based on the code path.
> >
> > Nhat, what do you think?
>
> Nhat, since we now have the referenced-based second chance algorithm, should
> we consider doing batch writeback for shrink_memcg() as well?
>
> Of course, we could pass a parameter to control whether batch writeback is
> needed, so as to preserve the original behavior of shrink_memcg().
Yeah probably best to parameterize the batch size and keep the current
behavior of the shrinker for this series. We can play with using batches
for the shrinker later.
>
> Thanks,
> Hao
^ permalink raw reply
* Re: [PATCH v3 1/4] mm/zswap: Make shrink_worker writeback cursor per-memcg
From: Yosry Ahmed @ 2026-06-03 17:53 UTC (permalink / raw)
To: Hao Jia
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: <c7870fe2-3588-79db-cbfb-bd6a2b78f594@gmail.com>
On Wed, Jun 03, 2026 at 11:02:54AM +0800, Hao Jia wrote:
>
>
> On 2026/6/3 07:19, Yosry Ahmed wrote:
> > > > > > > Proactive writeback also wants a similar per-memcg cursor that is
> > > > > > > scoped to the specified memcg, so that repeated invocations against
> > > > > > > the same memcg make forward progress across its descendant memcgs
> > > > > > > instead of restarting from the first child memcg each time.
> > > > > >
> > > > > > Is this a problem in practice?
> > > > > >
> > > > > > Is the concern the overhead of scanning memcgs repeatedly, or lack of
> > > > > > fairness? I wonder if we should just do writeback in batches from all
> > > > > > memcgs, similar to how reclaim does it, then evaluate at the end if we
> > > > > > need to start over?
> > > > > >
> > > > >
> > > > > Not using a per-cgroup cursor will cause issues for "repeated small-budget
> > > > > calls" cases. For example, repeatedly triggering a 2MB writeback might
> > > > > result in only writing back pages from the first few child memcgs every
> > > > > time. In the worst-case scenario (where the writeback amount is less than
> > > > > WB_BATCH), it might only ever write back from the first child memcg.
> > > >
> > > > Right, so a fairness concern?
> > > >
> > > > I wonder if we should just reclaim a batch from each memcg, then check
> > > > if we reached the goal, otherwise start over. If the batch size is small
> > > > enough that should work?
> > >
> > > Even with a small batch size, for small writeback requests triggered by
> > > user-space (e.g., 2MB, which is batch size * N), it might still repeatedly
> > > write back from only the first N child memcgs.
> >
> > Yes, I understand, I am asking if this is a problem in practice. For
> > this to be a problem we'd need to trigger small writeback requests and
> > have many memcgs.
> >
> > > This could cause the user-space agent to prematurely give up on zswap
> > > writeback.
> >
> > Why? The kernel should not return before trying to writeback from all
> > memcgs. If we scan the first N child memcgs and did not writeback
> > enough, we should keep going, right?
> >
>
> Yes, this issue is not caused by the kernel, but rather by our user-space
> agent itself.
>
> For instance, suppose a parent memcg has two children, memcg1 and memcg2,
> each with 200MB of zswap (100MB inactive). Triggering proactive writeback on
> the parent memcg will exhaust memcg1's inactive zswap pages. After that,
> even though memcg2 still has plenty of inactive zswap pages, it will
> continue to write back memcg1's active zswap pages. Writing back active
> zswap pages causes the user-space agent to prematurely abort the writeback
> because it detects that certain memcg metrics have exceeded predefined
> thresholds.
This will only happen if the reclaim size is smaller than the batch
size, right? Otherwise the kernel should reclaim more or less equally
from both memcgs?
> Of course, real-world scenarios are much more complex, and this kind of case
> is extremely rare in our environment.
>
> That being said, your suggestion of using the global lock for the per-memcg
> cursors makes the writeback fairer and would resolve these corner cases.
Right, but I'd rather not do per-memcg cursors at all if we can avoid
it. Will using batches help make reclaim fair over all memcgs without a
cursor?
We can always add the cursor later if needed.
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Nhat Pham @ 2026-06-03 17:22 UTC (permalink / raw)
To: Yosry Ahmed
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=MWX9KkSFAoN4xEMg3b+gZUN9=yd7rirAWG5NOBf26eAg@mail.gmail.com>
On Wed, Jun 3, 2026 at 10:12 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Tue, Jun 2, 2026 at 6:29 PM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > > 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
> > >
> > > We still have room for 3 more future backend types, for e.g. CRAM, i.e
> > > compressed-CXL-as-swap, which is laid out in [10] and [11]. Worst
> > > case scenario, we can add more fields to this extended struct.
> > >
> > > Other design points:
> > > - Both vswap entries (Case 1) and directly-mapped physical entries
> > > (Case 2) coexist as first-class citizens. All the common swap
> > > code paths — swapout, swapin, swap freeing, swapoff, zswap
> > > writeback, THP swapin, etc. work for both. When CONFIG_VSWAP=n,
> > > the vswap branches compile out and behavior should be identical to
> > > today's swap-table P4 (at least that is my intention).
> > > - Pointer-tagged swap_table on physical clusters for rmap (physical
> > > -> virtual) lookup.
> > > - Virtual swap slots not backed by physical swap are not charged to
> > > memcg swap counters — only physical backing is charged (I made the
> > > case for this in [7]).
> > > - Careful separation of vswap and physical swap allocation paths and
> > > structures adds a lot of complexity, but is crucial to make sure
> > > both paths are efficient and do not conflict with each other (for
> > > correctness and performance). I do re-use a lot of the allocation
> > > logic wherever possible though.
> >
> > Thanks for working on this! I mostly looked at the high-level design and
>
> Thank you for initiating this effort in LSFMMBPF 2023 (god, time
> flies). I was very excited by your presentation and decided to take a
> stab at it :)
>
> (I'll be sure to mention the full context in a non-RFC version - it
> has a lot of gems in our technical discussions).
>
> > the zswap parts, as the swap code has changed a lot since I was familiar
> > with it :)
>
> It has changed a lot since 6.19, when I was working on v6. Very
> exciting time to be a (z)swap developer right now - we have new ideas
> and new features every other week :) Reviewing code has been quite a
> joy (albeit a lot of work).
>
> >
> > It seems like the direction being taken here is that we have one
> > (massive) vswap swap device, and we keep normal physical swap devices
> > around as well.
>
> Yep.
>
> >
> > A vswap entry can point at a physical swap entry, or zswap, or zeromap.
> > If a vswap entry points at a physical swap entry, then the physical swap
> > entry points back at the vswap entry (a reverse mapping).
>
> Yep.
>
> >
> > I assume the main reason here is to avoid the extra overhead if
> > everything uses vswap, which would mainly be the reverse mapping
> > overhead? I guess there's also some simplicity that comes from reusing
> > the swap info infra as a whole, including the swap table.
>
> Yeah it helps a lot that we don't have to rewrite the whole allocator
> and swap entry reference counting logic again :)
>
> >
> > I don't like that the code bifurcates for vswap vs. normal swap entries
> > though. Not sure if this is an issue that can be fixed with proper
> > abstractions to hide it, or if the design needs modifications. I was
> > honestly really hoping we don't end up with this. I was hoping that the
> > physical swap device no longer uses a full swap table and all, and
> > everything goes through vswap.
> >
> > I hoping that if redirection isn't needed (e.g. zswap is disabled),
> > vswap can directly encode the physical swap slot so that the reverse
> > mapping isn't needed -- so we avoid the overhead without keeping the
> > physical swap device using a fully-fledged swap table.
>
> Can you expand on "vswap can directly encode the physical swap slot"?
> I'm not sure I follow here.
>
> >
> > All that being said, perhaps I am too out of touch with the code to
> > realize it's simply not possible.
> >
> > Honestly, if the main reason we can't have a single swap table for vswap
> > is saving 8 bytes on the reverse mapping, it sounds like a weak-ish
> > argument, even if we can't optimize the reverse mapping away. But maybe
> > I am also out of touch with RAM prices :)
>
> In terms of the space overhead I do agree, FWIW :)
>
> I think the other concern is the indirection overhead with going
> through the xarray for every swap operation, hence the per-CPU vswap
> cluster lookup caching idea:
>
> https://lore.kernel.org/all/20260505153854.1612033-23-nphamcs@gmail.com/
>
> >
> > I at least hope that, the current design is not painting us into a
> > corner (e.g. through userspace interfaces), and we can still achieve a
> > vswap-for-all implementation in the future (maybe that's what you have
> > in mind already?).
>
> That's still my plan. Operationally speaking, I want to make this
> completely transparent to users, with minimal to no performance
> overhead.
I do want to add that, even without achieving this, the current design
already enables a lot of use cases. I think it is a good compromise to
maintain both virtual and directly mapped physical swap entries for
now, and revisit the conversation of whether we can afford a mandatory
vswap layer once all the optimizations have been done :)
We should strive to simplify the codebase, and it will naturally
happen when the original overhead concern is no longer there. A
swap-related example: a few years ago, everyone thought swap slot
cache was needed. But then, Kairui optimized the swap allocator's lock
contention issue away, and that swap slot cache is suddenly redundant.
That finally allowed us to get rid of it. Similar thing happened (or
is happening?) with the SWP_SYNCHRONOUS_IO swapcache-skipping
heuristics.
^ permalink raw reply
* Re: [RFC PATCH 0/5] mm, swap: Virtual Swap Space (Swap Table Edition)
From: Nhat Pham @ 2026-06-03 17:12 UTC (permalink / raw)
To: Yosry Ahmed
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: <ah-A2gQ0GPgerXop@google.com>
On Tue, Jun 2, 2026 at 6:29 PM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > 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
> >
> > We still have room for 3 more future backend types, for e.g. CRAM, i.e
> > compressed-CXL-as-swap, which is laid out in [10] and [11]. Worst
> > case scenario, we can add more fields to this extended struct.
> >
> > Other design points:
> > - Both vswap entries (Case 1) and directly-mapped physical entries
> > (Case 2) coexist as first-class citizens. All the common swap
> > code paths — swapout, swapin, swap freeing, swapoff, zswap
> > writeback, THP swapin, etc. work for both. When CONFIG_VSWAP=n,
> > the vswap branches compile out and behavior should be identical to
> > today's swap-table P4 (at least that is my intention).
> > - Pointer-tagged swap_table on physical clusters for rmap (physical
> > -> virtual) lookup.
> > - Virtual swap slots not backed by physical swap are not charged to
> > memcg swap counters — only physical backing is charged (I made the
> > case for this in [7]).
> > - Careful separation of vswap and physical swap allocation paths and
> > structures adds a lot of complexity, but is crucial to make sure
> > both paths are efficient and do not conflict with each other (for
> > correctness and performance). I do re-use a lot of the allocation
> > logic wherever possible though.
>
> Thanks for working on this! I mostly looked at the high-level design and
Thank you for initiating this effort in LSFMMBPF 2023 (god, time
flies). I was very excited by your presentation and decided to take a
stab at it :)
(I'll be sure to mention the full context in a non-RFC version - it
has a lot of gems in our technical discussions).
> the zswap parts, as the swap code has changed a lot since I was familiar
> with it :)
It has changed a lot since 6.19, when I was working on v6. Very
exciting time to be a (z)swap developer right now - we have new ideas
and new features every other week :) Reviewing code has been quite a
joy (albeit a lot of work).
>
> It seems like the direction being taken here is that we have one
> (massive) vswap swap device, and we keep normal physical swap devices
> around as well.
Yep.
>
> A vswap entry can point at a physical swap entry, or zswap, or zeromap.
> If a vswap entry points at a physical swap entry, then the physical swap
> entry points back at the vswap entry (a reverse mapping).
Yep.
>
> I assume the main reason here is to avoid the extra overhead if
> everything uses vswap, which would mainly be the reverse mapping
> overhead? I guess there's also some simplicity that comes from reusing
> the swap info infra as a whole, including the swap table.
Yeah it helps a lot that we don't have to rewrite the whole allocator
and swap entry reference counting logic again :)
>
> I don't like that the code bifurcates for vswap vs. normal swap entries
> though. Not sure if this is an issue that can be fixed with proper
> abstractions to hide it, or if the design needs modifications. I was
> honestly really hoping we don't end up with this. I was hoping that the
> physical swap device no longer uses a full swap table and all, and
> everything goes through vswap.
>
> I hoping that if redirection isn't needed (e.g. zswap is disabled),
> vswap can directly encode the physical swap slot so that the reverse
> mapping isn't needed -- so we avoid the overhead without keeping the
> physical swap device using a fully-fledged swap table.
Can you expand on "vswap can directly encode the physical swap slot"?
I'm not sure I follow here.
>
> All that being said, perhaps I am too out of touch with the code to
> realize it's simply not possible.
>
> Honestly, if the main reason we can't have a single swap table for vswap
> is saving 8 bytes on the reverse mapping, it sounds like a weak-ish
> argument, even if we can't optimize the reverse mapping away. But maybe
> I am also out of touch with RAM prices :)
In terms of the space overhead I do agree, FWIW :)
I think the other concern is the indirection overhead with going
through the xarray for every swap operation, hence the per-CPU vswap
cluster lookup caching idea:
https://lore.kernel.org/all/20260505153854.1612033-23-nphamcs@gmail.com/
>
> I at least hope that, the current design is not painting us into a
> corner (e.g. through userspace interfaces), and we can still achieve a
> vswap-for-all implementation in the future (maybe that's what you have
> in mind already?).
That's still my plan. Operationally speaking, I want to make this
completely transparent to users, with minimal to no performance
overhead.
The next action item is to optimize for vswap-on-fast-swapfile case -
that was Kairui's main concerns regarding performance. I spent a lot
of time perfing and fixing issues for this case in v6. The issues with
the most egregious effects and simplest fix (vswap-less
swap-cache-only check for e.g) are already fixed in this new design,
and eventually I will move the rest (lookup caching) and more to here.
>
> Aside from the swap code, the only sticking point for me is the logic
> bifurcation in zswap. Why does zswap need to handle vswap vs. not vswap?
> I thought the point of the design is to use vswap when zswap is used,
> and otherwise use a normal swap table. In a way, one of the goals is to
> make zswap a first class swap citizen, but it doesn't seem like we are
> achieving that?
We already have all the machinery to make zswap completely
independent. Right now, if you use vswap, you'll skip the zswap's
internal xarray entirely, and just store a zswap entry in the virtual
swap cluster's vtable.
I just haven't removed the old code for 2 reasons:
1. Reduce the delta on this RFC, to ease the burden for reviewers (and
definitely not because I'm lazy :P)
2. The only other practical reason is so that we can let users compile
with !CONFIG_VSWAP and still uses zswap on top of the old swapfile
setup during the transition/experimentation period for now.
But logically and conceptually speaking, there is no reason I can come
up with to use zswap on without vswap. The CPU indirection overhead is
already partially there (since zswap uses an xarray) and further
optimized (cluster loopup caching etc.), as well as the space overhead
(vswap replaces the zswap xarray). I actually wrote a whole paragraph
about how we should always go for vswap if we're using zswap, but then
decide to remove it since there's no code for it yet.
If folks like it, what I can do is have CONFIG_ZSWAP depends on
CONFIG_VSWAP, removes all the non-vswap logic, and call it a day? :)
Then, on the swap allocation side, if vswap allocation fail and zswap
writeback is disabled, we can error out early.
^ permalink raw reply
* Re: [PATCH 0/5] blk-cgroup: fix blkg list and policy data races
From: Jens Axboe @ 2026-06-03 15:08 UTC (permalink / raw)
To: yukuai, Yu Kuai
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
In-Reply-To: <189b8601-87a6-423a-b267-9d550c31c086@fnnas.com>
On 6/3/26 9:06 AM, yukuai wrote:
> Hi,
>
> ? 2026/6/3 22:35, Jens Axboe ??:
>> On 6/3/26 7:27 AM, Yu Kuai wrote:
>>> This small series fixes races between blkg destruction, q->blkg_list
>>> iteration, and blkcg policy activation.
>> In spam: Authentication-Results: mx.google.com <mx.google.com>; spf=pass (google.com: <google.com:> domain of srs0=eh6w=d7=fygo.io=yukuai@kernel.org designates 2600:3c04:e001:324:0:1991:8:25 as permitted sender) smtp.mailfrom="SRS0=EH6w=D7=fygo.io=yukuai@kernel.org"; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=QUARANTINE) header.from=fygo.io <header.from=fygo.io> if you don't get this sorted out, your messages will be missed as they end up in spam for most people.
>
> Thanks again for the notice. I think I finally figured out why dmarc failed.
This email looks better!
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 0/5] blk-cgroup: fix blkg list and policy data races
From: Jens Axboe @ 2026-06-03 14:35 UTC (permalink / raw)
To: Yu Kuai
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
In-Reply-To: <cover.1780492756.git.yukuai@fygo.io>
On 6/3/26 7:27 AM, Yu Kuai wrote:
> This small series fixes races between blkg destruction, q->blkg_list
> iteration, and blkcg policy activation.
In spam:
Authentication-Results: mx.google.com; spf=pass (google.com: domain of srs0=eh6w=d7=fygo.io=yukuai@kernel.org designates 2600:3c04:e001:324:0:1991:8:25 as permitted sender) smtp.mailfrom="SRS0=EH6w=D7=fygo.io=yukuai@kernel.org"; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=QUARANTINE) header.from=fygo.io
if you don't get this sorted out, your messages will be missed as they
end up in spam for most people.
--
Jens Axboe
^ permalink raw reply
* [PATCH 5/5] blk-cgroup: factor policy pd teardown loop into helper
From: Yu Kuai @ 2026-06-03 13:27 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
In-Reply-To: <cover.1780492756.git.yukuai@fygo.io>
From: Zheng Qixing <zhengqixing@huawei.com>
Move the teardown sequence which offlines and frees per-policy
blkg_policy_data (pd) into a helper for readability.
No functional change intended.
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Yu Kuai <yukuai@fygo.io>
Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
block/blk-cgroup.c | 57 ++++++++++++++++++++++------------------------
1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index b479a9796fb3..c75b2a103bbc 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1575,10 +1575,35 @@ struct cgroup_subsys io_cgrp_subsys = {
.depends_on = 1 << memory_cgrp_id,
#endif
};
EXPORT_SYMBOL_GPL(io_cgrp_subsys);
+/*
+ * Tear down per-blkg policy data for @pol on @q.
+ */
+static void blkcg_policy_teardown_pds(struct request_queue *q,
+ const struct blkcg_policy *pol)
+{
+ struct blkcg_gq *blkg;
+
+ list_for_each_entry(blkg, &q->blkg_list, q_node) {
+ struct blkcg *blkcg = blkg->blkcg;
+ struct blkg_policy_data *pd;
+
+ spin_lock(&blkcg->lock);
+ pd = blkg->pd[pol->plid];
+ if (pd) {
+ if (pd->online && pol->pd_offline_fn)
+ pol->pd_offline_fn(pd);
+ pd->online = false;
+ pol->pd_free_fn(pd);
+ blkg->pd[pol->plid] = NULL;
+ }
+ spin_unlock(&blkcg->lock);
+ }
+}
+
/**
* blkcg_activate_policy - activate a blkcg policy on a gendisk
* @disk: gendisk of interest
* @pol: blkcg policy to activate
*
@@ -1690,25 +1715,11 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
return ret;
enomem:
/* alloc failed, take down everything */
spin_lock_irq(&q->queue_lock);
- list_for_each_entry(blkg, &q->blkg_list, q_node) {
- struct blkcg *blkcg = blkg->blkcg;
- struct blkg_policy_data *pd;
-
- spin_lock(&blkcg->lock);
- pd = blkg->pd[pol->plid];
- if (pd) {
- if (pd->online && pol->pd_offline_fn)
- pol->pd_offline_fn(pd);
- pd->online = false;
- pol->pd_free_fn(pd);
- blkg->pd[pol->plid] = NULL;
- }
- spin_unlock(&blkcg->lock);
- }
+ blkcg_policy_teardown_pds(q, pol);
spin_unlock_irq(&q->queue_lock);
ret = -ENOMEM;
goto out;
}
EXPORT_SYMBOL_GPL(blkcg_activate_policy);
@@ -1723,11 +1734,10 @@ EXPORT_SYMBOL_GPL(blkcg_activate_policy);
*/
void blkcg_deactivate_policy(struct gendisk *disk,
const struct blkcg_policy *pol)
{
struct request_queue *q = disk->queue;
- struct blkcg_gq *blkg;
unsigned int memflags;
if (!blkcg_policy_enabled(q, pol))
return;
@@ -1736,24 +1746,11 @@ void blkcg_deactivate_policy(struct gendisk *disk,
mutex_lock(&q->blkcg_mutex);
spin_lock_irq(&q->queue_lock);
__clear_bit(pol->plid, q->blkcg_pols);
-
- list_for_each_entry(blkg, &q->blkg_list, q_node) {
- struct blkcg *blkcg = blkg->blkcg;
-
- spin_lock(&blkcg->lock);
- if (blkg->pd[pol->plid]) {
- if (blkg->pd[pol->plid]->online && pol->pd_offline_fn)
- pol->pd_offline_fn(blkg->pd[pol->plid]);
- pol->pd_free_fn(blkg->pd[pol->plid]);
- blkg->pd[pol->plid] = NULL;
- }
- spin_unlock(&blkcg->lock);
- }
-
+ blkcg_policy_teardown_pds(q, pol);
spin_unlock_irq(&q->queue_lock);
mutex_unlock(&q->blkcg_mutex);
if (queue_is_mq(q))
blk_mq_unfreeze_queue(q, memflags);
--
2.51.0
^ permalink raw reply related
* [PATCH 4/5] blk-cgroup: skip dying blkg in blkcg_activate_policy()
From: Yu Kuai @ 2026-06-03 13:27 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
In-Reply-To: <cover.1780492756.git.yukuai@fygo.io>
From: Zheng Qixing <zhengqixing@huawei.com>
When switching IO schedulers on a block device, blkcg_activate_policy()
can race with concurrent blkcg deletion, leading to a use-after-free in
rcu_accelerate_cbs.
T1: T2:
blkg_destroy
kill(&blkg->refcnt) // blkg->refcnt=1->0
blkg_release // call_rcu(__blkg_release)
...
blkg_free_workfn
->pd_free_fn(pd)
elv_iosched_store
elevator_switch
...
iterate blkg list
blkg_get(blkg) // blkg->refcnt=0->1
list_del_init(&blkg->q_node)
blkg_put(pinned_blkg) // blkg->refcnt=1->0
blkg_release // call_rcu again
rcu_accelerate_cbs // uaf
Fix this by checking hlist_unhashed(&blkg->blkcg_node) before getting
a reference to the blkg. This is the same check used in blkg_destroy()
to detect if a blkg has already been destroyed. If the blkg is already
unhashed, skip processing it since it's being destroyed.
Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
block/blk-cgroup.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 007dfc4f9c59..b479a9796fb3 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1623,10 +1623,12 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) {
struct blkg_policy_data *pd;
if (blkg->pd[pol->plid])
continue;
+ if (hlist_unhashed(&blkg->blkcg_node))
+ continue;
/* If prealloc matches, use it; otherwise try GFP_NOWAIT */
if (blkg == pinned_blkg) {
pd = pd_prealloc;
pd_prealloc = NULL;
--
2.51.0
^ permalink raw reply related
* [PATCH 3/5] blk-cgroup: fix race between policy activation and blkg destruction
From: Yu Kuai @ 2026-06-03 13:27 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
In-Reply-To: <cover.1780492756.git.yukuai@fygo.io>
From: Zheng Qixing <zhengqixing@huawei.com>
When switching an IO scheduler on a block device, blkcg_activate_policy()
allocates blkg_policy_data (pd) for all blkgs attached to the queue.
However, blkcg_activate_policy() may race with concurrent blkcg deletion,
leading to use-after-free and memory leak issues.
The use-after-free occurs in the following race:
T1 (blkcg_activate_policy):
- Successfully allocates pd for blkg1 (loop0->queue, blkcgA)
- Fails to allocate pd for blkg2 (loop0->queue, blkcgB)
- Enters the enomem rollback path to release blkg1 resources
T2 (blkcg deletion):
- blkcgA is deleted concurrently
- blkg1 is freed via blkg_free_workfn()
- blkg1->pd is freed
T1 (continued):
- Rollback path accesses blkg1->pd->online after pd is freed
- Triggers use-after-free
In addition, blkg_free_workfn() frees pd before removing the blkg from
q->blkg_list. This allows blkcg_activate_policy() to allocate a new pd
for a blkg that is being destroyed, leaving the newly allocated pd
unreachable when the blkg is finally freed.
Fix these races by extending blkcg_mutex coverage to serialize
blkcg_activate_policy() rollback and blkg destruction, ensuring pd
lifecycle is synchronized with blkg list visibility.
Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
block/blk-cgroup.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index a98a22e06fd1..007dfc4f9c59 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1612,10 +1612,12 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
if (WARN_ON_ONCE(!pol->pd_alloc_fn || !pol->pd_free_fn))
return -EINVAL;
if (queue_is_mq(q))
memflags = blk_mq_freeze_queue(q);
+
+ mutex_lock(&q->blkcg_mutex);
retry:
spin_lock_irq(&q->queue_lock);
/* blkg_list is pushed at the head, reverse walk to initialize parents first */
list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) {
@@ -1674,10 +1676,11 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
__set_bit(pol->plid, q->blkcg_pols);
ret = 0;
spin_unlock_irq(&q->queue_lock);
out:
+ mutex_unlock(&q->blkcg_mutex);
if (queue_is_mq(q))
blk_mq_unfreeze_queue(q, memflags);
if (pinned_blkg)
blkg_put(pinned_blkg);
if (pd_prealloc)
--
2.51.0
^ permalink raw reply related
* [PATCH 2/5] bfq: protect q->blkg_list iteration in bfq_end_wr_async() with blkcg_mutex
From: Yu Kuai @ 2026-06-03 13:27 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
In-Reply-To: <cover.1780492756.git.yukuai@fygo.io>
bfq_end_wr_async() iterates q->blkg_list while only holding bfqd->lock,
but not blkcg_mutex. This can race with blkg_free_workfn() that removes
blkgs from the list while holding blkcg_mutex.
Add blkcg_mutex protection in bfq_end_wr() before taking bfqd->lock to
ensure proper synchronization when iterating q->blkg_list.
Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
block/bfq-cgroup.c | 3 ++-
block/bfq-iosched.c | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 37ab70930c8d..f765e767d36a 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -939,11 +939,12 @@ void bfq_end_wr_async(struct bfq_data *bfqd)
struct blkcg_gq *blkg;
list_for_each_entry(blkg, &bfqd->queue->blkg_list, q_node) {
struct bfq_group *bfqg = blkg_to_bfqg(blkg);
- bfq_end_wr_async_queues(bfqd, bfqg);
+ if (bfqg)
+ bfq_end_wr_async_queues(bfqd, bfqg);
}
bfq_end_wr_async_queues(bfqd, bfqd->root_group);
}
static int bfq_io_show_weight_legacy(struct seq_file *sf, void *v)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 141c602d5e85..42ccfd0c6140 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2643,10 +2643,13 @@ void bfq_end_wr_async_queues(struct bfq_data *bfqd,
static void bfq_end_wr(struct bfq_data *bfqd)
{
struct bfq_queue *bfqq;
int i;
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
+ mutex_lock(&bfqd->queue->blkcg_mutex);
+#endif
spin_lock_irq(&bfqd->lock);
for (i = 0; i < bfqd->num_actuators; i++) {
list_for_each_entry(bfqq, &bfqd->active_list[i], bfqq_list)
bfq_bfqq_end_wr(bfqq);
@@ -2654,10 +2657,13 @@ static void bfq_end_wr(struct bfq_data *bfqd)
list_for_each_entry(bfqq, &bfqd->idle_list, bfqq_list)
bfq_bfqq_end_wr(bfqq);
bfq_end_wr_async(bfqd);
spin_unlock_irq(&bfqd->lock);
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
+ mutex_unlock(&bfqd->queue->blkcg_mutex);
+#endif
}
static sector_t bfq_io_struct_pos(void *io_struct, bool request)
{
if (request)
--
2.51.0
^ permalink raw reply related
* [PATCH 1/5] blk-cgroup: protect q->blkg_list iteration in blkg_destroy_all() with blkcg_mutex
From: Yu Kuai @ 2026-06-03 13:27 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
In-Reply-To: <cover.1780492756.git.yukuai@fygo.io>
blkg_destroy_all() iterates q->blkg_list without holding blkcg_mutex,
which can race with blkg_free_workfn() that removes blkgs from the list
while holding blkcg_mutex.
Add blkcg_mutex protection around the q->blkg_list iteration to prevent
potential list corruption or use-after-free issues.
Signed-off-by: Yu Kuai <yukuai@fygo.io>
---
block/blk-cgroup.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 554c87bb4a86..a98a22e06fd1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -573,10 +573,11 @@ static void blkg_destroy_all(struct gendisk *disk)
struct blkcg_gq *blkg;
int count = BLKG_DESTROY_BATCH_SIZE;
int i;
restart:
+ mutex_lock(&q->blkcg_mutex);
spin_lock_irq(&q->queue_lock);
list_for_each_entry(blkg, &q->blkg_list, q_node) {
struct blkcg *blkcg = blkg->blkcg;
if (hlist_unhashed(&blkg->blkcg_node))
@@ -591,10 +592,11 @@ static void blkg_destroy_all(struct gendisk *disk)
* it when a batch of blkgs are destroyed.
*/
if (!(--count)) {
count = BLKG_DESTROY_BATCH_SIZE;
spin_unlock_irq(&q->queue_lock);
+ mutex_unlock(&q->blkcg_mutex);
cond_resched();
goto restart;
}
}
@@ -610,10 +612,11 @@ static void blkg_destroy_all(struct gendisk *disk)
__clear_bit(pol->plid, q->blkcg_pols);
}
q->root_blkg = NULL;
spin_unlock_irq(&q->queue_lock);
+ mutex_unlock(&q->blkcg_mutex);
wake_up_var(&q->root_blkg);
}
static void blkg_iostat_set(struct blkg_iostat *dst, struct blkg_iostat *src)
--
2.51.0
^ permalink raw reply related
* [PATCH 0/5] blk-cgroup: fix blkg list and policy data races
From: Yu Kuai @ 2026-06-03 13:27 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, Josef Bacik, Ming Lei, Nilay Shroff, Bart Van Assche,
linux-block, cgroups, linux-kernel
This small series fixes races between blkg destruction, q->blkg_list
iteration, and blkcg policy activation.
The first two patches serialize q->blkg_list walks in blkg_destroy_all()
and BFQ writeback weight-raising teardown with blkcg_mutex. The next two
patches close policy activation races with concurrent blkg destruction,
including skipping blkgs that are already dying. The final patch factors
the common policy data teardown loop.
This uses blkcg_mutex rather than extending queue_lock coverage because
the races are about blkg list visibility and policy-data lifetime, not
request-queue dispatch state. blkg_free_workfn() already uses
blkcg_mutex to serialize policy-data freeing with policy deactivation
and removes blkgs from q->blkg_list only after that teardown. Taking the
same mutex around the remaining q->blkg_list walkers gives one sleepable
serialization point for blkg lifetime, avoids adding more queue_lock
nesting, and prepares the follow-up conversion that removes queue_lock
from blkcg list protection entirely.
Yu Kuai (2):
blk-cgroup: protect q->blkg_list iteration in blkg_destroy_all() with
blkcg_mutex
bfq: protect q->blkg_list iteration in bfq_end_wr_async() with
blkcg_mutex
Zheng Qixing (3):
blk-cgroup: fix race between policy activation and blkg destruction
blk-cgroup: skip dying blkg in blkcg_activate_policy()
blk-cgroup: factor policy pd teardown loop into helper
block/bfq-cgroup.c | 3 ++-
block/bfq-iosched.c | 6 +++++
block/blk-cgroup.c | 65 ++++++++++++++++++++++++---------------------
3 files changed, 43 insertions(+), 31 deletions(-)
--
2.51.0
^ permalink raw reply
* Re: [PATCH v5 0/9] mm: switch THP shrinker to list_lru
From: Lance Yang @ 2026-06-03 11:53 UTC (permalink / raw)
To: Johannes Weiner
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: <aiAS7GxsffqXWILD@cmpxchg.org>
On 2026/6/3 19:41, Johannes Weiner wrote:
> On Wed, Jun 03, 2026 at 12:44:26PM +0800, Lance Yang wrote:
>>
>> On Tue, Jun 02, 2026 at 05:46:02PM -0400, Johannes Weiner wrote:
>>> On Mon, Jun 01, 2026 at 04:36:52PM +0800, Lance Yang wrote:
>>>> 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 :)
>>>
>>> Sorry for the delay, this took me a bit to think about. The shrinker
>>> code is a mess.
>>>
>>> I read it the same way you do. And this is true for all list_lru users
>>> when nokmem is set: we just set random nonsense shrinker bits.
>>>
>>> HOWEVER, the generic shrinker code fixes that up by IGNORING random
>>> shrinker bits like this when !memcg_kmem_online(). And shrinking
>>> correctly happens only against the shared root queue when the reclaim
>>> iterator walks root_mem_cgroup.
>>>
>>> HOWEVER, the THP shrinker explicitly sets SHRINKER_NONSLAB, which in
>>> turn overrides the previous override. So yes there is a weirdness: we
>>> get the root cgroup invocation against the shared queue, and then one
>>> more time triggered by that random memcg bit.
>>>
>>> The most direct fix is to just drop SHRINKER_NONSLAB. It declares
>>> independence from kmem, which is no longer true.
>>>
>>> Cleaning up the shrinker code is left for another day.
>>
>> Thanks for working on this!
>>
>> Wondering if this fix trades one problem for another, though ...
>>
>> Before this series, the deferred split shrinker had a real per-memcg
>> queue. Even with cgroup.memory=nokmem, memcg reclaim could still scan
>> that memcg's own deferred_split_queue:
>>
>> memcg reclaim -> deferred split shrinker -> sc->memcg->deferred_split_queue
>>
>> With the fix, nokmem + w/o SHRINKER_NONSLAB falls back to a
>> non-memcg-aware shrinker:
>>
>> memcg reclaim -> skip deferred split shrinker
>>
>> root/global reclaim -> deferred split shrinker -> shared list_lru
>>
>> Is that expected? There woud be no memcg-driven deferred split reclaim
>> under nokmem, IIUC ...
>
> Yes, this is all correct. list_lru is still inherently tied to the
> kmem component of memcg (memcg_kmem_id()).
>
> So without kmem, no isolation. But without kmem, no isolation *for a
> lot of stuff*. It's a legacy knob when slab accounting was new and
> expensive. But so many things depend on it now, disabling it just
> punches a nassive hole into memcg functionality and isolation
> coverage. It's not a sanctioned production use flag.
>
> This change is negligible from a memcg semantics POV.
Thanks for clarifying!
No strong objection from me. Just wanted to call out the nokmem
behavior change and hear what folks think :D
>> Not sure what the right fix is, as I am not a memcg expert ...
^ permalink raw reply
* Re: [PATCH v5 0/9] mm: switch THP shrinker to list_lru
From: Johannes Weiner @ 2026-06-03 11:41 UTC (permalink / raw)
To: Lance Yang
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: <20260603044426.54863-1-lance.yang@linux.dev>
On Wed, Jun 03, 2026 at 12:44:26PM +0800, Lance Yang wrote:
>
> On Tue, Jun 02, 2026 at 05:46:02PM -0400, Johannes Weiner wrote:
> >On Mon, Jun 01, 2026 at 04:36:52PM +0800, Lance Yang wrote:
> >> 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 :)
> >
> >Sorry for the delay, this took me a bit to think about. The shrinker
> >code is a mess.
> >
> >I read it the same way you do. And this is true for all list_lru users
> >when nokmem is set: we just set random nonsense shrinker bits.
> >
> >HOWEVER, the generic shrinker code fixes that up by IGNORING random
> >shrinker bits like this when !memcg_kmem_online(). And shrinking
> >correctly happens only against the shared root queue when the reclaim
> >iterator walks root_mem_cgroup.
> >
> >HOWEVER, the THP shrinker explicitly sets SHRINKER_NONSLAB, which in
> >turn overrides the previous override. So yes there is a weirdness: we
> >get the root cgroup invocation against the shared queue, and then one
> >more time triggered by that random memcg bit.
> >
> >The most direct fix is to just drop SHRINKER_NONSLAB. It declares
> >independence from kmem, which is no longer true.
> >
> >Cleaning up the shrinker code is left for another day.
>
> Thanks for working on this!
>
> Wondering if this fix trades one problem for another, though ...
>
> Before this series, the deferred split shrinker had a real per-memcg
> queue. Even with cgroup.memory=nokmem, memcg reclaim could still scan
> that memcg's own deferred_split_queue:
>
> memcg reclaim -> deferred split shrinker -> sc->memcg->deferred_split_queue
>
> With the fix, nokmem + w/o SHRINKER_NONSLAB falls back to a
> non-memcg-aware shrinker:
>
> memcg reclaim -> skip deferred split shrinker
>
> root/global reclaim -> deferred split shrinker -> shared list_lru
>
> Is that expected? There woud be no memcg-driven deferred split reclaim
> under nokmem, IIUC ...
Yes, this is all correct. list_lru is still inherently tied to the
kmem component of memcg (memcg_kmem_id()).
So without kmem, no isolation. But without kmem, no isolation *for a
lot of stuff*. It's a legacy knob when slab accounting was new and
expensive. But so many things depend on it now, disabling it just
punches a nassive hole into memcg functionality and isolation
coverage. It's not a sanctioned production use flag.
This change is negligible from a memcg semantics POV.
> Not sure what the right fix is, as I am not a memcg expert ...
^ permalink raw reply
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