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 >
