Message-ID: <CAP045Ao-onmzcVSic+4D+aDc_M9HTF7UK3NAdC5BcZvDmN370g@mail.gmail.com>
Date: Mon, 12 May 2025 10:16:12 -0700
From: Kyle Huey <me@...ehuey.com>
To: Peter Xu <peterx@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, open list <linux-kernel@...r.kernel.org>, 
	linux-mm@...ck.org, criu@...ts.linux.dev, 
	"Robert O'Callahan" <robert@...llahan.org>, Axel Rasmussen <axelrasmussen@...gle.com>, 
	Mike Rapoport <rppt@...nel.org>, Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: Suppress pte soft-dirty bit with UFFDIO_COPY?

On Mon, May 12, 2025 at 8:54 AM Peter Xu <peterx@...hat.com> wrote:
>
> On Sun, May 11, 2025 at 08:06:03PM -0700, Kyle Huey wrote:
> > On Mon, May 5, 2025 at 3:15 PM Kyle Huey <me@...ehuey.com> wrote:
> > >
> > > On Mon, May 5, 2025 at 1:05 PM Peter Xu <peterx@...hat.com> wrote:
> > > >
> > > > Hi, Kyle,
> > > >
> > > > On Mon, May 05, 2025 at 09:37:01AM -0700, Kyle Huey wrote:
> > > > > tl;dr I'd like to add UFFDIO_COPY_MODE_DONTSOFTDIRTY that does not add
> > > > > the _PAGE_SOFT_DIRTY bit to the relevant pte flags. Any
> > > > > thoughts/objections?
> > > > >
> > > > > The kernel has a "soft-dirty" bit on ptes which tracks if they've been
> > > > > written to since the last time /proc/pid/clear_refs was used to clear
> > > > > the soft-dirty bit. CRIU uses this to track which pages have been
> > > > > modified since a previous checkpoint and reduce the size of the
> > > > > checkpoints taken. I would like to use this in my debugger[0] to track
> > > > > which pages a program function dirties when that function is invoked
> > > > > from the debugger.
> > > > >
> > > > > However, the runtime environment for this function is rather unusual.
> > > > > In my debugger, the process being debugged doesn't actually exist
> > > > > while it's being debugged. Instead, we have a database of all program
> > > > > state (including registers and memory values) from when the process
> > > > > was executed. It's in some sense a giant core dump that spans multiple
> > > > > points in time. To execute a program function from the debugger we
> > > > > rematerialize the program state at the desired point in time from our
> > > > > database.
> > > > >
> > > > > For performance reasons, we fill in the memory lazily[1] via
> > > > > userfaultfd. This makes it difficult to use the soft-dirty bit to
> > > > > track the writes the function triggers, because UFFDIO_COPY (and
> > > > > friends) mark every page they touch as soft-dirty. Because we have the
> > > > > canonical source of truth for the pages we materialize via UFFDIO_COPY
> > > > > we're only interested in what happens after the userfaultfd operation.
> > > > >
> > > > > Clearing the soft-dirty bit is complicated by two things:
> > > > > 1. There's no way to clear the soft-dirty bit on a single pte, so
> > > > > instead we have to clear the soft-dirty bits for the entire process.
> > > > > That requires us to process all the soft-dirty bits on every other pte
> > > > > immediately to avoid data loss.
> > > > > 2. We need to clear the soft-dirty bits after the userfaultfd
> > > > > operation, but in order to avoid racing with the task that triggered
> > > > > the page fault we have to do a non-waking copy, then clear the bits,
> > > > > and then separately wake up the task.
> > > > >
> > > > > To work around all of this, we currently have a 4 step process:
> > > > > 1. Read /proc/pid/pagemap and note all ptes that are soft-dirty.
> > > > > 2. Do the UFFDIO_COPY with UFFDIO_COPY_MODE_DONTWAKE.
> > > > > 3. Write to /proc/pid/clear_refs to clear soft-dirty bits across the process.
> > > > > 4. Do a UFFDIO_WAKE.
> > > > >
> > > > > The overhead of all of this (particularly step 1) is a millisecond or
> > > > > two *per page* that we lazily materialize, and while that's not
> > > > > crippling for our purposes, it is rather undesirable. What I would
> > > > > like to have instead is a UFFDIO_COPY mode that leaves the soft-dirty
> > > > > bit unchanged, i.e. a UFFDIO_COPY_MODE_DONTSOFTDIRTY. Since we clear
> > > > > all the soft-dirty bits once after setting up all the mmaps in the
> > > > > process the relevant ptes would then "just do the right thing" from
> > > > > our perspective.
> > > > >
> > > > > But I do want to get some feedback on this before I spend time writing
> > > > > any code. Is there a reason not to do this? Or an alternate way to
> > > > > achieve the same goal?
> > > >
> > > > Have you looked at the wr-protect mode, and UFFDIO_COPY_MODE_WP for _COPY?
> > > >
> > > > If sync fault is a perf concern for frequent writes, just to mention at
> > > > least latest Linux also supports async tracking (UFFD_FEATURE_WP_ASYNC),
> > > > which is almost exactly soft dirty bits to me, though it solves a few
> > > > issues it has on e.g. false positives over vma merging and swapping, or
> > > > like you said missing of finer granule reset mechanisms.
> > > >
> > > > Maybe you also want to have a look at the pagemap ioctl introduced some
> > > > time ago ("Pagemap Scan IOCTL", which, IIRC was trying to use uffd-wp in
> > > > soft-dirty-like way):
> > > >
> > > > https://www.kernel.org/doc/Documentation/admin-guide/mm/pagemap.rst
> > >
> > >
> > > Thanks. This is all very helpful and I think I can construct what I
> > > need out of these building blocks.
> > >
> > > - Kyle
> >
> > That works like a charm, thanks.
> >
> > The only problem I ran into is that the man page for userfaultfd(2)
> > claims there's a handshake pattern where you can call UFFDIO_API
> > twice, once with 0 to enumerate all supported features, and then again
> > with the feature mask you want to initialize the API. In reality the
> > API only permits a single UFFDIO_API call because of the internal
> > UFFD_FEATURE_INITIALIZED flag, so doing this handshake requires
> > creating a sacrificial fd.
>
> This is true, almost all apps I'm aware that are using userfaultfd needs
> that.  It's indeed confusing.
>
> >
> > If the man page is not just totally wrong then this may have been an
> > unintentional regression from 22e5fe2a2a279.
>
> IMHO 22e5fe2a2a279 was correct, and it fixed a possible race due to
> ctx->state before. The new cmpxchg() plus the INITIALIZED flag should avoid
> the race.
>
> In this case it should be the man page that was wrong since this commit of
> man page, afaict:
>
> commit a252b3345f5b0a4ecafa7d4fb1ac73cb4fd4877f (HEAD)
> Author: Axel Rasmussen <axelrasmussen@...gle.com>
> Date:   Tue Oct 3 12:45:43 2023 -0700
>
>     ioctl_userfaultfd.2: Describe two-step feature handshake
>
> I'll see if Axel / Mike / Andrea has any comment, otherwise I'll propose a
> patch to fix the man-pages and state the fact (that we need a sacrificial
> fd).
>
> Maybe I should really add the UFFDIO_FEATURES ioctl to allow fetching the
> feature flags from kernel separately, considering how much trouble we've
> hit with this whole thing..

Personally I don't think it's a real issue to have to create a
sacrificial fd once at process initialization to see what features are
available. I wouldn't have even said anything if the man page hadn't
explicitly told me there was another way.

- Kyle

> Thanks,
>
> --
> Peter Xu
>

Powered by blists - more mailing lists