Re: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI
From: Suren Baghdasaryan
Date: Mon Oct 23 2023 - 13:44:10 EST
On Sun, Oct 22, 2023 at 10:02 AM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Thu, Oct 19, 2023 at 02:24:06PM -0700, Suren Baghdasaryan wrote:
> > On Thu, Oct 12, 2023 at 3:00 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
> > >
> > > On Sun, Oct 08, 2023 at 11:42:27PM -0700, Suren Baghdasaryan wrote:
> > > > From: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> > > >
> > > > Implement the uABI of UFFDIO_MOVE ioctl.
> > > > UFFDIO_COPY performs ~20% better than UFFDIO_MOVE when the application
> > > > needs pages to be allocated [1]. However, with UFFDIO_MOVE, if pages are
> > > > available (in userspace) for recycling, as is usually the case in heap
> > > > compaction algorithms, then we can avoid the page allocation and memcpy
> > > > (done by UFFDIO_COPY). Also, since the pages are recycled in the
> > > > userspace, we avoid the need to release (via madvise) the pages back to
> > > > the kernel [2].
> > > > We see over 40% reduction (on a Google pixel 6 device) in the compacting
> > > > thread’s completion time by using UFFDIO_MOVE vs. UFFDIO_COPY. This was
> > > > measured using a benchmark that emulates a heap compaction implementation
> > > > using userfaultfd (to allow concurrent accesses by application threads).
> > > > More details of the usecase are explained in [2].
> > > > Furthermore, UFFDIO_MOVE enables moving swapped-out pages without
> > > > touching them within the same vma. Today, it can only be done by mremap,
> > > > however it forces splitting the vma.
> > > >
> > > > [1] https://lore.kernel.org/all/1425575884-2574-1-git-send-email-aarcange@xxxxxxxxxx/
> > > > [2] https://lore.kernel.org/linux-mm/CA+EESO4uO84SSnBhArH4HvLNhaUQ5nZKNKXqxRCyjniNVjp0Aw@xxxxxxxxxxxxxx/
> > > >
> > > > Update for the ioctl_userfaultfd(2) manpage:
> > > >
> > > > UFFDIO_MOVE
> > > > (Since Linux xxx) Move a continuous memory chunk into the
> > > > userfault registered range and optionally wake up the blocked
> > > > thread. The source and destination addresses and the number of
> > > > bytes to move are specified by the src, dst, and len fields of
> > > > the uffdio_move structure pointed to by argp:
> > > >
> > > > struct uffdio_move {
> > > > __u64 dst; /* Destination of move */
> > > > __u64 src; /* Source of move */
> > > > __u64 len; /* Number of bytes to move */
> > > > __u64 mode; /* Flags controlling behavior of move */
> > > > __s64 move; /* Number of bytes moved, or negated error */
> > > > };
> > > >
> > > > The following value may be bitwise ORed in mode to change the
> > > > behavior of the UFFDIO_MOVE operation:
> > > >
> > > > UFFDIO_MOVE_MODE_DONTWAKE
> > > > Do not wake up the thread that waits for page-fault
> > > > resolution
> > > >
> > > > UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES
> > > > Allow holes in the source virtual range that is being moved.
> > > > When not specified, the holes will result in ENOENT error.
> > > > When specified, the holes will be accounted as successfully
> > > > moved memory. This is mostly useful to move hugepage aligned
> > > > virtual regions without knowing if there are transparent
> > > > hugepages in the regions or not, but preventing the risk of
> > > > having to split the hugepage during the operation.
> > > >
> > > > The move field is used by the kernel to return the number of
> > > > bytes that was actually moved, or an error (a negated errno-
> > > > style value). If the value returned in move doesn't match the
> > > > value that was specified in len, the operation fails with the
> > > > error EAGAIN. The move field is output-only; it is not read by
> > > > the UFFDIO_MOVE operation.
> > > >
> > > > The operation may fail for various reasons. Usually, remapping of
> > > > pages that are not exclusive to the given process fail; once KSM
> > > > might deduplicate pages or fork() COW-shares pages during fork()
> > > > with child processes, they are no longer exclusive. Further, the
> > > > kernel might only perform lightweight checks for detecting whether
> > > > the pages are exclusive, and return -EBUSY in case that check fails.
> > > > To make the operation more likely to succeed, KSM should be
> > > > disabled, fork() should be avoided or MADV_DONTFORK should be
> > > > configured for the source VMA before fork().
> > > >
> > > > This ioctl(2) operation returns 0 on success. In this case, the
> > > > entire area was moved. On error, -1 is returned and errno is
> > > > set to indicate the error. Possible errors include:
> > > >
> > > > EAGAIN The number of bytes moved (i.e., the value returned in
> > > > the move field) does not equal the value that was
> > > > specified in the len field.
> > > >
> > > > EINVAL Either dst or len was not a multiple of the system page
> > > > size, or the range specified by src and len or dst and len
> > > > was invalid.
> > > >
> > > > EINVAL An invalid bit was specified in the mode field.
> > > >
> > > > ENOENT
> > > > The source virtual memory range has unmapped holes and
> > > > UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES is not set.
> > > >
> > > > EEXIST
> > > > The destination virtual memory range is fully or partially
> > > > mapped.
> > > >
> > > > EBUSY
> > > > The pages in the source virtual memory range are not
> > > > exclusive to the process. The kernel might only perform
> > > > lightweight checks for detecting whether the pages are
> > > > exclusive. To make the operation more likely to succeed,
> > > > KSM should be disabled, fork() should be avoided or
> > > > MADV_DONTFORK should be configured for the source virtual
> > > > memory area before fork().
> > > >
> > > > ENOMEM Allocating memory needed for the operation failed.
> > > >
> > > > ESRCH
> > > > The faulting process has exited at the time of a
> > >
> > > Nit pick comment for future man page: there's no faulting process in this
> > > context. Perhaps "target process"?
> >
> > Ack.
> >
> > >
> > > > UFFDIO_MOVE operation.
> > > >
> > > > Signed-off-by: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> > > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > > > ---
> > > > Documentation/admin-guide/mm/userfaultfd.rst | 3 +
> > > > fs/userfaultfd.c | 63 ++
> > > > include/linux/rmap.h | 5 +
> > > > include/linux/userfaultfd_k.h | 12 +
> > > > include/uapi/linux/userfaultfd.h | 29 +-
> > > > mm/huge_memory.c | 138 +++++
> > > > mm/khugepaged.c | 3 +
> > > > mm/rmap.c | 6 +
> > > > mm/userfaultfd.c | 602 +++++++++++++++++++
> > > > 9 files changed, 860 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/admin-guide/mm/userfaultfd.rst b/Documentation/admin-guide/mm/userfaultfd.rst
> > > > index 203e26da5f92..e5cc8848dcb3 100644
> > > > --- a/Documentation/admin-guide/mm/userfaultfd.rst
> > > > +++ b/Documentation/admin-guide/mm/userfaultfd.rst
> > > > @@ -113,6 +113,9 @@ events, except page fault notifications, may be generated:
> > > > areas. ``UFFD_FEATURE_MINOR_SHMEM`` is the analogous feature indicating
> > > > support for shmem virtual memory areas.
> > > >
> > > > +- ``UFFD_FEATURE_MOVE`` indicates that the kernel supports moving an
> > > > + existing page contents from userspace.
> > > > +
> > > > The userland application should set the feature flags it intends to use
> > > > when invoking the ``UFFDIO_API`` ioctl, to request that those features be
> > > > enabled if supported.
> > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > > index a7c6ef764e63..ac52e0f99a69 100644
> > > > --- a/fs/userfaultfd.c
> > > > +++ b/fs/userfaultfd.c
> > > > @@ -2039,6 +2039,66 @@ static inline unsigned int uffd_ctx_features(__u64 user_features)
> > > > return (unsigned int)user_features | UFFD_FEATURE_INITIALIZED;
> > > > }
> > > >
> > > > +static int userfaultfd_remap(struct userfaultfd_ctx *ctx,
> > > > + unsigned long arg)
> > >
> > > If we do want to rename from REMAP to MOVE, we'd better rename the
> > > functions too, as "remap" still exists all over the place..
> >
> > Ok. I thought that since the current implementation only remaps and
> > never copies it would be correct to keep "remap" in these internal
> > names and change that later if we support copying. But I'm fine with
> > renaming them now to avoid confusion. Will do.
>
> "move", not "copy", btw.
>
> Not a big deal, take your preference at each place; "remap" sometimes can
> read better, maybe. Fundamentally, I think it's because both "remap" and
> "move" work in 99% cases. That's also why I think either name would work
> here.
Ok, "move" it is. That will avoid unnecessary churn in the future if
we decide to implement the copy fallback.
>
> >
> >
> > >
> > > > +{
> > > > + __s64 ret;
> > > > + struct uffdio_move uffdio_move;
> > > > + struct uffdio_move __user *user_uffdio_move;
> > > > + struct userfaultfd_wake_range range;
> > > > +
> > > > + user_uffdio_move = (struct uffdio_move __user *) arg;
> > > > +
> > > > + ret = -EAGAIN;
> > > > + if (atomic_read(&ctx->mmap_changing))
> > > > + goto out;
> > >
> > > I didn't notice this before, but I think we need to re-check this after
> > > taking target mm's mmap read lock..
> >
> > Ack.
> >
> > >
> > > maybe we'd want to pass in ctx* into remap_pages(), more reasoning below.
> >
> > Makes sense.
> >
> > >
> > > > +
> > > > + ret = -EFAULT;
> > > > + if (copy_from_user(&uffdio_move, user_uffdio_move,
> > > > + /* don't copy "remap" last field */
> > >
> > > s/remap/move/
> >
> > Ack.
> >
> > >
> > > > + sizeof(uffdio_move)-sizeof(__s64)))
> > > > + goto out;
> > > > +
> > > > + ret = validate_range(ctx->mm, uffdio_move.dst, uffdio_move.len);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = validate_range(current->mm, uffdio_move.src, uffdio_move.len);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = -EINVAL;
> > > > + if (uffdio_move.mode & ~(UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES|
> > > > + UFFDIO_MOVE_MODE_DONTWAKE))
> > > > + goto out;
> > > > +
> > > > + if (mmget_not_zero(ctx->mm)) {
> > > > + ret = remap_pages(ctx->mm, current->mm,
> > > > + uffdio_move.dst, uffdio_move.src,
> > > > + uffdio_move.len, uffdio_move.mode);
> > > > + mmput(ctx->mm);
> > > > + } else {
> > > > + return -ESRCH;
> > > > + }
> > > > +
> > > > + if (unlikely(put_user(ret, &user_uffdio_move->move)))
> > > > + return -EFAULT;
> > > > + if (ret < 0)
> > > > + goto out;
> > > > +
> > > > + /* len == 0 would wake all */
> > > > + BUG_ON(!ret);
> > > > + range.len = ret;
> > > > + if (!(uffdio_move.mode & UFFDIO_MOVE_MODE_DONTWAKE)) {
> > > > + range.start = uffdio_move.dst;
> > > > + wake_userfault(ctx, &range);
> > > > + }
> > > > + ret = range.len == uffdio_move.len ? 0 : -EAGAIN;
> > > > +
> > > > +out:
> > > > + return ret;
> > > > +}
> > > > +
> > > > /*
> > > > * userland asks for a certain API version and we return which bits
> > > > * and ioctl commands are implemented in this kernel for such API
> > > > @@ -2131,6 +2191,9 @@ static long userfaultfd_ioctl(struct file *file, unsigned cmd,
> > > > case UFFDIO_ZEROPAGE:
> > > > ret = userfaultfd_zeropage(ctx, arg);
> > > > break;
> > > > + case UFFDIO_MOVE:
> > > > + ret = userfaultfd_remap(ctx, arg);
> > > > + break;
> > > > case UFFDIO_WRITEPROTECT:
> > > > ret = userfaultfd_writeprotect(ctx, arg);
> > > > break;
> > > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > > > index b26fe858fd44..8034eda972e5 100644
> > > > --- a/include/linux/rmap.h
> > > > +++ b/include/linux/rmap.h
> > > > @@ -121,6 +121,11 @@ static inline void anon_vma_lock_write(struct anon_vma *anon_vma)
> > > > down_write(&anon_vma->root->rwsem);
> > > > }
> > > >
> > > > +static inline int anon_vma_trylock_write(struct anon_vma *anon_vma)
> > > > +{
> > > > + return down_write_trylock(&anon_vma->root->rwsem);
> > > > +}
> > > > +
> > > > static inline void anon_vma_unlock_write(struct anon_vma *anon_vma)
> > > > {
> > > > up_write(&anon_vma->root->rwsem);
> > > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > > > index f2dc19f40d05..ce8d20b57e8c 100644
> > > > --- a/include/linux/userfaultfd_k.h
> > > > +++ b/include/linux/userfaultfd_k.h
> > > > @@ -93,6 +93,18 @@ extern int mwriteprotect_range(struct mm_struct *dst_mm,
> > > > extern long uffd_wp_range(struct vm_area_struct *vma,
> > > > unsigned long start, unsigned long len, bool enable_wp);
> > > >
> > > > +/* remap_pages */
> > > > +void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2);
> > > > +void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2);
> > > > +ssize_t remap_pages(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> > > > + unsigned long dst_start, unsigned long src_start,
> > > > + unsigned long len, __u64 flags);
> > > > +int remap_pages_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> > > > + pmd_t *dst_pmd, pmd_t *src_pmd, pmd_t dst_pmdval,
> > > > + struct vm_area_struct *dst_vma,
> > > > + struct vm_area_struct *src_vma,
> > > > + unsigned long dst_addr, unsigned long src_addr);
> > > > +
> > > > /* mm helpers */
> > > > static inline bool is_mergeable_vm_userfaultfd_ctx(struct vm_area_struct *vma,
> > > > struct vm_userfaultfd_ctx vm_ctx)
> > > > diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h
> > > > index 0dbc81015018..2841e4ea8f2c 100644
> > > > --- a/include/uapi/linux/userfaultfd.h
> > > > +++ b/include/uapi/linux/userfaultfd.h
> > > > @@ -41,7 +41,8 @@
> > > > UFFD_FEATURE_WP_HUGETLBFS_SHMEM | \
> > > > UFFD_FEATURE_WP_UNPOPULATED | \
> > > > UFFD_FEATURE_POISON | \
> > > > - UFFD_FEATURE_WP_ASYNC)
> > > > + UFFD_FEATURE_WP_ASYNC | \
> > > > + UFFD_FEATURE_MOVE)
> > > > #define UFFD_API_IOCTLS \
> > > > ((__u64)1 << _UFFDIO_REGISTER | \
> > > > (__u64)1 << _UFFDIO_UNREGISTER | \
> > > > @@ -50,6 +51,7 @@
> > > > ((__u64)1 << _UFFDIO_WAKE | \
> > > > (__u64)1 << _UFFDIO_COPY | \
> > > > (__u64)1 << _UFFDIO_ZEROPAGE | \
> > > > + (__u64)1 << _UFFDIO_MOVE | \
> > > > (__u64)1 << _UFFDIO_WRITEPROTECT | \
> > > > (__u64)1 << _UFFDIO_CONTINUE | \
> > > > (__u64)1 << _UFFDIO_POISON)
> > > > @@ -73,6 +75,7 @@
> > > > #define _UFFDIO_WAKE (0x02)
> > > > #define _UFFDIO_COPY (0x03)
> > > > #define _UFFDIO_ZEROPAGE (0x04)
> > > > +#define _UFFDIO_MOVE (0x05)
> > > > #define _UFFDIO_WRITEPROTECT (0x06)
> > > > #define _UFFDIO_CONTINUE (0x07)
> > > > #define _UFFDIO_POISON (0x08)
> > > > @@ -92,6 +95,8 @@
> > > > struct uffdio_copy)
> > > > #define UFFDIO_ZEROPAGE _IOWR(UFFDIO, _UFFDIO_ZEROPAGE, \
> > > > struct uffdio_zeropage)
> > > > +#define UFFDIO_MOVE _IOWR(UFFDIO, _UFFDIO_MOVE, \
> > > > + struct uffdio_move)
> > > > #define UFFDIO_WRITEPROTECT _IOWR(UFFDIO, _UFFDIO_WRITEPROTECT, \
> > > > struct uffdio_writeprotect)
> > > > #define UFFDIO_CONTINUE _IOWR(UFFDIO, _UFFDIO_CONTINUE, \
> > > > @@ -222,6 +227,9 @@ struct uffdio_api {
> > > > * asynchronous mode is supported in which the write fault is
> > > > * automatically resolved and write-protection is un-set.
> > > > * It implies UFFD_FEATURE_WP_UNPOPULATED.
> > > > + *
> > > > + * UFFD_FEATURE_MOVE indicates that the kernel supports moving an
> > > > + * existing page contents from userspace.
> > > > */
> > > > #define UFFD_FEATURE_PAGEFAULT_FLAG_WP (1<<0)
> > > > #define UFFD_FEATURE_EVENT_FORK (1<<1)
> > > > @@ -239,6 +247,7 @@ struct uffdio_api {
> > > > #define UFFD_FEATURE_WP_UNPOPULATED (1<<13)
> > > > #define UFFD_FEATURE_POISON (1<<14)
> > > > #define UFFD_FEATURE_WP_ASYNC (1<<15)
> > > > +#define UFFD_FEATURE_MOVE (1<<16)
> > > > __u64 features;
> > > >
> > > > __u64 ioctls;
> > > > @@ -347,6 +356,24 @@ struct uffdio_poison {
> > > > __s64 updated;
> > > > };
> > > >
> > > > +struct uffdio_move {
> > > > + __u64 dst;
> > > > + __u64 src;
> > > > + __u64 len;
> > > > + /*
> > > > + * Especially if used to atomically remove memory from the
> > > > + * address space the wake on the dst range is not needed.
> > > > + */
> > > > +#define UFFDIO_MOVE_MODE_DONTWAKE ((__u64)1<<0)
> > > > +#define UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES ((__u64)1<<1)
> > > > + __u64 mode;
> > > > + /*
> > > > + * "move" is written by the ioctl and must be at the end: the
> > > > + * copy_from_user will not read the last 8 bytes.
> > > > + */
> > > > + __s64 move;
> > > > +};
> > > > +
> > > > /*
> > > > * Flags for the userfaultfd(2) system call itself.
> > > > */
> > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > > index 9656be95a542..6fac5c3d66e6 100644
> > > > --- a/mm/huge_memory.c
> > > > +++ b/mm/huge_memory.c
> > > > @@ -2086,6 +2086,144 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> > > > return ret;
> > > > }
> > > >
> > > > +#ifdef CONFIG_USERFAULTFD
> > > > +/*
> > > > + * The PT lock for src_pmd and the mmap_lock for reading are held by
> > > > + * the caller, but it must return after releasing the
> > > > + * page_table_lock. Just move the page from src_pmd to dst_pmd if possible.
> > > > + * Return zero if succeeded in moving the page, -EAGAIN if it needs to be
> > > > + * repeated by the caller, or other errors in case of failure.
> > > > + */
> > > > +int remap_pages_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> > > > + pmd_t *dst_pmd, pmd_t *src_pmd, pmd_t dst_pmdval,
> > > > + struct vm_area_struct *dst_vma,
> > > > + struct vm_area_struct *src_vma,
> > > > + unsigned long dst_addr, unsigned long src_addr)
> > > > +{
> > > > + pmd_t _dst_pmd, src_pmdval;
> > > > + struct page *src_page;
> > > > + struct folio *src_folio;
> > > > + struct anon_vma *src_anon_vma;
> > > > + spinlock_t *src_ptl, *dst_ptl;
> > > > + pgtable_t src_pgtable, dst_pgtable;
> > > > + struct mmu_notifier_range range;
> > > > + int err = 0;
> > > > +
> > > > + src_pmdval = *src_pmd;
> > > > + src_ptl = pmd_lockptr(src_mm, src_pmd);
> > > > +
> > > > + lockdep_assert_held(src_ptl);
> > > > + mmap_assert_locked(src_mm);
> > > > + mmap_assert_locked(dst_mm);
> > > > +
> > > > + BUG_ON(!pmd_none(dst_pmdval));
> > > > + BUG_ON(src_addr & ~HPAGE_PMD_MASK);
> > > > + BUG_ON(dst_addr & ~HPAGE_PMD_MASK);
> > > > +
> > > > + if (!pmd_trans_huge(src_pmdval)) {
> > > > + spin_unlock(src_ptl);
> > > > + if (is_pmd_migration_entry(src_pmdval)) {
> > > > + pmd_migration_entry_wait(src_mm, &src_pmdval);
> > > > + return -EAGAIN;
> > > > + }
> > > > + return -ENOENT;
> > > > + }
> > > > +
> > > > + src_page = pmd_page(src_pmdval);
> > > > + if (unlikely(!PageAnonExclusive(src_page))) {
> > > > + spin_unlock(src_ptl);
> > > > + return -EBUSY;
> > > > + }
> > > > +
> > > > + src_folio = page_folio(src_page);
> > > > + folio_get(src_folio);
> > > > + spin_unlock(src_ptl);
> > > > +
> > > > + /* preallocate dst_pgtable if needed */
> > > > + if (dst_mm != src_mm) {
> > > > + dst_pgtable = pte_alloc_one(dst_mm);
> > > > + if (unlikely(!dst_pgtable)) {
> > > > + err = -ENOMEM;
> > > > + goto put_folio;
> > > > + }
> > > > + } else {
> > > > + dst_pgtable = NULL;
> > > > + }
> > > > +
> > >
> > > IIUC Lokesh's comment applies here, we probably need the
> > > flush_cache_range(), not for x86 but for the other ones..
> > >
> > > cachetlb.rst:
> > >
> > > Next, we have the cache flushing interfaces. In general, when Linux
> > > is changing an existing virtual-->physical mapping to a new value,
> > > the sequence will be in one of the following forms::
> > >
> > > 1) flush_cache_mm(mm);
> > > change_all_page_tables_of(mm);
> > > flush_tlb_mm(mm);
> > >
> > > 2) flush_cache_range(vma, start, end);
> > > change_range_of_page_tables(mm, start, end);
> > > flush_tlb_range(vma, start, end);
> > >
> > > 3) flush_cache_page(vma, addr, pfn);
> > > set_pte(pte_pointer, new_pte_val);
> > > flush_tlb_page(vma, addr);
> >
> > Thanks for the reference. I guess that's to support VIVT caches?
>
> I'm not 100% sure VIVT the only case, but.. yeah flush anything to ram as
> long as things cached in va form would be required.
Ack.
>
> >
> > >
> > > > + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, src_mm, src_addr,
> > > > + src_addr + HPAGE_PMD_SIZE);
> > > > + mmu_notifier_invalidate_range_start(&range);
> > > > +
> > > > + folio_lock(src_folio);
> > > > +
> > > > + /*
> > > > + * split_huge_page walks the anon_vma chain without the page
> > > > + * lock. Serialize against it with the anon_vma lock, the page
> > > > + * lock is not enough.
> > > > + */
> > > > + src_anon_vma = folio_get_anon_vma(src_folio);
> > > > + if (!src_anon_vma) {
> > > > + err = -EAGAIN;
> > > > + goto unlock_folio;
> > > > + }
> > > > + anon_vma_lock_write(src_anon_vma);
> > > > +
> > > > + dst_ptl = pmd_lockptr(dst_mm, dst_pmd);
> > > > + double_pt_lock(src_ptl, dst_ptl);
> > > > + if (unlikely(!pmd_same(*src_pmd, src_pmdval) ||
> > > > + !pmd_same(*dst_pmd, dst_pmdval))) {
> > > > + double_pt_unlock(src_ptl, dst_ptl);
> > > > + err = -EAGAIN;
> > > > + goto put_anon_vma;
> > > > + }
> > > > + if (!PageAnonExclusive(&src_folio->page)) {
> > > > + double_pt_unlock(src_ptl, dst_ptl);
> > > > + err = -EBUSY;
> > > > + goto put_anon_vma;
> > > > + }
> > > > +
> > > > + BUG_ON(!folio_test_head(src_folio));
> > > > + BUG_ON(!folio_test_anon(src_folio));
> > > > +
> > > > + folio_move_anon_rmap(src_folio, dst_vma);
> > > > + WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr));
> > > > +
> > > > + src_pmdval = pmdp_huge_clear_flush(src_vma, src_addr, src_pmd);
> > > > + _dst_pmd = mk_huge_pmd(&src_folio->page, dst_vma->vm_page_prot);
> > > > + _dst_pmd = maybe_pmd_mkwrite(pmd_mkdirty(_dst_pmd), dst_vma);
> > >
> > > Last time the conclusion is we leverage can_change_pmd_writable(), no?
> >
> > After your explanation that this works correctly for soft-dirty and
> > UFFD_WP I thought the only thing left to handle was the check for
> > VM_WRITE in both src_vma and dst_vma (which I added into
> > validate_remap_areas()). Maybe I misunderstood and if so, I can
> > replace the above PageAnonExclusive() with can_change_pmd_writable()
> > (note that we err out on VM_SHARED VMAs, so PageAnonExclusive() will
> > be included in that check).
>
> I think we still need PageAnonExclusive() because that's the first guard to
> decide whether the page can be moved over at all.
>
> What I meant is something like keeping that, then:
>
> if (pmd_soft_dirty(src_pmdval))
> _dst_pmd = pmd_swp_mksoft_dirty(_dst_pmd);
> if (pmd_uffd_wp(src_pmdval))
> _dst_pmd = pte_swp_mkuffd_wp(swp_pte);
> if (can_change_pmd_writable(_dst_vma, addr, _dst_pmd))
> _dst_pmd = pmd_mkwrite(_dst_pmd, dst_vma);
>
> But I'm not really sure anyone can leverage that, especially after I just
> saw move_soft_dirty_pte(): mremap() treat everything dirty after movement.
> I don't think there's a clear definition of how we treat memory dirty after
> remap.
>
> Maybe we should follow what it does with mremap()? Then your current code
> is fine. Maybe that's the better start.
I think that was the original intention, basically treating remapping
as a write operation. Maybe I should add a comment here to make it
more clear?
>
> >
> > >
> > > > + set_pmd_at(dst_mm, dst_addr, dst_pmd, _dst_pmd);
> > > > +
> > > > + src_pgtable = pgtable_trans_huge_withdraw(src_mm, src_pmd);
> > > > + if (dst_pgtable) {
> > > > + pgtable_trans_huge_deposit(dst_mm, dst_pmd, dst_pgtable);
> > > > + pte_free(src_mm, src_pgtable);
> > > > + dst_pgtable = NULL;
> > > > +
> > > > + mm_inc_nr_ptes(dst_mm);
> > > > + mm_dec_nr_ptes(src_mm);
> > > > + add_mm_counter(dst_mm, MM_ANONPAGES, HPAGE_PMD_NR);
> > > > + add_mm_counter(src_mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> > > > + } else {
> > > > + pgtable_trans_huge_deposit(dst_mm, dst_pmd, src_pgtable);
> > > > + }
> > > > + double_pt_unlock(src_ptl, dst_ptl);
> > > > +
> > > > +put_anon_vma:
> > > > + anon_vma_unlock_write(src_anon_vma);
> > > > + put_anon_vma(src_anon_vma);
> > > > +unlock_folio:
> > > > + /* unblock rmap walks */
> > > > + folio_unlock(src_folio);
> > > > + mmu_notifier_invalidate_range_end(&range);
> > > > + if (dst_pgtable)
> > > > + pte_free(dst_mm, dst_pgtable);
> > > > +put_folio:
> > > > + folio_put(src_folio);
> > > > +
> > > > + return err;
> > > > +}
> > > > +#endif /* CONFIG_USERFAULTFD */
> > > > +
> > > > /*
> > > > * Returns page table lock pointer if a given pmd maps a thp, NULL otherwise.
> > > > *
> > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > > index 2b5c0321d96b..0c1ee7172852 100644
> > > > --- a/mm/khugepaged.c
> > > > +++ b/mm/khugepaged.c
> > > > @@ -1136,6 +1136,9 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address,
> > > > * Prevent all access to pagetables with the exception of
> > > > * gup_fast later handled by the ptep_clear_flush and the VM
> > > > * handled by the anon_vma lock + PG_lock.
> > > > + *
> > > > + * UFFDIO_MOVE is prevented to race as well thanks to the
> > > > + * mmap_lock.
> > > > */
> > > > mmap_write_lock(mm);
> > > > result = hugepage_vma_revalidate(mm, address, true, &vma, cc);
> > > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > > index f9ddc50269d2..a5919cac9a08 100644
> > > > --- a/mm/rmap.c
> > > > +++ b/mm/rmap.c
> > > > @@ -490,6 +490,12 @@ void __init anon_vma_init(void)
> > > > * page_remove_rmap() that the anon_vma pointer from page->mapping is valid
> > > > * if there is a mapcount, we can dereference the anon_vma after observing
> > > > * those.
> > > > + *
> > > > + * NOTE: the caller should normally hold folio lock when calling this. If
> > > > + * not, the caller needs to double check the anon_vma didn't change after
> > > > + * taking the anon_vma lock for either read or write (UFFDIO_MOVE can modify it
> > > > + * concurrently without folio lock protection). See folio_lock_anon_vma_read()
> > > > + * which has already covered that, and comment above remap_pages().
> > > > */
> > > > struct anon_vma *folio_get_anon_vma(struct folio *folio)
> > > > {
> > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > > index 96d9eae5c7cc..45ce1a8b8ab9 100644
> > > > --- a/mm/userfaultfd.c
> > > > +++ b/mm/userfaultfd.c
> > > > @@ -842,3 +842,605 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
> > > > mmap_read_unlock(dst_mm);
> > > > return err;
> > > > }
> > > > +
> > > > +
> > > > +void double_pt_lock(spinlock_t *ptl1,
> > > > + spinlock_t *ptl2)
> > > > + __acquires(ptl1)
> > > > + __acquires(ptl2)
> > > > +{
> > > > + spinlock_t *ptl_tmp;
> > > > +
> > > > + if (ptl1 > ptl2) {
> > > > + /* exchange ptl1 and ptl2 */
> > > > + ptl_tmp = ptl1;
> > > > + ptl1 = ptl2;
> > > > + ptl2 = ptl_tmp;
> > > > + }
> > > > + /* lock in virtual address order to avoid lock inversion */
> > > > + spin_lock(ptl1);
> > > > + if (ptl1 != ptl2)
> > > > + spin_lock_nested(ptl2, SINGLE_DEPTH_NESTING);
> > > > + else
> > > > + __acquire(ptl2);
> > > > +}
> > > > +
> > > > +void double_pt_unlock(spinlock_t *ptl1,
> > > > + spinlock_t *ptl2)
> > > > + __releases(ptl1)
> > > > + __releases(ptl2)
> > > > +{
> > > > + spin_unlock(ptl1);
> > > > + if (ptl1 != ptl2)
> > > > + spin_unlock(ptl2);
> > > > + else
> > > > + __release(ptl2);
> > > > +}
> > > > +
> > > > +
> > > > +static int remap_present_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> > > > + struct vm_area_struct *dst_vma,
> > > > + struct vm_area_struct *src_vma,
> > > > + unsigned long dst_addr, unsigned long src_addr,
> > > > + pte_t *dst_pte, pte_t *src_pte,
> > > > + pte_t orig_dst_pte, pte_t orig_src_pte,
> > > > + spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > > > + struct folio *src_folio)
> > > > +{
> > > > + double_pt_lock(dst_ptl, src_ptl);
> > > > +
> > > > + if (!pte_same(*src_pte, orig_src_pte) ||
> > > > + !pte_same(*dst_pte, orig_dst_pte)) {
> > > > + double_pt_unlock(dst_ptl, src_ptl);
> > > > + return -EAGAIN;
> > > > + }
> > > > + if (folio_test_large(src_folio) ||
> > > > + !PageAnonExclusive(&src_folio->page)) {
> > > > + double_pt_unlock(dst_ptl, src_ptl);
> > > > + return -EBUSY;
> > > > + }
> > > > +
> > > > + BUG_ON(!folio_test_anon(src_folio));
> > > > +
> > > > + folio_move_anon_rmap(src_folio, dst_vma);
> > > > + WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, dst_addr));
> > > > +
> > > > + orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
> > > > + orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
> > > > + orig_dst_pte = maybe_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
> > >
> > > can_change_pte_writable()?
> >
> > Same as my previous comment. If that's still needed I'll replace the
> > above PageAnonExclusive() check with can_change_pte_writable().
>
> If no one else sees any problem, let's keep your current code, per my above
> observations.. to match mremap(), also keep it simple.
Ack.
>
> One more thing I just remembered on memcg: only uncharge+charge may not
> work, I think the lruvec needs to be maintained as well, or memcg shrink
> can try to swap some irrelevant page at least, and memcg accounting can
> also go wrong.
>
> AFAICT, that means something like another pair of:
>
> folio_isolate_lru() + folio_putback_lru()
>
> Besides the charge/uncharge.
>
> Yu Zhao should be familiar with that code, maybe you can double check with
> him before sending the new version.
Ok, I'll double check with Yu before sending cross-mm parts.
>
> I think this will belong to the separate patch to add cross-mm support, but
> please also double check even just in case there can be implication of
> single-mm that I missed.
Hmm. For single-mm we do not recharge memcgs. Why would we need to
isolate and put the pages back? Maybe I'm missing something... I'll
ask Yu in any case.
>
> Please also don't feel stressed over cross-mm support: at some point if you
> see that separate patch grows we can stop from there, listing all the
> cross-mm todos/investigations in the cover letter and start with single-mm.
Yes, I think this would be the best way forward (see my previous reply).
Thanks,
Suren.
>
> Thanks,
>
> --
> Peter Xu
>