*Changes in v7:* - Add uffd wp async - Update the IOCTL to use uffd under the hood instead of soft-dirty flags
Stop using the soft-dirty flags for finding which pages have been written to. It is too delicate and wrong as it shows more soft-dirty pages than the actual soft-dirty pages. There is no interest in correcting it [A][B] as this is how the feature was written years ago. It shouldn't be updated to changed behaviour. Peter Xu has suggested using the async version of the UFFD WP [C] as it is based inherently on the PTEs.
So in this patch series, I've added a new mode to the UFFD which is asynchronous version of the write protect. When this variant of the UFFD WP is used, the page faults are resolved automatically by the kernel. The pages which have been written-to can be found by reading pagemap file (!PM_UFFD_WP). This feature can be used successfully to find which pages have been written to from the time the pages were write protected. This works just like the soft-dirty flag without showing any extra pages which aren't soft-dirty in reality.
[A] https://lore.kernel.org/all/20221220162606.1595355-1-usama.anjum@collabora.c... [B] https://lore.kernel.org/all/20221122115007.2787017-1-usama.anjum@collabora.c... [C] https://lore.kernel.org/all/Y6Hc2d+7eTKs7AiH@x1n
*Changes in v6:* - Updated the interface and made cosmetic changes
*Cover Letter in v5:* Hello,
This patch series implements IOCTL on the pagemap procfs file to get the information about the page table entries (PTEs). The following operations are supported in this ioctl: - Get the information if the pages are soft-dirty, file mapped, present or swapped. - Clear the soft-dirty PTE bit of the pages. - Get and clear the soft-dirty PTE bit of the pages atomically.
Soft-dirty PTE bit of the memory pages can be read by using the pagemap procfs file. The soft-dirty PTE bit for the whole memory range of the process can be cleared by writing to the clear_refs file. There are other methods to mimic this information entirely in userspace with poor performance: - The mprotect syscall and SIGSEGV handler for bookkeeping - The userfaultfd syscall with the handler for bookkeeping Some benchmarks can be seen here[1]. This series adds features that weren't present earlier: - There is no atomic get soft-dirty PTE bit status and clear operation possible. - The soft-dirty PTE bit of only a part of memory cannot be cleared.
Historically, soft-dirty PTE bit tracking has been used in the CRIU project. The procfs interface is enough for finding the soft-dirty bit status and clearing the soft-dirty bit of all the pages of a process. We have the use case where we need to track the soft-dirty PTE bit for only specific pages on demand. We need this tracking and clear mechanism of a region of memory while the process is running to emulate the getWriteWatch() syscall of Windows. This syscall is used by games to keep track of dirty pages to process only the dirty pages.
The information related to pages if the page is file mapped, present and swapped is required for the CRIU project[2][3]. The addition of the required mask, any mask, excluded mask and return masks are also required for the CRIU project[2].
The IOCTL returns the addresses of the pages which match the specific masks. The page addresses are returned in struct page_region in a compact form. The max_pages is needed to support a use case where user only wants to get a specific number of pages. So there is no need to find all the pages of interest in the range when max_pages is specified. The IOCTL returns when the maximum number of the pages are found. The max_pages is optional. If max_pages is specified, it must be equal or greater than the vec_size. This restriction is needed to handle worse case when one page_region only contains info of one page and it cannot be compacted. This is needed to emulate the Windows getWriteWatch() syscall.
Some non-dirty pages get marked as dirty because of the kernel's internal activity (such as VMA merging as soft-dirty bit difference isn't considered while deciding to merge VMAs). The dirty bit of the pages is stored in the VMA flags and in the per page flags. If any of these two bits are set, the page is considered to be soft dirty. Suppose you have cleared the soft dirty bit of half of VMA which will be done by splitting the VMA and clearing soft dirty bit flag in the half VMA and the pages in it. Now kernel may decide to merge the VMAs again. So the half VMA becomes dirty again. This splitting/merging costs performance. The application receives a lot of pages which aren't dirty in reality but marked as dirty. Performance is lost again here. Also sometimes user doesn't want the newly allocated memory to be marked as dirty. PAGEMAP_NO_REUSED_REGIONS flag solves both the problems. It is used to not depend on the soft dirty flag in the VMA flags. So VMA splitting and merging doesn't happen. It only depends on the soft dirty bit of the individual pages. Thus by using this flag, there may be a scenerio such that the new memory regions which are just created, doesn't look dirty when seen with the IOCTL, but look dirty when seen from procfs. This seems okay as the user of this flag know the implication of using it.
[1] https://lore.kernel.org/lkml/54d4c322-cd6e-eefd-b161-2af2b56aae24@collabora.... [2] https://lore.kernel.org/all/YyiDg79flhWoMDZB@gmail.com/ [3] https://lore.kernel.org/all/20221014134802.1361436-1-mdanylo@google.com/
Regards, Muhammad Usama Anjum
Muhammad Usama Anjum (4): userfaultfd: Add UFFD WP Async support userfaultfd: split mwriteprotect_range() fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs selftests: vm: add pagemap ioctl tests
fs/proc/task_mmu.c | 300 +++++++ fs/userfaultfd.c | 161 ++-- include/linux/userfaultfd_k.h | 10 + include/uapi/linux/fs.h | 50 ++ include/uapi/linux/userfaultfd.h | 6 + mm/userfaultfd.c | 40 +- tools/include/uapi/linux/fs.h | 50 ++ tools/testing/selftests/vm/.gitignore | 1 + tools/testing/selftests/vm/Makefile | 5 +- tools/testing/selftests/vm/pagemap_ioctl.c | 884 +++++++++++++++++++++ 10 files changed, 1424 insertions(+), 83 deletions(-) create mode 100644 tools/testing/selftests/vm/pagemap_ioctl.c
Add new WP Async mode (UFFDIO_WRITEPROTECT_MODE_ASYNC_WP) which resolves the page faults on its own. It can be used to track that which pages have been written to from the time the pages were write protected. It is very efficient way to track the changes as uffd is by nature pte/pmd based.
UFFD WP (UFFDIO_WRITEPROTECT_MODE_WP) sends the page faults to the userspace where the pages which have been written-to can be tracked. But it is not efficient. This is why this async version is being added. After setting the WP Async, the pages which have been written to can be found in the pagemap file or information can be obtained from the PAGEMAP_IOCTL (see next patches).
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- fs/userfaultfd.c | 150 +++++++++++++++++-------------- include/uapi/linux/userfaultfd.h | 6 ++ 2 files changed, 90 insertions(+), 66 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 15a5bf765d43..be5e10d15058 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -69,6 +69,7 @@ struct userfaultfd_ctx { unsigned int features; /* released */ bool released; + bool async; /* memory mappings are changing because of non-cooperative event */ atomic_t mmap_changing; /* mm with one ore more vmas attached to this userfaultfd_ctx */ @@ -497,80 +498,93 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
/* take the reference before dropping the mmap_lock */ userfaultfd_ctx_get(ctx); + if (ctx->async) { + // Resolve page fault of this page + unsigned long addr = (ctx->features & UFFD_FEATURE_EXACT_ADDRESS) ? + vmf->real_address : vmf->address; + struct vm_area_struct *dst_vma = find_vma(ctx->mm, addr); + size_t s = PAGE_SIZE; + + if (dst_vma->vm_flags & VM_HUGEPAGE) { + s = HPAGE_SIZE; + addr &= HPAGE_MASK; + }
- init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function); - uwq.wq.private = current; - uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags, - reason, ctx->features); - uwq.ctx = ctx; - uwq.waken = false; - - blocking_state = userfaultfd_get_blocking_state(vmf->flags); + ret = mwriteprotect_range(ctx->mm, addr, s, false, &ctx->mmap_changing); + } else { + init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function); + uwq.wq.private = current; + uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags, + reason, ctx->features); + uwq.ctx = ctx; + uwq.waken = false;
- /* - * Take the vma lock now, in order to safely call - * userfaultfd_huge_must_wait() later. Since acquiring the - * (sleepable) vma lock can modify the current task state, that - * must be before explicitly calling set_current_state(). - */ - if (is_vm_hugetlb_page(vma)) - hugetlb_vma_lock_read(vma); + blocking_state = userfaultfd_get_blocking_state(vmf->flags);
- spin_lock_irq(&ctx->fault_pending_wqh.lock); - /* - * After the __add_wait_queue the uwq is visible to userland - * through poll/read(). - */ - __add_wait_queue(&ctx->fault_pending_wqh, &uwq.wq); - /* - * The smp_mb() after __set_current_state prevents the reads - * following the spin_unlock to happen before the list_add in - * __add_wait_queue. - */ - set_current_state(blocking_state); - spin_unlock_irq(&ctx->fault_pending_wqh.lock); + /* + * Take the vma lock now, in order to safely call + * userfaultfd_huge_must_wait() later. Since acquiring the + * (sleepable) vma lock can modify the current task state, that + * must be before explicitly calling set_current_state(). + */ + if (is_vm_hugetlb_page(vma)) + hugetlb_vma_lock_read(vma);
- if (!is_vm_hugetlb_page(vma)) - must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags, - reason); - else - must_wait = userfaultfd_huge_must_wait(ctx, vma, - vmf->address, - vmf->flags, reason); - if (is_vm_hugetlb_page(vma)) - hugetlb_vma_unlock_read(vma); - mmap_read_unlock(mm); + spin_lock_irq(&ctx->fault_pending_wqh.lock); + /* + * After the __add_wait_queue the uwq is visible to userland + * through poll/read(). + */ + __add_wait_queue(&ctx->fault_pending_wqh, &uwq.wq); + /* + * The smp_mb() after __set_current_state prevents the reads + * following the spin_unlock to happen before the list_add in + * __add_wait_queue. + */ + set_current_state(blocking_state); + spin_unlock_irq(&ctx->fault_pending_wqh.lock);
- if (likely(must_wait && !READ_ONCE(ctx->released))) { - wake_up_poll(&ctx->fd_wqh, EPOLLIN); - schedule(); - } + if (!is_vm_hugetlb_page(vma)) + must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags, + reason); + else + must_wait = userfaultfd_huge_must_wait(ctx, vma, + vmf->address, + vmf->flags, reason); + if (is_vm_hugetlb_page(vma)) + hugetlb_vma_unlock_read(vma); + mmap_read_unlock(mm); + + if (likely(must_wait && !READ_ONCE(ctx->released))) { + wake_up_poll(&ctx->fd_wqh, EPOLLIN); + schedule(); + }
- __set_current_state(TASK_RUNNING); + __set_current_state(TASK_RUNNING);
- /* - * Here we race with the list_del; list_add in - * userfaultfd_ctx_read(), however because we don't ever run - * list_del_init() to refile across the two lists, the prev - * and next pointers will never point to self. list_add also - * would never let any of the two pointers to point to - * self. So list_empty_careful won't risk to see both pointers - * pointing to self at any time during the list refile. The - * only case where list_del_init() is called is the full - * removal in the wake function and there we don't re-list_add - * and it's fine not to block on the spinlock. The uwq on this - * kernel stack can be released after the list_del_init. - */ - if (!list_empty_careful(&uwq.wq.entry)) { - spin_lock_irq(&ctx->fault_pending_wqh.lock); /* - * No need of list_del_init(), the uwq on the stack - * will be freed shortly anyway. + * Here we race with the list_del; list_add in + * userfaultfd_ctx_read(), however because we don't ever run + * list_del_init() to refile across the two lists, the prev + * and next pointers will never point to self. list_add also + * would never let any of the two pointers to point to + * self. So list_empty_careful won't risk to see both pointers + * pointing to self at any time during the list refile. The + * only case where list_del_init() is called is the full + * removal in the wake function and there we don't re-list_add + * and it's fine not to block on the spinlock. The uwq on this + * kernel stack can be released after the list_del_init. */ - list_del(&uwq.wq.entry); - spin_unlock_irq(&ctx->fault_pending_wqh.lock); + if (!list_empty_careful(&uwq.wq.entry)) { + spin_lock_irq(&ctx->fault_pending_wqh.lock); + /* + * No need of list_del_init(), the uwq on the stack + * will be freed shortly anyway. + */ + list_del(&uwq.wq.entry); + spin_unlock_irq(&ctx->fault_pending_wqh.lock); + } } - /* * ctx may go away after this if the userfault pseudo fd is * already released. @@ -1861,11 +1875,14 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx, return ret;
if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE | - UFFDIO_WRITEPROTECT_MODE_WP)) + UFFDIO_WRITEPROTECT_MODE_WP | + UFFDIO_WRITEPROTECT_MODE_ASYNC_WP)) return -EINVAL;
- mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP; + mode_wp = uffdio_wp.mode & (UFFDIO_WRITEPROTECT_MODE_WP | + UFFDIO_WRITEPROTECT_MODE_ASYNC_WP); mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE; + ctx->async = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_ASYNC_WP;
if (mode_wp && mode_dontwake) return -EINVAL; @@ -2126,6 +2143,7 @@ static int new_userfaultfd(int flags) ctx->flags = flags; ctx->features = 0; ctx->released = false; + ctx->async = false; atomic_set(&ctx->mmap_changing, 0); ctx->mm = current->mm; /* prevent the mm struct to be freed */ diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 005e5e306266..b89665653861 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -284,6 +284,11 @@ struct uffdio_writeprotect { * UFFDIO_WRITEPROTECT_MODE_DONTWAKE: set the flag to avoid waking up * any wait thread after the operation succeeds. * + * UFFDIO_WRITEPROTECT_MODE_ASYNC_WP: set the flag to write protect a + * range, the flag is unset automatically when the page is written. + * This is used to track which pages have been written to from the + * time the memory was write protected. + * * NOTE: Write protecting a region (WP=1) is unrelated to page faults, * therefore DONTWAKE flag is meaningless with WP=1. Removing write * protection (WP=0) in response to a page fault wakes the faulting @@ -291,6 +296,7 @@ struct uffdio_writeprotect { */ #define UFFDIO_WRITEPROTECT_MODE_WP ((__u64)1<<0) #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE ((__u64)1<<1) +#define UFFDIO_WRITEPROTECT_MODE_ASYNC_WP ((__u64)1<<2) __u64 mode; };
Hi, Muhammad,
On Mon, Jan 09, 2023 at 11:45:16AM +0500, Muhammad Usama Anjum wrote:
Add new WP Async mode (UFFDIO_WRITEPROTECT_MODE_ASYNC_WP) which resolves the page faults on its own. It can be used to track that which pages have been written to from the time the pages were write protected. It is very efficient way to track the changes as uffd is by nature pte/pmd based.
UFFD WP (UFFDIO_WRITEPROTECT_MODE_WP) sends the page faults to the userspace where the pages which have been written-to can be tracked. But it is not efficient. This is why this async version is being added. After setting the WP Async, the pages which have been written to can be found in the pagemap file or information can be obtained from the PAGEMAP_IOCTL (see next patches).
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
fs/userfaultfd.c | 150 +++++++++++++++++-------------- include/uapi/linux/userfaultfd.h | 6 ++ 2 files changed, 90 insertions(+), 66 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 15a5bf765d43..be5e10d15058 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -69,6 +69,7 @@ struct userfaultfd_ctx { unsigned int features; /* released */ bool released;
- bool async;
Let's just make it a feature flag,
UFFD_FEATURE_WP_ASYNC
/* memory mappings are changing because of non-cooperative event */ atomic_t mmap_changing; /* mm with one ore more vmas attached to this userfaultfd_ctx */ @@ -497,80 +498,93 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) /* take the reference before dropping the mmap_lock */ userfaultfd_ctx_get(ctx);
- if (ctx->async) {
Firstly, please consider not touching the existing code/indent as much as what this patch did. Hopefully we can keep the major part of sync uffd be there with its git log, it also helps reviewing your code. You can add the async block before that, handle the fault and return just earlier.
And, I think this is a bit too late because we're going to return with VM_FAULT_RETRY here, while maybe we don't need to retry at all here because we're going to resolve the page fault immediately.
I assume you added this because you wanted userfaultfd_ctx_get() to make sure the uffd context will not go away from under us, but it's not needed if we're still holding the mmap read lock. I'd expect for async mode we don't really need to release it at all.
// Resolve page fault of this page
Please use "/* ... */" as that's the common pattern of commenting in the Linux kernel, at least what I see in mm/.
unsigned long addr = (ctx->features & UFFD_FEATURE_EXACT_ADDRESS) ?vmf->real_address : vmf->address;struct vm_area_struct *dst_vma = find_vma(ctx->mm, addr);size_t s = PAGE_SIZE;
This is weird - if we want async uffd-wp, let's consider huge page from the 1st day.
if (dst_vma->vm_flags & VM_HUGEPAGE) {
VM_HUGEPAGE is only a hint. It doesn't mean this page is always a huge page. For anon, we can have thp wr-protected as a whole, not happening for !anon because we'll split already.
For anon, if a write happens to a thp being uffd-wp-ed, we'll keep that pmd wr-protected and report the uffd message. The pmd split happens when the user invokes UFFDIO_WRITEPROTECT on the small page. I think it'll stop working for async uffd-wp because we're going to resolve the page faults right away.
So for async uffd-wp (note: this will be different from hugetlb), you may want to consider having a pre-requisite patch to change wp_huge_pmd() behavior: rather than calling handle_userfault(), IIUC you can also just fallback to the split path right below (__split_huge_pmd) so the thp will split now even before the uffd message is generated.
I think it should be transparent to the user and it'll start working for you with async uffd-wp here, because it means when reaching handle_userfault, it should not be possible to have thp at all since they should have all split up.
s = HPAGE_SIZE;addr &= HPAGE_MASK;}
- init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function);
- uwq.wq.private = current;
- uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags,
reason, ctx->features);- uwq.ctx = ctx;
- uwq.waken = false;
- blocking_state = userfaultfd_get_blocking_state(vmf->flags);
ret = mwriteprotect_range(ctx->mm, addr, s, false, &ctx->mmap_changing);
This is an overkill - we're pretty sure it's a single page, no need to call a range function here.
- } else {
init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function);uwq.wq.private = current;uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags,reason, ctx->features);uwq.ctx = ctx;uwq.waken = false;
/** Take the vma lock now, in order to safely call* userfaultfd_huge_must_wait() later. Since acquiring the* (sleepable) vma lock can modify the current task state, that* must be before explicitly calling set_current_state().*/- if (is_vm_hugetlb_page(vma))
hugetlb_vma_lock_read(vma);
blocking_state = userfaultfd_get_blocking_state(vmf->flags);
- spin_lock_irq(&ctx->fault_pending_wqh.lock);
- /*
* After the __add_wait_queue the uwq is visible to userland* through poll/read().*/- __add_wait_queue(&ctx->fault_pending_wqh, &uwq.wq);
- /*
* The smp_mb() after __set_current_state prevents the reads* following the spin_unlock to happen before the list_add in* __add_wait_queue.*/- set_current_state(blocking_state);
- spin_unlock_irq(&ctx->fault_pending_wqh.lock);
/** Take the vma lock now, in order to safely call* userfaultfd_huge_must_wait() later. Since acquiring the* (sleepable) vma lock can modify the current task state, that* must be before explicitly calling set_current_state().*/if (is_vm_hugetlb_page(vma))hugetlb_vma_lock_read(vma);
- if (!is_vm_hugetlb_page(vma))
must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags,reason);- else
must_wait = userfaultfd_huge_must_wait(ctx, vma,vmf->address,vmf->flags, reason);- if (is_vm_hugetlb_page(vma))
hugetlb_vma_unlock_read(vma);- mmap_read_unlock(mm);
spin_lock_irq(&ctx->fault_pending_wqh.lock);/** After the __add_wait_queue the uwq is visible to userland* through poll/read().*/__add_wait_queue(&ctx->fault_pending_wqh, &uwq.wq);/** The smp_mb() after __set_current_state prevents the reads* following the spin_unlock to happen before the list_add in* __add_wait_queue.*/set_current_state(blocking_state);spin_unlock_irq(&ctx->fault_pending_wqh.lock);
- if (likely(must_wait && !READ_ONCE(ctx->released))) {
wake_up_poll(&ctx->fd_wqh, EPOLLIN);schedule();- }
if (!is_vm_hugetlb_page(vma))must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags,reason);elsemust_wait = userfaultfd_huge_must_wait(ctx, vma,vmf->address,vmf->flags, reason);if (is_vm_hugetlb_page(vma))hugetlb_vma_unlock_read(vma);mmap_read_unlock(mm);if (likely(must_wait && !READ_ONCE(ctx->released))) {wake_up_poll(&ctx->fd_wqh, EPOLLIN);schedule();}
- __set_current_state(TASK_RUNNING);
__set_current_state(TASK_RUNNING);
- /*
* Here we race with the list_del; list_add in* userfaultfd_ctx_read(), however because we don't ever run* list_del_init() to refile across the two lists, the prev* and next pointers will never point to self. list_add also* would never let any of the two pointers to point to* self. So list_empty_careful won't risk to see both pointers* pointing to self at any time during the list refile. The* only case where list_del_init() is called is the full* removal in the wake function and there we don't re-list_add* and it's fine not to block on the spinlock. The uwq on this* kernel stack can be released after the list_del_init.*/- if (!list_empty_careful(&uwq.wq.entry)) {
spin_lock_irq(&ctx->fault_pending_wqh.lock);* No need of list_del_init(), the uwq on the stack* will be freed shortly anyway.
* Here we race with the list_del; list_add in* userfaultfd_ctx_read(), however because we don't ever run* list_del_init() to refile across the two lists, the prev* and next pointers will never point to self. list_add also* would never let any of the two pointers to point to* self. So list_empty_careful won't risk to see both pointers* pointing to self at any time during the list refile. The* only case where list_del_init() is called is the full* removal in the wake function and there we don't re-list_add* and it's fine not to block on the spinlock. The uwq on this* kernel stack can be released after the list_del_init.
list_del(&uwq.wq.entry);spin_unlock_irq(&ctx->fault_pending_wqh.lock);
if (!list_empty_careful(&uwq.wq.entry)) {spin_lock_irq(&ctx->fault_pending_wqh.lock);/** No need of list_del_init(), the uwq on the stack* will be freed shortly anyway.*/list_del(&uwq.wq.entry);spin_unlock_irq(&ctx->fault_pending_wqh.lock);}
- /*
- ctx may go away after this if the userfault pseudo fd is
- already released.
@@ -1861,11 +1875,14 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx, return ret; if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
UFFDIO_WRITEPROTECT_MODE_WP))
UFFDIO_WRITEPROTECT_MODE_WP |UFFDIO_WRITEPROTECT_MODE_ASYNC_WP))
- mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
- mode_wp = uffdio_wp.mode & (UFFDIO_WRITEPROTECT_MODE_WP |
UFFDIO_WRITEPROTECT_MODE_ASYNC_WP);- ctx->async = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_ASYNC_WP;
Please no.. ctx attributes shouldn't be easily changed by a single ioctl.
I suggest we have a new feature bit as I mentioned above (say, UFFD_FEATURE_WP_ASYNC), set it once with UFFDIO_API and it should apply to the whole lifecycle of this uffd handle. That flag should (something I can quickly think of):
- Have effect only if the uffd will be registered with WP mode (of course) or ignored in any other modes,
- Should fail any attempts of UFFDIO_WRITEPROTECT with wp=false on this uffd handle because with async faults no page fault resolution needed from userspace,
- Should apply to any region registered with this uffd ctx, so it's exclusively used with sync uffd-wp mode.
Then when the app wants to wr-protect in async mode, it simply goes ahead with UFFDIO_WRITEPROTECT(wp=true), it'll happen exactly the same as when it was sync mode. It's only the pf handling procedure that's different (along with how the fault is reported - rather than as a message but it'll be consolidated into the soft-dirty bit).
if (mode_wp && mode_dontwake) return -EINVAL; @@ -2126,6 +2143,7 @@ static int new_userfaultfd(int flags) ctx->flags = flags; ctx->features = 0; ctx->released = false;
- ctx->async = false; atomic_set(&ctx->mmap_changing, 0); ctx->mm = current->mm; /* prevent the mm struct to be freed */
diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 005e5e306266..b89665653861 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -284,6 +284,11 @@ struct uffdio_writeprotect {
- UFFDIO_WRITEPROTECT_MODE_DONTWAKE: set the flag to avoid waking up
- any wait thread after the operation succeeds.
- UFFDIO_WRITEPROTECT_MODE_ASYNC_WP: set the flag to write protect a
- range, the flag is unset automatically when the page is written.
- This is used to track which pages have been written to from the
- time the memory was write protected.
- NOTE: Write protecting a region (WP=1) is unrelated to page faults,
- therefore DONTWAKE flag is meaningless with WP=1. Removing write
- protection (WP=0) in response to a page fault wakes the faulting
@@ -291,6 +296,7 @@ struct uffdio_writeprotect { */ #define UFFDIO_WRITEPROTECT_MODE_WP ((__u64)1<<0) #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE ((__u64)1<<1) +#define UFFDIO_WRITEPROTECT_MODE_ASYNC_WP ((__u64)1<<2) __u64 mode; }; -- 2.30.2
Hi Peter,
Thank you so much for reviewing.
On 1/18/23 9:54 PM, Peter Xu wrote:
Hi, Muhammad,
On Mon, Jan 09, 2023 at 11:45:16AM +0500, Muhammad Usama Anjum wrote:
Add new WP Async mode (UFFDIO_WRITEPROTECT_MODE_ASYNC_WP) which resolves the page faults on its own. It can be used to track that which pages have been written to from the time the pages were write protected. It is very efficient way to track the changes as uffd is by nature pte/pmd based.
UFFD WP (UFFDIO_WRITEPROTECT_MODE_WP) sends the page faults to the userspace where the pages which have been written-to can be tracked. But it is not efficient. This is why this async version is being added. After setting the WP Async, the pages which have been written to can be found in the pagemap file or information can be obtained from the PAGEMAP_IOCTL (see next patches).
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
fs/userfaultfd.c | 150 +++++++++++++++++-------------- include/uapi/linux/userfaultfd.h | 6 ++ 2 files changed, 90 insertions(+), 66 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 15a5bf765d43..be5e10d15058 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -69,6 +69,7 @@ struct userfaultfd_ctx { unsigned int features; /* released */ bool released;
- bool async;
Let's just make it a feature flag,
UFFD_FEATURE_WP_ASYNC
This would really make things easier. Thank you so much for suggesting it.
/* memory mappings are changing because of non-cooperative event */ atomic_t mmap_changing; /* mm with one ore more vmas attached to this userfaultfd_ctx */ @@ -497,80 +498,93 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) /* take the reference before dropping the mmap_lock */ userfaultfd_ctx_get(ctx);
- if (ctx->async) {
Firstly, please consider not touching the existing code/indent as much as what this patch did. Hopefully we can keep the major part of sync uffd be there with its git log, it also helps reviewing your code. You can add the async block before that, handle the fault and return just earlier.
This is possible. Will do in next revision.
And, I think this is a bit too late because we're going to return with VM_FAULT_RETRY here, while maybe we don't need to retry at all here because we're going to resolve the page fault immediately.
I assume you added this because you wanted userfaultfd_ctx_get() to make sure the uffd context will not go away from under us, but it's not needed if we're still holding the mmap read lock. I'd expect for async mode we don't really need to release it at all.
I'll have to check the what should be returned here. We should return something which shows that the fault has been resolved.
// Resolve page fault of this pagePlease use "/* ... */" as that's the common pattern of commenting in the Linux kernel, at least what I see in mm/.
Will do.
unsigned long addr = (ctx->features & UFFD_FEATURE_EXACT_ADDRESS) ?vmf->real_address : vmf->address;struct vm_area_struct *dst_vma = find_vma(ctx->mm, addr);size_t s = PAGE_SIZE;This is weird - if we want async uffd-wp, let's consider huge page from the 1st day.
if (dst_vma->vm_flags & VM_HUGEPAGE) {VM_HUGEPAGE is only a hint. It doesn't mean this page is always a huge page. For anon, we can have thp wr-protected as a whole, not happening for !anon because we'll split already.
For anon, if a write happens to a thp being uffd-wp-ed, we'll keep that pmd wr-protected and report the uffd message. The pmd split happens when the user invokes UFFDIO_WRITEPROTECT on the small page. I think it'll stop working for async uffd-wp because we're going to resolve the page faults right away.
So for async uffd-wp (note: this will be different from hugetlb), you may want to consider having a pre-requisite patch to change wp_huge_pmd() behavior: rather than calling handle_userfault(), IIUC you can also just fallback to the split path right below (__split_huge_pmd) so the thp will split now even before the uffd message is generated.
I'll make the changes and make this. I wasn't aware that the thp is being broken in the UFFD WP. At this time, I'm not sure if thp will be handled by handle_userfault() in one go. Probably it will as the length is stored in the vmf.
I think it should be transparent to the user and it'll start working for you with async uffd-wp here, because it means when reaching handle_userfault, it should not be possible to have thp at all since they should have all split up.
s = HPAGE_SIZE;addr &= HPAGE_MASK;}
- init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function);
- uwq.wq.private = current;
- uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags,
reason, ctx->features);- uwq.ctx = ctx;
- uwq.waken = false;
- blocking_state = userfaultfd_get_blocking_state(vmf->flags);
ret = mwriteprotect_range(ctx->mm, addr, s, false, &ctx->mmap_changing);This is an overkill - we're pretty sure it's a single page, no need to call a range function here.
Probably change_pte_range() should be used here to directly remove the WP here?
- } else {
init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function);uwq.wq.private = current;uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags,reason, ctx->features);uwq.ctx = ctx;uwq.waken = false;
/** Take the vma lock now, in order to safely call* userfaultfd_huge_must_wait() later. Since acquiring the* (sleepable) vma lock can modify the current task state, that* must be before explicitly calling set_current_state().*/- if (is_vm_hugetlb_page(vma))
hugetlb_vma_lock_read(vma);
blocking_state = userfaultfd_get_blocking_state(vmf->flags);
- spin_lock_irq(&ctx->fault_pending_wqh.lock);
- /*
* After the __add_wait_queue the uwq is visible to userland* through poll/read().*/- __add_wait_queue(&ctx->fault_pending_wqh, &uwq.wq);
- /*
* The smp_mb() after __set_current_state prevents the reads* following the spin_unlock to happen before the list_add in* __add_wait_queue.*/- set_current_state(blocking_state);
- spin_unlock_irq(&ctx->fault_pending_wqh.lock);
/** Take the vma lock now, in order to safely call* userfaultfd_huge_must_wait() later. Since acquiring the* (sleepable) vma lock can modify the current task state, that* must be before explicitly calling set_current_state().*/if (is_vm_hugetlb_page(vma))hugetlb_vma_lock_read(vma);
- if (!is_vm_hugetlb_page(vma))
must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags,reason);- else
must_wait = userfaultfd_huge_must_wait(ctx, vma,vmf->address,vmf->flags, reason);- if (is_vm_hugetlb_page(vma))
hugetlb_vma_unlock_read(vma);- mmap_read_unlock(mm);
spin_lock_irq(&ctx->fault_pending_wqh.lock);/** After the __add_wait_queue the uwq is visible to userland* through poll/read().*/__add_wait_queue(&ctx->fault_pending_wqh, &uwq.wq);/** The smp_mb() after __set_current_state prevents the reads* following the spin_unlock to happen before the list_add in* __add_wait_queue.*/set_current_state(blocking_state);spin_unlock_irq(&ctx->fault_pending_wqh.lock);
- if (likely(must_wait && !READ_ONCE(ctx->released))) {
wake_up_poll(&ctx->fd_wqh, EPOLLIN);schedule();- }
if (!is_vm_hugetlb_page(vma))must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags,reason);elsemust_wait = userfaultfd_huge_must_wait(ctx, vma,vmf->address,vmf->flags, reason);if (is_vm_hugetlb_page(vma))hugetlb_vma_unlock_read(vma);mmap_read_unlock(mm);if (likely(must_wait && !READ_ONCE(ctx->released))) {wake_up_poll(&ctx->fd_wqh, EPOLLIN);schedule();}
- __set_current_state(TASK_RUNNING);
__set_current_state(TASK_RUNNING);
- /*
* Here we race with the list_del; list_add in* userfaultfd_ctx_read(), however because we don't ever run* list_del_init() to refile across the two lists, the prev* and next pointers will never point to self. list_add also* would never let any of the two pointers to point to* self. So list_empty_careful won't risk to see both pointers* pointing to self at any time during the list refile. The* only case where list_del_init() is called is the full* removal in the wake function and there we don't re-list_add* and it's fine not to block on the spinlock. The uwq on this* kernel stack can be released after the list_del_init.*/- if (!list_empty_careful(&uwq.wq.entry)) {
spin_lock_irq(&ctx->fault_pending_wqh.lock);* No need of list_del_init(), the uwq on the stack* will be freed shortly anyway.
* Here we race with the list_del; list_add in* userfaultfd_ctx_read(), however because we don't ever run* list_del_init() to refile across the two lists, the prev* and next pointers will never point to self. list_add also* would never let any of the two pointers to point to* self. So list_empty_careful won't risk to see both pointers* pointing to self at any time during the list refile. The* only case where list_del_init() is called is the full* removal in the wake function and there we don't re-list_add* and it's fine not to block on the spinlock. The uwq on this* kernel stack can be released after the list_del_init.
list_del(&uwq.wq.entry);spin_unlock_irq(&ctx->fault_pending_wqh.lock);
if (!list_empty_careful(&uwq.wq.entry)) {spin_lock_irq(&ctx->fault_pending_wqh.lock);/** No need of list_del_init(), the uwq on the stack* will be freed shortly anyway.*/list_del(&uwq.wq.entry);spin_unlock_irq(&ctx->fault_pending_wqh.lock);}
- /*
- ctx may go away after this if the userfault pseudo fd is
- already released.
@@ -1861,11 +1875,14 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx, return ret; if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
UFFDIO_WRITEPROTECT_MODE_WP))
UFFDIO_WRITEPROTECT_MODE_WP |UFFDIO_WRITEPROTECT_MODE_ASYNC_WP))
- mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
- mode_wp = uffdio_wp.mode & (UFFDIO_WRITEPROTECT_MODE_WP |
UFFDIO_WRITEPROTECT_MODE_ASYNC_WP);- ctx->async = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_ASYNC_WP;
Please no.. ctx attributes shouldn't be easily changed by a single ioctl.
I suggest we have a new feature bit as I mentioned above (say, UFFD_FEATURE_WP_ASYNC), set it once with UFFDIO_API and it should apply to the whole lifecycle of this uffd handle. That flag should (something I can quickly think of):
Have effect only if the uffd will be registered with WP mode (of course) or ignored in any other modes,
Should fail any attempts of UFFDIO_WRITEPROTECT with wp=false on this uffd handle because with async faults no page fault resolution needed from userspace,
Should apply to any region registered with this uffd ctx, so it's exclusively used with sync uffd-wp mode.
All of these are necesary and must be done to consolidate the interface of UFFD. Agreed!
Then when the app wants to wr-protect in async mode, it simply goes ahead with UFFDIO_WRITEPROTECT(wp=true), it'll happen exactly the same as when it was sync mode. It's only the pf handling procedure that's different (along with how the fault is reported - rather than as a message but it'll be consolidated into the soft-dirty bit).
PF handling will resovle the fault after un-setting the _PAGE_*_UFFD_WP on the page. I'm not changing the soft-dirty bit. It is too delicate (if you get the joke).
if (mode_wp && mode_dontwake) return -EINVAL; @@ -2126,6 +2143,7 @@ static int new_userfaultfd(int flags) ctx->flags = flags; ctx->features = 0; ctx->released = false;
- ctx->async = false; atomic_set(&ctx->mmap_changing, 0); ctx->mm = current->mm; /* prevent the mm struct to be freed */
diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 005e5e306266..b89665653861 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -284,6 +284,11 @@ struct uffdio_writeprotect {
- UFFDIO_WRITEPROTECT_MODE_DONTWAKE: set the flag to avoid waking up
- any wait thread after the operation succeeds.
- UFFDIO_WRITEPROTECT_MODE_ASYNC_WP: set the flag to write protect a
- range, the flag is unset automatically when the page is written.
- This is used to track which pages have been written to from the
- time the memory was write protected.
- NOTE: Write protecting a region (WP=1) is unrelated to page faults,
- therefore DONTWAKE flag is meaningless with WP=1. Removing write
- protection (WP=0) in response to a page fault wakes the faulting
@@ -291,6 +296,7 @@ struct uffdio_writeprotect { */ #define UFFDIO_WRITEPROTECT_MODE_WP ((__u64)1<<0) #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE ((__u64)1<<1) +#define UFFDIO_WRITEPROTECT_MODE_ASYNC_WP ((__u64)1<<2) __u64 mode; }; -- 2.30.2
I should have added Suggested-by: Peter Xy peterx@redhat.com to this patch. I'll add in the next revision if you don't object.
I've started working on next revision. I'll reply to other highly valuable review emails a bit later.
On Thu, Jan 19, 2023 at 08:09:52PM +0500, Muhammad Usama Anjum wrote:
[...]
@@ -497,80 +498,93 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) /* take the reference before dropping the mmap_lock */ userfaultfd_ctx_get(ctx);
- if (ctx->async) {
Firstly, please consider not touching the existing code/indent as much as what this patch did. Hopefully we can keep the major part of sync uffd be there with its git log, it also helps reviewing your code. You can add the async block before that, handle the fault and return just earlier.
This is possible. Will do in next revision.
And, I think this is a bit too late because we're going to return with VM_FAULT_RETRY here, while maybe we don't need to retry at all here because we're going to resolve the page fault immediately.
I assume you added this because you wanted userfaultfd_ctx_get() to make sure the uffd context will not go away from under us, but it's not needed if we're still holding the mmap read lock. I'd expect for async mode we don't really need to release it at all.
I'll have to check the what should be returned here. We should return something which shows that the fault has been resolved.
VM_FAULT_NOPAGE may be the best to describe it, but I guess it shouldn't have a difference here if to just return zero. And, I guess you don't even need to worry on the retval here because I think you can leverage do_wp_page. More below.
// Resolve page fault of this pagePlease use "/* ... */" as that's the common pattern of commenting in the Linux kernel, at least what I see in mm/.
Will do.
unsigned long addr = (ctx->features & UFFD_FEATURE_EXACT_ADDRESS) ?vmf->real_address : vmf->address;struct vm_area_struct *dst_vma = find_vma(ctx->mm, addr);size_t s = PAGE_SIZE;This is weird - if we want async uffd-wp, let's consider huge page from the 1st day.
if (dst_vma->vm_flags & VM_HUGEPAGE) {VM_HUGEPAGE is only a hint. It doesn't mean this page is always a huge page. For anon, we can have thp wr-protected as a whole, not happening for !anon because we'll split already.
For anon, if a write happens to a thp being uffd-wp-ed, we'll keep that pmd wr-protected and report the uffd message. The pmd split happens when the user invokes UFFDIO_WRITEPROTECT on the small page. I think it'll stop working for async uffd-wp because we're going to resolve the page faults right away.
So for async uffd-wp (note: this will be different from hugetlb), you may want to consider having a pre-requisite patch to change wp_huge_pmd() behavior: rather than calling handle_userfault(), IIUC you can also just fallback to the split path right below (__split_huge_pmd) so the thp will split now even before the uffd message is generated.
I'll make the changes and make this. I wasn't aware that the thp is being broken in the UFFD WP. At this time, I'm not sure if thp will be handled by handle_userfault() in one go. Probably it will as the length is stored in the vmf.
Yes I think THP can actually be handled in one go with uffd-wp anon (even if vmf doesn't store any length because page fault is about address only not length, afaict). E.g. thp firstly get wr-protected in thp size, then when unprotect the user app sends UFFDIO_WRITEPROTECT(wp=false) with a range covering the whole thp.
But AFAIU that should be quite rare because most uffd-wp scenarios are latency sensitive, resolving page faults in large chunk definitely enlarges that. It could happen though when it's not resolving an immediate page fault, so it could happen in the background.
So after a second thought, a safer approach is we only go to the split path if async is enabled, in wp_huge_pmd(). Then it doesn't need to be a pre-requisite patch too, it can be part of the major patch to implement the uffd-wp async mode.
I think it should be transparent to the user and it'll start working for you with async uffd-wp here, because it means when reaching handle_userfault, it should not be possible to have thp at all since they should have all split up.
s = HPAGE_SIZE;addr &= HPAGE_MASK;}
- init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function);
- uwq.wq.private = current;
- uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags,
reason, ctx->features);- uwq.ctx = ctx;
- uwq.waken = false;
- blocking_state = userfaultfd_get_blocking_state(vmf->flags);
ret = mwriteprotect_range(ctx->mm, addr, s, false, &ctx->mmap_changing);This is an overkill - we're pretty sure it's a single page, no need to call a range function here.
Probably change_pte_range() should be used here to directly remove the WP here?
Here we can persue the best performance, or we can also persue the easist way to implement. I think the best we can have is we don't release either the mmap read lock _and_ the pgtable lock, so we resolve the page fault completely here. But that requires more code changes.
So far an probably intermediate (and very easy to implement) solution is:
(1) Remap the pte (vmf->pte) and retake the lock (vmf->ptl). Note: you need to move the chunk to be before mmap read lock released first, because we'll need that to make sure pgtable lock and the pgtable page being still exist at the first place.
(2) If *vmf->pte != vmf->orig_pte, it means the pgtable changed, retry (with VM_FAULT_NOPAGE). We must have orig_pte set btw in this path.
(2) Remove the uffd-wp bit if it's set (and it must be set, because we checked again on orig_pte with pgtable lock held).
(3) Invoke do_wp_page() again with the same vmf.
This will focus the resolution on the single page and resolve CoW in one shot if needed. We may need to redo the map/lock of pte* but I suppose it won't hurt a lot if we just modified the fields anyway, so we can leave that for later.
[...]
Then when the app wants to wr-protect in async mode, it simply goes ahead with UFFDIO_WRITEPROTECT(wp=true), it'll happen exactly the same as when it was sync mode. It's only the pf handling procedure that's different (along with how the fault is reported - rather than as a message but it'll be consolidated into the soft-dirty bit).
PF handling will resovle the fault after un-setting the _PAGE_*_UFFD_WP on the page. I'm not changing the soft-dirty bit. It is too delicate (if you get the joke).
It's unfortunate that the old soft-dirty solution didn't go through easily. Soft-dirty still covers something that uffd-wp cannot do right now, e.g. on tracking mostly any type of pte mappings. Uffd-wp can so far only track fully ram backed pages like shmem or hugetlb for files but not any random page cache. Hopefully it still works at least for your use case, or it's time to rethink otherwise.
if (mode_wp && mode_dontwake) return -EINVAL; @@ -2126,6 +2143,7 @@ static int new_userfaultfd(int flags) ctx->flags = flags; ctx->features = 0; ctx->released = false;
- ctx->async = false; atomic_set(&ctx->mmap_changing, 0); ctx->mm = current->mm; /* prevent the mm struct to be freed */
diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 005e5e306266..b89665653861 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -284,6 +284,11 @@ struct uffdio_writeprotect {
- UFFDIO_WRITEPROTECT_MODE_DONTWAKE: set the flag to avoid waking up
- any wait thread after the operation succeeds.
- UFFDIO_WRITEPROTECT_MODE_ASYNC_WP: set the flag to write protect a
- range, the flag is unset automatically when the page is written.
- This is used to track which pages have been written to from the
- time the memory was write protected.
- NOTE: Write protecting a region (WP=1) is unrelated to page faults,
- therefore DONTWAKE flag is meaningless with WP=1. Removing write
- protection (WP=0) in response to a page fault wakes the faulting
@@ -291,6 +296,7 @@ struct uffdio_writeprotect { */ #define UFFDIO_WRITEPROTECT_MODE_WP ((__u64)1<<0) #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE ((__u64)1<<1) +#define UFFDIO_WRITEPROTECT_MODE_ASYNC_WP ((__u64)1<<2) __u64 mode; }; -- 2.30.2
I should have added Suggested-by: Peter Xy peterx@redhat.com to this patch. I'll add in the next revision if you don't object.
I'm fine with it. If so, please do s/Xy/Xu/.
I've started working on next revision. I'll reply to other highly valuable review emails a bit later.
Thanks,
On Thu, Jan 19, 2023 at 11:35:39AM -0500, Peter Xu wrote:
On Thu, Jan 19, 2023 at 08:09:52PM +0500, Muhammad Usama Anjum wrote:
[...]
@@ -497,80 +498,93 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) /* take the reference before dropping the mmap_lock */ userfaultfd_ctx_get(ctx);
- if (ctx->async) {
Firstly, please consider not touching the existing code/indent as much as what this patch did. Hopefully we can keep the major part of sync uffd be there with its git log, it also helps reviewing your code. You can add the async block before that, handle the fault and return just earlier.
This is possible. Will do in next revision.
And, I think this is a bit too late because we're going to return with VM_FAULT_RETRY here, while maybe we don't need to retry at all here because we're going to resolve the page fault immediately.
I assume you added this because you wanted userfaultfd_ctx_get() to make sure the uffd context will not go away from under us, but it's not needed if we're still holding the mmap read lock. I'd expect for async mode we don't really need to release it at all.
I'll have to check the what should be returned here. We should return something which shows that the fault has been resolved.
VM_FAULT_NOPAGE may be the best to describe it, but I guess it shouldn't have a difference here if to just return zero. And, I guess you don't even need to worry on the retval here because I think you can leverage do_wp_page. More below.
// Resolve page fault of this pagePlease use "/* ... */" as that's the common pattern of commenting in the Linux kernel, at least what I see in mm/.
Will do.
unsigned long addr = (ctx->features & UFFD_FEATURE_EXACT_ADDRESS) ?vmf->real_address : vmf->address;struct vm_area_struct *dst_vma = find_vma(ctx->mm, addr);size_t s = PAGE_SIZE;This is weird - if we want async uffd-wp, let's consider huge page from the 1st day.
if (dst_vma->vm_flags & VM_HUGEPAGE) {VM_HUGEPAGE is only a hint. It doesn't mean this page is always a huge page. For anon, we can have thp wr-protected as a whole, not happening for !anon because we'll split already.
For anon, if a write happens to a thp being uffd-wp-ed, we'll keep that pmd wr-protected and report the uffd message. The pmd split happens when the user invokes UFFDIO_WRITEPROTECT on the small page. I think it'll stop working for async uffd-wp because we're going to resolve the page faults right away.
So for async uffd-wp (note: this will be different from hugetlb), you may want to consider having a pre-requisite patch to change wp_huge_pmd() behavior: rather than calling handle_userfault(), IIUC you can also just fallback to the split path right below (__split_huge_pmd) so the thp will split now even before the uffd message is generated.
I'll make the changes and make this. I wasn't aware that the thp is being broken in the UFFD WP. At this time, I'm not sure if thp will be handled by handle_userfault() in one go. Probably it will as the length is stored in the vmf.
Yes I think THP can actually be handled in one go with uffd-wp anon (even if vmf doesn't store any length because page fault is about address only not length, afaict). E.g. thp firstly get wr-protected in thp size, then when unprotect the user app sends UFFDIO_WRITEPROTECT(wp=false) with a range covering the whole thp.
But AFAIU that should be quite rare because most uffd-wp scenarios are latency sensitive, resolving page faults in large chunk definitely enlarges that. It could happen though when it's not resolving an immediate page fault, so it could happen in the background.
So after a second thought, a safer approach is we only go to the split path if async is enabled, in wp_huge_pmd(). Then it doesn't need to be a pre-requisite patch too, it can be part of the major patch to implement the uffd-wp async mode.
I think it should be transparent to the user and it'll start working for you with async uffd-wp here, because it means when reaching handle_userfault, it should not be possible to have thp at all since they should have all split up.
s = HPAGE_SIZE;addr &= HPAGE_MASK;}
- init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function);
- uwq.wq.private = current;
- uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags,
reason, ctx->features);- uwq.ctx = ctx;
- uwq.waken = false;
- blocking_state = userfaultfd_get_blocking_state(vmf->flags);
ret = mwriteprotect_range(ctx->mm, addr, s, false, &ctx->mmap_changing);This is an overkill - we're pretty sure it's a single page, no need to call a range function here.
Probably change_pte_range() should be used here to directly remove the WP here?
Here we can persue the best performance, or we can also persue the easist way to implement. I think the best we can have is we don't release either the mmap read lock _and_ the pgtable lock, so we resolve the page fault completely here. But that requires more code changes.
So far an probably intermediate (and very easy to implement) solution is:
(1) Remap the pte (vmf->pte) and retake the lock (vmf->ptl). Note: you need to move the chunk to be before mmap read lock released first, because we'll need that to make sure pgtable lock and the pgtable page being still exist at the first place.
(2) If *vmf->pte != vmf->orig_pte, it means the pgtable changed, retry (with VM_FAULT_NOPAGE). We must have orig_pte set btw in this path.
(2) Remove the uffd-wp bit if it's set (and it must be set, because we checked again on orig_pte with pgtable lock held).
(3) Invoke do_wp_page() again with the same vmf.
This will focus the resolution on the single page and resolve CoW in one shot if needed. We may need to redo the map/lock of pte* but I suppose it won't hurt a lot if we just modified the fields anyway, so we can leave that for later.
I just noticed it's actually quite straigtforward to just not fall into handle_userfault at all. It can be as simple as:
---8<--- diff --git a/mm/memory.c b/mm/memory.c index 4000e9f017e0..09aab434654c 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3351,8 +3351,20 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
if (likely(!unshare)) { if (userfaultfd_pte_wp(vma, *vmf->pte)) { - pte_unmap_unlock(vmf->pte, vmf->ptl); - return handle_userfault(vmf, VM_UFFD_WP); + if (userfaultfd_uffd_wp_async(vma)) { + /* + * Nothing needed (cache flush, TLB + * invalidations, etc.) because we're only + * removing the uffd-wp bit, which is + * completely invisible to the user. + * This falls through to possible CoW. + */ + set_pte_at(vma->vm_mm, vmf->address, vmf->pte, + pte_clear_uffd_wp(*vmf->pte)); + } else { + pte_unmap_unlock(vmf->pte, vmf->ptl); + return handle_userfault(vmf, VM_UFFD_WP); + } } ---8<---
Similar thing will be needed for hugetlb if that'll be supported.
One thing worth mention is, I think for async wp it doesn't need to be restricted by UFFD_USER_MODE_ONLY, because comparing to the sync messages it has no risk of being utilized for malicious purposes.
[...]
Then when the app wants to wr-protect in async mode, it simply goes ahead with UFFDIO_WRITEPROTECT(wp=true), it'll happen exactly the same as when it was sync mode. It's only the pf handling procedure that's different (along with how the fault is reported - rather than as a message but it'll be consolidated into the soft-dirty bit).
PF handling will resovle the fault after un-setting the _PAGE_*_UFFD_WP on the page. I'm not changing the soft-dirty bit. It is too delicate (if you get the joke).
It's unfortunate that the old soft-dirty solution didn't go through easily. Soft-dirty still covers something that uffd-wp cannot do right now, e.g. on tracking mostly any type of pte mappings. Uffd-wp can so far only track fully ram backed pages like shmem or hugetlb for files but not any random page cache. Hopefully it still works at least for your use case, or it's time to rethink otherwise.
if (mode_wp && mode_dontwake) return -EINVAL; @@ -2126,6 +2143,7 @@ static int new_userfaultfd(int flags) ctx->flags = flags; ctx->features = 0; ctx->released = false;
- ctx->async = false; atomic_set(&ctx->mmap_changing, 0); ctx->mm = current->mm; /* prevent the mm struct to be freed */
diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 005e5e306266..b89665653861 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -284,6 +284,11 @@ struct uffdio_writeprotect {
- UFFDIO_WRITEPROTECT_MODE_DONTWAKE: set the flag to avoid waking up
- any wait thread after the operation succeeds.
- UFFDIO_WRITEPROTECT_MODE_ASYNC_WP: set the flag to write protect a
- range, the flag is unset automatically when the page is written.
- This is used to track which pages have been written to from the
- time the memory was write protected.
- NOTE: Write protecting a region (WP=1) is unrelated to page faults,
- therefore DONTWAKE flag is meaningless with WP=1. Removing write
- protection (WP=0) in response to a page fault wakes the faulting
@@ -291,6 +296,7 @@ struct uffdio_writeprotect { */ #define UFFDIO_WRITEPROTECT_MODE_WP ((__u64)1<<0) #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE ((__u64)1<<1) +#define UFFDIO_WRITEPROTECT_MODE_ASYNC_WP ((__u64)1<<2) __u64 mode; }; -- 2.30.2
I should have added Suggested-by: Peter Xy peterx@redhat.com to this patch. I'll add in the next revision if you don't object.
I'm fine with it. If so, please do s/Xy/Xu/.
I've started working on next revision. I'll reply to other highly valuable review emails a bit later.
Thanks,
-- Peter Xu
On 1/20/23 7:53 PM, Peter Xu wrote:
On Thu, Jan 19, 2023 at 11:35:39AM -0500, Peter Xu wrote:
On Thu, Jan 19, 2023 at 08:09:52PM +0500, Muhammad Usama Anjum wrote:
[...]
@@ -497,80 +498,93 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) /* take the reference before dropping the mmap_lock */ userfaultfd_ctx_get(ctx);
- if (ctx->async) {
Firstly, please consider not touching the existing code/indent as much as what this patch did. Hopefully we can keep the major part of sync uffd be there with its git log, it also helps reviewing your code. You can add the async block before that, handle the fault and return just earlier.
This is possible. Will do in next revision.
And, I think this is a bit too late because we're going to return with VM_FAULT_RETRY here, while maybe we don't need to retry at all here because we're going to resolve the page fault immediately.
I assume you added this because you wanted userfaultfd_ctx_get() to make sure the uffd context will not go away from under us, but it's not needed if we're still holding the mmap read lock. I'd expect for async mode we don't really need to release it at all.
I'll have to check the what should be returned here. We should return something which shows that the fault has been resolved.
VM_FAULT_NOPAGE may be the best to describe it, but I guess it shouldn't have a difference here if to just return zero. And, I guess you don't even need to worry on the retval here because I think you can leverage do_wp_page. More below.
Yeah, this has been changed with your below patch.
// Resolve page fault of this pagePlease use "/* ... */" as that's the common pattern of commenting in the Linux kernel, at least what I see in mm/.
Will do.
unsigned long addr = (ctx->features & UFFD_FEATURE_EXACT_ADDRESS) ?vmf->real_address : vmf->address;struct vm_area_struct *dst_vma = find_vma(ctx->mm, addr);size_t s = PAGE_SIZE;This is weird - if we want async uffd-wp, let's consider huge page from the 1st day.
if (dst_vma->vm_flags & VM_HUGEPAGE) {VM_HUGEPAGE is only a hint. It doesn't mean this page is always a huge page. For anon, we can have thp wr-protected as a whole, not happening for !anon because we'll split already.
For anon, if a write happens to a thp being uffd-wp-ed, we'll keep that pmd wr-protected and report the uffd message. The pmd split happens when the user invokes UFFDIO_WRITEPROTECT on the small page. I think it'll stop working for async uffd-wp because we're going to resolve the page faults right away.
So for async uffd-wp (note: this will be different from hugetlb), you may want to consider having a pre-requisite patch to change wp_huge_pmd() behavior: rather than calling handle_userfault(), IIUC you can also just fallback to the split path right below (__split_huge_pmd) so the thp will split now even before the uffd message is generated.
I'll make the changes and make this. I wasn't aware that the thp is being broken in the UFFD WP. At this time, I'm not sure if thp will be handled by handle_userfault() in one go. Probably it will as the length is stored in the vmf.
Yes I think THP can actually be handled in one go with uffd-wp anon (even if vmf doesn't store any length because page fault is about address only not length, afaict). E.g. thp firstly get wr-protected in thp size, then when unprotect the user app sends UFFDIO_WRITEPROTECT(wp=false) with a range covering the whole thp.
But AFAIU that should be quite rare because most uffd-wp scenarios are latency sensitive, resolving page faults in large chunk definitely enlarges that. It could happen though when it's not resolving an immediate page fault, so it could happen in the background.
So after a second thought, a safer approach is we only go to the split path if async is enabled, in wp_huge_pmd(). Then it doesn't need to be a pre-requisite patch too, it can be part of the major patch to implement the uffd-wp async mode.
I think it should be transparent to the user and it'll start working for you with async uffd-wp here, because it means when reaching handle_userfault, it should not be possible to have thp at all since they should have all split up.
s = HPAGE_SIZE;addr &= HPAGE_MASK;}
- init_waitqueue_func_entry(&uwq.wq, userfaultfd_wake_function);
- uwq.wq.private = current;
- uwq.msg = userfault_msg(vmf->address, vmf->real_address, vmf->flags,
reason, ctx->features);- uwq.ctx = ctx;
- uwq.waken = false;
- blocking_state = userfaultfd_get_blocking_state(vmf->flags);
ret = mwriteprotect_range(ctx->mm, addr, s, false, &ctx->mmap_changing);This is an overkill - we're pretty sure it's a single page, no need to call a range function here.
Probably change_pte_range() should be used here to directly remove the WP here?
Here we can persue the best performance, or we can also persue the easist way to implement. I think the best we can have is we don't release either the mmap read lock _and_ the pgtable lock, so we resolve the page fault completely here. But that requires more code changes.
So far an probably intermediate (and very easy to implement) solution is:
(1) Remap the pte (vmf->pte) and retake the lock (vmf->ptl). Note: you need to move the chunk to be before mmap read lock released first, because we'll need that to make sure pgtable lock and the pgtable page being still exist at the first place.
(2) If *vmf->pte != vmf->orig_pte, it means the pgtable changed, retry (with VM_FAULT_NOPAGE). We must have orig_pte set btw in this path.
(2) Remove the uffd-wp bit if it's set (and it must be set, because we checked again on orig_pte with pgtable lock held).
(3) Invoke do_wp_page() again with the same vmf.
This will focus the resolution on the single page and resolve CoW in one shot if needed. We may need to redo the map/lock of pte* but I suppose it won't hurt a lot if we just modified the fields anyway, so we can leave that for later.
I just noticed it's actually quite straigtforward to just not fall into handle_userfault at all. It can be as simple as:
---8<--- diff --git a/mm/memory.c b/mm/memory.c index 4000e9f017e0..09aab434654c 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3351,8 +3351,20 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
if (likely(!unshare)) { if (userfaultfd_pte_wp(vma, *vmf->pte)) {
pte_unmap_unlock(vmf->pte, vmf->ptl);return handle_userfault(vmf, VM_UFFD_WP);
if (userfaultfd_uffd_wp_async(vma)) {/** Nothing needed (cache flush, TLB* invalidations, etc.) because we're only* removing the uffd-wp bit, which is* completely invisible to the user.* This falls through to possible CoW.*/set_pte_at(vma->vm_mm, vmf->address, vmf->pte,pte_clear_uffd_wp(*vmf->pte));} else {pte_unmap_unlock(vmf->pte, vmf->ptl);return handle_userfault(vmf, VM_UFFD_WP);} }---8<---
Similar thing will be needed for hugetlb if that'll be supported.
At the moment, hugetlb support is required. Thank you so much for sending this up. This has made things simple. I've replicated this same patch in wp_huge_pmd() for huge pages.
One thing worth mention is, I think for async wp it doesn't need to be restricted by UFFD_USER_MODE_ONLY, because comparing to the sync messages it has no risk of being utilized for malicious purposes.
I think with updated handling path updated in do_wp_page() and wp_huge_pmd() in version, UFFD_USER_MODE_ONLY will not affect us.
[...]
Then when the app wants to wr-protect in async mode, it simply goes ahead with UFFDIO_WRITEPROTECT(wp=true), it'll happen exactly the same as when it was sync mode. It's only the pf handling procedure that's different (along with how the fault is reported - rather than as a message but it'll be consolidated into the soft-dirty bit).
PF handling will resovle the fault after un-setting the _PAGE_*_UFFD_WP on the page. I'm not changing the soft-dirty bit. It is too delicate (if you get the joke).
It's unfortunate that the old soft-dirty solution didn't go through easily. Soft-dirty still covers something that uffd-wp cannot do right now, e.g. on tracking mostly any type of pte mappings. Uffd-wp can so far only track fully ram backed pages like shmem or hugetlb for files but not any random page cache. Hopefully it still works at least for your use case, or it's time to rethink otherwise.
if (mode_wp && mode_dontwake) return -EINVAL; @@ -2126,6 +2143,7 @@ static int new_userfaultfd(int flags) ctx->flags = flags; ctx->features = 0; ctx->released = false;
- ctx->async = false; atomic_set(&ctx->mmap_changing, 0); ctx->mm = current->mm; /* prevent the mm struct to be freed */
diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 005e5e306266..b89665653861 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -284,6 +284,11 @@ struct uffdio_writeprotect {
- UFFDIO_WRITEPROTECT_MODE_DONTWAKE: set the flag to avoid waking up
- any wait thread after the operation succeeds.
- UFFDIO_WRITEPROTECT_MODE_ASYNC_WP: set the flag to write protect a
- range, the flag is unset automatically when the page is written.
- This is used to track which pages have been written to from the
- time the memory was write protected.
- NOTE: Write protecting a region (WP=1) is unrelated to page faults,
- therefore DONTWAKE flag is meaningless with WP=1. Removing write
- protection (WP=0) in response to a page fault wakes the faulting
@@ -291,6 +296,7 @@ struct uffdio_writeprotect { */ #define UFFDIO_WRITEPROTECT_MODE_WP ((__u64)1<<0) #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE ((__u64)1<<1) +#define UFFDIO_WRITEPROTECT_MODE_ASYNC_WP ((__u64)1<<2) __u64 mode; }; -- 2.30.2
I should have added Suggested-by: Peter Xy peterx@redhat.com to this patch. I'll add in the next revision if you don't object.
I'm fine with it. If so, please do s/Xy/Xu/.
Sorry,
Suggested-by: Peter Xu peterx@redhat.com
I've started working on next revision. I'll reply to other highly valuable review emails a bit later.
Thanks,
-- Peter Xu
On Mon, Jan 23, 2023 at 03:11:20PM +0500, Muhammad Usama Anjum wrote:
One thing worth mention is, I think for async wp it doesn't need to be restricted by UFFD_USER_MODE_ONLY, because comparing to the sync messages it has no risk of being utilized for malicious purposes.
I think with updated handling path updated in do_wp_page() and wp_huge_pmd() in version, UFFD_USER_MODE_ONLY will not affect us.
This is more or less a comment for the design, the new code should work (by bypassing handle_userfaultfd(), where this bit was checked).
We'll need an man page update if this feature will be merged [1], and if so it'll need to be updated for the UFFD_USER_MODE_ONLY section regarding to async uffd-wp support too. I think that can also be worked out after the series being accepted first, so just a heads up.
[1] https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
On 1/24/23 10:26 PM, Peter Xu wrote:
On Mon, Jan 23, 2023 at 03:11:20PM +0500, Muhammad Usama Anjum wrote:
One thing worth mention is, I think for async wp it doesn't need to be restricted by UFFD_USER_MODE_ONLY, because comparing to the sync messages it has no risk of being utilized for malicious purposes.
I think with updated handling path updated in do_wp_page() and wp_huge_pmd() in version, UFFD_USER_MODE_ONLY will not affect us.
This is more or less a comment for the design, the new code should work (by bypassing handle_userfaultfd(), where this bit was checked).
We'll need an man page update if this feature will be merged [1], and if so it'll need to be updated for the UFFD_USER_MODE_ONLY section regarding to async uffd-wp support too. I think that can also be worked out after the series being accepted first, so just a heads up.
[1] https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
Thank you for explaining it. Definitely, we'll update the man pages after the change has been merged. I've added it to my notes.>
Split mwriteprotect_range() to create a unlocked version. This will be used in the next patch to write protect a memory area. Add a helper function, wp_range_async() as well.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- fs/userfaultfd.c | 11 ++++++++++ include/linux/userfaultfd_k.h | 10 +++++++++ mm/userfaultfd.c | 40 ++++++++++++++++++++++------------- 3 files changed, 46 insertions(+), 15 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index be5e10d15058..5ff5ff4314e3 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1967,6 +1967,17 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg) return ret; }
+int wp_range_async(struct vm_area_struct *vma, unsigned long start, unsigned long len) +{ + struct userfaultfd_ctx *ctx = vma->vm_userfaultfd_ctx.ctx; + + if (!ctx) + return -1; + + ctx->async = true; + return __mwriteprotect_range(ctx->mm, start, len, true, &ctx->mmap_changing); +} + static inline unsigned int uffd_ctx_features(__u64 user_features) { /* diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h index 9df0b9a762cc..fba05a32f8e9 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -73,6 +73,9 @@ extern ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long dst_start, extern int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, unsigned long len, bool enable_wp, atomic_t *mmap_changing); +extern int __mwriteprotect_range(struct mm_struct *dst_mm, + unsigned long start, unsigned long len, + bool enable_wp, atomic_t *mmap_changing); extern void uffd_wp_range(struct mm_struct *dst_mm, struct vm_area_struct *vma, unsigned long start, unsigned long len, bool enable_wp);
@@ -179,6 +182,8 @@ extern int userfaultfd_unmap_prep(struct mm_struct *mm, unsigned long start, unsigned long end, struct list_head *uf); extern void userfaultfd_unmap_complete(struct mm_struct *mm, struct list_head *uf); +extern int wp_range_async(struct vm_area_struct *vma, unsigned long start, + unsigned long len);
#else /* CONFIG_USERFAULTFD */
@@ -274,6 +279,11 @@ static inline bool uffd_disable_fault_around(struct vm_area_struct *vma) return false; }
+int wp_range_async(struct vm_area_struct *vma, unsigned long start, unsigned long len) +{ + return -1; +} + #endif /* CONFIG_USERFAULTFD */
static inline bool pte_marker_entry_uffd_wp(swp_entry_t entry) diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 65ad172add27..9d8a43faf764 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -734,25 +734,13 @@ void uffd_wp_range(struct mm_struct *dst_mm, struct vm_area_struct *dst_vma, tlb_finish_mmu(&tlb); }
-int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, - unsigned long len, bool enable_wp, - atomic_t *mmap_changing) +int __mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, + unsigned long len, bool enable_wp, + atomic_t *mmap_changing) { struct vm_area_struct *dst_vma; unsigned long page_mask; int err; - - /* - * Sanitize the command parameters: - */ - BUG_ON(start & ~PAGE_MASK); - BUG_ON(len & ~PAGE_MASK); - - /* Does the address range wrap, or is the span zero-sized? */ - BUG_ON(start + len <= start); - - mmap_read_lock(dst_mm); - /* * If memory mappings are changing because of non-cooperative * operation (e.g. mremap) running in parallel, bail out and @@ -783,6 +771,28 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
err = 0; out_unlock: + return err; +} + +int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, + unsigned long len, bool enable_wp, + atomic_t *mmap_changing) +{ + int err; + + /* + * Sanitize the command parameters: + */ + BUG_ON(start & ~PAGE_MASK); + BUG_ON(len & ~PAGE_MASK); + + /* Does the address range wrap, or is the span zero-sized? */ + BUG_ON(start + len <= start); + + mmap_read_lock(dst_mm); + + err = __mwriteprotect_range(dst_mm, start, len, enable_wp, mmap_changing); + mmap_read_unlock(dst_mm); return err; }
This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear the info about page table entries. The following operations are supported in this ioctl: - Get the information if the pages have been written-to (PAGE_IS_Wt), file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped (PAGE_IS_SWAPPED). - Write-protect the pages (PAGEMAP_WP_ENGAGE) to start finding which pages have been written-to. - Find pages which have been written-to and write protect the pages (atomic PAGE_IS_NOT_WP + PAGEMAP_WP_ENGAGE)
The uffd should have been registered on the memory range before performing any WP/WT (Write Protect/Writtern-To) related operations with the IOCTL.
struct pagemap_scan_args is used as the argument of the IOCTL. In this struct: - The range is specified through start and len. - The output buffer and size is specified as vec and vec_len. - The optional maximum requested pages are specified in the max_pages. - The flags can be specified in the flags field. The PAGEMAP_WP_ENGAGE is the only added flag at this time. - The masks are specified in required_mask, anyof_mask, excluded_ mask and return_mask.
This IOCTL can be extended to get information about more PTE bits. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- Changes in v7: - Rebase on top of latest next - Fix some corner cases - Base soft-dirty on the uffd wp async - Update the terminologies - Optimize the memory usage inside the ioctl
Changes in v6: - Rename variables and update comments - Make IOCTL independent of soft_dirty config - Change masks and bitmap type to _u64 - Improve code quality
Changes in v5: - Remove tlb flushing even for clear operation
Changes in v4: - Update the interface and implementation
Changes in v3: - Tighten the user-kernel interface by using explicit types and add more error checking
Changes in v2: - Convert the interface from syscall to ioctl - Remove pidfd support as it doesn't make sense in ioctl --- fs/proc/task_mmu.c | 300 ++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 ++++++ tools/include/uapi/linux/fs.h | 50 ++++++ 3 files changed, 400 insertions(+)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..ba70faadf403 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -19,6 +19,7 @@ #include <linux/shmem_fs.h> #include <linux/uaccess.h> #include <linux/pkeys.h> +#include <linux/minmax.h>
#include <asm/elf.h> #include <asm/tlb.h> @@ -1135,6 +1136,22 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma, } #endif
+static inline bool is_pte_uffd_wp(pte_t pte) +{ + if ((pte_present(pte) && pte_uffd_wp(pte)) || + (is_swap_pte(pte) && pte_swp_uffd_wp(pte))) + return true; + return false; +} + +static inline bool is_pmd_uffd_wp(pmd_t pmd) +{ + if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) || + (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd))) + return true; + return false; +} + #if defined(CONFIG_MEM_SOFT_DIRTY) && defined(CONFIG_TRANSPARENT_HUGEPAGE) static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmdp) @@ -1763,11 +1780,294 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; }
+#define PAGEMAP_OP_MASK (PAGE_IS_WT | PAGE_IS_FILE | \ + PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define PAGEMAP_NONWT_OP_MASK (PAGE_IS_FILE | PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define IS_WP_ENGAGE_OP(a) (a->flags & PAGEMAP_WP_ENGAGE) +#define IS_GET_OP(a) (a->vec) +#define PAGEMAP_SCAN_BITMAP(wt, file, present, swap) \ + (wt | file << 1 | present << 2 | swap << 3) +#define IS_WT_REQUIRED(a) \ + ((a->required_mask & PAGE_IS_WT) || \ + (a->anyof_mask & PAGE_IS_WT)) + +struct pagemap_scan_private { + struct page_region *vec; + struct page_region prev; + unsigned long vec_len, vec_index; + unsigned int max_pages, found_pages, flags; + unsigned long required_mask, anyof_mask, excluded_mask, return_mask; +}; + +static int pagemap_scan_test_walk(unsigned long start, unsigned long end, struct mm_walk *walk) +{ + struct pagemap_scan_private *p = walk->private; + struct vm_area_struct *vma = walk->vma; + + if (IS_WT_REQUIRED(p) && !vma_can_userfault(vma, vma->vm_flags)) + return -EPERM; + if (IS_GET_OP(p) && p->max_pages && (p->found_pages == p->max_pages)) + return -ENOSPC; + if (vma->vm_flags & VM_PFNMAP) + return 1; + return 0; +} + +static inline int add_to_out(bool wt, bool file, bool pres, bool swap, + struct pagemap_scan_private *p, unsigned long addr, unsigned int len) +{ + unsigned long bitmap, cur = PAGEMAP_SCAN_BITMAP(wt, file, pres, swap); + bool cpy = true; + struct page_region *prev = &p->prev; + + if (p->required_mask) + cpy = ((p->required_mask & cur) == p->required_mask); + if (cpy && p->anyof_mask) + cpy = (p->anyof_mask & cur); + if (cpy && p->excluded_mask) + cpy = !(p->excluded_mask & cur); + bitmap = cur & p->return_mask; + if (cpy && bitmap) { + if ((prev->len) && (prev->bitmap == bitmap) && + (prev->start + prev->len * PAGE_SIZE == addr)) { + prev->len += len; + p->found_pages += len; + } else if (p->vec_index < p->vec_len) { + if (prev->len) { + memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region)); + p->vec_index++; + } + prev->start = addr; + prev->len = len; + prev->bitmap = bitmap; + p->found_pages += len; + } else { + return -ENOSPC; + } + } + return 0; +} + +static inline int export_prev_to_out(struct pagemap_scan_private *p, struct page_region __user *vec, + unsigned long *vec_index) +{ + struct page_region *prev = &p->prev; + + if (prev->len) { + if (copy_to_user(&vec[*vec_index], prev, sizeof(struct page_region))) + return -EFAULT; + p->vec_index++; + (*vec_index)++; + prev->len = 0; + } + return 0; +} + +static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long addr, + unsigned long end, struct mm_walk *walk) +{ + struct pagemap_scan_private *p = walk->private; + struct vm_area_struct *vma = walk->vma; + unsigned long start = addr; + unsigned int len; + spinlock_t *ptl; + int ret = 0; + pte_t *pte; + bool pmd_wt; + + if ((walk->vma->vm_end < addr) || (p->max_pages && p->found_pages == p->max_pages)) + return 0; + end = min(end, walk->vma->vm_end); + +#ifdef CONFIG_TRANSPARENT_HUGEPAGE + ptl = pmd_trans_huge_lock(pmd, vma); + if (ptl) { + pmd_wt = !is_pmd_uffd_wp(*pmd); + /* + * Break huge page into small pages if operation needs to be performed is + * on a portion of the huge page or the return buffer cannot store complete + * data. + */ + if (pmd_wt && (IS_WP_ENGAGE_OP(p) && (end - addr < HPAGE_SIZE))) { + spin_unlock(ptl); + split_huge_pmd(vma, pmd, addr); + goto process_smaller_pages; + } + if (IS_GET_OP(p)) { + len = (end - addr)/PAGE_SIZE; + if (p->max_pages && p->found_pages + len > p->max_pages) + len = p->max_pages - p->found_pages; + + ret = add_to_out(pmd_wt, vma->vm_file, pmd_present(*pmd), + is_swap_pmd(*pmd), p, addr, len); + if (ret) { + spin_unlock(ptl); + return ret; + } + } + spin_unlock(ptl); + if (IS_WP_ENGAGE_OP(p) && pmd_wt) { + BUG_ON(start & ~HPAGE_MASK); + + ret = wp_range_async(vma, addr, HPAGE_SIZE); + } + return ret; + } +process_smaller_pages: + if (pmd_trans_unstable(pmd)) + return 0; +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */ + + pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl); + for (; addr < end && !ret && (!p->max_pages || (p->found_pages < p->max_pages)) + ; pte++, addr += PAGE_SIZE) { + if (IS_GET_OP(p)) { + ret = add_to_out(!is_pte_uffd_wp(*pte), vma->vm_file, pte_present(*pte), + is_swap_pte(*pte), p, addr, 1); + if (ret) + break; + } + } + pte_unmap_unlock(pte - 1, ptl); + if (IS_WP_ENGAGE_OP(p)) { + BUG_ON(start & ~PAGE_MASK); + ret = wp_range_async(vma, start, addr - start); + } + + cond_resched(); + return ret; +} + +static int pagemap_scan_pte_hole(unsigned long addr, unsigned long end, int depth, + struct mm_walk *walk) +{ + struct pagemap_scan_private *p = walk->private; + struct vm_area_struct *vma = walk->vma; + unsigned int len; + int ret = 0; + + if (vma) { + len = (end - addr)/PAGE_SIZE; + if (p->max_pages && p->found_pages + len > p->max_pages) + len = p->max_pages - p->found_pages; + ret = add_to_out(false, vma->vm_file, false, false, p, addr, len); + } + + return ret; +} + +static const struct mm_walk_ops pagemap_scan_ops = { + .test_walk = pagemap_scan_test_walk, + .pmd_entry = pagemap_scan_pmd_entry, + .pte_hole = pagemap_scan_pte_hole, +}; + +static long do_pagemap_cmd(struct mm_struct *mm, struct pagemap_scan_arg *arg) +{ + unsigned long empty_slots, vec_index = 0; + unsigned long __user start, end; + unsigned long __start, __end; + struct page_region __user *vec; + struct pagemap_scan_private p; + int ret; + + start = (unsigned long)untagged_addr(arg->start); + vec = (struct page_region *)untagged_addr(arg->vec); + if ((!IS_ALIGNED(start, PAGE_SIZE)) || (!access_ok((void __user *)start, arg->len))) + return -EINVAL; + if (IS_GET_OP(arg) && ((arg->vec_len == 0) || + (!access_ok((void __user *)vec, arg->vec_len * sizeof(struct page_region))))) + return -ENOMEM; + if ((arg->flags & ~PAGEMAP_WP_ENGAGE) || (arg->required_mask & ~PAGEMAP_OP_MASK) || + (arg->anyof_mask & ~PAGEMAP_OP_MASK) || (arg->excluded_mask & ~PAGEMAP_OP_MASK) || + (arg->return_mask & ~PAGEMAP_OP_MASK)) + return -EINVAL; + if (IS_GET_OP(arg) && ((!arg->required_mask && !arg->anyof_mask && !arg->excluded_mask) || + !arg->return_mask)) + return -EINVAL; + /* The non-WT flags cannot be obtained if PAGEMAP_WP_ENGAGE is also specified. */ + if (IS_WP_ENGAGE_OP(arg) && ((arg->required_mask & PAGEMAP_NONWT_OP_MASK) || + (arg->anyof_mask & PAGEMAP_NONWT_OP_MASK))) + return -EINVAL; + + end = start + arg->len; + p.max_pages = arg->max_pages; + p.found_pages = 0; + p.flags = arg->flags; + p.required_mask = arg->required_mask; + p.anyof_mask = arg->anyof_mask; + p.excluded_mask = arg->excluded_mask; + p.return_mask = arg->return_mask; + p.prev.len = 0; + p.vec_len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT); + + if (IS_GET_OP(arg)) { + p.vec = kmalloc_array(p.vec_len, sizeof(struct page_region), GFP_KERNEL); + if (!p.vec) + return -ENOMEM; + } else { + p.vec = NULL; + } + + __start = __end = start; + while (__end < end) { + p.vec_index = 0; + empty_slots = arg->vec_len - vec_index; + if (p.vec_len > empty_slots) + p.vec_len = empty_slots; + + __end = (__start + PAGEMAP_WALK_SIZE) & PAGEMAP_WALK_MASK; + if (__end > end) + __end = end; + + mmap_read_lock(mm); + ret = walk_page_range(mm, __start, __end, &pagemap_scan_ops, &p); + mmap_read_unlock(mm); + + if (!(!ret || ret == -ENOSPC)) + goto free_data; + + __start = __end; + if (IS_GET_OP(arg) && p.vec_index) { + if (copy_to_user(&vec[vec_index], p.vec, + p.vec_index * sizeof(struct page_region))) { + ret = -EFAULT; + goto free_data; + } + vec_index += p.vec_index; + } + } + ret = export_prev_to_out(&p, vec, &vec_index); + if (!ret) + ret = vec_index; +free_data: + if (IS_GET_OP(arg)) + kfree(p.vec); + + return ret; +} + +static long pagemap_scan_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{ + struct pagemap_scan_arg __user *uarg = (struct pagemap_scan_arg __user *)arg; + struct mm_struct *mm = file->private_data; + struct pagemap_scan_arg argument; + + if (cmd == PAGEMAP_SCAN) { + if (copy_from_user(&argument, uarg, sizeof(struct pagemap_scan_arg))) + return -EFAULT; + return do_pagemap_cmd(mm, &argument); + } + return -EINVAL; +} + const struct file_operations proc_pagemap_operations = { .llseek = mem_lseek, /* borrow this */ .read = pagemap_read, .open = pagemap_open, .release = pagemap_release, + .unlocked_ioctl = pagemap_scan_ioctl, + .compat_ioctl = pagemap_scan_ioctl, }; #endif /* CONFIG_PROC_PAGE_MONITOR */
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..6d03a903a745 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -305,4 +305,54 @@ typedef int __bitwise __kernel_rwf_t; #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\ RWF_APPEND)
+/* Pagemap ioctl */ +#define PAGEMAP_SCAN _IOWR('f', 16, struct pagemap_scan_arg) + +/* Bits are set in the bitmap of the page_region and masks in pagemap_scan_args */ +#define PAGE_IS_WT (1 << 0) +#define PAGE_IS_FILE (1 << 1) +#define PAGE_IS_PRESENT (1 << 2) +#define PAGE_IS_SWAPPED (1 << 3) + +/* + * struct page_region - Page region with bitmap flags + * @start: Start of the region + * @len: Length of the region + * bitmap: Bits sets for the region + */ +struct page_region { + __u64 start; + __u64 len; + __u64 bitmap; +}; + +/* + * struct pagemap_scan_arg - Pagemap ioctl argument + * @start: Starting address of the region + * @len: Length of the region (All the pages in this length are included) + * @vec: Address of page_region struct array for output + * @vec_len: Length of the page_region struct array + * @max_pages: Optional max return pages + * @flags: Flags for the IOCTL + * @required_mask: Required mask - All of these bits have to be set in the PTE + * @anyof_mask: Any mask - Any of these bits are set in the PTE + * @excluded_mask: Exclude mask - None of these bits are set in the PTE + * @return_mask: Bits that are to be reported in page_region + */ +struct pagemap_scan_arg { + __u64 start; + __u64 len; + __u64 vec; + __u64 vec_len; + __u32 max_pages; + __u32 flags; + __u64 required_mask; + __u64 anyof_mask; + __u64 excluded_mask; + __u64 return_mask; +}; + +/* Special flags */ +#define PAGEMAP_WP_ENGAGE (1 << 0) + #endif /* _UAPI_LINUX_FS_H */ diff --git a/tools/include/uapi/linux/fs.h b/tools/include/uapi/linux/fs.h index b7b56871029c..6d03a903a745 100644 --- a/tools/include/uapi/linux/fs.h +++ b/tools/include/uapi/linux/fs.h @@ -305,4 +305,54 @@ typedef int __bitwise __kernel_rwf_t; #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\ RWF_APPEND)
+/* Pagemap ioctl */ +#define PAGEMAP_SCAN _IOWR('f', 16, struct pagemap_scan_arg) + +/* Bits are set in the bitmap of the page_region and masks in pagemap_scan_args */ +#define PAGE_IS_WT (1 << 0) +#define PAGE_IS_FILE (1 << 1) +#define PAGE_IS_PRESENT (1 << 2) +#define PAGE_IS_SWAPPED (1 << 3) + +/* + * struct page_region - Page region with bitmap flags + * @start: Start of the region + * @len: Length of the region + * bitmap: Bits sets for the region + */ +struct page_region { + __u64 start; + __u64 len; + __u64 bitmap; +}; + +/* + * struct pagemap_scan_arg - Pagemap ioctl argument + * @start: Starting address of the region + * @len: Length of the region (All the pages in this length are included) + * @vec: Address of page_region struct array for output + * @vec_len: Length of the page_region struct array + * @max_pages: Optional max return pages + * @flags: Flags for the IOCTL + * @required_mask: Required mask - All of these bits have to be set in the PTE + * @anyof_mask: Any mask - Any of these bits are set in the PTE + * @excluded_mask: Exclude mask - None of these bits are set in the PTE + * @return_mask: Bits that are to be reported in page_region + */ +struct pagemap_scan_arg { + __u64 start; + __u64 len; + __u64 vec; + __u64 vec_len; + __u32 max_pages; + __u32 flags; + __u64 required_mask; + __u64 anyof_mask; + __u64 excluded_mask; + __u64 return_mask; +}; + +/* Special flags */ +#define PAGEMAP_WP_ENGAGE (1 << 0) + #endif /* _UAPI_LINUX_FS_H */
On Mon, Jan 09, 2023 at 11:45:18AM +0500, Muhammad Usama Anjum wrote:
This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear the info about page table entries. The following operations are supported in this ioctl:
- Get the information if the pages have been written-to (PAGE_IS_Wt), file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped (PAGE_IS_SWAPPED).
- Write-protect the pages (PAGEMAP_WP_ENGAGE) to start finding which pages have been written-to.
- Find pages which have been written-to and write protect the pages (atomic PAGE_IS_NOT_WP + PAGEMAP_WP_ENGAGE)
The uffd should have been registered on the memory range before performing any WP/WT (Write Protect/Writtern-To) related operations with the IOCTL.
struct pagemap_scan_args is used as the argument of the IOCTL. In this struct:
- The range is specified through start and len.
- The output buffer and size is specified as vec and vec_len.
- The optional maximum requested pages are specified in the max_pages.
- The flags can be specified in the flags field. The PAGEMAP_WP_ENGAGE is the only added flag at this time.
- The masks are specified in required_mask, anyof_mask, excluded_ mask and return_mask.
This IOCTL can be extended to get information about more PTE bits. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes in v7:
- Rebase on top of latest next
- Fix some corner cases
- Base soft-dirty on the uffd wp async
- Update the terminologies
- Optimize the memory usage inside the ioctl
Changes in v6:
- Rename variables and update comments
- Make IOCTL independent of soft_dirty config
- Change masks and bitmap type to _u64
- Improve code quality
Changes in v5:
- Remove tlb flushing even for clear operation
Changes in v4:
- Update the interface and implementation
Changes in v3:
- Tighten the user-kernel interface by using explicit types and add more error checking
Changes in v2:
- Convert the interface from syscall to ioctl
- Remove pidfd support as it doesn't make sense in ioctl
fs/proc/task_mmu.c | 300 ++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 ++++++ tools/include/uapi/linux/fs.h | 50 ++++++ 3 files changed, 400 insertions(+)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..ba70faadf403 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -19,6 +19,7 @@ #include <linux/shmem_fs.h> #include <linux/uaccess.h> #include <linux/pkeys.h> +#include <linux/minmax.h> #include <asm/elf.h> #include <asm/tlb.h> @@ -1135,6 +1136,22 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma, } #endif +static inline bool is_pte_uffd_wp(pte_t pte) +{
- if ((pte_present(pte) && pte_uffd_wp(pte)) ||
(is_swap_pte(pte) && pte_swp_uffd_wp(pte)))return true;
This is an interesting way to detect whether the page is written.. I would have thought you would reuse soft-dirty bit.
For swap case, you missed the pte markers (which can exist for !anon memory). You can have a look at pte_swp_uffd_wp_any().
- return false;
+}
+static inline bool is_pmd_uffd_wp(pmd_t pmd) +{
- if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) ||
(is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd)))return true;- return false;
+}
#if defined(CONFIG_MEM_SOFT_DIRTY) && defined(CONFIG_TRANSPARENT_HUGEPAGE) static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmdp) @@ -1763,11 +1780,294 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; } +#define PAGEMAP_OP_MASK (PAGE_IS_WT | PAGE_IS_FILE | \
PAGE_IS_PRESENT | PAGE_IS_SWAPPED)+#define PAGEMAP_NONWT_OP_MASK (PAGE_IS_FILE | PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define IS_WP_ENGAGE_OP(a) (a->flags & PAGEMAP_WP_ENGAGE) +#define IS_GET_OP(a) (a->vec) +#define PAGEMAP_SCAN_BITMAP(wt, file, present, swap) \
- (wt | file << 1 | present << 2 | swap << 3)
+#define IS_WT_REQUIRED(a) \
- ((a->required_mask & PAGE_IS_WT) || \
(a->anyof_mask & PAGE_IS_WT))+struct pagemap_scan_private {
- struct page_region *vec;
- struct page_region prev;
- unsigned long vec_len, vec_index;
- unsigned int max_pages, found_pages, flags;
- unsigned long required_mask, anyof_mask, excluded_mask, return_mask;
+};
+static int pagemap_scan_test_walk(unsigned long start, unsigned long end, struct mm_walk *walk) +{
- struct pagemap_scan_private *p = walk->private;
- struct vm_area_struct *vma = walk->vma;
- if (IS_WT_REQUIRED(p) && !vma_can_userfault(vma, vma->vm_flags))
return -EPERM;
vma_can_userfault() is too coarse. Maybe what you wanted to check is userfaultfd_wp(vma)?
- if (IS_GET_OP(p) && p->max_pages && (p->found_pages == p->max_pages))
return -ENOSPC;
This is the function to test "whether the walker should walk the vma specified". This check should IIUC be meaningless because found_pages doesn't boost during vma switching, while OTOH your pmd walker fn should do proper check when increasing found_pages and return -ENOSPC properly when the same condition met. That should be enough, IMHO.
I saw it already a few times in this patch, I think maybe you want a helper for this one taking *p as argument.
- if (vma->vm_flags & VM_PFNMAP)
return 1;- return 0;
+}
+static inline int add_to_out(bool wt, bool file, bool pres, bool swap,
struct pagemap_scan_private *p, unsigned long addr, unsigned int len)+{
- unsigned long bitmap, cur = PAGEMAP_SCAN_BITMAP(wt, file, pres, swap);
- bool cpy = true;
- struct page_region *prev = &p->prev;
- if (p->required_mask)
cpy = ((p->required_mask & cur) == p->required_mask);- if (cpy && p->anyof_mask)
cpy = (p->anyof_mask & cur);- if (cpy && p->excluded_mask)
cpy = !(p->excluded_mask & cur);- bitmap = cur & p->return_mask;
- if (cpy && bitmap) {
if ((prev->len) && (prev->bitmap == bitmap) &&(prev->start + prev->len * PAGE_SIZE == addr)) {prev->len += len;p->found_pages += len;} else if (p->vec_index < p->vec_len) {if (prev->len) {memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));p->vec_index++;}prev->start = addr;prev->len = len;prev->bitmap = bitmap;p->found_pages += len;} else {return -ENOSPC;}
[1]
- }
- return 0;
+}
+static inline int export_prev_to_out(struct pagemap_scan_private *p, struct page_region __user *vec,
unsigned long *vec_index)+{
- struct page_region *prev = &p->prev;
- if (prev->len) {
if (copy_to_user(&vec[*vec_index], prev, sizeof(struct page_region)))return -EFAULT;p->vec_index++;(*vec_index)++;prev->len = 0;- }
- return 0;
+}
I'd rather not have this helper; it doesn't really help a lot.
+static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long addr,
unsigned long end, struct mm_walk *walk)+{
- struct pagemap_scan_private *p = walk->private;
- struct vm_area_struct *vma = walk->vma;
- unsigned long start = addr;
- unsigned int len;
- spinlock_t *ptl;
- int ret = 0;
- pte_t *pte;
- bool pmd_wt;
- if ((walk->vma->vm_end < addr) || (p->max_pages && p->found_pages == p->max_pages))
return 0;- end = min(end, walk->vma->vm_end);
None of the check above is needed, I think..
vma ranges are checked before pmd_entry() called. found_pages should be checked when increased (I think it's [1] above).
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- ptl = pmd_trans_huge_lock(pmd, vma);
- if (ptl) {
pmd_wt = !is_pmd_uffd_wp(*pmd);/** Break huge page into small pages if operation needs to be performed is* on a portion of the huge page or the return buffer cannot store complete* data.*/if (pmd_wt && (IS_WP_ENGAGE_OP(p) && (end - addr < HPAGE_SIZE))) {spin_unlock(ptl);split_huge_pmd(vma, pmd, addr);goto process_smaller_pages;}if (IS_GET_OP(p)) {len = (end - addr)/PAGE_SIZE;if (p->max_pages && p->found_pages + len > p->max_pages)len = p->max_pages - p->found_pages;ret = add_to_out(pmd_wt, vma->vm_file, pmd_present(*pmd),is_swap_pmd(*pmd), p, addr, len);if (ret) {spin_unlock(ptl);return ret;}}spin_unlock(ptl);if (IS_WP_ENGAGE_OP(p) && pmd_wt) {BUG_ON(start & ~HPAGE_MASK);ret = wp_range_async(vma, addr, HPAGE_SIZE);}return ret;- }
+process_smaller_pages:
- if (pmd_trans_unstable(pmd))
return 0;+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
- pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
- for (; addr < end && !ret && (!p->max_pages || (p->found_pages < p->max_pages))
; pte++, addr += PAGE_SIZE) {if (IS_GET_OP(p)) {
This 'if' should be out - skip loop if not needed.
ret = add_to_out(!is_pte_uffd_wp(*pte), vma->vm_file, pte_present(*pte),is_swap_pte(*pte), p, addr, 1);if (ret)break;}- }
- pte_unmap_unlock(pte - 1, ptl);
- if (IS_WP_ENGAGE_OP(p)) {
BUG_ON(start & ~PAGE_MASK);ret = wp_range_async(vma, start, addr - start);- }
- cond_resched();
- return ret;
+}
+static int pagemap_scan_pte_hole(unsigned long addr, unsigned long end, int depth,
struct mm_walk *walk)+{
- struct pagemap_scan_private *p = walk->private;
- struct vm_area_struct *vma = walk->vma;
- unsigned int len;
- int ret = 0;
- if (vma) {
len = (end - addr)/PAGE_SIZE;if (p->max_pages && p->found_pages + len > p->max_pages)len = p->max_pages - p->found_pages;ret = add_to_out(false, vma->vm_file, false, false, p, addr, len);- }
- return ret;
+}
+static const struct mm_walk_ops pagemap_scan_ops = {
- .test_walk = pagemap_scan_test_walk,
- .pmd_entry = pagemap_scan_pmd_entry,
Do we care about hugetlb at all? So far it seems you don't. It's fine, then if you decided to go the uffd-wp way you can explicit declare no support on hugetlb, as uffd-wp sync supports it so it should be by default supported otherwise.
- .pte_hole = pagemap_scan_pte_hole,
+};
On 1/19/23 3:28 AM, Peter Xu wrote:
On Mon, Jan 09, 2023 at 11:45:18AM +0500, Muhammad Usama Anjum wrote:
This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear the info about page table entries. The following operations are supported in this ioctl:
- Get the information if the pages have been written-to (PAGE_IS_Wt), file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped (PAGE_IS_SWAPPED).
- Write-protect the pages (PAGEMAP_WP_ENGAGE) to start finding which pages have been written-to.
- Find pages which have been written-to and write protect the pages (atomic PAGE_IS_NOT_WP + PAGEMAP_WP_ENGAGE)
The uffd should have been registered on the memory range before performing any WP/WT (Write Protect/Writtern-To) related operations with the IOCTL.
struct pagemap_scan_args is used as the argument of the IOCTL. In this struct:
- The range is specified through start and len.
- The output buffer and size is specified as vec and vec_len.
- The optional maximum requested pages are specified in the max_pages.
- The flags can be specified in the flags field. The PAGEMAP_WP_ENGAGE is the only added flag at this time.
- The masks are specified in required_mask, anyof_mask, excluded_ mask and return_mask.
This IOCTL can be extended to get information about more PTE bits. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes in v7:
- Rebase on top of latest next
- Fix some corner cases
- Base soft-dirty on the uffd wp async
- Update the terminologies
- Optimize the memory usage inside the ioctl
Changes in v6:
- Rename variables and update comments
- Make IOCTL independent of soft_dirty config
- Change masks and bitmap type to _u64
- Improve code quality
Changes in v5:
- Remove tlb flushing even for clear operation
Changes in v4:
- Update the interface and implementation
Changes in v3:
- Tighten the user-kernel interface by using explicit types and add more error checking
Changes in v2:
- Convert the interface from syscall to ioctl
- Remove pidfd support as it doesn't make sense in ioctl
fs/proc/task_mmu.c | 300 ++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 ++++++ tools/include/uapi/linux/fs.h | 50 ++++++ 3 files changed, 400 insertions(+)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..ba70faadf403 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -19,6 +19,7 @@ #include <linux/shmem_fs.h> #include <linux/uaccess.h> #include <linux/pkeys.h> +#include <linux/minmax.h> #include <asm/elf.h> #include <asm/tlb.h> @@ -1135,6 +1136,22 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma, } #endif +static inline bool is_pte_uffd_wp(pte_t pte) +{
- if ((pte_present(pte) && pte_uffd_wp(pte)) ||
(is_swap_pte(pte) && pte_swp_uffd_wp(pte)))return true;This is an interesting way to detect whether the page is written.. I would have thought you would reuse soft-dirty bit.
For swap case, you missed the pte markers (which can exist for !anon memory). You can have a look at pte_swp_uffd_wp_any().
Thank you for mentioning it. I'll update in next version.
- return false;
+}
+static inline bool is_pmd_uffd_wp(pmd_t pmd) +{
- if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) ||
(is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd)))return true;- return false;
+}
#if defined(CONFIG_MEM_SOFT_DIRTY) && defined(CONFIG_TRANSPARENT_HUGEPAGE) static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmdp) @@ -1763,11 +1780,294 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; } +#define PAGEMAP_OP_MASK (PAGE_IS_WT | PAGE_IS_FILE | \
PAGE_IS_PRESENT | PAGE_IS_SWAPPED)+#define PAGEMAP_NONWT_OP_MASK (PAGE_IS_FILE | PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define IS_WP_ENGAGE_OP(a) (a->flags & PAGEMAP_WP_ENGAGE) +#define IS_GET_OP(a) (a->vec) +#define PAGEMAP_SCAN_BITMAP(wt, file, present, swap) \
- (wt | file << 1 | present << 2 | swap << 3)
+#define IS_WT_REQUIRED(a) \
- ((a->required_mask & PAGE_IS_WT) || \
(a->anyof_mask & PAGE_IS_WT))+struct pagemap_scan_private {
- struct page_region *vec;
- struct page_region prev;
- unsigned long vec_len, vec_index;
- unsigned int max_pages, found_pages, flags;
- unsigned long required_mask, anyof_mask, excluded_mask, return_mask;
+};
+static int pagemap_scan_test_walk(unsigned long start, unsigned long end, struct mm_walk *walk) +{
- struct pagemap_scan_private *p = walk->private;
- struct vm_area_struct *vma = walk->vma;
- if (IS_WT_REQUIRED(p) && !vma_can_userfault(vma, vma->vm_flags))
return -EPERM;vma_can_userfault() is too coarse. Maybe what you wanted to check is userfaultfd_wp(vma)?
- if (IS_GET_OP(p) && p->max_pages && (p->found_pages == p->max_pages))
return -ENOSPC;This is the function to test "whether the walker should walk the vma specified". This check should IIUC be meaningless because found_pages doesn't boost during vma switching, while OTOH your pmd walker fn should do proper check when increasing found_pages and return -ENOSPC properly when the same condition met. That should be enough, IMHO.
This check is needed in case we want to abort the walk at once. We return negative value from here which aborts the walk. Returning negative value from pmd_entry doesn't abort the walk. So this check is needed in the test_walk.
I saw it already a few times in this patch, I think maybe you want a helper for this one taking *p as argument.
- if (vma->vm_flags & VM_PFNMAP)
return 1;- return 0;
+}
+static inline int add_to_out(bool wt, bool file, bool pres, bool swap,
struct pagemap_scan_private *p, unsigned long addr, unsigned int len)+{
- unsigned long bitmap, cur = PAGEMAP_SCAN_BITMAP(wt, file, pres, swap);
- bool cpy = true;
- struct page_region *prev = &p->prev;
- if (p->required_mask)
cpy = ((p->required_mask & cur) == p->required_mask);- if (cpy && p->anyof_mask)
cpy = (p->anyof_mask & cur);- if (cpy && p->excluded_mask)
cpy = !(p->excluded_mask & cur);- bitmap = cur & p->return_mask;
- if (cpy && bitmap) {
if ((prev->len) && (prev->bitmap == bitmap) &&(prev->start + prev->len * PAGE_SIZE == addr)) {prev->len += len;p->found_pages += len;} else if (p->vec_index < p->vec_len) {if (prev->len) {memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));p->vec_index++;}prev->start = addr;prev->len = len;prev->bitmap = bitmap;p->found_pages += len;} else {return -ENOSPC;}[1]
- }
- return 0;
+}
+static inline int export_prev_to_out(struct pagemap_scan_private *p, struct page_region __user *vec,
unsigned long *vec_index)+{
- struct page_region *prev = &p->prev;
- if (prev->len) {
if (copy_to_user(&vec[*vec_index], prev, sizeof(struct page_region)))return -EFAULT;p->vec_index++;(*vec_index)++;prev->len = 0;- }
- return 0;
+}
I'd rather not have this helper; it doesn't really help a lot.
It just helps in keeping do_pagemap_cmd() clean. Please let me know if I should remove it in next revision.
+static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long addr,
unsigned long end, struct mm_walk *walk)+{
- struct pagemap_scan_private *p = walk->private;
- struct vm_area_struct *vma = walk->vma;
- unsigned long start = addr;
- unsigned int len;
- spinlock_t *ptl;
- int ret = 0;
- pte_t *pte;
- bool pmd_wt;
- if ((walk->vma->vm_end < addr) || (p->max_pages && p->found_pages == p->max_pages))
return 0;- end = min(end, walk->vma->vm_end);
None of the check above is needed, I think..
Sorry, it is residue from prevision revision as VMA was being split there. It'll be removed in next revision.
vma ranges are checked before pmd_entry() called. found_pages should be checked when increased (I think it's [1] above).
Okay. I'll do it.
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- ptl = pmd_trans_huge_lock(pmd, vma);
- if (ptl) {
pmd_wt = !is_pmd_uffd_wp(*pmd);/** Break huge page into small pages if operation needs to be performed is* on a portion of the huge page or the return buffer cannot store complete* data.*/if (pmd_wt && (IS_WP_ENGAGE_OP(p) && (end - addr < HPAGE_SIZE))) {spin_unlock(ptl);split_huge_pmd(vma, pmd, addr);goto process_smaller_pages;}if (IS_GET_OP(p)) {len = (end - addr)/PAGE_SIZE;if (p->max_pages && p->found_pages + len > p->max_pages)len = p->max_pages - p->found_pages;ret = add_to_out(pmd_wt, vma->vm_file, pmd_present(*pmd),is_swap_pmd(*pmd), p, addr, len);if (ret) {spin_unlock(ptl);return ret;}}spin_unlock(ptl);if (IS_WP_ENGAGE_OP(p) && pmd_wt) {BUG_ON(start & ~HPAGE_MASK);ret = wp_range_async(vma, addr, HPAGE_SIZE);}return ret;- }
+process_smaller_pages:
- if (pmd_trans_unstable(pmd))
return 0;+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
- pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
- for (; addr < end && !ret && (!p->max_pages || (p->found_pages < p->max_pages))
; pte++, addr += PAGE_SIZE) {if (IS_GET_OP(p)) {This 'if' should be out - skip loop if not needed.
Thanks for spotting it.
ret = add_to_out(!is_pte_uffd_wp(*pte), vma->vm_file, pte_present(*pte),is_swap_pte(*pte), p, addr, 1);if (ret)break;}- }
- pte_unmap_unlock(pte - 1, ptl);
- if (IS_WP_ENGAGE_OP(p)) {
BUG_ON(start & ~PAGE_MASK);ret = wp_range_async(vma, start, addr - start);- }
- cond_resched();
- return ret;
+}
+static int pagemap_scan_pte_hole(unsigned long addr, unsigned long end, int depth,
struct mm_walk *walk)+{
- struct pagemap_scan_private *p = walk->private;
- struct vm_area_struct *vma = walk->vma;
- unsigned int len;
- int ret = 0;
- if (vma) {
len = (end - addr)/PAGE_SIZE;if (p->max_pages && p->found_pages + len > p->max_pages)len = p->max_pages - p->found_pages;ret = add_to_out(false, vma->vm_file, false, false, p, addr, len);- }
- return ret;
+}
+static const struct mm_walk_ops pagemap_scan_ops = {
- .test_walk = pagemap_scan_test_walk,
- .pmd_entry = pagemap_scan_pmd_entry,
Do we care about hugetlb at all? So far it seems you don't. It's fine, then if you decided to go the uffd-wp way you can explicit declare no support on hugetlb, as uffd-wp sync supports it so it should be by default supported otherwise.
Okay. I'll mention in the commit message and in the comment here as well.
- .pte_hole = pagemap_scan_pte_hole,
+};
On Mon, Jan 23, 2023 at 05:18:13PM +0500, Muhammad Usama Anjum wrote:
- if (IS_GET_OP(p) && p->max_pages && (p->found_pages == p->max_pages))
return -ENOSPC;This is the function to test "whether the walker should walk the vma specified". This check should IIUC be meaningless because found_pages doesn't boost during vma switching, while OTOH your pmd walker fn should do proper check when increasing found_pages and return -ENOSPC properly when the same condition met. That should be enough, IMHO.
This check is needed in case we want to abort the walk at once. We return negative value from here which aborts the walk. Returning negative value from pmd_entry doesn't abort the walk. So this check is needed in the test_walk.
Why? What I see locally is (walk_pmd_range):
if (ops->pmd_entry) err = ops->pmd_entry(pmd, addr, next, walk); if (err) break;
Thanks,
On 1/24/23 10:30 PM, Peter Xu wrote:
On Mon, Jan 23, 2023 at 05:18:13PM +0500, Muhammad Usama Anjum wrote:
- if (IS_GET_OP(p) && p->max_pages && (p->found_pages == p->max_pages))
return -ENOSPC;This is the function to test "whether the walker should walk the vma specified". This check should IIUC be meaningless because found_pages doesn't boost during vma switching, while OTOH your pmd walker fn should do proper check when increasing found_pages and return -ENOSPC properly when the same condition met. That should be enough, IMHO.
This check is needed in case we want to abort the walk at once. We return negative value from here which aborts the walk. Returning negative value from pmd_entry doesn't abort the walk. So this check is needed in the test_walk.
Why? What I see locally is (walk_pmd_range):
if (ops->pmd_entry) err = ops->pmd_entry(pmd, addr, next, walk); if (err) break;
Sorry, mistake on my part. I'll correct it in next version (v9).
Thanks,
Add pagemap ioctl tests. Add several different types of tests to judge the correction of the interface.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- Chages in v7: - Add and update all test cases
Changes in v6: - Rename variables
Changes in v4: - Updated all the tests to conform to new IOCTL
Changes in v3: - Add another test to do sanity of flags
Changes in v2: - Update the tests to use the ioctl interface instead of syscall
TAP version 13 1..54 ok 1 sanity_tests_sd wrong flag specified ok 2 sanity_tests_sd wrong mask specified ok 3 sanity_tests_sd wrong return mask specified ok 4 sanity_tests_sd mixture of correct and wrong flag ok 5 sanity_tests_sd Clear area with larger vec size ok 6 sanity_tests_sd Repeated pattern of dirty and non-dirty pages ok 7 sanity_tests_sd Repeated pattern of dirty and non-dirty pages in parts ok 8 sanity_tests_sd Two regions ok 9 Page testing: all new pages must be soft dirty ok 10 Page testing: all pages must not be soft dirty ok 11 Page testing: all pages dirty other than first and the last one ok 12 Page testing: only middle page dirty ok 13 Page testing: only two middle pages dirty ok 14 Page testing: only get 2 dirty pages and clear them as well ok 15 Page testing: Range clear only ok 16 Large Page testing: all new pages must be soft dirty ok 17 Large Page testing: all pages must not be soft dirty ok 18 Large Page testing: all pages dirty other than first and the last one ok 19 Large Page testing: only middle page dirty ok 20 Large Page testing: only two middle pages dirty ok 21 Large Page testing: only get 2 dirty pages and clear them as well ok 22 Large Page testing: Range clear only ok 23 Huge page testing: all new pages must be soft dirty ok 24 Huge page testing: all pages must not be soft dirty ok 25 Huge page testing: all pages dirty other than first and the last one ok 26 Huge page testing: only middle page dirty ok 27 Huge page testing: only two middle pages dirty ok 28 Huge page testing: only get 2 dirty pages and clear them as well ok 29 Huge page testing: Range clear only ok 30 hpage_unit_tests all new huge page must be dirty ok 31 hpage_unit_tests all the huge page must not be dirty ok 32 hpage_unit_tests all the huge page must be dirty and clear ok 33 hpage_unit_tests only middle page dirty ok 34 hpage_unit_tests clear first half of huge page ok 35 hpage_unit_tests clear first half of huge page with limited buffer ok 36 hpage_unit_tests clear second half huge page ok 37 Test test_simple ok 38 mprotect_tests Both pages dirty ok 39 mprotect_tests Both pages are not soft dirty ok 40 mprotect_tests Both pages dirty after remap and mprotect ok 41 mprotect_tests Clear and make the pages dirty ok 42 sanity_tests clear op can only be specified with PAGE_IS_WT ok 43 sanity_tests required_mask specified ok 44 sanity_tests anyof_mask specified ok 45 sanity_tests excluded_mask specified ok 46 sanity_tests required_mask and anyof_mask specified ok 47 sanity_tests Get sd and present pages with anyof_mask ok 48 sanity_tests Get all the pages with required_mask ok 49 sanity_tests Get sd and present pages with required_mask and anyof_mask ok 50 sanity_tests Don't get sd pages ok 51 sanity_tests Don't get present pages ok 52 sanity_tests Find dirty present pages with return mask ok 53 sanity_tests Memory mapped file ok 54 unmapped_region_tests Get status of pages # Totals: pass:54 fail:0 xfail:0 xpass:0 skip:0 error:0 --- tools/testing/selftests/vm/.gitignore | 1 + tools/testing/selftests/vm/Makefile | 5 +- tools/testing/selftests/vm/pagemap_ioctl.c | 884 +++++++++++++++++++++ 3 files changed, 888 insertions(+), 2 deletions(-) create mode 100644 tools/testing/selftests/vm/pagemap_ioctl.c
diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore index 1f8c36a9fa10..9e7e0ae26582 100644 --- a/tools/testing/selftests/vm/.gitignore +++ b/tools/testing/selftests/vm/.gitignore @@ -17,6 +17,7 @@ mremap_dontunmap mremap_test on-fault-limit transhuge-stress +pagemap_ioctl protection_keys protection_keys_32 protection_keys_64 diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile index 89c14e41bd43..54c074440a1b 100644 --- a/tools/testing/selftests/vm/Makefile +++ b/tools/testing/selftests/vm/Makefile @@ -24,9 +24,8 @@ MACHINE ?= $(shell echo $(uname_M) | sed -e 's/aarch64.*/arm64/' -e 's/ppc64.*/p # things despite using incorrect values such as an *occasionally* incomplete # LDLIBS. MAKEFLAGS += --no-builtin-rules - CFLAGS = -Wall -I $(top_srcdir) -I $(top_srcdir)/usr/include $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -LDLIBS = -lrt -lpthread +LDLIBS = -lrt -lpthread -lm TEST_GEN_FILES = cow TEST_GEN_FILES += compaction_test TEST_GEN_FILES += gup_test @@ -52,6 +51,7 @@ TEST_GEN_FILES += on-fault-limit TEST_GEN_FILES += thuge-gen TEST_GEN_FILES += transhuge-stress TEST_GEN_FILES += userfaultfd +TEST_GEN_PROGS += pagemap_ioctl TEST_GEN_PROGS += soft-dirty TEST_GEN_PROGS += split_huge_page_test TEST_GEN_FILES += ksm_tests @@ -103,6 +103,7 @@ $(OUTPUT)/cow: vm_util.c $(OUTPUT)/khugepaged: vm_util.c $(OUTPUT)/ksm_functional_tests: vm_util.c $(OUTPUT)/madv_populate: vm_util.c +$(OUTPUT)/pagemap_ioctl: vm_util.c $(OUTPUT)/soft-dirty: vm_util.c $(OUTPUT)/split_huge_page_test: vm_util.c $(OUTPUT)/userfaultfd: vm_util.c diff --git a/tools/testing/selftests/vm/pagemap_ioctl.c b/tools/testing/selftests/vm/pagemap_ioctl.c new file mode 100644 index 000000000000..668faa042e76 --- /dev/null +++ b/tools/testing/selftests/vm/pagemap_ioctl.c @@ -0,0 +1,884 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <stdio.h> +#include <fcntl.h> +#include <string.h> +#include <sys/mman.h> +#include <errno.h> +#include <malloc.h> +#include "vm_util.h" +#include "../kselftest.h" +#include <linux/types.h> +#include <linux/userfaultfd.h> +#include <linux/fs.h> +#include <sys/ioctl.h> +#include <sys/stat.h> +#include <math.h> +#include <asm/unistd.h> + +#define PAGEMAP_OP_MASK (PAGE_IS_WT | PAGE_IS_FILE | \ + PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define PAGEMAP_NON_WT_MASK (PAGE_IS_FILE | PAGE_IS_PRESENT | \ + PAGE_IS_SWAPPED) + +#define TEST_ITERATIONS 10 +#define PAGEMAP "/proc/self/pagemap" +int pagemap_fd; +int uffd; +int page_size; +int hpage_size; + +static long pagemap_ioctl(void *start, int len, void *vec, int vec_len, int flag, + int max_pages, long required_mask, long anyof_mask, long excluded_mask, + long return_mask) +{ + struct pagemap_scan_arg arg; + int ret; + + arg.start = (uintptr_t)start; + arg.len = len; + arg.vec = (uintptr_t)vec; + arg.vec_len = vec_len; + arg.flags = flag; + arg.max_pages = max_pages; + arg.required_mask = required_mask; + arg.anyof_mask = anyof_mask; + arg.excluded_mask = excluded_mask; + arg.return_mask = return_mask; + + ret = ioctl(pagemap_fd, PAGEMAP_SCAN, &arg); + + return ret; +} + +int init_uffd(void) +{ + struct uffdio_api uffdio_api; + + uffd = syscall(323, O_CLOEXEC | O_NONBLOCK); + if (uffd == -1) + ksft_exit_fail_msg("uffd syscall failed\n"); + + uffdio_api.api = UFFD_API; + uffdio_api.features = 0; + if (ioctl(uffd, UFFDIO_API, &uffdio_api)) + ksft_exit_fail_msg("UFFDIO_API\n"); + + if (uffdio_api.api != UFFD_API) + ksft_exit_fail_msg("UFFDIO_API error %llu\n", uffdio_api.api); + + return 0; +} + +int wp_init(void *lpBaseAddress, int dwRegionSize) +{ + struct uffdio_register uffdio_register; + struct uffdio_writeprotect wp; + + /* TODO: can it be avoided? Write protect doesn't engage on the pages if they aren't + * present already. The pages can be made present by writing to them. + */ + memset(lpBaseAddress, -1, dwRegionSize); + + uffdio_register.range.start = (unsigned long)lpBaseAddress; + uffdio_register.range.len = dwRegionSize; + uffdio_register.mode = UFFDIO_REGISTER_MODE_WP; + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) + ksft_exit_fail_msg("ioctl(UFFDIO_REGISTER)\n"); + + if (!(uffdio_register.ioctls & UFFDIO_WRITEPROTECT)) + ksft_exit_fail_msg("ioctl set is incorrect\n"); + + if (rand() % 2) { + wp.range.start = (unsigned long)lpBaseAddress; + wp.range.len = dwRegionSize; + wp.mode = UFFDIO_WRITEPROTECT_MODE_ASYNC_WP; + + if (ioctl(uffd, UFFDIO_WRITEPROTECT, &wp) == -1) + ksft_exit_fail_msg("ioctl(UFFDIO_WRITEPROTECT)\n"); + } else { + if (pagemap_ioctl(lpBaseAddress, dwRegionSize, NULL, 0, PAGEMAP_WP_ENGAGE, 0, + 0, 0, 0, 0) < 0) + ksft_exit_fail_msg("error %d %d %s\n", 1, errno, strerror(errno)); + } + return 0; +} + +int wp_free(void *lpBaseAddress, int dwRegionSize) +{ + struct uffdio_register uffdio_register; + + uffdio_register.range.start = (unsigned long)lpBaseAddress; + uffdio_register.range.len = dwRegionSize; + uffdio_register.mode = UFFDIO_REGISTER_MODE_WP; + if (ioctl(uffd, UFFDIO_UNREGISTER, &uffdio_register.range)) + ksft_exit_fail_msg("ioctl unregister failure\n"); + return 0; +} + +int clear_softdirty_wp(void *lpBaseAddress, int dwRegionSize) +{ + struct uffdio_writeprotect wp; + + if (rand() % 2) { + wp.range.start = (unsigned long)lpBaseAddress; + wp.range.len = dwRegionSize; + wp.mode = UFFDIO_WRITEPROTECT_MODE_ASYNC_WP; + + if (ioctl(uffd, UFFDIO_WRITEPROTECT, &wp) == -1) + ksft_exit_fail_msg("ioctl(UFFDIO_WRITEPROTECT)\n"); + } else { + if (pagemap_ioctl(lpBaseAddress, dwRegionSize, NULL, 0, PAGEMAP_WP_ENGAGE, 0, + 0, 0, 0, 0) < 0) + ksft_exit_fail_msg("error %d %d %s\n", 1, errno, strerror(errno)); + } + return 0; +} + +int sanity_tests_sd(void) +{ + char *mem, *m[2]; + int mem_size, vec_size, ret, ret2, ret3, i, num_pages = 10; + struct page_region *vec; + + vec_size = 100; + mem_size = num_pages * page_size; + + vec = malloc(sizeof(struct page_region) * vec_size); + if (!vec) + ksft_exit_fail_msg("error nomem\n"); + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(mem, mem_size); + + /* 1. wrong operation */ + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, -1, + 0, PAGE_IS_WT, 0, 0, PAGE_IS_WT) < 0, + "%s wrong flag specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, 8, + 0, 0x1111, 0, 0, PAGE_IS_WT) < 0, + "%s wrong mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, 0, + 0, PAGE_IS_WT, 0, 0, 0x1000) < 0, + "%s wrong return mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, + PAGEMAP_WP_ENGAGE | 0x32, + 0, PAGE_IS_WT, 0, 0, PAGE_IS_WT) < 0, + "%s mixture of correct and wrong flag\n", __func__); + + /* 2. Clear area with larger vec size */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PAGEMAP_WP_ENGAGE, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + ksft_test_result(ret >= 0, "%s Clear area with larger vec size\n", __func__); + + /* 3. Repeated pattern of dirty and non-dirty pages */ + for (i = 0; i < mem_size; i += 2 * page_size) + mem[i]++; + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, PAGE_IS_WT, 0, 0, + PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == mem_size/(page_size * 2), + "%s Repeated pattern of dirty and non-dirty pages\n", __func__); + + /* 4. Repeated pattern of dirty and non-dirty pages in parts */ + ret = pagemap_ioctl(mem, mem_size, vec, num_pages/5, PAGEMAP_WP_ENGAGE, + num_pages/2 - 2, PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret2 = pagemap_ioctl(mem, mem_size, vec, 2, 0, 0, PAGE_IS_WT, 0, 0, + PAGE_IS_WT); + if (ret2 < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret2, errno, strerror(errno)); + + ret3 = pagemap_ioctl(mem, mem_size, vec, num_pages/2, 0, 0, PAGE_IS_WT, 0, 0, + PAGE_IS_WT); + if (ret3 < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret3, errno, strerror(errno)); + + ksft_test_result((ret + ret3) == num_pages/2 && ret2 == 2, + "%s Repeated pattern of dirty and non-dirty pages in parts\n", __func__); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 5. Two regions */ + m[0] = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (m[0] == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + m[1] = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (m[1] == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(m[0], mem_size); + wp_init(m[1], mem_size); + + memset(m[0], 'a', mem_size); + memset(m[1], 'b', mem_size); + + ret = pagemap_ioctl(m[0], mem_size, NULL, 0, PAGEMAP_WP_ENGAGE, 0, + 0, 0, 0, 0); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret = pagemap_ioctl(m[1], mem_size, vec, 1, 0, 0, PAGE_IS_WT, 0, 0, + PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == mem_size/page_size, + "%s Two regions\n", __func__); + + wp_free(m[0], mem_size); + wp_free(m[1], mem_size); + munmap(m[0], mem_size); + munmap(m[1], mem_size); + + free(vec); + return 0; +} + +int base_tests(char *prefix, char *mem, int mem_size, int skip) +{ + int vec_size, ret, dirty, dirty2; + struct page_region *vec, *vec2; + + if (skip) { + ksft_test_result_skip("%s all new pages must be soft dirty\n", prefix); + ksft_test_result_skip("%s all pages must not be soft dirty\n", prefix); + ksft_test_result_skip("%s all pages dirty other than first and the last one\n", + prefix); + ksft_test_result_skip("%s only middle page dirty\n", prefix); + ksft_test_result_skip("%s only two middle pages dirty\n", prefix); + ksft_test_result_skip("%s only get 2 dirty pages and clear them as well\n", prefix); + ksft_test_result_skip("%s Range clear only\n", prefix); + return 0; + } + + vec_size = mem_size/page_size; + vec = malloc(sizeof(struct page_region) * vec_size); + vec2 = malloc(sizeof(struct page_region) * vec_size); + + /* 1. all new pages must be not be soft dirty */ + dirty = pagemap_ioctl(mem, mem_size, vec, 1, PAGEMAP_WP_ENGAGE, vec_size - 2, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + dirty2 = pagemap_ioctl(mem, mem_size, vec2, 1, PAGEMAP_WP_ENGAGE, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (dirty2 < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty2, errno, strerror(errno)); + + ksft_test_result(dirty == 0 && dirty2 == 0, + "%s all new pages must be soft dirty\n", prefix); + + /* 2. all pages must not be soft dirty */ + dirty = pagemap_ioctl(mem, mem_size, vec, 1, 0, 0, PAGE_IS_WT, 0, 0, + PAGE_IS_WT); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + ksft_test_result(dirty == 0, "%s all pages must not be soft dirty\n", prefix); + + /* 3. all pages dirty other than first and the last one */ + memset(mem + page_size, 0, mem_size - (2 * page_size)); + + dirty = pagemap_ioctl(mem, mem_size, vec, 1, 0, 0, PAGE_IS_WT, 0, 0, + PAGE_IS_WT); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + ksft_test_result(dirty == 1 && vec[0].len >= vec_size - 2 && vec[0].len <= vec_size, + "%s all pages dirty other than first and the last one\n", prefix); + + /* 4. only middle page dirty */ + clear_softdirty_wp(mem, mem_size); + mem[vec_size/2 * page_size]++; + + dirty = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, PAGE_IS_WT, 0, 0, + PAGE_IS_WT); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + if (check_huge_anon(mem, mem_size/hpage_size, hpage_size)) + ksft_test_result(dirty == 1 && vec[0].len >= 1, + "%s only middle page dirty\n", prefix); + else + ksft_test_result(vec[0].start == (uintptr_t)(mem + vec_size/2 * page_size), + "%s only middle page dirty\n", prefix); + + /* 5. only two middle pages dirty and walk over only middle pages */ + clear_softdirty_wp(mem, mem_size); + mem[vec_size/2 * page_size]++; + mem[(vec_size/2 + 1) * page_size]++; + + dirty = pagemap_ioctl(&mem[vec_size/2 * page_size], 2 * page_size, vec, 1, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + ksft_test_result(dirty == 1 && vec[0].start == (uintptr_t)(&mem[vec_size/2 * page_size]) && + vec[0].len == 2, + "%s only two middle pages dirty\n", prefix); + + /* 6. only get 2 dirty pages and clear them as well */ + memset(mem, -1, mem_size); + + /* get and clear second and third pages */ + ret = pagemap_ioctl(mem + page_size, 2 * page_size, vec, 1, PAGEMAP_WP_ENGAGE, + 2, PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + dirty = pagemap_ioctl(mem, mem_size, vec2, vec_size, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == 2 && + vec[0].start == (uintptr_t)(mem + page_size) && + dirty == 2 && vec2[0].len == 1 && vec2[0].start == (uintptr_t)mem && + vec2[1].len == vec_size - 3 && + vec2[1].start == (uintptr_t)(mem + 3 * page_size), + "%s only get 2 dirty pages and clear them as well\n", prefix); + + /* 7. Range clear only */ + memset(mem, -1, mem_size); + + dirty = pagemap_ioctl(mem, mem_size, NULL, 0, PAGEMAP_WP_ENGAGE, 0, + 0, 0, 0, 0); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + dirty2 = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (dirty2 < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty2, errno, strerror(errno)); + + ksft_test_result(dirty == 0 && dirty2 == 0, "%s Range clear only\n", + prefix); + + free(vec); + free(vec2); + return 0; +} + +void *gethugepage(int map_size) +{ + int ret; + char *map; + + map = memalign(hpage_size, map_size); + if (!map) + ksft_exit_fail_msg("memalign failed %d %s\n", errno, strerror(errno)); + + ret = madvise(map, map_size, MADV_HUGEPAGE); + if (ret) + ksft_exit_fail_msg("madvise failed %d %d %s\n", ret, errno, strerror(errno)); + + wp_init(map, map_size); + + if (check_huge_anon(map, map_size/hpage_size, hpage_size)) + return map; + + free(map); + return NULL; + +} + +int hpage_unit_tests(void) +{ + char *map; + int ret; + size_t num_pages = 10; + int map_size = hpage_size * num_pages; + int vec_size = map_size/page_size; + struct page_region *vec, *vec2; + + vec = malloc(sizeof(struct page_region) * vec_size); + vec2 = malloc(sizeof(struct page_region) * vec_size); + if (!vec || !vec2) + ksft_exit_fail_msg("malloc failed\n"); + + map = gethugepage(map_size); + if (map) { + /* 1. all new huge page must not be dirty */ + ret = pagemap_ioctl(map, map_size, vec, vec_size, PAGEMAP_WP_ENGAGE, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 0, "%s all new huge page must be dirty\n", __func__); + + /* 2. all the huge page must not be dirty */ + ret = pagemap_ioctl(map, map_size, vec, vec_size, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 0, "%s all the huge page must not be dirty\n", __func__); + + /* 3. all the huge page must be dirty and clear dirty as well */ + memset(map, -1, map_size); + ret = pagemap_ioctl(map, map_size, vec, vec_size, PAGEMAP_WP_ENGAGE, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].start == (uintptr_t)map && + vec[0].len == vec_size && vec[0].bitmap == PAGE_IS_WT, + "%s all the huge page must be dirty and clear\n", __func__); + + /* 4. only middle page dirty */ + wp_free(map, map_size); + free(map); + map = gethugepage(map_size); + wp_init(map, map_size); + clear_softdirty_wp(map, map_size); + map[vec_size/2 * page_size]++; + + ret = pagemap_ioctl(map, map_size, vec, vec_size, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len > 0, + "%s only middle page dirty\n", __func__); + + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("all new huge page must be dirty\n"); + ksft_test_result_skip("all the huge page must not be dirty\n"); + ksft_test_result_skip("all the huge page must be dirty and clear\n"); + ksft_test_result_skip("only middle page dirty\n"); + } + + /* 5. clear first half of huge page */ + map = gethugepage(map_size); + if (map) { + + memset(map, 0, map_size); + + ret = pagemap_ioctl(map, map_size/2, NULL, 0, PAGEMAP_WP_ENGAGE, 0, + 0, 0, 0, 0); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == vec_size/2 && + vec[0].start == (uintptr_t)(map + map_size/2), + "%s clear first half of huge page\n", __func__); + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("clear first half of huge page\n"); + } + + /* 6. clear first half of huge page with limited buffer */ + map = gethugepage(map_size); + if (map) { + memset(map, 0, map_size); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, PAGEMAP_WP_ENGAGE, + vec_size/2, PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == vec_size/2 && + vec[0].start == (uintptr_t)(map + map_size/2), + "%s clear first half of huge page with limited buffer\n", + __func__); + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("clear first half of huge page with limited buffer\n"); + } + + /* 7. clear second half of huge page */ + map = gethugepage(map_size); + if (map) { + memset(map, -1, map_size); + ret = pagemap_ioctl(map + map_size/2, map_size/2, NULL, 0, PAGEMAP_WP_ENGAGE, + 0, 0, 0, 0, 0); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == vec_size/2, + "%s clear second half huge page\n", __func__); + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("clear second half huge page\n"); + } + + free(vec); + free(vec2); + return 0; +} + +int unmapped_region_tests(void) +{ + void *start = (void *)0x10000000; + int dirty, len = 0x00040000; + int vec_size = len / page_size; + struct page_region *vec = malloc(sizeof(struct page_region) * vec_size); + + /* 1. Get dirty pages */ + dirty = pagemap_ioctl(start, len, vec, vec_size, 0, 0, PAGEMAP_NON_WT_MASK, 0, 0, + PAGEMAP_NON_WT_MASK); + if (dirty < 0) + ksft_exit_fail_msg("error %d %d %s\n", dirty, errno, strerror(errno)); + + ksft_test_result(dirty >= 0, "%s Get status of pages\n", __func__); + + free(vec); + return 0; +} + +static void test_simple(void) +{ + int i; + char *map; + struct page_region vec; + + map = aligned_alloc(page_size, page_size); + if (!map) + ksft_exit_fail_msg("aligned_alloc failed\n"); + wp_init(map, page_size); + + clear_softdirty_wp(map, page_size); + + for (i = 0 ; i < TEST_ITERATIONS; i++) { + if (pagemap_ioctl(map, page_size, &vec, 1, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT) == 1) { + ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i); + break; + } + + clear_softdirty_wp(map, page_size); + /* Write something to the page to get the dirty bit enabled on the page */ + map[0]++; + + if (pagemap_ioctl(map, page_size, &vec, 1, 0, 0, + PAGE_IS_WT, 0, 0, PAGE_IS_WT) == 0) { + ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i); + break; + } + + clear_softdirty_wp(map, page_size); + } + wp_free(map, page_size); + free(map); + + ksft_test_result(i == TEST_ITERATIONS, "Test %s\n", __func__); +} + +int sanity_tests(void) +{ + char *mem, *fmem; + int mem_size, vec_size, ret; + struct page_region *vec; + + /* 1. wrong operation */ + mem_size = 10 * page_size; + vec_size = mem_size / page_size; + + vec = malloc(sizeof(struct page_region) * vec_size); + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED || vec == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(mem, mem_size); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PAGEMAP_WP_ENGAGE, 0, + PAGEMAP_OP_MASK, 0, 0, PAGEMAP_OP_MASK) < 0, + "%s clear op can only be specified with PAGE_IS_WT\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + PAGEMAP_OP_MASK, 0, 0, PAGEMAP_OP_MASK) >= 0, + "%s required_mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + 0, PAGEMAP_OP_MASK, 0, PAGEMAP_OP_MASK) >= 0, + "%s anyof_mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + 0, 0, PAGEMAP_OP_MASK, PAGEMAP_OP_MASK) >= 0, + "%s excluded_mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + PAGEMAP_OP_MASK, PAGEMAP_OP_MASK, 0, PAGEMAP_OP_MASK) >= 0, + "%s required_mask and anyof_mask specified\n", __func__); + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 2. Get sd and present pages with anyof_mask */ + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + + memset(mem, 0, mem_size); + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + 0, PAGEMAP_OP_MASK, 0, PAGEMAP_OP_MASK); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == (PAGE_IS_WT | PAGE_IS_PRESENT), + "%s Get sd and present pages with anyof_mask\n", __func__); + + /* 3. Get sd and present pages with required_mask */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + PAGEMAP_OP_MASK, 0, 0, PAGEMAP_OP_MASK); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == (PAGE_IS_WT | PAGE_IS_PRESENT), + "%s Get all the pages with required_mask\n", __func__); + + /* 4. Get sd and present pages with required_mask and anyof_mask */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + PAGE_IS_WT, PAGE_IS_PRESENT, 0, PAGEMAP_OP_MASK); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == (PAGE_IS_WT | PAGE_IS_PRESENT), + "%s Get sd and present pages with required_mask and anyof_mask\n", + __func__); + + /* 5. Don't get sd pages */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + 0, 0, PAGE_IS_WT, PAGEMAP_OP_MASK); + ksft_test_result(ret == 0, "%s Don't get sd pages\n", __func__); + + /* 6. Don't get present pages */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + 0, 0, PAGE_IS_PRESENT, PAGEMAP_OP_MASK); + ksft_test_result(ret == 0, "%s Don't get present pages\n", __func__); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 8. Find dirty present pages with return mask */ + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + + memset(mem, 0, mem_size); + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, 0, 0, + 0, PAGEMAP_OP_MASK, 0, PAGE_IS_WT); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == PAGE_IS_WT, + "%s Find dirty present pages with return mask\n", __func__); + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 9. Memory mapped file */ + int fd; + struct stat sbuf; + + fd = open(__FILE__, O_RDONLY); + if (fd < 0) { + ksft_test_result_skip("%s Memory mapped file\n"); + goto free_vec_and_return; + } + + ret = stat(__FILE__, &sbuf); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + fmem = mmap(NULL, sbuf.st_size, PROT_READ, MAP_SHARED, fd, 0); + if (fmem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + ret = pagemap_ioctl(fmem, sbuf.st_size, vec, vec_size, 0, 0, + 0, PAGEMAP_NON_WT_MASK, 0, PAGEMAP_NON_WT_MASK); + + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)fmem && + vec[0].len == ceilf((float)sbuf.st_size/page_size) && + vec[0].bitmap == PAGE_IS_FILE, + "%s Memory mapped file\n", __func__); + + munmap(fmem, sbuf.st_size); + close(fd); + +free_vec_and_return: + free(vec); + return 0; +} + +int mprotect_tests(void) +{ + int ret; + char *mem, *mem2; + struct page_region vec; + int pagemap_fd = open("/proc/self/pagemap", O_RDONLY); + + if (pagemap_fd < 0) { + fprintf(stderr, "open() failed\n"); + exit(1); + } + + /* 1. Map two pages */ + mem = mmap(0, 2 * page_size, PROT_READ|PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, 2 * page_size); + + /* Populate both pages. */ + memset(mem, 1, 2 * page_size); + + ret = pagemap_ioctl(mem, 2 * page_size, &vec, 1, 0, 0, PAGE_IS_WT, + 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec.len == 2, "%s Both pages dirty\n", __func__); + + /* 2. Start softdirty tracking. Clear VM_SOFTDIRTY and clear the softdirty PTE bit. */ + ret = pagemap_ioctl(mem, 2 * page_size, NULL, 0, PAGEMAP_WP_ENGAGE, 0, + 0, 0, 0, 0); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(pagemap_ioctl(mem, 2 * page_size, &vec, 1, 0, 0, PAGE_IS_WT, + 0, 0, PAGE_IS_WT) == 0, + "%s Both pages are not soft dirty\n", __func__); + + /* 3. Remap the second page */ + mem2 = mmap(mem + page_size, page_size, PROT_READ|PROT_WRITE, + MAP_PRIVATE|MAP_ANON|MAP_FIXED, -1, 0); + if (mem2 == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem2, page_size); + + /* Protect + unprotect. */ + mprotect(mem, 2 * page_size, PROT_READ); + mprotect(mem, 2 * page_size, PROT_READ|PROT_WRITE); + + /* Modify both pages. */ + memset(mem, 2, 2 * page_size); + + ret = pagemap_ioctl(mem, 2 * page_size, &vec, 1, 0, 0, PAGE_IS_WT, + 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec.len == 2, + "%s Both pages dirty after remap and mprotect\n", __func__); + + /* 4. Clear and make the pages dirty */ + ret = pagemap_ioctl(mem, 2 * page_size, NULL, 0, PAGEMAP_WP_ENGAGE, 0, + 0, 0, 0, 0); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + memset(mem, 'A', 2 * page_size); + + ret = pagemap_ioctl(mem, 2 * page_size, &vec, 1, 0, 0, PAGE_IS_WT, + 0, 0, PAGE_IS_WT); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec.len == 2, + "%s Clear and make the pages dirty\n", __func__); + + wp_free(mem, 2 * page_size); + munmap(mem, 2 * page_size); + return 0; +} + +int main(void) +{ + char *mem, *map; + int mem_size; + + ksft_print_header(); + ksft_set_plan(54); + + page_size = getpagesize(); + hpage_size = read_pmd_pagesize(); + + pagemap_fd = open(PAGEMAP, O_RDWR); + if (pagemap_fd < 0) + return -EINVAL; + + if (init_uffd()) + ksft_exit_fail_msg("uffd init failed\n"); + + /* + * Soft-dirty PTE bit tests + */ + + /* 1. Sanity testing */ + sanity_tests_sd(); + + /* 2. Normal page testing */ + mem_size = 10 * page_size; + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + + base_tests("Page testing:", mem, mem_size, 0); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 3. Large page testing */ + mem_size = 512 * 10 * page_size; + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + + base_tests("Large Page testing:", mem, mem_size, 0); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 4. Huge page testing */ + map = gethugepage(hpage_size); + if (map) + base_tests("Huge page testing:", map, hpage_size, 0); + else + base_tests("Huge page testing:", NULL, 0, 1); + + wp_free(map, hpage_size); + free(map); + + /* 6. Huge page tests */ + hpage_unit_tests(); + + /* 7. Iterative test */ + test_simple(); + + /* 8. Mprotect test */ + mprotect_tests(); + + /* + * Other PTE bit tests + */ + + /* 1. Sanity testing */ + sanity_tests(); + + /* 2. Unmapped address test */ + unmapped_region_tests(); + + close(pagemap_fd); + return ksft_exit_pass(); +}
On 1/9/23 11:45 AM, Muhammad Usama Anjum wrote:
*Changes in v7:*
- Add uffd wp async
- Update the IOCTL to use uffd under the hood instead of soft-dirty flags
Stop using the soft-dirty flags for finding which pages have been written to. It is too delicate and wrong as it shows more soft-dirty pages than the actual soft-dirty pages. There is no interest in correcting it [A][B] as this is how the feature was written years ago. It shouldn't be updated to changed behaviour. Peter Xu has suggested using the async version of the UFFD WP [C] as it is based inherently on the PTEs.
So in this patch series, I've added a new mode to the UFFD which is asynchronous version of the write protect. When this variant of the UFFD WP is used, the page faults are resolved automatically by the kernel. The pages which have been written-to can be found by reading pagemap file (!PM_UFFD_WP). This feature can be used successfully to find which pages have been written to from the time the pages were write protected. This works just like the soft-dirty flag without showing any extra pages which aren't soft-dirty in reality.
Any thoughts on this version are highly welcome. Please review.
[A] https://lore.kernel.org/all/20221220162606.1595355-1-usama.anjum@collabora.c... [B] https://lore.kernel.org/all/20221122115007.2787017-1-usama.anjum@collabora.c... [C] https://lore.kernel.org/all/Y6Hc2d+7eTKs7AiH@x1n
*Changes in v6:*
- Updated the interface and made cosmetic changes
*Cover Letter in v5:* Hello,
This patch series implements IOCTL on the pagemap procfs file to get the information about the page table entries (PTEs). The following operations are supported in this ioctl:
- Get the information if the pages are soft-dirty, file mapped, present or swapped.
- Clear the soft-dirty PTE bit of the pages.
- Get and clear the soft-dirty PTE bit of the pages atomically.
Soft-dirty PTE bit of the memory pages can be read by using the pagemap procfs file. The soft-dirty PTE bit for the whole memory range of the process can be cleared by writing to the clear_refs file. There are other methods to mimic this information entirely in userspace with poor performance:
- The mprotect syscall and SIGSEGV handler for bookkeeping
- The userfaultfd syscall with the handler for bookkeeping
Some benchmarks can be seen here[1]. This series adds features that weren't present earlier:
- There is no atomic get soft-dirty PTE bit status and clear operation possible.
- The soft-dirty PTE bit of only a part of memory cannot be cleared.
Historically, soft-dirty PTE bit tracking has been used in the CRIU project. The procfs interface is enough for finding the soft-dirty bit status and clearing the soft-dirty bit of all the pages of a process. We have the use case where we need to track the soft-dirty PTE bit for only specific pages on demand. We need this tracking and clear mechanism of a region of memory while the process is running to emulate the getWriteWatch() syscall of Windows. This syscall is used by games to keep track of dirty pages to process only the dirty pages.
The information related to pages if the page is file mapped, present and swapped is required for the CRIU project[2][3]. The addition of the required mask, any mask, excluded mask and return masks are also required for the CRIU project[2].
The IOCTL returns the addresses of the pages which match the specific masks. The page addresses are returned in struct page_region in a compact form. The max_pages is needed to support a use case where user only wants to get a specific number of pages. So there is no need to find all the pages of interest in the range when max_pages is specified. The IOCTL returns when the maximum number of the pages are found. The max_pages is optional. If max_pages is specified, it must be equal or greater than the vec_size. This restriction is needed to handle worse case when one page_region only contains info of one page and it cannot be compacted. This is needed to emulate the Windows getWriteWatch() syscall.
Some non-dirty pages get marked as dirty because of the kernel's internal activity (such as VMA merging as soft-dirty bit difference isn't considered while deciding to merge VMAs). The dirty bit of the pages is stored in the VMA flags and in the per page flags. If any of these two bits are set, the page is considered to be soft dirty. Suppose you have cleared the soft dirty bit of half of VMA which will be done by splitting the VMA and clearing soft dirty bit flag in the half VMA and the pages in it. Now kernel may decide to merge the VMAs again. So the half VMA becomes dirty again. This splitting/merging costs performance. The application receives a lot of pages which aren't dirty in reality but marked as dirty. Performance is lost again here. Also sometimes user doesn't want the newly allocated memory to be marked as dirty. PAGEMAP_NO_REUSED_REGIONS flag solves both the problems. It is used to not depend on the soft dirty flag in the VMA flags. So VMA splitting and merging doesn't happen. It only depends on the soft dirty bit of the individual pages. Thus by using this flag, there may be a scenerio such that the new memory regions which are just created, doesn't look dirty when seen with the IOCTL, but look dirty when seen from procfs. This seems okay as the user of this flag know the implication of using it.
[1] https://lore.kernel.org/lkml/54d4c322-cd6e-eefd-b161-2af2b56aae24@collabora.... [2] https://lore.kernel.org/all/YyiDg79flhWoMDZB@gmail.com/ [3] https://lore.kernel.org/all/20221014134802.1361436-1-mdanylo@google.com/
Regards, Muhammad Usama Anjum
Muhammad Usama Anjum (4): userfaultfd: Add UFFD WP Async support userfaultfd: split mwriteprotect_range() fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs selftests: vm: add pagemap ioctl tests
fs/proc/task_mmu.c | 300 +++++++ fs/userfaultfd.c | 161 ++-- include/linux/userfaultfd_k.h | 10 + include/uapi/linux/fs.h | 50 ++ include/uapi/linux/userfaultfd.h | 6 + mm/userfaultfd.c | 40 +- tools/include/uapi/linux/fs.h | 50 ++ tools/testing/selftests/vm/.gitignore | 1 + tools/testing/selftests/vm/Makefile | 5 +- tools/testing/selftests/vm/pagemap_ioctl.c | 884 +++++++++++++++++++++ 10 files changed, 1424 insertions(+), 83 deletions(-) create mode 100644 tools/testing/selftests/vm/pagemap_ioctl.c
On Mon, Jan 09, 2023 at 11:45:15AM +0500, Muhammad Usama Anjum wrote:
*Changes in v7:*
- Add uffd wp async
- Update the IOCTL to use uffd under the hood instead of soft-dirty flags
Stop using the soft-dirty flags for finding which pages have been written to. It is too delicate and wrong as it shows more soft-dirty pages than the actual soft-dirty pages. There is no interest in correcting it [A][B] as this is how the feature was written years ago. It shouldn't be updated to changed behaviour. Peter Xu has suggested using the async version of the UFFD WP [C] as it is based inherently on the PTEs.
So in this patch series, I've added a new mode to the UFFD which is asynchronous version of the write protect. When this variant of the UFFD WP is used, the page faults are resolved automatically by the kernel. The pages which have been written-to can be found by reading pagemap file (!PM_UFFD_WP). This feature can be used successfully to find which pages have been written to from the time the pages were write protected. This works just like the soft-dirty flag without showing any extra pages which aren't soft-dirty in reality.
[A] https://lore.kernel.org/all/20221220162606.1595355-1-usama.anjum@collabora.c... [B] https://lore.kernel.org/all/20221122115007.2787017-1-usama.anjum@collabora.c... [C] https://lore.kernel.org/all/Y6Hc2d+7eTKs7AiH@x1n
*Changes in v6:*
- Updated the interface and made cosmetic changes
*Cover Letter in v5:* Hello,
Please consider either drop the cover letter below this point or rephrase, otherwise many of them are not true anymore and it can confuse the reviewers.
I have a few high level comments/questions here, please bare with me if any of them are already discussed by others in the old versions; I'd be happy to read them when there's a pointer to the relevant answers.
Firstly, doc update is more than welcomed to explain the new interface first (before throwing the code..). That can be done in pagemap.rst on pagemap changes, or userfaultfd.rst on userfaultfd.
Besides, can you provide more justification on the new pagemap-side interface design?
It seems it came from the Windows API GetWriteWatch(), but it's definitely not exactly that. Let me spell some points out..
There're four kinds of masks (required/anyof/excluded/return). Are they all needed? Why this is a good interface design?
I saw you used page_region structure to keep the information. I think you wanted to have a densed output, especially if counting in the "return mask" above it starts to make more sense. If with a very limited return mask it means many of the (continuous) page information can be merged into a single page_region struct when the kernel is scanning.
However, at the meantime the other three masks (required/anyof/excluded) made me quite confused - it means you wanted to somehow filter the pages and only some of them will get collected. The thing is for a continuous page range if any of the page got skipped due to the masks (e.g. not in "required" or in "excluded") it also means it can never be merged into previous page_region either. That seems to be against the principle of having densed output.
I hope you can help clarify what's the major use case here.
There's also the new interface to do atomic "fetch + update" on wrprotected pages. Is that just for efficiency or is the accuracy required in some of the applications?
Thanks,
On 1/19/23 3:12 AM, Peter Xu wrote:
On Mon, Jan 09, 2023 at 11:45:15AM +0500, Muhammad Usama Anjum wrote:
*Changes in v7:*
- Add uffd wp async
- Update the IOCTL to use uffd under the hood instead of soft-dirty flags
Stop using the soft-dirty flags for finding which pages have been written to. It is too delicate and wrong as it shows more soft-dirty pages than the actual soft-dirty pages. There is no interest in correcting it [A][B] as this is how the feature was written years ago. It shouldn't be updated to changed behaviour. Peter Xu has suggested using the async version of the UFFD WP [C] as it is based inherently on the PTEs.
So in this patch series, I've added a new mode to the UFFD which is asynchronous version of the write protect. When this variant of the UFFD WP is used, the page faults are resolved automatically by the kernel. The pages which have been written-to can be found by reading pagemap file (!PM_UFFD_WP). This feature can be used successfully to find which pages have been written to from the time the pages were write protected. This works just like the soft-dirty flag without showing any extra pages which aren't soft-dirty in reality.
[A] https://lore.kernel.org/all/20221220162606.1595355-1-usama.anjum@collabora.c... [B] https://lore.kernel.org/all/20221122115007.2787017-1-usama.anjum@collabora.c... [C] https://lore.kernel.org/all/Y6Hc2d+7eTKs7AiH@x1n
*Changes in v6:*
- Updated the interface and made cosmetic changes
*Cover Letter in v5:* Hello,
Please consider either drop the cover letter below this point or rephrase, otherwise many of them are not true anymore and it can confuse the reviewers.
I'll remove.
I have a few high level comments/questions here, please bare with me if any of them are already discussed by others in the old versions; I'd be happy to read them when there's a pointer to the relevant answers.
Firstly, doc update is more than welcomed to explain the new interface first (before throwing the code..). That can be done in pagemap.rst on pagemap changes, or userfaultfd.rst on userfaultfd.
Okay. I'll add the documentation in next version or after the series has been accepted. Initially I'd added the documentation. But the code kept on changing so much that I had to spend considerable time on updating the documentation. I know it is better to add documentation with the patches. I'll try to add it.
Besides, can you provide more justification on the new pagemap-side interface design?
It seems it came from the Windows API GetWriteWatch(), but it's definitely not exactly that. Let me spell some points out..
Initially, we just wanted a way to emulate Windows API GetWriteWatch(). So we had added `max_pages` in the IOCTL arguments which is optional and can be used to specify how many pages we want to find of our interest. There was only one set of flags to be matched with the pages.
There're four kinds of masks (required/anyof/excluded/return). Are they all needed? Why this is a good interface design?
Then, CRIU developers Andrea [1] and Danylo [2], asked to include all these different kinds of masks. I'd thought of these masks as fancy filter inside the kernel. But there wasn't anyone else to review. So I'd included them to move forward. Please let me know your thoughts after reading emails from [1].
I saw you used page_region structure to keep the information. I think you wanted to have a densed output, especially if counting in the "return mask" above it starts to make more sense. If with a very limited return mask it means many of the (continuous) page information can be merged into a single page_region struct when the kernel is scanning.
Correct.
However, at the meantime the other three masks (required/anyof/excluded) made me quite confused - it means you wanted to somehow filter the pages and only some of them will get collected. The thing is for a continuous page range if any of the page got skipped due to the masks (e.g. not in "required" or in "excluded") it also means it can never be merged into previous page_region either. That seems to be against the principle of having densed output.
The filtering is being done. But the output can still be condensed regardless. There isn't that randomness in the page flags of the consecutive pages.
I hope you can help clarify what's the major use case here.
There's also the new interface to do atomic "fetch + update" on wrprotected pages. Is that just for efficiency or is the accuracy required in some of the applications?
"Atomic fetch and update/clear" or "Atomic fetch Written-to status and clear it" is needed to support GetWriteWatch() and there is no already present way to perform this operation atomically. We want efficiency and accuracy both to get good performance/speed. So this IOCTL is needed to achieve: 1) New functionality which isn't already present 2) Most efficient and accurate method to perform the operation (it isn't possible through soft-dirty feature)
Thanks,
[1] https://lore.kernel.org/all/YyiDg79flhWoMDZB@gmail.com [2] https://lore.kernel.org/all/20221014134802.1361436-1-mdanylo@google.com
On Mon, Jan 23, 2023 at 06:15:00PM +0500, Muhammad Usama Anjum wrote:
Firstly, doc update is more than welcomed to explain the new interface first (before throwing the code..). That can be done in pagemap.rst on pagemap changes, or userfaultfd.rst on userfaultfd.
Okay. I'll add the documentation in next version or after the series has been accepted. Initially I'd added the documentation. But the code kept on changing so much that I had to spend considerable time on updating the documentation. I know it is better to add documentation with the patches. I'll try to add it.
Yes, logically it should be the thing people start looking with. It'll help reviewers to understand how does it work in general if relevant description is not in the cover letter, so it can matter even before the series is merged.
There're four kinds of masks (required/anyof/excluded/return). Are they all needed? Why this is a good interface design?
Then, CRIU developers Andrea [1] and Danylo [2], asked to include all these different kinds of masks. I'd thought of these masks as fancy filter inside the kernel. But there wasn't anyone else to review. So I'd included them to move forward. Please let me know your thoughts after reading emails from [1].
The idea makes sense to me, thanks. I just hope "moving it forward" is not the only reason that you included it.
Please also consider to attach relevant links to your next cover letter so new reviewers can be aware of why the interface is proposed like that.
IMHO it would be also great if the CRIU people can acknowledge the interface at some point to make sure it satisfies the needs. An POC would be even better on CRIU, but maybe that's asking too much.
On Tue, Jan 24, 2023 at 8:49 PM Peter Xu peterx@redhat.com wrote:
On Mon, Jan 23, 2023 at 06:15:00PM +0500, Muhammad Usama Anjum wrote:
Firstly, doc update is more than welcomed to explain the new interface
first (before throwing the code..). That can be done in pagemap.rst on
pagemap changes, or userfaultfd.rst on userfaultfd.
Okay. I'll add the documentation in next version or after the series has been accepted. Initially I'd added the documentation. But the code kept on changing so much that I had to spend considerable time on updating the documentation. I know it is better to add documentation with the patches. I'll try to add it.
Yes, logically it should be the thing people start looking with. It'll help reviewers to understand how does it work in general if relevant description is not in the cover letter, so it can matter even before the series is merged.
There're four kinds of masks (required/anyof/excluded/return). Are they all needed? Why this is a good interface design?
Then, CRIU developers Andrea [1] and Danylo [2], asked to include all these different kinds of masks. I'd thought of these masks as fancy filter inside the kernel. But there wasn't anyone else to review. So I'd included them to move forward. Please let me know your thoughts after reading emails from [1].
The idea makes sense to me, thanks. I just hope "moving it forward" is not the only reason that you included it. Please also consider to attach relevant links to your next cover letter so new reviewers can be aware of why the interface is proposed like that. IMHO it would be also great if the CRIU people can acknowledge the interface at some point to make sure it satisfies the needs.
I acknowledge that this interface looks good for my use case to get interesting pages from a big sparse mapping. For Andrei's use case to iteratively dump memory it also looks good IMO.
An POC would be even better on CRIU, but maybe that's asking too much.
Can't promise now, but happy to do this when there'll be a clear signal that this patchset is about to be merged. Meanwhile, I'll make some smaller tests with this patchset rebased and will get back if there are some problems with that.
-- Peter Xu
linux-kselftest-mirror@lists.linaro.org
-
Danylo Mocherniuk -
Muhammad Usama Anjum -
Peter Xu