This patch series introduces UFFDIO_MOVE feature to userfaultfd, which has long been implemented and maintained by Andrea in his local tree [1], but was not upstreamed due to lack of use cases where this approach would be better than allocating a new page and copying the contents. Previous upstraming attempts could be found at [6] and [7].
UFFDIO_COPY performs ~20% better than UFFDIO_MOVE when the application needs pages to be allocated [2]. 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 [3]. 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 [3].
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.
TODOs for follow-up improvements: - cross-mm support. Known differences from single-mm and missing pieces: - memcg recharging (might need to isolate pages in the process) - mm counters - cross-mm deposit table moves - cross-mm test - document the address space where src and dest reside in struct uffdio_move
- TLB flush batching. Will require extensive changes to PTL locking in move_pages_pte(). OTOH that might let us reuse parts of mremap code.
Changes since v4 [9]: - added Acked-by in patch 1, per Peter Xu - added description for ctx, mm and mode parameters of move_pages(), per kernel test robot - added Reviewed-by's, per Peter Xu and Axel Rasmussen - removed unused operations in uffd_test_case_ops - refactored uffd-unit-test changes to avoid using global variables and handle pmd moves without page size overrides, per Peter Xu
Changes since v3 [8]: - changed retry path in folio_lock_anon_vma_read() to unlock and then relock RCU, per Peter Xu - removed cross-mm support from initial patchset, per David Hildenbrand - replaced BUG_ONs with VM_WARN_ON or WARN_ON_ONCE, per David Hildenbrand - added missing cache flushing, per Lokesh Gidra and Peter Xu - updated manpage text in the patch description, per Peter Xu - renamed internal functions from "remap" to "move", per Peter Xu - added mmap_changing check after taking mmap_lock, per Peter Xu - changed uffd context check to ensure dst_mm is registered onto uffd we are operating on, Peter Xu and David Hildenbrand - changed to non-maybe variants of maybe*_mkwrite(), per David Hildenbrand - fixed warning for CONFIG_TRANSPARENT_HUGEPAGE=n, per kernel test robot - comments cleanup, per David Hildenbrand and Peter Xu - checks for VM_IO,VM_PFNMAP,VM_HUGETLB,..., per David Hildenbrand - prevent moving pinned pages, per Peter Xu - changed uffd tests to call move uffd_test_ctx_clear() at the end of the test run instead of in the beginning of the next run - added support for testcase-specific ops - added test for moving PMD-aligned blocks
Changes since v2 [5]: - renamed UFFDIO_REMAP to UFFDIO_MOVE, per David Hildenbrand - rebase over mm-unstable to use folio_move_anon_rmap(), per David Hildenbrand - added text for manpage explaining DONTFORK and KSM requirements for this feature, per David Hildenbrand - check for anon_vma changes in the fast path of folio_lock_anon_vma_read, per Peter Xu - updated the title and description of the first patch, per David Hildenbrand - updating comments in folio_lock_anon_vma_read() explaining the need for anon_vma checks, per David Hildenbrand - changed all mapcount checks to PageAnonExclusive, per Jann Horn and David Hildenbrand - changed counters in remap_swap_pte() from MM_ANONPAGES to MM_SWAPENTS, per Jann Horn - added a check for PTE change after folio is locked in remap_pages_pte(), per Jann Horn - added handling of PMD migration entries and bailout when pmd_devmap(), per Jann Horn - added checks to ensure both src and dst VMAs are writable, per Peter Xu - added UFFD_FEATURE_MOVE, per Peter Xu - removed obsolete comments, per Peter Xu - renamed remap_anon_pte to remap_present_pte, per Peter Xu - added a comment for folio_get_anon_vma() explaining the need for anon_vma checks, per Peter Xu - changed error handling in remap_pages() to make it more clear, per Peter Xu - changed EFAULT to EAGAIN to retry when a hugepage appears or disappears from under us, per Peter Xu - added links to previous upstreaming attempts, per David Hildenbrand
Changes since v1 [4]: - add mmget_not_zero in userfaultfd_remap, per Jann Horn - removed extern from function definitions, per Matthew Wilcox - converted to folios in remap_pages_huge_pmd, per Matthew Wilcox - use PageAnonExclusive in remap_pages_huge_pmd, per David Hildenbrand - handle pgtable transfers between MMs, per Jann Horn - ignore concurrent A/D pte bit changes, per Jann Horn - split functions into smaller units, per David Hildenbrand - test for folio_test_large in remap_anon_pte, per Matthew Wilcox - use pte_swp_exclusive for swapcount check, per David Hildenbrand - eliminated use of mmu_notifier_invalidate_range_start_nonblock, per Jann Horn - simplified THP alignment checks, per Jann Horn - refactored the loop inside remap_pages, per Jann Horn - additional clarifying comments, per Jann Horn
Main changes since Andrea's last version [1]: - Trivial translations from page to folio, mmap_sem to mmap_lock - Replace pmd_trans_unstable() with pte_offset_map_nolock() and handle its possible failure - Move pte mapping into remap_pages_pte to allow for retries when source page or anon_vma is contended. Since pte_offset_map_nolock() start RCU read section, we can't block anymore after mapping a pte, so have to unmap the ptesm do the locking and retry. - Add and use anon_vma_trylock_write() to avoid blocking while in RCU read section. - Accommodate changes in mmu_notifier_range_init() API, switch to mmu_notifier_invalidate_range_start_nonblock() to avoid blocking while in RCU read section. - Open-code now removed __swp_swapcount() - Replace pmd_read_atomic() with pmdp_get_lockless() - Add new selftest for UFFDIO_MOVE
[1] https://gitlab.com/aarcange/aa/-/commit/2aec7aea56b10438a3881a20a411aa4b1fc1... [2] https://lore.kernel.org/all/1425575884-2574-1-git-send-email-aarcange@redhat... [3] https://lore.kernel.org/linux-mm/CA+EESO4uO84SSnBhArH4HvLNhaUQ5nZKNKXqxRCyjn... [4] https://lore.kernel.org/all/20230914152620.2743033-1-surenb@google.com/ [5] https://lore.kernel.org/all/20230923013148.1390521-1-surenb@google.com/ [6] https://lore.kernel.org/all/1425575884-2574-21-git-send-email-aarcange@redha... [7] https://lore.kernel.org/all/cover.1547251023.git.blake.caldwell@colorado.edu... [8] https://lore.kernel.org/all/20231009064230.2952396-1-surenb@google.com/ [9] https://lore.kernel.org/all/20231028003819.652322-1-surenb@google.com/
Andrea Arcangeli (2): mm/rmap: support move to different root anon_vma in folio_move_anon_rmap() userfaultfd: UFFDIO_MOVE uABI
Suren Baghdasaryan (3): selftests/mm: call uffd_test_ctx_clear at the end of the test selftests/mm: add uffd_test_case_ops to allow test case-specific operations selftests/mm: add UFFDIO_MOVE ioctl test
Documentation/admin-guide/mm/userfaultfd.rst | 3 + fs/userfaultfd.c | 72 +++ include/linux/rmap.h | 5 + include/linux/userfaultfd_k.h | 11 + include/uapi/linux/userfaultfd.h | 29 +- mm/huge_memory.c | 122 ++++ mm/khugepaged.c | 3 + mm/rmap.c | 30 + mm/userfaultfd.c | 599 +++++++++++++++++++ tools/testing/selftests/mm/uffd-common.c | 39 +- tools/testing/selftests/mm/uffd-common.h | 9 + tools/testing/selftests/mm/uffd-stress.c | 5 +- tools/testing/selftests/mm/uffd-unit-tests.c | 192 ++++++ 13 files changed, 1115 insertions(+), 4 deletions(-)
From: Andrea Arcangeli aarcange@redhat.com
For now, folio_move_anon_rmap() was only used to move a folio to a different anon_vma after fork(), whereby the root anon_vma stayed unchanged. For that, it was sufficient to hold the folio lock when calling folio_move_anon_rmap().
However, we want to make use of folio_move_anon_rmap() to move folios between VMAs that have a different root anon_vma. As folio_referenced() performs an RMAP walk without holding the folio lock but only holding the anon_vma in read mode, holding the folio lock is insufficient.
When moving to an anon_vma with a different root anon_vma, we'll have to hold both, the folio lock and the anon_vma lock in write mode. Consequently, whenever we succeeded in folio_lock_anon_vma_read() to read-lock the anon_vma, we have to re-check if the mapping was changed in the meantime. If that was the case, we have to retry.
Note that folio_move_anon_rmap() must only be called if the anon page is exclusive to a process, and must not be called on KSM folios.
This is a preparation for UFFDIO_MOVE, which will hold the folio lock, the anon_vma lock in write mode, and the mmap_lock in read mode.
Signed-off-by: Andrea Arcangeli aarcange@redhat.com Signed-off-by: Suren Baghdasaryan surenb@google.com Acked-by: Peter Xu peterx@redhat.com --- mm/rmap.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/mm/rmap.c b/mm/rmap.c index 7a27a2b41802..525c5bc0b0b3 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -542,6 +542,7 @@ struct anon_vma *folio_lock_anon_vma_read(struct folio *folio, struct anon_vma *root_anon_vma; unsigned long anon_mapping;
+retry: rcu_read_lock(); anon_mapping = (unsigned long)READ_ONCE(folio->mapping); if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON) @@ -552,6 +553,17 @@ struct anon_vma *folio_lock_anon_vma_read(struct folio *folio, anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON); root_anon_vma = READ_ONCE(anon_vma->root); if (down_read_trylock(&root_anon_vma->rwsem)) { + /* + * folio_move_anon_rmap() might have changed the anon_vma as we + * might not hold the folio lock here. + */ + if (unlikely((unsigned long)READ_ONCE(folio->mapping) != + anon_mapping)) { + up_read(&root_anon_vma->rwsem); + rcu_read_unlock(); + goto retry; + } + /* * If the folio is still mapped, then this anon_vma is still * its anon_vma, and holding the mutex ensures that it will @@ -586,6 +598,18 @@ struct anon_vma *folio_lock_anon_vma_read(struct folio *folio, rcu_read_unlock(); anon_vma_lock_read(anon_vma);
+ /* + * folio_move_anon_rmap() might have changed the anon_vma as we might + * not hold the folio lock here. + */ + if (unlikely((unsigned long)READ_ONCE(folio->mapping) != + anon_mapping)) { + anon_vma_unlock_read(anon_vma); + put_anon_vma(anon_vma); + anon_vma = NULL; + goto retry; + } + if (atomic_dec_and_test(&anon_vma->refcount)) { /* * Oops, we held the last refcount, release the lock
From: Andrea Arcangeli aarcange@redhat.com
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@redhat... [2] https://lore.kernel.org/linux-mm/CA+EESO4uO84SSnBhArH4HvLNhaUQ5nZKNKXqxRCyjn...
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 either pinned or 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 target process has exited at the time of a UFFDIO_MOVE operation.
Signed-off-by: Andrea Arcangeli aarcange@redhat.com Signed-off-by: Suren Baghdasaryan surenb@google.com --- Documentation/admin-guide/mm/userfaultfd.rst | 3 + fs/userfaultfd.c | 72 +++ include/linux/rmap.h | 5 + include/linux/userfaultfd_k.h | 11 + include/uapi/linux/userfaultfd.h | 29 +- mm/huge_memory.c | 122 ++++ mm/khugepaged.c | 3 + mm/rmap.c | 6 + mm/userfaultfd.c | 599 +++++++++++++++++++ 9 files changed, 849 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 e8af40b05549..6e2a4d6a0d8f 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -2005,6 +2005,75 @@ static inline unsigned int uffd_ctx_features(__u64 user_features) return (unsigned int)user_features | UFFD_FEATURE_INITIALIZED; }
+static int userfaultfd_move(struct userfaultfd_ctx *ctx, + unsigned long arg) +{ + __s64 ret; + struct uffdio_move uffdio_move; + struct uffdio_move __user *user_uffdio_move; + struct userfaultfd_wake_range range; + struct mm_struct *mm = ctx->mm; + + user_uffdio_move = (struct uffdio_move __user *) arg; + + if (atomic_read(&ctx->mmap_changing)) + return -EAGAIN; + + if (copy_from_user(&uffdio_move, user_uffdio_move, + /* don't copy "move" last field */ + sizeof(uffdio_move)-sizeof(__s64))) + return -EFAULT; + + /* Do not allow cross-mm moves. */ + if (mm != current->mm) + return -EINVAL; + + ret = validate_range(mm, uffdio_move.dst, uffdio_move.len); + if (ret) + return ret; + + ret = validate_range(mm, uffdio_move.src, uffdio_move.len); + if (ret) + return ret; + + if (uffdio_move.mode & ~(UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES| + UFFDIO_MOVE_MODE_DONTWAKE)) + return -EINVAL; + + if (mmget_not_zero(mm)) { + mmap_read_lock(mm); + + /* Re-check after taking mmap_lock */ + if (likely(!atomic_read(&ctx->mmap_changing))) + ret = move_pages(ctx, mm, uffdio_move.dst, uffdio_move.src, + uffdio_move.len, uffdio_move.mode); + else + ret = -EINVAL; + + mmap_read_unlock(mm); + mmput(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 */ + VM_WARN_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 @@ -2097,6 +2166,9 @@ static long userfaultfd_ioctl(struct file *file, unsigned cmd, case UFFDIO_ZEROPAGE: ret = userfaultfd_zeropage(ctx, arg); break; + case UFFDIO_MOVE: + ret = userfaultfd_move(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..e4056547fbe6 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -93,6 +93,17 @@ 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);
+/* move_pages */ +void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2); +void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2); +ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm, + unsigned long dst_start, unsigned long src_start, + unsigned long len, __u64 flags); +int move_pages_huge_pmd(struct mm_struct *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 4f542444a91f..315968db1ca4 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -1964,6 +1964,128 @@ 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 move_pages_huge_pmd(struct mm_struct *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; + struct mmu_notifier_range range; + int err = 0; + + src_pmdval = *src_pmd; + src_ptl = pmd_lockptr(mm, src_pmd); + + lockdep_assert_held(src_ptl); + mmap_assert_locked(mm); + + /* Sanity checks before the operation */ + if (WARN_ON_ONCE(!pmd_none(dst_pmdval)) || WARN_ON_ONCE(src_addr & ~HPAGE_PMD_MASK) || + WARN_ON_ONCE(dst_addr & ~HPAGE_PMD_MASK)) { + spin_unlock(src_ptl); + return -EINVAL; + } + + if (!pmd_trans_huge(src_pmdval)) { + spin_unlock(src_ptl); + if (is_pmd_migration_entry(src_pmdval)) { + pmd_migration_entry_wait(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); + + flush_cache_range(src_vma, src_addr, src_addr + HPAGE_PMD_SIZE); + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, 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(mm, dst_pmd); + double_pt_lock(src_ptl, dst_ptl); + if (unlikely(!pmd_same(*src_pmd, src_pmdval) || + !pmd_same(*dst_pmd, dst_pmdval))) { + err = -EAGAIN; + goto unlock_ptls; + } + if (folio_maybe_dma_pinned(src_folio) || + !PageAnonExclusive(&src_folio->page)) { + err = -EBUSY; + goto unlock_ptls; + } + + if (WARN_ON_ONCE(!folio_test_head(src_folio)) || + WARN_ON_ONCE(!folio_test_anon(src_folio))) { + err = -EBUSY; + goto unlock_ptls; + } + + 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); + /* Folio got pinned from under us. Put it back and fail the move. */ + if (folio_maybe_dma_pinned(src_folio)) { + set_pmd_at(mm, src_addr, src_pmd, src_pmdval); + err = -EBUSY; + goto unlock_ptls; + } + + _dst_pmd = mk_huge_pmd(&src_folio->page, dst_vma->vm_page_prot); + /* Follow mremap() behavior and treat the entry dirty after the move */ + _dst_pmd = pmd_mkwrite(pmd_mkdirty(_dst_pmd), dst_vma); + set_pmd_at(mm, dst_addr, dst_pmd, _dst_pmd); + + src_pgtable = pgtable_trans_huge_withdraw(mm, src_pmd); + pgtable_trans_huge_deposit(mm, dst_pmd, src_pgtable); +unlock_ptls: + double_pt_unlock(src_ptl, dst_ptl); + 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); + 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 064654717843..0da6937572cf 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -1139,6 +1139,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 525c5bc0b0b3..de9426ad0f1b 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 0b6ca553bebe..71d0281f1162 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -842,3 +842,602 @@ 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 move_present_pte(struct mm_struct *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) +{ + int err = 0; + + double_pt_lock(dst_ptl, src_ptl); + + if (!pte_same(*src_pte, orig_src_pte) || + !pte_same(*dst_pte, orig_dst_pte)) { + err = -EAGAIN; + goto out; + } + if (folio_test_large(src_folio) || + folio_maybe_dma_pinned(src_folio) || + !PageAnonExclusive(&src_folio->page)) { + err = -EBUSY; + goto out; + } + + 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); + /* Folio got pinned from under us. Put it back and fail the move. */ + if (folio_maybe_dma_pinned(src_folio)) { + set_pte_at(mm, src_addr, src_pte, orig_src_pte); + err = -EBUSY; + goto out; + } + + orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot); + /* Follow mremap() behavior and treat the entry dirty after the move */ + orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma); + + set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte); +out: + double_pt_unlock(dst_ptl, src_ptl); + return err; +} + +static int move_swap_pte(struct mm_struct *mm, + 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) +{ + if (!pte_swp_exclusive(orig_src_pte)) + return -EBUSY; + + 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; + } + + orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte); + set_pte_at(mm, dst_addr, dst_pte, orig_src_pte); + double_pt_unlock(dst_ptl, src_ptl); + + return 0; +} + +/* + * The mmap_lock for reading is held by the caller. Just move the page + * from src_pmd to dst_pmd if possible, and return true if succeeded + * in moving the page. + */ +static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, + struct vm_area_struct *dst_vma, + struct vm_area_struct *src_vma, + unsigned long dst_addr, unsigned long src_addr, + __u64 mode) +{ + swp_entry_t entry; + pte_t orig_src_pte, orig_dst_pte; + pte_t src_folio_pte; + spinlock_t *src_ptl, *dst_ptl; + pte_t *src_pte = NULL; + pte_t *dst_pte = NULL; + + struct folio *src_folio = NULL; + struct anon_vma *src_anon_vma = NULL; + struct mmu_notifier_range range; + int err = 0; + + flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE); + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, + src_addr, src_addr + PAGE_SIZE); + mmu_notifier_invalidate_range_start(&range); +retry: + dst_pte = pte_offset_map_nolock(mm, dst_pmd, dst_addr, &dst_ptl); + + /* Retry if a huge pmd materialized from under us */ + if (unlikely(!dst_pte)) { + err = -EAGAIN; + goto out; + } + + src_pte = pte_offset_map_nolock(mm, src_pmd, src_addr, &src_ptl); + + /* + * We held the mmap_lock for reading so MADV_DONTNEED + * can zap transparent huge pages under us, or the + * transparent huge page fault can establish new + * transparent huge pages under us. + */ + if (unlikely(!src_pte)) { + err = -EAGAIN; + goto out; + } + + /* Sanity checks before the operation */ + if (WARN_ON_ONCE(pmd_none(*dst_pmd)) || WARN_ON_ONCE(pmd_none(*src_pmd)) || + WARN_ON_ONCE(pmd_trans_huge(*dst_pmd)) || WARN_ON_ONCE(pmd_trans_huge(*src_pmd))) { + err = -EINVAL; + goto out; + } + + spin_lock(dst_ptl); + orig_dst_pte = *dst_pte; + spin_unlock(dst_ptl); + if (!pte_none(orig_dst_pte)) { + err = -EEXIST; + goto out; + } + + spin_lock(src_ptl); + orig_src_pte = *src_pte; + spin_unlock(src_ptl); + if (pte_none(orig_src_pte)) { + if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) + err = -ENOENT; + else /* nothing to do to move a hole */ + err = 0; + goto out; + } + + /* If PTE changed after we locked the folio them start over */ + if (src_folio && unlikely(!pte_same(src_folio_pte, orig_src_pte))) { + err = -EAGAIN; + goto out; + } + + if (pte_present(orig_src_pte)) { + /* + * Pin and lock both source folio and anon_vma. Since we are in + * RCU read section, we can't block, so on contention have to + * unmap the ptes, obtain the lock and retry. + */ + if (!src_folio) { + struct folio *folio; + + /* + * Pin the page while holding the lock to be sure the + * page isn't freed under us + */ + spin_lock(src_ptl); + if (!pte_same(orig_src_pte, *src_pte)) { + spin_unlock(src_ptl); + err = -EAGAIN; + goto out; + } + + folio = vm_normal_folio(src_vma, src_addr, orig_src_pte); + if (!folio || folio_test_large(folio) || + !PageAnonExclusive(&folio->page)) { + spin_unlock(src_ptl); + err = -EBUSY; + goto out; + } + + folio_get(folio); + src_folio = folio; + src_folio_pte = orig_src_pte; + spin_unlock(src_ptl); + + if (!folio_trylock(src_folio)) { + pte_unmap(&orig_src_pte); + pte_unmap(&orig_dst_pte); + src_pte = dst_pte = NULL; + /* now we can block and wait */ + folio_lock(src_folio); + goto retry; + } + + if (WARN_ON_ONCE(!folio_test_anon(src_folio))) { + err = -EBUSY; + goto out; + } + } + + if (!src_anon_vma) { + /* + * folio_referenced walks the anon_vma chain + * without the folio lock. Serialize against it with + * the anon_vma lock, the folio lock is not enough. + */ + src_anon_vma = folio_get_anon_vma(src_folio); + if (!src_anon_vma) { + /* page was unmapped from under us */ + err = -EAGAIN; + goto out; + } + if (!anon_vma_trylock_write(src_anon_vma)) { + pte_unmap(&orig_src_pte); + pte_unmap(&orig_dst_pte); + src_pte = dst_pte = NULL; + /* now we can block and wait */ + anon_vma_lock_write(src_anon_vma); + goto retry; + } + } + + err = move_present_pte(mm, dst_vma, src_vma, + dst_addr, src_addr, dst_pte, src_pte, + orig_dst_pte, orig_src_pte, + dst_ptl, src_ptl, src_folio); + } else { + entry = pte_to_swp_entry(orig_src_pte); + if (non_swap_entry(entry)) { + if (is_migration_entry(entry)) { + pte_unmap(&orig_src_pte); + pte_unmap(&orig_dst_pte); + src_pte = dst_pte = NULL; + migration_entry_wait(mm, src_pmd, src_addr); + err = -EAGAIN; + } else + err = -EFAULT; + goto out; + } + + err = move_swap_pte(mm, dst_addr, src_addr, + dst_pte, src_pte, + orig_dst_pte, orig_src_pte, + dst_ptl, src_ptl); + } + +out: + if (src_anon_vma) { + anon_vma_unlock_write(src_anon_vma); + put_anon_vma(src_anon_vma); + } + if (src_folio) { + folio_unlock(src_folio); + folio_put(src_folio); + } + if (dst_pte) + pte_unmap(dst_pte); + if (src_pte) + pte_unmap(src_pte); + mmu_notifier_invalidate_range_end(&range); + + return err; +} + +#ifdef CONFIG_TRANSPARENT_HUGEPAGE +static inline bool move_splits_huge_pmd(unsigned long dst_addr, + unsigned long src_addr, + unsigned long src_end) +{ + return (src_addr & ~HPAGE_PMD_MASK) || (dst_addr & ~HPAGE_PMD_MASK) || + src_end - src_addr < HPAGE_PMD_SIZE; +} +#else +static inline bool move_splits_huge_pmd(unsigned long dst_addr, + unsigned long src_addr, + unsigned long src_end) +{ + /* This is unreachable anyway, just to avoid warnings when HPAGE_PMD_SIZE==0 */ + return false; +} +#endif + +static inline bool vma_move_compatible(struct vm_area_struct *vma) +{ + return !(vma->vm_flags & (VM_PFNMAP | VM_IO | VM_HUGETLB | + VM_MIXEDMAP | VM_SHADOW_STACK)); +} + +static int validate_move_areas(struct userfaultfd_ctx *ctx, + struct vm_area_struct *src_vma, + struct vm_area_struct *dst_vma) +{ + /* Only allow moving if both have the same access and protection */ + if ((src_vma->vm_flags & VM_ACCESS_FLAGS) != (dst_vma->vm_flags & VM_ACCESS_FLAGS) || + pgprot_val(src_vma->vm_page_prot) != pgprot_val(dst_vma->vm_page_prot)) + return -EINVAL; + + /* Only allow moving if both are mlocked or both aren't */ + if ((src_vma->vm_flags & VM_LOCKED) != (dst_vma->vm_flags & VM_LOCKED)) + return -EINVAL; + + /* + * For now, we keep it simple and only move between writable VMAs. + * Access flags are equal, therefore cheching only the source is enough. + */ + if (!(src_vma->vm_flags & VM_WRITE)) + return -EINVAL; + + /* Check if vma flags indicate content which can be moved */ + if (!vma_move_compatible(src_vma) || !vma_move_compatible(dst_vma)) + return -EINVAL; + + /* Ensure dst_vma is registered in uffd we are operating on */ + if (!dst_vma->vm_userfaultfd_ctx.ctx || + dst_vma->vm_userfaultfd_ctx.ctx != ctx) + return -EINVAL; + + /* Only allow moving across anonymous vmas */ + if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma)) + return -EINVAL; + + /* + * Ensure the dst_vma has a anon_vma or this page + * would get a NULL anon_vma when moved in the + * dst_vma. + */ + if (unlikely(anon_vma_prepare(dst_vma))) + return -ENOMEM; + + return 0; +} + +/** + * move_pages - move arbitrary anonymous pages of an existing vma + * @ctx: pointer to the userfaultfd context + * @mm: the address space to move pages + * @dst_start: start of the destination virtual memory range + * @src_start: start of the source virtual memory range + * @len: length of the virtual memory range + * @mode: flags from uffdio_move.mode + * + * Must be called with mmap_lock held for read. + * + * move_pages() remaps arbitrary anonymous pages atomically in zero + * copy. It only works on non shared anonymous pages because those can + * be relocated without generating non linear anon_vmas in the rmap + * code. + * + * It provides a zero copy mechanism to handle userspace page faults. + * The source vma pages should have mapcount == 1, which can be + * enforced by using madvise(MADV_DONTFORK) on src vma. + * + * The thread receiving the page during the userland page fault + * will receive the faulting page in the source vma through the network, + * storage or any other I/O device (MADV_DONTFORK in the source vma + * avoids move_pages() to fail with -EBUSY if the process forks before + * move_pages() is called), then it will call move_pages() to map the + * page in the faulting address in the destination vma. + * + * This userfaultfd command works purely via pagetables, so it's the + * most efficient way to move physical non shared anonymous pages + * across different virtual addresses. Unlike mremap()/mmap()/munmap() + * it does not create any new vmas. The mapping in the destination + * address is atomic. + * + * It only works if the vma protection bits are identical from the + * source and destination vma. + * + * It can remap non shared anonymous pages within the same vma too. + * + * If the source virtual memory range has any unmapped holes, or if + * the destination virtual memory range is not a whole unmapped hole, + * move_pages() will fail respectively with -ENOENT or -EEXIST. This + * provides a very strict behavior to avoid any chance of memory + * corruption going unnoticed if there are userland race conditions. + * Only one thread should resolve the userland page fault at any given + * time for any given faulting address. This means that if two threads + * try to both call move_pages() on the same destination address at the + * same time, the second thread will get an explicit error from this + * command. + * + * The command retval will return "len" is successful. The command + * however can be interrupted by fatal signals or errors. If + * interrupted it will return the number of bytes successfully + * remapped before the interruption if any, or the negative error if + * none. It will never return zero. Either it will return an error or + * an amount of bytes successfully moved. If the retval reports a + * "short" remap, the move_pages() command should be repeated by + * userland with src+retval, dst+reval, len-retval if it wants to know + * about the error that interrupted it. + * + * The UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES flag can be specified to + * prevent -ENOENT errors to materialize if there are holes in the + * source virtual range that is being remapped. The holes will be + * accounted as successfully remapped in the retval of the + * command. This is mostly useful to remap hugepage naturally aligned + * virtual regions without knowing if there are transparent hugepage + * in the regions or not, but preventing the risk of having to split + * the hugepmd during the remap. + * + * If there's any rmap walk that is taking the anon_vma locks without + * first obtaining the folio lock (the only current instance is + * folio_referenced), they will have to verify if the folio->mapping + * has changed after taking the anon_vma lock. If it changed they + * should release the lock and retry obtaining a new anon_vma, because + * it means the anon_vma was changed by move_pages() before the lock + * could be obtained. This is the only additional complexity added to + * the rmap code to provide this anonymous page remapping functionality. + */ +ssize_t move_pages(struct userfaultfd_ctx *ctx, struct mm_struct *mm, + unsigned long dst_start, unsigned long src_start, + unsigned long len, __u64 mode) +{ + struct vm_area_struct *src_vma, *dst_vma; + unsigned long src_addr, dst_addr; + pmd_t *src_pmd, *dst_pmd; + long err = -EINVAL; + ssize_t moved = 0; + + /* Sanitize the command parameters. */ + if (WARN_ON_ONCE(src_start & ~PAGE_MASK) || + WARN_ON_ONCE(dst_start & ~PAGE_MASK) || + WARN_ON_ONCE(len & ~PAGE_MASK)) + goto out; + + /* Does the address range wrap, or is the span zero-sized? */ + if (WARN_ON_ONCE(src_start + len <= src_start) || + WARN_ON_ONCE(dst_start + len <= dst_start)) + goto out; + + /* + * Make sure the vma is not shared, that the src and dst remap + * ranges are both valid and fully within a single existing + * vma. + */ + src_vma = find_vma(mm, src_start); + if (!src_vma || (src_vma->vm_flags & VM_SHARED)) + goto out; + if (src_start < src_vma->vm_start || + src_start + len > src_vma->vm_end) + goto out; + + dst_vma = find_vma(mm, dst_start); + if (!dst_vma || (dst_vma->vm_flags & VM_SHARED)) + goto out; + if (dst_start < dst_vma->vm_start || + dst_start + len > dst_vma->vm_end) + goto out; + + err = validate_move_areas(ctx, src_vma, dst_vma); + if (err) + goto out; + + for (src_addr = src_start, dst_addr = dst_start; + src_addr < src_start + len;) { + spinlock_t *ptl; + pmd_t dst_pmdval; + unsigned long step_size; + + /* + * Below works because anonymous area would not have a + * transparent huge PUD. If file-backed support is added, + * that case would need to be handled here. + */ + src_pmd = mm_find_pmd(mm, src_addr); + if (unlikely(!src_pmd)) { + if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) { + err = -ENOENT; + break; + } + src_pmd = mm_alloc_pmd(mm, src_addr); + if (unlikely(!src_pmd)) { + err = -ENOMEM; + break; + } + } + dst_pmd = mm_alloc_pmd(mm, dst_addr); + if (unlikely(!dst_pmd)) { + err = -ENOMEM; + break; + } + + dst_pmdval = pmdp_get_lockless(dst_pmd); + /* + * If the dst_pmd is mapped as THP don't override it and just + * be strict. If dst_pmd changes into TPH after this check, the + * move_pages_huge_pmd() will detect the change and retry + * while move_pages_pte() will detect the change and fail. + */ + if (unlikely(pmd_trans_huge(dst_pmdval))) { + err = -EEXIST; + break; + } + + ptl = pmd_trans_huge_lock(src_pmd, src_vma); + if (ptl) { + if (pmd_devmap(*src_pmd)) { + spin_unlock(ptl); + err = -ENOENT; + break; + } + + /* Check if we can move the pmd without splitting it. */ + if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) || + !pmd_none(dst_pmdval)) { + spin_unlock(ptl); + split_huge_pmd(src_vma, src_pmd, src_addr); + continue; + } + + err = move_pages_huge_pmd(mm, dst_pmd, src_pmd, + dst_pmdval, dst_vma, src_vma, + dst_addr, src_addr); + step_size = HPAGE_PMD_SIZE; + } else { + if (pmd_none(*src_pmd)) { + if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) { + err = -ENOENT; + break; + } + if (unlikely(__pte_alloc(mm, src_pmd))) { + err = -ENOMEM; + break; + } + } + + if (unlikely(pte_alloc(mm, dst_pmd))) { + err = -ENOMEM; + break; + } + + err = move_pages_pte(mm, dst_pmd, src_pmd, + dst_vma, src_vma, + dst_addr, src_addr, mode); + step_size = PAGE_SIZE; + } + + cond_resched(); + + if (fatal_signal_pending(current)) { + /* Do not override an error */ + if (!err || err == -EAGAIN) + err = -EINTR; + break; + } + + if (err) { + if (err == -EAGAIN) + continue; + break; + } + + /* Proceed to the next page */ + dst_addr += step_size; + src_addr += step_size; + moved += step_size; + } + +out: + VM_WARN_ON(moved < 0); + VM_WARN_ON(err > 0); + VM_WARN_ON(!moved && !err); + return moved ? moved : err; +}
uffd_test_ctx_clear() is being called from uffd_test_ctx_init() to unmap areas used in the previous test run. This approach is problematic because while unmapping areas uffd_test_ctx_clear() uses page_size and nr_pages which might differ from one test run to another. Fix this by calling uffd_test_ctx_clear() after each test is done.
Signed-off-by: Suren Baghdasaryan surenb@google.com Reviewed-by: Peter Xu peterx@redhat.com Reviewed-by: Axel Rasmussen axelrasmussen@google.com --- tools/testing/selftests/mm/uffd-common.c | 4 +--- tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-stress.c | 5 ++++- tools/testing/selftests/mm/uffd-unit-tests.c | 1 + 4 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index 02b89860e193..583e5a4cc0fd 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -262,7 +262,7 @@ static inline void munmap_area(void **area) *area = NULL; }
-static void uffd_test_ctx_clear(void) +void uffd_test_ctx_clear(void) { size_t i;
@@ -298,8 +298,6 @@ int uffd_test_ctx_init(uint64_t features, const char **errmsg) unsigned long nr, cpu; int ret;
- uffd_test_ctx_clear(); - ret = uffd_test_ops->allocate_area((void **)&area_src, true); ret |= uffd_test_ops->allocate_area((void **)&area_dst, false); if (ret) { diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index 7c4fa964c3b0..870776b5a323 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -105,6 +105,7 @@ extern uffd_test_ops_t *uffd_test_ops;
void uffd_stats_report(struct uffd_args *args, int n_cpus); int uffd_test_ctx_init(uint64_t features, const char **errmsg); +void uffd_test_ctx_clear(void); int userfaultfd_open(uint64_t *features); int uffd_read_msg(int ufd, struct uffd_msg *msg); void wp_range(int ufd, __u64 start, __u64 len, bool wp); diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c index 469e0476af26..7e83829bbb33 100644 --- a/tools/testing/selftests/mm/uffd-stress.c +++ b/tools/testing/selftests/mm/uffd-stress.c @@ -323,8 +323,10 @@ static int userfaultfd_stress(void) uffd_stats_reset(args, nr_cpus);
/* bounce pass */ - if (stress(args)) + if (stress(args)) { + uffd_test_ctx_clear(); return 1; + }
/* Clear all the write protections if there is any */ if (test_uffdio_wp) @@ -354,6 +356,7 @@ static int userfaultfd_stress(void)
uffd_stats_report(args, nr_cpus); } + uffd_test_ctx_clear();
return 0; } diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index 2709a34a39c5..e7d43c198041 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -1319,6 +1319,7 @@ int main(int argc, char *argv[]) continue; } test->uffd_fn(&args); + uffd_test_ctx_clear(); } }
Currently each test can specify unique operations using uffd_test_ops, however these operations are per-memory type and not per-test. Add uffd_test_case_ops which each test case can customize for its own needs regardless of the memory type being used. Pre- and post-allocation operations are added, some of which will be used in the next patch to implement test-specific operations like madvise after memory is allocated but before it is accessed.
Signed-off-by: Suren Baghdasaryan surenb@google.com --- tools/testing/selftests/mm/uffd-common.c | 13 +++++++++++++ tools/testing/selftests/mm/uffd-common.h | 7 +++++++ tools/testing/selftests/mm/uffd-unit-tests.c | 2 ++ 3 files changed, 22 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index 583e5a4cc0fd..fb3bbc77fd00 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -17,6 +17,7 @@ bool map_shared; bool test_uffdio_wp = true; unsigned long long *count_verify; uffd_test_ops_t *uffd_test_ops; +uffd_test_case_ops_t *uffd_test_case_ops;
static int uffd_mem_fd_create(off_t mem_size, bool hugetlb) { @@ -298,6 +299,12 @@ int uffd_test_ctx_init(uint64_t features, const char **errmsg) unsigned long nr, cpu; int ret;
+ if (uffd_test_case_ops && uffd_test_case_ops->pre_alloc) { + ret = uffd_test_case_ops->pre_alloc(errmsg); + if (ret) + return ret; + } + ret = uffd_test_ops->allocate_area((void **)&area_src, true); ret |= uffd_test_ops->allocate_area((void **)&area_dst, false); if (ret) { @@ -306,6 +313,12 @@ int uffd_test_ctx_init(uint64_t features, const char **errmsg) return ret; }
+ if (uffd_test_case_ops && uffd_test_case_ops->post_alloc) { + ret = uffd_test_case_ops->post_alloc(errmsg); + if (ret) + return ret; + } + ret = userfaultfd_open(&features); if (ret) { if (errmsg) diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index 870776b5a323..774595ee629e 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -90,6 +90,12 @@ struct uffd_test_ops { }; typedef struct uffd_test_ops uffd_test_ops_t;
+struct uffd_test_case_ops { + int (*pre_alloc)(const char **errmsg); + int (*post_alloc)(const char **errmsg); +}; +typedef struct uffd_test_case_ops uffd_test_case_ops_t; + extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size; extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap; extern int uffd, uffd_flags, finished, *pipefd, test_type; @@ -102,6 +108,7 @@ extern uffd_test_ops_t anon_uffd_test_ops; extern uffd_test_ops_t shmem_uffd_test_ops; extern uffd_test_ops_t hugetlb_uffd_test_ops; extern uffd_test_ops_t *uffd_test_ops; +extern uffd_test_case_ops_t *uffd_test_case_ops;
void uffd_stats_report(struct uffd_args *args, int n_cpus); int uffd_test_ctx_init(uint64_t features, const char **errmsg); diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index e7d43c198041..debc423bdbf4 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -78,6 +78,7 @@ typedef struct { uffd_test_fn uffd_fn; unsigned int mem_targets; uint64_t uffd_feature_required; + uffd_test_case_ops_t *test_case_ops; } uffd_test_case_t;
static void uffd_test_report(void) @@ -185,6 +186,7 @@ uffd_setup_environment(uffd_test_args_t *args, uffd_test_case_t *test, { map_shared = mem_type->shared; uffd_test_ops = mem_type->mem_ops; + uffd_test_case_ops = test->test_case_ops;
if (mem_type->mem_flag & (MEM_HUGETLB_PRIVATE | MEM_HUGETLB)) page_size = default_huge_page_size();
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com --- tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); }
+int move_page(int ufd, unsigned long offset, unsigned long len) +{ + struct uffdio_move uffdio_move; + + if (offset + len > nr_pages * page_size) + err("unexpected offset %lu and length %lu\n", offset, len); + uffdio_move.dst = (unsigned long) area_dst + offset; + uffdio_move.src = (unsigned long) area_src + offset; + uffdio_move.len = len; + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; + uffdio_move.move = 0; + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { + /* real retval in uffdio_move.move */ + if (uffdio_move.move != -EEXIST) + err("UFFDIO_MOVE error: %"PRId64, + (int64_t)uffdio_move.move); + wake_range(ufd, uffdio_move.dst, len); + } else if (uffdio_move.move != len) { + err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move); + } else + return 1; + return 0; +} + int uffd_open_dev(unsigned int flags) { int fd, uffd; diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h index 774595ee629e..cb055282c89c 100644 --- a/tools/testing/selftests/mm/uffd-common.h +++ b/tools/testing/selftests/mm/uffd-common.h @@ -119,6 +119,7 @@ void wp_range(int ufd, __u64 start, __u64 len, bool wp); void uffd_handle_page_fault(struct uffd_msg *msg, struct uffd_args *args); int __copy_page(int ufd, unsigned long offset, bool retry, bool wp); int copy_page(int ufd, unsigned long offset, bool wp); +int move_page(int ufd, unsigned long offset, unsigned long len); void *uffd_poll_thread(void *arg);
int uffd_open_dev(unsigned int flags); diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c index debc423bdbf4..e4e271511db9 100644 --- a/tools/testing/selftests/mm/uffd-unit-tests.c +++ b/tools/testing/selftests/mm/uffd-unit-tests.c @@ -23,6 +23,9 @@ #define MEM_ALL (MEM_ANON | MEM_SHMEM | MEM_SHMEM_PRIVATE | \ MEM_HUGETLB | MEM_HUGETLB_PRIVATE)
+#define ALIGN_UP(x, align_to) \ + ((__typeof__(x))((((unsigned long)(x)) + ((align_to)-1)) & ~((align_to)-1))) + struct mem_type { const char *name; unsigned int mem_flag; @@ -1064,6 +1067,178 @@ static void uffd_poison_test(uffd_test_args_t *targs) uffd_test_pass(); }
+static void +uffd_move_handle_fault_common(struct uffd_msg *msg, struct uffd_args *args, + unsigned long len) +{ + unsigned long offset; + + if (msg->event != UFFD_EVENT_PAGEFAULT) + err("unexpected msg event %u", msg->event); + + if (msg->arg.pagefault.flags & + (UFFD_PAGEFAULT_FLAG_WP | UFFD_PAGEFAULT_FLAG_MINOR | UFFD_PAGEFAULT_FLAG_WRITE)) + err("unexpected fault type %llu", msg->arg.pagefault.flags); + + offset = (char *)(unsigned long)msg->arg.pagefault.address - area_dst; + offset &= ~(len-1); + + if (move_page(uffd, offset, len)) + args->missing_faults++; +} + +static void uffd_move_handle_fault(struct uffd_msg *msg, + struct uffd_args *args) +{ + uffd_move_handle_fault_common(msg, args, page_size); +} + +static void uffd_move_pmd_handle_fault(struct uffd_msg *msg, + struct uffd_args *args) +{ + uffd_move_handle_fault_common(msg, args, default_huge_page_size()); +} + +static void +uffd_move_test_common(uffd_test_args_t *targs, unsigned long chunk_size, + void (*handle_fault)(struct uffd_msg *msg, struct uffd_args *args)) +{ + unsigned long nr; + pthread_t uffd_mon; + char c; + unsigned long long count; + struct uffd_args args = { 0 }; + char *orig_area_src, *orig_area_dst; + unsigned long step_size, step_count; + unsigned long src_offs = 0; + unsigned long dst_offs = 0; + + /* Prevent source pages from being mapped more than once */ + if (madvise(area_src, nr_pages * page_size, MADV_DONTFORK)) + err("madvise(MADV_DONTFORK) failure"); + + if (uffd_register(uffd, area_dst, nr_pages * page_size, + true, false, false)) + err("register failure"); + + args.handle_fault = handle_fault; + if (pthread_create(&uffd_mon, NULL, uffd_poll_thread, &args)) + err("uffd_poll_thread create"); + + step_size = chunk_size / page_size; + step_count = nr_pages / step_size; + + if (step_size > page_size) { + char *aligned_src = ALIGN_UP(area_src, chunk_size); + char *aligned_dst = ALIGN_UP(area_dst, chunk_size); + + if (aligned_src != area_src || aligned_dst != area_dst) { + src_offs = (aligned_src - area_src) / page_size; + dst_offs = (aligned_dst - area_dst) / page_size; + step_count--; + } + orig_area_src = area_src; + orig_area_dst = area_dst; + area_src = aligned_src; + area_dst = aligned_dst; + } + + /* + * Read each of the pages back using the UFFD-registered mapping. We + * expect that the first time we touch a page, it will result in a missing + * fault. uffd_poll_thread will resolve the fault by moving source + * page to destination. + */ + for (nr = 0; nr < step_count * step_size; nr += step_size) { + unsigned long i; + + /* Check area_src content */ + for (i = 0; i < step_size; i++) { + count = *area_count(area_src, nr + i); + if (count != count_verify[src_offs + nr + i]) + err("nr %lu source memory invalid %llu %llu\n", + nr + i, count, count_verify[src_offs + nr + i]); + } + + /* Faulting into area_dst should move the page or the huge page */ + for (i = 0; i < step_size; i++) { + count = *area_count(area_dst, nr + i); + if (count != count_verify[dst_offs + nr + i]) + err("nr %lu memory corruption %llu %llu\n", + nr, count, count_verify[dst_offs + nr + i]); + } + + /* Re-check area_src content which should be empty */ + for (i = 0; i < step_size; i++) { + count = *area_count(area_src, nr + i); + if (count != 0) + err("nr %lu move failed %llu %llu\n", + nr, count, count_verify[src_offs + nr + i]); + } + } + if (step_size > page_size) { + area_src = orig_area_src; + area_dst = orig_area_dst; + } + + if (write(pipefd[1], &c, sizeof(c)) != sizeof(c)) + err("pipe write"); + if (pthread_join(uffd_mon, NULL)) + err("join() failed"); + + if (args.missing_faults != step_count || args.minor_faults != 0) + uffd_test_fail("stats check error"); + else + uffd_test_pass(); +} + +static void uffd_move_test(uffd_test_args_t *targs) +{ + uffd_move_test_common(targs, page_size, uffd_move_handle_fault); +} + +static void uffd_move_pmd_test(uffd_test_args_t *targs) +{ + uffd_move_test_common(targs, default_huge_page_size(), + uffd_move_pmd_handle_fault); +} + +static int prevent_hugepages(const char **errmsg) +{ + /* This should be done before source area is populated */ + if (madvise(area_src, nr_pages * page_size, MADV_NOHUGEPAGE)) { + /* Ignore only if CONFIG_TRANSPARENT_HUGEPAGE=n */ + if (errno != EINVAL) { + if (errmsg) + *errmsg = "madvise(MADV_NOHUGEPAGE) failed"; + return -errno; + } + } + return 0; +} + +static int request_hugepages(const char **errmsg) +{ + /* This should be done before source area is populated */ + if (madvise(area_src, nr_pages * page_size, MADV_HUGEPAGE)) { + if (errmsg) { + *errmsg = (errno == EINVAL) ? + "CONFIG_TRANSPARENT_HUGEPAGE is not set" : + "madvise(MADV_HUGEPAGE) failed"; + } + return -errno; + } + return 0; +} + +struct uffd_test_case_ops uffd_move_test_case_ops = { + .post_alloc = prevent_hugepages, +}; + +struct uffd_test_case_ops uffd_move_test_pmd_case_ops = { + .post_alloc = request_hugepages, +}; + /* * Test the returned uffdio_register.ioctls with different register modes. * Note that _UFFDIO_ZEROPAGE is tested separately in the zeropage test. @@ -1141,6 +1316,20 @@ uffd_test_case_t uffd_tests[] = { .mem_targets = MEM_ALL, .uffd_feature_required = 0, }, + { + .name = "move", + .uffd_fn = uffd_move_test, + .mem_targets = MEM_ANON, + .uffd_feature_required = UFFD_FEATURE_MOVE, + .test_case_ops = &uffd_move_test_case_ops, + }, + { + .name = "move-pmd", + .uffd_fn = uffd_move_pmd_test, + .mem_targets = MEM_ANON, + .uffd_feature_required = UFFD_FEATURE_MOVE, + .test_case_ops = &uffd_move_test_pmd_case_ops, + }, { .name = "wp-fork", .uffd_fn = uffd_wp_fork_test,
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); } +int move_page(int ufd, unsigned long offset, unsigned long len) +{
- struct uffdio_move uffdio_move;
- if (offset + len > nr_pages * page_size)
err("unexpected offset %lu and length %lu\n", offset, len);- uffdio_move.dst = (unsigned long) area_dst + offset;
- uffdio_move.src = (unsigned long) area_src + offset;
- uffdio_move.len = len;
- uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES;
- uffdio_move.move = 0;
- if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) {
/* real retval in uffdio_move.move */if (uffdio_move.move != -EEXIST)err("UFFDIO_MOVE error: %"PRId64,(int64_t)uffdio_move.move);
Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Thanks, Ryan
wake_range(ufd, uffdio_move.dst, len);- } else if (uffdio_move.move != len) {
err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move);- } else
return 1;- return 0;
+}
On Fri, Dec 1, 2023 at 1:29 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); }
+int move_page(int ufd, unsigned long offset, unsigned long len) +{
struct uffdio_move uffdio_move;if (offset + len > nr_pages * page_size)err("unexpected offset %lu and length %lu\n", offset, len);uffdio_move.dst = (unsigned long) area_dst + offset;uffdio_move.src = (unsigned long) area_src + offset;uffdio_move.len = len;uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES;uffdio_move.move = 0;if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) {/* real retval in uffdio_move.move */if (uffdio_move.move != -EEXIST)err("UFFDIO_MOVE error: %"PRId64,(int64_t)uffdio_move.move);Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Hi Ryan, Thanks for reporting! Could you please share your kernel config file?
There are several places UFFDIO_MOVE returns EBUSY: 4 places in move_pages_huge_pmd(), 2 places in move_present_pte(), 2 places in move_pages_pte() and once in move_swap_pte(). While I'm trying to reproduce, it would be useful if you could check which place is triggering the error. Thanks, Suren.
Thanks, Ryan
wake_range(ufd, uffdio_move.dst, len);} else if (uffdio_move.move != len) {err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move);} elsereturn 1;return 0;+}
-- To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
On 01/12/2023 16:26, Suren Baghdasaryan wrote:
On Fri, Dec 1, 2023 at 1:29 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); }
+int move_page(int ufd, unsigned long offset, unsigned long len) +{
struct uffdio_move uffdio_move;if (offset + len > nr_pages * page_size)err("unexpected offset %lu and length %lu\n", offset, len);uffdio_move.dst = (unsigned long) area_dst + offset;uffdio_move.src = (unsigned long) area_src + offset;uffdio_move.len = len;uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES;uffdio_move.move = 0;if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) {/* real retval in uffdio_move.move */if (uffdio_move.move != -EEXIST)err("UFFDIO_MOVE error: %"PRId64,(int64_t)uffdio_move.move);Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Hi Ryan, Thanks for reporting! Could you please share your kernel config file?
It's arm64 defconfig (so 4K base pages) plus:
# Squashfs for snaps, xfs for large file folios. ./scripts/config --enable CONFIG_SQUASHFS_LZ4 ./scripts/config --enable CONFIG_SQUASHFS_LZO ./scripts/config --enable CONFIG_SQUASHFS_XZ ./scripts/config --enable CONFIG_SQUASHFS_ZSTD ./scripts/config --enable CONFIG_XFS_FS
# Useful trace features (on for Ubuntu configs). ./scripts/config --enable CONFIG_FTRACE ./scripts/config --enable CONFIG_FUNCTION_TRACER ./scripts/config --enable CONFIG_KPROBES ./scripts/config --enable CONFIG_HIST_TRIGGERS ./scripts/config --enable CONFIG_FTRACE_SYSCALLS
# For general mm debug. ./scripts/config --enable CONFIG_DEBUG_VM ./scripts/config --enable CONFIG_DEBUG_VM_MAPLE_TREE ./scripts/config --enable CONFIG_DEBUG_VM_RB ./scripts/config --enable CONFIG_DEBUG_VM_PGFLAGS ./scripts/config --enable CONFIG_DEBUG_VM_PGTABLE ./scripts/config --enable CONFIG_PAGE_TABLE_CHECK
# For mm selftests. ./scripts/config --enable CONFIG_USERFAULTFD ./scripts/config --enable CONFIG_TEST_VMALLOC ./scripts/config --enable CONFIG_GUP_TEST
This is the config I always use when running mm selftests. I'll send you the config file separately.
Then I'm running in a QEMU VM with 12G RAM, equally split across 2 (emulated) numa nodes. I have these pertinent kernel command line args (intended to ensure all the mm selftests can run):
transparent_hugepage=madvise secretmem.enable hugepagesz=1G hugepages=0:2,1:2 hugepagesz=32M hugepages=0:2,1:2 default_hugepagesz=2M hugepages=0:64,1:64 hugepagesz=64K hugepages=0:2,1:2
There are several places UFFDIO_MOVE returns EBUSY: 4 places in move_pages_huge_pmd(), 2 places in move_present_pte(), 2 places in move_pages_pte() and once in move_swap_pte(). While I'm trying to reproduce, it would be useful if you could check which place is triggering the error.
Happy to, but will have to wait for Monday. I should say, the test fails consistently for me. But sometimes the failure is EAGAIN (11). Most of the time it is EBUSY though and I haven't figured out what causes the difference.
Thanks, Suren.
Thanks, Ryan
wake_range(ufd, uffdio_move.dst, len);} else if (uffdio_move.move != len) {err("UFFDIO_MOVE error: %"PRId64, (int64_t)uffdio_move.move);} elsereturn 1;return 0;+}
-- To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
On 01.12.23 10:29, Ryan Roberts wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); } +int move_page(int ufd, unsigned long offset, unsigned long len) +{
- struct uffdio_move uffdio_move;
- if (offset + len > nr_pages * page_size)
err("unexpected offset %lu and length %lu\n", offset, len);- uffdio_move.dst = (unsigned long) area_dst + offset;
- uffdio_move.src = (unsigned long) area_src + offset;
- uffdio_move.len = len;
- uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES;
- uffdio_move.move = 0;
- if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) {
/* real retval in uffdio_move.move */if (uffdio_move.move != -EEXIST)err("UFFDIO_MOVE error: %"PRId64,(int64_t)uffdio_move.move);Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
That, however, does not necessarily correspond to the THP size. That one can be obtained using read_pmd_pagesize() in vm_util.c
I quickly scanned the code (still want to take a deeper look), but all PAE checks looked sane to me.
I think the issue is folio split handling. I replied to the patch.
On Fri, Dec 1, 2023 at 12:47 PM David Hildenbrand david@redhat.com wrote:
On 01.12.23 10:29, Ryan Roberts wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); }
+int move_page(int ufd, unsigned long offset, unsigned long len) +{
- struct uffdio_move uffdio_move;
- if (offset + len > nr_pages * page_size)
err("unexpected offset %lu and length %lu\n", offset, len);- uffdio_move.dst = (unsigned long) area_dst + offset;
- uffdio_move.src = (unsigned long) area_src + offset;
- uffdio_move.len = len;
- uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES;
- uffdio_move.move = 0;
- if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) {
/* real retval in uffdio_move.move */if (uffdio_move.move != -EEXIST)err("UFFDIO_MOVE error: %"PRId64,(int64_t)uffdio_move.move);Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
That, however, does not necessarily correspond to the THP size. That one can be obtained using read_pmd_pagesize() in vm_util.c
Oh, I didn't realize that default_huge_page_size() is not always the actual THP size. Will fix.
I quickly scanned the code (still want to take a deeper look), but all PAE checks looked sane to me.
I think the issue is folio split handling. I replied to the patch.
Thanks for your hint! That's very possibly the reason for the test failure. I'm in the process of trying to reproduce this issue on ARM64. If I'm unable to do so then I'll create a patch to split PTE-mapped THP when encountered and will ask Ryan to try that. Thanks, Suren.
-- Cheers,
David / dhildenb
On 01/12/2023 20:47, David Hildenbrand wrote:
On 01.12.23 10:29, Ryan Roberts wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); } +int move_page(int ufd, unsigned long offset, unsigned long len) +{ + struct uffdio_move uffdio_move;
+ if (offset + len > nr_pages * page_size) + err("unexpected offset %lu and length %lu\n", offset, len); + uffdio_move.dst = (unsigned long) area_dst + offset; + uffdio_move.src = (unsigned long) area_src + offset; + uffdio_move.len = len; + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; + uffdio_move.move = 0; + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { + /* real retval in uffdio_move.move */ + if (uffdio_move.move != -EEXIST) + err("UFFDIO_MOVE error: %"PRId64, + (int64_t)uffdio_move.move);
Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config.
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
My kernel command line is explicitly seting the default huge page size to 2M.
That, however, does not necessarily correspond to the THP size. That one can be obtained using read_pmd_pagesize() in vm_util.c
I quickly scanned the code (still want to take a deeper look), but all PAE checks looked sane to me.
I think the issue is folio split handling. I replied to the patch.
On 02.12.23 09:04, Ryan Roberts wrote:
On 01/12/2023 20:47, David Hildenbrand wrote:
On 01.12.23 10:29, Ryan Roberts wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); } +int move_page(int ufd, unsigned long offset, unsigned long len) +{ + struct uffdio_move uffdio_move;
+ if (offset + len > nr_pages * page_size) + err("unexpected offset %lu and length %lu\n", offset, len); + uffdio_move.dst = (unsigned long) area_dst + offset; + uffdio_move.src = (unsigned long) area_src + offset; + uffdio_move.len = len; + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; + uffdio_move.move = 0; + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { + /* real retval in uffdio_move.move */ + if (uffdio_move.move != -EEXIST) + err("UFFDIO_MOVE error: %"PRId64, + (int64_t)uffdio_move.move);
Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config.
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
My kernel command line is explicitly seting the default huge page size to 2M.
Okay, so that likely won't affect it.
I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64.
uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified?
So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest?
Not sure if that could trigger the THP splitting issue, though.
But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote:
On 02.12.23 09:04, Ryan Roberts wrote:
On 01/12/2023 20:47, David Hildenbrand wrote:
On 01.12.23 10:29, Ryan Roberts wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); } +int move_page(int ufd, unsigned long offset, unsigned long len) +{
- struct uffdio_move uffdio_move;
- if (offset + len > nr_pages * page_size)
err("unexpected offset %lu and length %lu\n", offset, len);- uffdio_move.dst = (unsigned long) area_dst + offset;
- uffdio_move.src = (unsigned long) area_src + offset;
- uffdio_move.len = len;
- uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES;
- uffdio_move.move = 0;
- if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) {
/* real retval in uffdio_move.move */if (uffdio_move.move != -EEXIST)err("UFFDIO_MOVE error: %"PRId64,(int64_t)uffdio_move.move);Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config.
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
My kernel command line is explicitly seting the default huge page size to 2M.
Okay, so that likely won't affect it.
I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64.
uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified?
So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest?
Not sure if that could trigger the THP splitting issue, though.
But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again? Thanks, Suren.
-- Cheers,
David / dhildenb
On 04/12/2023 04:09, Suren Baghdasaryan wrote:
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote:
On 02.12.23 09:04, Ryan Roberts wrote:
On 01/12/2023 20:47, David Hildenbrand wrote:
On 01.12.23 10:29, Ryan Roberts wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote:
Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source into destination buffer while checking the contents of both after the move. After the operation the content of the destination buffer should match the original source buffer's content while the source buffer should be zeroed. Separate tests are designed for PMD aligned and unaligned cases because they utilize different code paths in the kernel.
Signed-off-by: Suren Baghdasaryan surenb@google.com
tools/testing/selftests/mm/uffd-common.c | 24 +++ tools/testing/selftests/mm/uffd-common.h | 1 + tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ 3 files changed, 214 insertions(+)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c index fb3bbc77fd00..b0ac0ec2356d 100644 --- a/tools/testing/selftests/mm/uffd-common.c +++ b/tools/testing/selftests/mm/uffd-common.c @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) return __copy_page(ufd, offset, false, wp); } +int move_page(int ufd, unsigned long offset, unsigned long len) +{
- struct uffdio_move uffdio_move;
- if (offset + len > nr_pages * page_size)
err("unexpected offset %lu and length %lu\n", offset, len);- uffdio_move.dst = (unsigned long) area_dst + offset;
- uffdio_move.src = (unsigned long) area_src + offset;
- uffdio_move.len = len;
- uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES;
- uffdio_move.move = 0;
- if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) {
/* real retval in uffdio_move.move */if (uffdio_move.move != -EEXIST)err("UFFDIO_MOVE error: %"PRId64,(int64_t)uffdio_move.move);Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config.
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
My kernel command line is explicitly seting the default huge page size to 2M.
Okay, so that likely won't affect it.
I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64.
uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified?
So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest?
Not sure if that could trigger the THP splitting issue, though.
But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again?
Yep, all fixed with those patches!
Thanks, Suren.
-- Cheers,
David / dhildenb
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 04/12/2023 04:09, Suren Baghdasaryan wrote:
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote:
On 02.12.23 09:04, Ryan Roberts wrote:
On 01/12/2023 20:47, David Hildenbrand wrote:
On 01.12.23 10:29, Ryan Roberts wrote:
On 21/11/2023 17:16, Suren Baghdasaryan wrote: > Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source > into destination buffer while checking the contents of both after > the move. After the operation the content of the destination buffer > should match the original source buffer's content while the source > buffer should be zeroed. Separate tests are designed for PMD aligned and > unaligned cases because they utilize different code paths in the kernel. > > Signed-off-by: Suren Baghdasaryan surenb@google.com > --- > tools/testing/selftests/mm/uffd-common.c | 24 +++ > tools/testing/selftests/mm/uffd-common.h | 1 + > tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ > 3 files changed, 214 insertions(+) > > diff --git a/tools/testing/selftests/mm/uffd-common.c > b/tools/testing/selftests/mm/uffd-common.c > index fb3bbc77fd00..b0ac0ec2356d 100644 > --- a/tools/testing/selftests/mm/uffd-common.c > +++ b/tools/testing/selftests/mm/uffd-common.c > @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) > return __copy_page(ufd, offset, false, wp); > } > +int move_page(int ufd, unsigned long offset, unsigned long len) > +{ > + struct uffdio_move uffdio_move; > + > + if (offset + len > nr_pages * page_size) > + err("unexpected offset %lu and length %lu\n", offset, len); > + uffdio_move.dst = (unsigned long) area_dst + offset; > + uffdio_move.src = (unsigned long) area_src + offset; > + uffdio_move.len = len; > + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; > + uffdio_move.move = 0; > + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { > + /* real retval in uffdio_move.move */ > + if (uffdio_move.move != -EEXIST) > + err("UFFDIO_MOVE error: %"PRId64, > + (int64_t)uffdio_move.move);
Hi Suren,
FYI this error is triggering in mm-unstable (715b67adf4c8):
Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, @uffd-common.c:648)
I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config.
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
My kernel command line is explicitly seting the default huge page size to 2M.
Okay, so that likely won't affect it.
I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64.
uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified?
So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest?
Not sure if that could trigger the THP splitting issue, though.
But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again?
Yep, all fixed with those patches!
Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect. Thanks, Suren.
Thanks, Suren.
-- Cheers,
David / dhildenb
On 04.12.23 17:35, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 04/12/2023 04:09, Suren Baghdasaryan wrote:
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote:
On 02.12.23 09:04, Ryan Roberts wrote:
On 01/12/2023 20:47, David Hildenbrand wrote:
On 01.12.23 10:29, Ryan Roberts wrote: > On 21/11/2023 17:16, Suren Baghdasaryan wrote: >> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >> into destination buffer while checking the contents of both after >> the move. After the operation the content of the destination buffer >> should match the original source buffer's content while the source >> buffer should be zeroed. Separate tests are designed for PMD aligned and >> unaligned cases because they utilize different code paths in the kernel. >> >> Signed-off-by: Suren Baghdasaryan surenb@google.com >> --- >> tools/testing/selftests/mm/uffd-common.c | 24 +++ >> tools/testing/selftests/mm/uffd-common.h | 1 + >> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >> 3 files changed, 214 insertions(+) >> >> diff --git a/tools/testing/selftests/mm/uffd-common.c >> b/tools/testing/selftests/mm/uffd-common.c >> index fb3bbc77fd00..b0ac0ec2356d 100644 >> --- a/tools/testing/selftests/mm/uffd-common.c >> +++ b/tools/testing/selftests/mm/uffd-common.c >> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >> return __copy_page(ufd, offset, false, wp); >> } >> +int move_page(int ufd, unsigned long offset, unsigned long len) >> +{ >> + struct uffdio_move uffdio_move; >> + >> + if (offset + len > nr_pages * page_size) >> + err("unexpected offset %lu and length %lu\n", offset, len); >> + uffdio_move.dst = (unsigned long) area_dst + offset; >> + uffdio_move.src = (unsigned long) area_src + offset; >> + uffdio_move.len = len; >> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >> + uffdio_move.move = 0; >> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >> + /* real retval in uffdio_move.move */ >> + if (uffdio_move.move != -EEXIST) >> + err("UFFDIO_MOVE error: %"PRId64, >> + (int64_t)uffdio_move.move); > > Hi Suren, > > FYI this error is triggering in mm-unstable (715b67adf4c8): > > Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, > @uffd-common.c:648) > > I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but > happy to go deeper if you can direct.
Does it trigger reliably? Which pagesize is that kernel using?
Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config.
I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses default_huge_page_size(), which reads the default hugetlb size.
My kernel command line is explicitly seting the default huge page size to 2M.
Okay, so that likely won't affect it.
I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64.
uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified?
So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest?
Not sure if that could trigger the THP splitting issue, though.
But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again?
Yep, all fixed with those patches!
Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect.
I only skimmed the diff briefly, but likely you also want to try splitting in move_pages_pte(), if you encounter an already-pte-mapped THP.
On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand david@redhat.com wrote:
On 04.12.23 17:35, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 04/12/2023 04:09, Suren Baghdasaryan wrote:
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote:
On 02.12.23 09:04, Ryan Roberts wrote:
On 01/12/2023 20:47, David Hildenbrand wrote: > On 01.12.23 10:29, Ryan Roberts wrote: >> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>> into destination buffer while checking the contents of both after >>> the move. After the operation the content of the destination buffer >>> should match the original source buffer's content while the source >>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>> unaligned cases because they utilize different code paths in the kernel. >>> >>> Signed-off-by: Suren Baghdasaryan surenb@google.com >>> --- >>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>> tools/testing/selftests/mm/uffd-common.h | 1 + >>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>> 3 files changed, 214 insertions(+) >>> >>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>> b/tools/testing/selftests/mm/uffd-common.c >>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>> --- a/tools/testing/selftests/mm/uffd-common.c >>> +++ b/tools/testing/selftests/mm/uffd-common.c >>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>> return __copy_page(ufd, offset, false, wp); >>> } >>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>> +{ >>> + struct uffdio_move uffdio_move; >>> + >>> + if (offset + len > nr_pages * page_size) >>> + err("unexpected offset %lu and length %lu\n", offset, len); >>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>> + uffdio_move.src = (unsigned long) area_src + offset; >>> + uffdio_move.len = len; >>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>> + uffdio_move.move = 0; >>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>> + /* real retval in uffdio_move.move */ >>> + if (uffdio_move.move != -EEXIST) >>> + err("UFFDIO_MOVE error: %"PRId64, >>> + (int64_t)uffdio_move.move); >> >> Hi Suren, >> >> FYI this error is triggering in mm-unstable (715b67adf4c8): >> >> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >> @uffd-common.c:648) >> >> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >> happy to go deeper if you can direct. > > Does it trigger reliably? Which pagesize is that kernel using?
Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email for full config.
> > I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses > default_huge_page_size(), which reads the default hugetlb size.
My kernel command line is explicitly seting the default huge page size to 2M.
Okay, so that likely won't affect it.
I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64.
uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified?
So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest?
Not sure if that could trigger the THP splitting issue, though.
But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again?
Yep, all fixed with those patches!
Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect.
I only skimmed the diff briefly, but likely you also want to try splitting in move_pages_pte(), if you encounter an already-pte-mapped THP.
Huh, good point. I might be able to move the folio splitting code into pte-mapped case and do a retry after splitting. That should minimize the additional code required. Will do and post a new set shortly. Thanks!
-- Cheers,
David / dhildenb
On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan surenb@google.com wrote:
On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand david@redhat.com wrote:
On 04.12.23 17:35, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 04/12/2023 04:09, Suren Baghdasaryan wrote:
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote:
On 02.12.23 09:04, Ryan Roberts wrote: > On 01/12/2023 20:47, David Hildenbrand wrote: >> On 01.12.23 10:29, Ryan Roberts wrote: >>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>> into destination buffer while checking the contents of both after >>>> the move. After the operation the content of the destination buffer >>>> should match the original source buffer's content while the source >>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>> unaligned cases because they utilize different code paths in the kernel. >>>> >>>> Signed-off-by: Suren Baghdasaryan surenb@google.com >>>> --- >>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>> 3 files changed, 214 insertions(+) >>>> >>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>> b/tools/testing/selftests/mm/uffd-common.c >>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>> return __copy_page(ufd, offset, false, wp); >>>> } >>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>> +{ >>>> + struct uffdio_move uffdio_move; >>>> + >>>> + if (offset + len > nr_pages * page_size) >>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>> + uffdio_move.len = len; >>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>> + uffdio_move.move = 0; >>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>> + /* real retval in uffdio_move.move */ >>>> + if (uffdio_move.move != -EEXIST) >>>> + err("UFFDIO_MOVE error: %"PRId64, >>>> + (int64_t)uffdio_move.move); >>> >>> Hi Suren, >>> >>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>> >>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>> @uffd-common.c:648) >>> >>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>> happy to go deeper if you can direct. >> >> Does it trigger reliably? Which pagesize is that kernel using? > > Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email > for full config. > >> >> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >> default_huge_page_size(), which reads the default hugetlb size. > > My kernel command line is explicitly seting the default huge page size to 2M. >
Okay, so that likely won't affect it.
I can only guess that it has to do with the alignment of the virtual area we are testing with, and that we do seem to get more odd patterns on arm64.
uffd_move_test_common() is a bit more elaborate, but if we aligned the src+start area up, surely "step_count" cannot be left unmodified?
So assuming we get either an unaligned source or an unaligned dst from mmap(), I am not convinced that we won't be moving areas that are not necessarily fully backed by PMDs and maybe don't even fall into the VMA of interest?
Not sure if that could trigger the THP splitting issue, though.
But I just quickly scanned that test setup, could be I am missing something. It might make sense to just print the mmap'ed range and the actual ranges we are trying to move. Maybe something "obvious" can be observed.
I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again?
Yep, all fixed with those patches!
Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect.
I only skimmed the diff briefly, but likely you also want to try splitting in move_pages_pte(), if you encounter an already-pte-mapped THP.
Huh, good point. I might be able to move the folio splitting code into pte-mapped case and do a retry after splitting. That should minimize the additional code required. Will do and post a new set shortly. Thanks!
Was planning to post an update today but need some more time. Will try to send it tomorrow.
-- Cheers,
David / dhildenb
On 05.12.23 05:46, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan surenb@google.com wrote:
On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand david@redhat.com wrote:
On 04.12.23 17:35, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 04/12/2023 04:09, Suren Baghdasaryan wrote:
On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote: > > On 02.12.23 09:04, Ryan Roberts wrote: >> On 01/12/2023 20:47, David Hildenbrand wrote: >>> On 01.12.23 10:29, Ryan Roberts wrote: >>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>>> into destination buffer while checking the contents of both after >>>>> the move. After the operation the content of the destination buffer >>>>> should match the original source buffer's content while the source >>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>>> unaligned cases because they utilize different code paths in the kernel. >>>>> >>>>> Signed-off-by: Suren Baghdasaryan surenb@google.com >>>>> --- >>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>>> 3 files changed, 214 insertions(+) >>>>> >>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>>> b/tools/testing/selftests/mm/uffd-common.c >>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>>> return __copy_page(ufd, offset, false, wp); >>>>> } >>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>>> +{ >>>>> + struct uffdio_move uffdio_move; >>>>> + >>>>> + if (offset + len > nr_pages * page_size) >>>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>>> + uffdio_move.len = len; >>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>>> + uffdio_move.move = 0; >>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>>> + /* real retval in uffdio_move.move */ >>>>> + if (uffdio_move.move != -EEXIST) >>>>> + err("UFFDIO_MOVE error: %"PRId64, >>>>> + (int64_t)uffdio_move.move); >>>> >>>> Hi Suren, >>>> >>>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>>> >>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>>> @uffd-common.c:648) >>>> >>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>>> happy to go deeper if you can direct. >>> >>> Does it trigger reliably? Which pagesize is that kernel using? >> >> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email >> for full config. >> >>> >>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >>> default_huge_page_size(), which reads the default hugetlb size. >> >> My kernel command line is explicitly seting the default huge page size to 2M. >> > > Okay, so that likely won't affect it. > > I can only guess that it has to do with the alignment of the virtual > area we are testing with, and that we do seem to get more odd patterns > on arm64. > > uffd_move_test_common() is a bit more elaborate, but if we aligned the > src+start area up, surely "step_count" cannot be left unmodified? > > So assuming we get either an unaligned source or an unaligned dst from > mmap(), I am not convinced that we won't be moving areas that are not > necessarily fully backed by PMDs and maybe don't even fall into the VMA > of interest? > > Not sure if that could trigger the THP splitting issue, though. > > But I just quickly scanned that test setup, could be I am missing > something. It might make sense to just print the mmap'ed range and the > actual ranges we are trying to move. Maybe something "obvious" can be > observed.
I was able to reproduce the issue on an Android device and after implementing David's suggestions to split the large folio and after replacing default_huge_page_size() with read_pmd_pagesize(), the move-pmd test started working for me. Ryan, could you please apply attached patches (over mm-unstable) and try the test again?
Yep, all fixed with those patches!
Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect.
I only skimmed the diff briefly, but likely you also want to try splitting in move_pages_pte(), if you encounter an already-pte-mapped THP.
Huh, good point. I might be able to move the folio splitting code into pte-mapped case and do a retry after splitting. That should minimize the additional code required. Will do and post a new set shortly. Thanks!
Was planning to post an update today but need some more time. Will try to send it tomorrow.
It would be great to have tests that cover these cases (having to PTE-map a PMD-mapped THP, and stumbling over an already-PTE-mapped one).
On Wed, Dec 6, 2023 at 1:21 AM David Hildenbrand david@redhat.com wrote:
On 05.12.23 05:46, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan surenb@google.com wrote:
On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand david@redhat.com wrote:
On 04.12.23 17:35, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts ryan.roberts@arm.com wrote:
On 04/12/2023 04:09, Suren Baghdasaryan wrote: > On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote: >> >> On 02.12.23 09:04, Ryan Roberts wrote: >>> On 01/12/2023 20:47, David Hildenbrand wrote: >>>> On 01.12.23 10:29, Ryan Roberts wrote: >>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>>>> into destination buffer while checking the contents of both after >>>>>> the move. After the operation the content of the destination buffer >>>>>> should match the original source buffer's content while the source >>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>>>> unaligned cases because they utilize different code paths in the kernel. >>>>>> >>>>>> Signed-off-by: Suren Baghdasaryan surenb@google.com >>>>>> --- >>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>>>> 3 files changed, 214 insertions(+) >>>>>> >>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>>>> b/tools/testing/selftests/mm/uffd-common.c >>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>>>> return __copy_page(ufd, offset, false, wp); >>>>>> } >>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>>>> +{ >>>>>> + struct uffdio_move uffdio_move; >>>>>> + >>>>>> + if (offset + len > nr_pages * page_size) >>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>>>> + uffdio_move.len = len; >>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>>>> + uffdio_move.move = 0; >>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>>>> + /* real retval in uffdio_move.move */ >>>>>> + if (uffdio_move.move != -EEXIST) >>>>>> + err("UFFDIO_MOVE error: %"PRId64, >>>>>> + (int64_t)uffdio_move.move); >>>>> >>>>> Hi Suren, >>>>> >>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>>>> >>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>>>> @uffd-common.c:648) >>>>> >>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>>>> happy to go deeper if you can direct. >>>> >>>> Does it trigger reliably? Which pagesize is that kernel using? >>> >>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email >>> for full config. >>> >>>> >>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >>>> default_huge_page_size(), which reads the default hugetlb size. >>> >>> My kernel command line is explicitly seting the default huge page size to 2M. >>> >> >> Okay, so that likely won't affect it. >> >> I can only guess that it has to do with the alignment of the virtual >> area we are testing with, and that we do seem to get more odd patterns >> on arm64. >> >> uffd_move_test_common() is a bit more elaborate, but if we aligned the >> src+start area up, surely "step_count" cannot be left unmodified? >> >> So assuming we get either an unaligned source or an unaligned dst from >> mmap(), I am not convinced that we won't be moving areas that are not >> necessarily fully backed by PMDs and maybe don't even fall into the VMA >> of interest? >> >> Not sure if that could trigger the THP splitting issue, though. >> >> But I just quickly scanned that test setup, could be I am missing >> something. It might make sense to just print the mmap'ed range and the >> actual ranges we are trying to move. Maybe something "obvious" can be >> observed. > > I was able to reproduce the issue on an Android device and after > implementing David's suggestions to split the large folio and after > replacing default_huge_page_size() with read_pmd_pagesize(), the > move-pmd test started working for me. Ryan, could you please apply > attached patches (over mm-unstable) and try the test again?
Yep, all fixed with those patches!
Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect.
I only skimmed the diff briefly, but likely you also want to try splitting in move_pages_pte(), if you encounter an already-pte-mapped THP.
Huh, good point. I might be able to move the folio splitting code into pte-mapped case and do a retry after splitting. That should minimize the additional code required. Will do and post a new set shortly. Thanks!
Was planning to post an update today but need some more time. Will try to send it tomorrow.
It would be great to have tests that cover these cases (having to PTE-map a PMD-mapped THP, and stumbling over an already-PTE-mapped one).
Agree. Let me post the new version so that mm-unstable does not produce these failures and will start working on covering additional cases in the tests. The new patchset is almost ready, just finishing final tests.
-- Cheers,
David / dhildenb
On Wed, Dec 6, 2023 at 2:30 AM Suren Baghdasaryan surenb@google.com wrote:
On Wed, Dec 6, 2023 at 1:21 AM David Hildenbrand david@redhat.com wrote:
On 05.12.23 05:46, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 10:44 AM Suren Baghdasaryan surenb@google.com wrote:
On Mon, Dec 4, 2023 at 10:27 AM David Hildenbrand david@redhat.com wrote:
On 04.12.23 17:35, Suren Baghdasaryan wrote:
On Mon, Dec 4, 2023 at 1:27 AM Ryan Roberts ryan.roberts@arm.com wrote: > > On 04/12/2023 04:09, Suren Baghdasaryan wrote: >> On Sat, Dec 2, 2023 at 2:11 AM David Hildenbrand david@redhat.com wrote: >>> >>> On 02.12.23 09:04, Ryan Roberts wrote: >>>> On 01/12/2023 20:47, David Hildenbrand wrote: >>>>> On 01.12.23 10:29, Ryan Roberts wrote: >>>>>> On 21/11/2023 17:16, Suren Baghdasaryan wrote: >>>>>>> Add tests for new UFFDIO_MOVE ioctl which uses uffd to move source >>>>>>> into destination buffer while checking the contents of both after >>>>>>> the move. After the operation the content of the destination buffer >>>>>>> should match the original source buffer's content while the source >>>>>>> buffer should be zeroed. Separate tests are designed for PMD aligned and >>>>>>> unaligned cases because they utilize different code paths in the kernel. >>>>>>> >>>>>>> Signed-off-by: Suren Baghdasaryan surenb@google.com >>>>>>> --- >>>>>>> tools/testing/selftests/mm/uffd-common.c | 24 +++ >>>>>>> tools/testing/selftests/mm/uffd-common.h | 1 + >>>>>>> tools/testing/selftests/mm/uffd-unit-tests.c | 189 +++++++++++++++++++ >>>>>>> 3 files changed, 214 insertions(+) >>>>>>> >>>>>>> diff --git a/tools/testing/selftests/mm/uffd-common.c >>>>>>> b/tools/testing/selftests/mm/uffd-common.c >>>>>>> index fb3bbc77fd00..b0ac0ec2356d 100644 >>>>>>> --- a/tools/testing/selftests/mm/uffd-common.c >>>>>>> +++ b/tools/testing/selftests/mm/uffd-common.c >>>>>>> @@ -631,6 +631,30 @@ int copy_page(int ufd, unsigned long offset, bool wp) >>>>>>> return __copy_page(ufd, offset, false, wp); >>>>>>> } >>>>>>> +int move_page(int ufd, unsigned long offset, unsigned long len) >>>>>>> +{ >>>>>>> + struct uffdio_move uffdio_move; >>>>>>> + >>>>>>> + if (offset + len > nr_pages * page_size) >>>>>>> + err("unexpected offset %lu and length %lu\n", offset, len); >>>>>>> + uffdio_move.dst = (unsigned long) area_dst + offset; >>>>>>> + uffdio_move.src = (unsigned long) area_src + offset; >>>>>>> + uffdio_move.len = len; >>>>>>> + uffdio_move.mode = UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES; >>>>>>> + uffdio_move.move = 0; >>>>>>> + if (ioctl(ufd, UFFDIO_MOVE, &uffdio_move)) { >>>>>>> + /* real retval in uffdio_move.move */ >>>>>>> + if (uffdio_move.move != -EEXIST) >>>>>>> + err("UFFDIO_MOVE error: %"PRId64, >>>>>>> + (int64_t)uffdio_move.move); >>>>>> >>>>>> Hi Suren, >>>>>> >>>>>> FYI this error is triggering in mm-unstable (715b67adf4c8): >>>>>> >>>>>> Testing move-pmd on anon... ERROR: UFFDIO_MOVE error: -16 (errno=16, >>>>>> @uffd-common.c:648) >>>>>> >>>>>> I'm running in a VM on Apple M2 (arm64). I haven't debugged any further, but >>>>>> happy to go deeper if you can direct. >>>>> >>>>> Does it trigger reliably? Which pagesize is that kernel using? >>>> >>>> Yep, although very occasionally it fails with EAGAIN. 4K kernel; see other email >>>> for full config. >>>> >>>>> >>>>> I can spot that uffd_move_pmd_test()/uffd_move_pmd_handle_fault() uses >>>>> default_huge_page_size(), which reads the default hugetlb size. >>>> >>>> My kernel command line is explicitly seting the default huge page size to 2M. >>>> >>> >>> Okay, so that likely won't affect it. >>> >>> I can only guess that it has to do with the alignment of the virtual >>> area we are testing with, and that we do seem to get more odd patterns >>> on arm64. >>> >>> uffd_move_test_common() is a bit more elaborate, but if we aligned the >>> src+start area up, surely "step_count" cannot be left unmodified? >>> >>> So assuming we get either an unaligned source or an unaligned dst from >>> mmap(), I am not convinced that we won't be moving areas that are not >>> necessarily fully backed by PMDs and maybe don't even fall into the VMA >>> of interest? >>> >>> Not sure if that could trigger the THP splitting issue, though. >>> >>> But I just quickly scanned that test setup, could be I am missing >>> something. It might make sense to just print the mmap'ed range and the >>> actual ranges we are trying to move. Maybe something "obvious" can be >>> observed. >> >> I was able to reproduce the issue on an Android device and after >> implementing David's suggestions to split the large folio and after >> replacing default_huge_page_size() with read_pmd_pagesize(), the >> move-pmd test started working for me. Ryan, could you please apply >> attached patches (over mm-unstable) and try the test again? > > Yep, all fixed with those patches!
Great! Thanks for testing and confirming. I'll post an updated patchset later today and will ask Andrew to replace the current one with it. I'll also look into the reasons we need to split PMD on ARM64 in this test. It's good that this happened and we were able to test the PMD split path but I'm curious about the reason. It's possible my address alignment calculations are somehow incorrect.
I only skimmed the diff briefly, but likely you also want to try splitting in move_pages_pte(), if you encounter an already-pte-mapped THP.
Huh, good point. I might be able to move the folio splitting code into pte-mapped case and do a retry after splitting. That should minimize the additional code required. Will do and post a new set shortly. Thanks!
Was planning to post an update today but need some more time. Will try to send it tomorrow.
It would be great to have tests that cover these cases (having to PTE-map a PMD-mapped THP, and stumbling over an already-PTE-mapped one).
Agree. Let me post the new version so that mm-unstable does not produce these failures and will start working on covering additional cases in the tests. The new patchset is almost ready, just finishing final tests.
Posted v6 at https://lore.kernel.org/all/20231206103702.3873743-1-surenb@google.com/. Changes are listed in the cover letter.
Andrew, could you please replace the current v5 version in mm-unstable with this new one? Thanks, Suren.
-- Cheers,
David / dhildenb
linux-kselftest-mirror@lists.linaro.org
-
David Hildenbrand -
Ryan Roberts -
Suren Baghdasaryan