Re: [RFC PATCH 00/14] Virtual Swap Space

[Nhat Pham <nphamcs@gmail.com>]

On Tue, Apr 22, 2025 at 10:15 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Tue, Apr 22, 2025 at 8:03 AM Yosry Ahmed <yosry.ahmed@linux.dev> wrote:
> >
> > On Mon, Apr 07, 2025 at 04:42:01PM -0700, Nhat Pham wrote:
> > It's exciting to see this proposal materilizing :)
> >
> > I didn't get a chance to look too closely at the code, but I have a few
> > high-level comments.
> >
> > Do we need separate refcnt and swap_count? I am aware that there are
> > cases where we need to hold a reference to prevent the descriptor from
> > going away, without an extra page table entry referencing the swap
> > descriptor -- but I am wondering if we can get away by just incrementing
> > the swap count in these cases too? Would this mess things up?
>
> Actually, you're right - we might not even need a separate refcnt
> field at all :) Here's my original thought process:
>
> 1. We need something that keeps the virtual swap slot and its metadata
> data structure (the swap descriptor) valid while we work with it.
>
> 2. In the old design, this is all stored at the swap device, so we
> need to obtain a reference to the swap device itself.
>
> 3. In the new design, this is no longer even possible. The backend
> might change under us even! So the refcnting needs to be done at the
> virtual swap level.
>
> 3. The refcnting needs to be separate from the swap count field,
> because certain operations/optimizations do check for the actual swap
> count, and incrementing the swap count willy nilly like that might
> accidentally throw these off. Think readahead-induced swap reads, for
> example. So I need a separate refcnt field that takes into account 3
> sources: PTE references (swap count), swap cache, and "ephemeral" (i.e
> temporary) references, that replace the role of the swap device
> reference in the old design.
>
> However, I have thought more about it. I don't think I need to obtain
> any ephemeral reference. I do need a refcnting mechanism, but one
> atomic field (that stores both the swap count and swap cache pin)
> should suffice.
>
> Refcnt + RCU should already guarantee the existence of the swap
> descriptor while I work with it. So there won't be any UAF issue, as
> long as I am disciplined and check if the swap descriptor still exists
> etc. in the virtual swap implementation, which I already am doing
> anyway.
>
> This should be safe enough, even in the face of swapoff, because
> swapoff also relies on the same reference counting mechanism to free
> the virtual swap slot and its descriptor. It tries to swap_free() the
> virtual swap slot, as it unmaps the virtual swap slot from the page
> table entry, which will decrement the swap count. So we're all good on
> this front.
>
> We DO need to obtain a reference to the swap device in certain places
> though, if we want to use it down the line for some sort of
> optimizations (for example, to look at its swap device flags to check
> if it is a SWP_SYNCHRONOUS_IO device - see do_swap_page()). But this
> is a separate matter.
>
> The end result is I will reduce 4 fields:
>
> 1. swp_entry_t vswap
> 2. atomic_t in_swapcache
> 3. atomic_t swap_count
> 4. struct kref kref;
>
> Into a single swap_refs field.
>
>
> >
> > >
> > > This design allows us to:
> > > * Decouple zswap (and zeromapped swap entry) from backing swapfile:
> > >   simply associate the virtual swap slot with one of the supported
> > >   backends: a zswap entry, a zero-filled swap page, a slot on the
> > >   swapfile, or an in-memory page .
> > > * Simplify and optimize swapoff: we only have to fault the page in and
> > >   have the virtual swap slot points to the page instead of the on-disk
> > >   physical swap slot. No need to perform any page table walking.
> > >
> > > Please see the attached patches for implementation details.
> > >
> > > Note that I do not remove the old implementation for now. Users can
> > > select between the old and the new implementation via the
> > > CONFIG_VIRTUAL_SWAP build config. This will also allow us to land the
> > > new design, and iteratively optimize upon it (without having to include
> > > everything in an even more massive patch series).
> >
> > I know this is easier, but honestly I'd prefer if we do an incremental
> > replacement (if possible) rather than introducing a new implementation
> > and slowly deprecating the old one, which historically doesn't seem to
> > go well :P
>
> I know, I know :P
>
> >
> > Once the series is organized as Johannes suggested, and we have better
> > insights into how this will be integrated with Kairui's work, it should
> > be clearer whether it's possible to incrementally update the current
> > implemetation rather than add a parallel implementation.
>
> Will take a look at Kairui's work when it's available :)
>
> >
> > >
> > > III. Future Use Cases
> > >
> > > Other than decoupling swap backends and optimizing swapoff, this new
> > > design allows us to implement the following more easily and
> > > efficiently:
> > >
> > > * Multi-tier swapping (as mentioned in [5]), with transparent
> > >   transferring (promotion/demotion) of pages across tiers (see [8] and
> > >   [9]). Similar to swapoff, with the old design we would need to
> > >   perform the expensive page table walk.
> > > * Swapfile compaction to alleviate fragmentation (as proposed by Ying
> > >   Huang in [6]).
> > > * Mixed backing THP swapin (see [7]): Once you have pinned down the
> > >   backing store of THPs, then you can dispatch each range of subpages
> > >   to appropriate swapin handle.
> > > * Swapping a folio out with discontiguous physical swap slots (see [10])
> > >
> > >
> > > IV. Potential Issues
> > >
> > > Here is a couple of issues I can think of, along with some potential
> > > solutions:
> > >
> > > 1. Space overhead: we need one swap descriptor per swap entry.
> > > * Note that this overhead is dynamic, i.e only incurred when we actually
> > >   need to swap a page out.
> > > * It can be further offset by the reduction of swap map and the
> > >   elimination of zeromapped bitmap.
> > >
> > > 2. Lock contention: since the virtual swap space is dynamic/unbounded,
> > > we cannot naively range partition it anymore. This can increase lock
> > > contention on swap-related data structures (swap cache, zswap’s xarray,
> > > etc.).
> > > * The problem is slightly alleviated by the lockless nature of the new
> > >   reference counting scheme, as well as the per-entry locking for
> > >   backing store information.
> > > * Johannes suggested that I can implement a dynamic partition scheme, in
> > >   which new partitions (along with associated data structures) are
> > >   allocated on demand. It is one extra layer of indirection, but global
> > >   locking will only be done only on partition allocation, rather than on
> > >   each access. All other accesses only take local (per-partition)
> > >   locks, or are completely lockless (such as partition lookup).
> > >
> > >
> > > V. Benchmarking
> > >
> > > As a proof of concept, I run the prototype through some simple
> > > benchmarks:
> > >
> > > 1. usemem: 16 threads, 2G each, memory.max = 16G
> > >
> > > I benchmarked the following usemem commands:
> > >
> > > time usemem --init-time -w -O -s 10 -n 16 2g
> > >
> > > Baseline:
> > > real: 33.96s
> > > user: 25.31s
> > > sys: 341.09s
> > > average throughput: 111295.45 KB/s
> > > average free time: 2079258.68 usecs
> > >
> > > New Design:
> > > real: 35.87s
> > > user: 25.15s
> > > sys: 373.01s
> > > average throughput: 106965.46 KB/s
> > > average free time: 3192465.62 usecs
> > >
> > > To root cause this regression, I ran perf on the usemem program, as
> > > well as on the following stress-ng program:
> > >
> > > perf record -ag -e cycles -G perf_cg -- ./stress-ng/stress-ng  --pageswap $(nproc) --pageswap-ops 100000
> > >
> > > and observed the (predicted) increase in lock contention on swap cache
> > > accesses. This regression is alleviated if I put together the
> > > following hack: limit the virtual swap space to a sufficient size for
> > > the benchmark, range partition the swap-related data structures (swap
> > > cache, zswap tree, etc.) based on the limit, and distribute the
> > > allocation of virtual swap slotss among these partitions (on a per-CPU
> > > basis):
> > >
> > > real: 34.94s
> > > user: 25.28s
> > > sys: 360.25s
> > > average throughput: 108181.15 KB/s
> > > average free time: 2680890.24 usecs
> > >
> > > As mentioned above, I will implement proper dynamic swap range
> > > partitioning in a follow up work.
> >
> > I thought there would be some improvements with the new design once the
> > lock contention is gone, due to the colocation of all swap metadata. Do
> > we know why this isn't the case?
>
> The lock contention is reduced on access, but increased on allocation
> and free step (because we have to go through a global lock now due to
> the loss of swap space partitioning).
>
> Virtual swap allocation optimization will be the next step, or it can
> be done concurrently, if we can figure out a way to make Kairui's work
> compatible with this.

To clarify a bit - what Kairui's proposal gives us (IIUC) is a dynamic
clustered approach on swap slot allocation. It's already done at the
physical level.

This is precisely what this RFC is missing. So if there is a way to
combine the work, I think it will go a long way in reducing the
regression.

That said, I haven't looked closely at his code yet, so I don't know
how easy/hard it is to combine the efforts :)