LinuxLists.cc - Re: fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

2023-07-21 11:00:40

Subject: Re: fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On 7/21/23 12:28 AM, Michał Mirosław wrote:
> This is a massaged version of patch by Muhammad Usama Anjum [1]
> to illustrate my review comments and hopefully push the implementation
> efforts closer to conclusion. The changes are:
Thank you so much for this effort. I also want to reach conclusion. I'll
agree with all the changes which don't affect me. But some requirements
aren't being fulfilled with this current design.

>
> 1. the API:
>
> a. return ranges as {begin, end} instead of {begin + len};
Agreed.

> b. rename match "flags" to 'page categories' everywhere - this makes
> it easier to differentiate the ioctl()s categorisation of pages
> from struct page flags;
I've no problem with it.

#define PAGE_IS_WPASYNC (1 << 0)
#define PAGE_IS_WRITTEN (1 << 1)
You have another new flag PAGE_IS_WPASYNC. But there is no application of
PAGE_IS_WPASYNC. We must not add a flag which don't have any user.

> c. change {required + excluded} to {inverted + required}. This was
> rejected before, but I'd like to illustrate the difference.
> Old interface can be translated to the new by:
> categories_inverted = excluded_mask
> categories_mask = required_mask | excluded_mask
> categories_anyof_mask = anyof_mask
> The new way allows filtering by: A & (B | !C)
> categories_inverted = C
> categories_mask = A
> categories_anyof_mask = B | C
I'm still unable to get the idea of inverted masks. IIRC Andei had also not
supported/accepted this masking scheme. But I'll be okay with it if he
supports this masking.

> d. change the ioctl to be a SCAN with optional WP. Addressing the
> original use-case, GetWriteWatch() can be implemented as:
As I've mentioned several times previously (without the name of
ResetWriteWatch()) that we need exclusive WP without GET. This could be
implemented with UFFDIO_WRITEPROTECT. But when we use UFFDIO_WRITEPROTECT,
we hit some special case and performance is very slow. So with UFFD WP
expert, Peter Xu we have decided to put exclusive WP in this IOCTL for
implementation of ResetWriteWatch().

A lot of simplification of the patch is made possible because of not
keeping exclusive WP. (You have also written some quality code, more better.)

>
> memset(&args, 0, sizeof(args));
> args.start = lpBaseAddress;
> args.end = lpBaseAddress + dwRegionSize;
> args.max_pages = *lpdwCount;
> *lpdwGranularity = PAGE_SIZE;
> args.flags = PM_SCAN_CHECK_WPASYNC;
> if (dwFlags & WRITE_WATCH_FLAG_RESET)
> args.flags |= PM_SCAN_WP_MATCHED;
> args.categories_mask = PAGE_IS_WRITTEN;
> args.return_mask = PAGE_IS_WRITTEN;
> args.vec = tmp_buffer;
> args.vec_len = tmp_buffer_len;
> do {
> n = ioctl(..., &args);
> if (n < 0)
> return error(n);
> for (page_region *p = ...) {
> write page addresses;
> }
> if (output_full)
> return ...;
> } while (args.start != args.end);
> return ...;
>
> For the CRIU's usecase of live migration this would be similar,
> but without using max_pages limit. The other CRIU usecase is to
> efficiently dump sparse memory mappings - those could use the
> full range of page category filtering as needed in a checkpoint
> phase.a
> e. allow no-op calls
I've no issue with it.

>
> 2. the implementation:
> a. gather the page-categorising and write-protecting code in one place;
Agreed.

> b. optimization: add whole-vma skipping for WP usecase;
I don't know who can benefit from it. Do you have any user in mind? When
the user come of this optimization, this can be added later.

> c. extracted output limiting code to pagemap_scan_output();
If user passes half THP, current code wouldn't split huge page and WP the
whole THP. We would loose the dirty state of other half huge page. This is
bug. consoliding the output limiting code looks optimal, but we'll need to
same limiting code to detect if full THP hasn't been passed in case of THP
and HugeTLB.

> d. extracted range coalescing to pagemap_scan_push_range();
My old pagemap_scan_output has become pagemap_scan_push_range().

> e. extracted THP entry handling to pagemap_scan_thp_entry();
Good. But I didn't found value in seperating it just like other historic
pagemap code.

> f. added a shortcut for non-WP hugetlb scan; avoids conditional
> locking;
Yeah, some if conditions have been removed. But overall did couple of calls
to is_interesting and scan_output functions instead of just one.

> g. extracted scan buffer handling code out of do_pagemap_scan();
> h. rework output code to always try to write pending ranges; if EFAULT
> is generated it always overwrites the original error code;
> (the case of SIGKILL is needlessly trying to write the output
> now, but this should be rare case and ignoring it makes the code
> not needing a goto)
>
> PTAL and comment/fix. Compile tested with allyesconfig. For the (unlikely)
> case that the patch is good as is:
>
> Signed-off-by: Muhammad Usama Anjum <[email protected]>
> Signed-off-by: Michał Mirosław <[email protected]>
>
> [1] https://lore.kernel.org/lkml/[email protected]/
> "[PATCH v25 2/5] fs/proc/task_mmu: Implement IOCTL to get and
> optionally clear info about PTEs"
>
> Best Regards
> Michał Mirosław
>
> Signed-off-by: Michał Mirosław <[email protected]>
> ---
> fs/proc/task_mmu.c | 649 ++++++++++++++++++++++++++++++++++++++++
> include/linux/hugetlb.h | 1 +
> include/uapi/linux/fs.h | 56 ++++
> mm/hugetlb.c | 2 +-
> 4 files changed, 707 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index c1e6531cb02a..7d624bafecf9 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -19,6 +19,8 @@
> #include <linux/shmem_fs.h>
> #include <linux/uaccess.h>
> #include <linux/pkeys.h>
> +#include <linux/minmax.h>
> +#include <linux/overflow.h>
>
> #include <asm/elf.h>
> #include <asm/tlb.h>
> @@ -1749,11 +1751,658 @@ static int pagemap_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> +#define PM_SCAN_CATEGORIES (PAGE_IS_WPASYNC | PAGE_IS_WRITTEN | \
> + PAGE_IS_FILE | PAGE_IS_PRESENT | \
> + PAGE_IS_SWAPPED | PAGE_IS_PFNZERO)
> +#define PM_SCAN_FLAGS (PM_SCAN_WP_MATCHING | PM_SCAN_CHECK_WPASYNC)
> +
> +struct pagemap_scan_private {
> + struct pm_scan_arg arg;
> + unsigned long cur_vma_category;
> + struct page_region *vec_buf, cur_buf;
> + unsigned long vec_buf_len, vec_buf_index, found_pages, end_addr;
> + struct page_region __user* vec_out;
> +};
> +
> +static unsigned long pagemap_page_category(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> +{
> + unsigned long categories = 0;
> +
> + if (pte_present(pte)) {
> + struct page *page = vm_normal_page(vma, addr, pte);
> +
> + categories |= PAGE_IS_PRESENT;
> + if (!pte_uffd_wp(pte))
> + categories |= PAGE_IS_WRITTEN;
> + if (page && !PageAnon(page))
> + categories |= PAGE_IS_FILE;
> + if (is_zero_pfn(pte_pfn(pte)))
> + categories |= PAGE_IS_PFNZERO;
> + } else if (is_swap_pte(pte)) {
> + swp_entry_t swp = pte_to_swp_entry(pte);
> +
> + categories |= PAGE_IS_SWAPPED;
> + if (!pte_swp_uffd_wp_any(pte))
> + categories |= PAGE_IS_WRITTEN;
> + if (is_pfn_swap_entry(swp) && !PageAnon(pfn_swap_entry_to_page(swp)))
> + categories |= PAGE_IS_FILE;
> + }
> +
> + return categories;
> +}
> +
> +static void make_uffd_wp_pte(struct vm_area_struct *vma,
> + unsigned long addr, pte_t *pte)
> +{
> + pte_t ptent = ptep_get(pte);
> +
> + if (pte_present(ptent)) {
> + pte_t old_pte;
> +
> + old_pte = ptep_modify_prot_start(vma, addr, pte);
> + ptent = pte_mkuffd_wp(ptent);
> + ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent);
> + } else if (is_swap_pte(ptent)) {
> + ptent = pte_swp_mkuffd_wp(ptent);
> + set_pte_at(vma->vm_mm, addr, pte, ptent);
> + } else {
> + set_pte_at(vma->vm_mm, addr, pte,
> + make_pte_marker(PTE_MARKER_UFFD_WP));
> + }
> +}
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static unsigned long pagemap_thp_category(pmd_t pmd)
> +{
> + unsigned long categories = 0;
> +
> + if (pmd_present(pmd)) {
> + categories |= PAGE_IS_PRESENT;
> + if (!pmd_uffd_wp(pmd))
> + categories |= PAGE_IS_WRITTEN;
> + if (is_zero_pfn(pmd_pfn(pmd)))
> + categories |= PAGE_IS_PFNZERO;
> + } else if (is_swap_pmd(pmd)) {
> + categories |= PAGE_IS_SWAPPED;
> + if (!pmd_swp_uffd_wp(pmd))
> + categories |= PAGE_IS_WRITTEN;
> + }
> +
> + return categories;
> +}
> +
> +static void make_uffd_wp_pmd(struct vm_area_struct *vma,
> + unsigned long addr, pmd_t *pmdp)
> +{
> + pmd_t old, pmd = *pmdp;
> +
> + if (pmd_present(pmd)) {
> + old = pmdp_invalidate_ad(vma, addr, pmdp);
> + pmd = pmd_mkuffd_wp(old);
> + set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
> + } else if (is_migration_entry(pmd_to_swp_entry(pmd))) {
> + pmd = pmd_swp_mkuffd_wp(pmd);
> + set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
> + }
> +}
> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
> +
> +#ifdef CONFIG_HUGETLB_PAGE
> +static unsigned long pagemap_hugetlb_category(pte_t pte)
> +{
> + unsigned long categories = 0;
> +
> + if (pte_present(pte)) {
> + categories |= PAGE_IS_PRESENT;
> + if (!huge_pte_uffd_wp(pte))
> + categories |= PAGE_IS_WRITTEN;
> + if (!PageAnon(pte_page(pte)))
> + categories |= PAGE_IS_FILE;
> + if (is_zero_pfn(pte_pfn(pte)))
> + categories |= PAGE_IS_PFNZERO;
> + } else if (is_swap_pte(pte)) {
> + categories |= PAGE_IS_SWAPPED;
> + if (!pte_swp_uffd_wp_any(pte))
> + categories |= PAGE_IS_WRITTEN;
> + }
> +
> + return categories;
> +}
> +
> +static void make_uffd_wp_huge_pte(struct vm_area_struct *vma,
> + unsigned long addr, pte_t *ptep,
> + pte_t ptent)
> +{
> + if (is_hugetlb_entry_hwpoisoned(ptent) || is_pte_marker(ptent))
> + return;
> +
> + if (is_hugetlb_entry_migration(ptent))
> + set_huge_pte_at(vma->vm_mm, addr, ptep,
> + pte_swp_mkuffd_wp(ptent));
> + else if (!huge_pte_none(ptent))
> + huge_ptep_modify_prot_commit(vma, addr, ptep, ptent,
> + huge_pte_mkuffd_wp(ptent));
> + else
> + set_huge_pte_at(vma->vm_mm, addr, ptep,
> + make_pte_marker(PTE_MARKER_UFFD_WP));
> +}
> +#endif /* CONFIG_HUGETLB_PAGE */
> +
> +static bool pagemap_scan_is_interesting_page(unsigned long categories,
> + const struct pagemap_scan_private *p)
> +{
> + categories ^= p->arg.category_inverted;
> + if ((categories & p->arg.category_mask) != p->arg.category_mask)
> + return false;
> + if (p->arg.category_anyof_mask && !(categories & p->arg.category_anyof_mask))
> + return false;
> +
> + return true;
> +}
> +
> +static bool pagemap_scan_is_interesting_vma(unsigned long categories,
> + const struct pagemap_scan_private *p)
> +{
> + unsigned long required = p->arg.category_mask & PAGE_IS_WPASYNC;
> +
> + categories ^= p->arg.category_inverted;
> + if ((categories & required) != required)
> + return false;
> + return true;
> +}
> +
> +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;
> + unsigned long vma_category = 0;
> +
> + if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
> + vma_category |= PAGE_IS_WPASYNC;
> + else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> + return -EPERM;
> +
> + if (vma->vm_flags & VM_PFNMAP)
> + return 1;
> +
> + if (!pagemap_scan_is_interesting_vma(vma_category, p))
> + return 1;
> +
> + p->cur_vma_category = vma_category;
> + return 0;
> +}
> +
> +static bool pagemap_scan_push_range(unsigned long categories,
> + struct pagemap_scan_private *p,
> + unsigned long addr, unsigned long end)
> +{
> + struct page_region *cur_buf = &p->cur_buf;
> +
> + /*
> + * When there is no output buffer provided at all, the sentinel values
> + * won't match here. There is no other way for `cur_buf->end` to be
> + * non-zero other than it being non-empty.
> + */
> + if (addr == cur_buf->end && categories == cur_buf->categories) {
> + cur_buf->end = end;
> + return true;
> + }
> +
> + if (cur_buf->end) {
> + if (p->vec_buf_index >= p->vec_buf_len)
> + return false;
> +
> + memcpy(&p->vec_buf[p->vec_buf_index], cur_buf,
> + sizeof(*p->vec_buf));
> + ++p->vec_buf_index;
> + }
> +
> + cur_buf->start = addr;
> + cur_buf->end = end;
> + cur_buf->categories = categories;
> + return true;
> +}
> +
> +static void pagemap_scan_backout_range(struct pagemap_scan_private *p,
> + unsigned long addr, unsigned long end)
> +{
> + struct page_region *cur_buf = &p->cur_buf;
> +
> + if (cur_buf->start != addr) {
> + cur_buf->end = addr;
> + } else {
> + cur_buf->start = cur_buf->end = 0;
> + }
> +
> + p->end_addr = 0;
> +}
> +
> +static int pagemap_scan_output(unsigned long categories,
> + struct pagemap_scan_private *p,
> + unsigned long addr, unsigned long *end)
> +{
> + unsigned long n_pages, total_pages;
> + int ret = 0;
> +
> + if (!p->arg.return_mask)
> + return 0;
> +
> + categories &= p->arg.return_mask;
> +
> + n_pages = (*end - addr) / PAGE_SIZE;
> + if (check_add_overflow(p->found_pages, n_pages, &total_pages) || total_pages > p->arg.max_pages) {
> + size_t n_too_much = total_pages - p->arg.max_pages;
> + *end -= n_too_much * PAGE_SIZE;
> + n_pages -= n_too_much;
> + ret = -ENOSPC;
> + }
> +
> + if (!pagemap_scan_push_range(categories, p, addr, *end)) {
> + *end = addr;
> + n_pages = 0;
> + ret = -ENOSPC;
> + }
> +
> + p->found_pages += n_pages;
> + if (ret)
> + p->end_addr = *end;
> + return ret;
> +}
> +
> +static int pagemap_scan_thp_entry(pmd_t *pmd, unsigned long start,
> + unsigned long end, struct mm_walk *walk)
> +{
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> + struct pagemap_scan_private *p = walk->private;
> + struct vm_area_struct *vma = walk->vma;
> + unsigned long categories;
> + spinlock_t *ptl;
> + int ret = 0;
> +
> + ptl = pmd_trans_huge_lock(pmd, vma);
> + if (!ptl)
> + return -ENOENT;
> +
> + categories = p->cur_vma_category | pagemap_thp_category(*pmd);
> +
> + if (!pagemap_scan_is_interesting_page(categories, p))
> + goto out_unlock;
> +
> + ret = pagemap_scan_output(categories, p, start, &end);
> + if (start == end)
> + goto out_unlock;
> +
> + if (~p->arg.flags & PM_SCAN_WP_MATCHING)
> + goto out_unlock;
> + if (~categories & PAGE_IS_WRITTEN)
> + goto out_unlock;
> +
> + /*
> + * Break huge page into small pages if the WP operation
> + * need to be performed is on a portion of the huge page.
> + */
> + if (ret == -ENOSPC) {
> + spin_unlock(ptl);
> + split_huge_pmd(vma, pmd, start);
> + pagemap_scan_backout_range(p, start, end);
> + return -ENOENT;
> + }
> +
> + make_uffd_wp_pmd(vma, start, pmd);
> + flush_tlb_range(vma, start, end);
> +out_unlock:
> + spin_unlock(ptl);
> + return ret;
> +#else /* !CONFIG_TRANSPARENT_HUGEPAGE */
> + return -ENOENT;
> +#endif
> +}
> +
> +static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
> + unsigned long end, struct mm_walk *walk)
> +{
> + struct pagemap_scan_private *p = walk->private;
> + struct vm_area_struct *vma = walk->vma;
> + pte_t *pte, *start_pte;
> + unsigned long addr;
> + bool flush = false;
> + spinlock_t *ptl;
> + int ret;
> +
> + arch_enter_lazy_mmu_mode();
> +
> + ret = pagemap_scan_thp_entry(pmd, start, end, walk);
> + if (ret != -ENOENT) {
> + arch_leave_lazy_mmu_mode();
> + return ret;
> + }
> +
> + start_pte = pte = pte_offset_map_lock(vma->vm_mm, pmd, start, &ptl);
> + if (!pte) {
> + arch_leave_lazy_mmu_mode();
> + walk->action = ACTION_AGAIN;
> + return 0;
> + }
> +
> + for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> + unsigned long categories = p->cur_vma_category |
> + pagemap_page_category(vma, addr, ptep_get(pte));
> + unsigned long next = addr + PAGE_SIZE;
> +
> + if (!pagemap_scan_is_interesting_page(categories, p))
> + continue;
> +
> + ret = pagemap_scan_output(categories, p, addr, &next);
> + if (next == addr)
> + break;
> +
> + if (~p->arg.flags & PM_SCAN_WP_MATCHING)
> + continue;
> + if (~categories & PAGE_IS_WRITTEN)
> + continue;
> +
> + make_uffd_wp_pte(vma, addr, pte);
> + if (!flush) {
> + start = addr;
> + flush = true;
> + }
> + }
> +
> + if (flush)
> + flush_tlb_range(vma, start, addr);
> +
> + pte_unmap_unlock(start_pte, ptl);
> + arch_leave_lazy_mmu_mode();
> +
> + cond_resched();
> + return ret;
> +}
> +
> +#ifdef CONFIG_HUGETLB_PAGE
> +static int pagemap_scan_hugetlb_entry(pte_t *ptep, unsigned long hmask,
> + unsigned long start, unsigned long end,
> + struct mm_walk *walk)
> +{
> + unsigned long n_pages = (end - start)/PAGE_SIZE;
> + struct pagemap_scan_private *p = walk->private;
> + struct vm_area_struct *vma = walk->vma;
> + unsigned long categories;
> + spinlock_t *ptl;
> + int ret = 0;
> + pte_t pte;
> +
> + if (~p->arg.flags & PM_SCAN_WP_MATCHING) {
> + /* Go the short route when not write-protecting pages. */
> +
> + pte = huge_ptep_get(ptep);
> + categories = p->cur_vma_category | pagemap_hugetlb_category(pte);
> +
> + if (!pagemap_scan_is_interesting_page(categories, p))
> + return 0;
> +
> + return pagemap_scan_output(categories, p, start, &end);
> + }
> +
> + if (n_pages < HPAGE_SIZE/PAGE_SIZE) {
> + /*
> + * Partial hugetlb page clear isn't supported
> + */
> + p->end_addr = start;
> + return -EINVAL;
> + }
> +
> + i_mmap_lock_write(vma->vm_file->f_mapping);
> + ptl = huge_pte_lock(hstate_vma(vma), vma->vm_mm, ptep);
> +
> + pte = huge_ptep_get(ptep);
> + categories = p->cur_vma_category | pagemap_hugetlb_category(pte);
> +
> + if (!pagemap_scan_is_interesting_page(categories, p))
> + goto out_unlock;
> +
> + ret = pagemap_scan_output(categories, p, start, &end);
> + if (start == end)
> + goto out_unlock;
> +
> + if (categories & PAGE_IS_WRITTEN) {
> + make_uffd_wp_huge_pte(vma, start, ptep, pte);
> + flush_hugetlb_tlb_range(vma, start, end);
> + }
> +
> +out_unlock:
> + spin_unlock(ptl);
> + i_mmap_unlock_write(vma->vm_file->f_mapping);
> +
> + return ret;
> +}
> +#else
> +#define pagemap_scan_hugetlb_entry NULL
> +#endif
> +
> +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;
> + int ret;
> +
> + if (!vma || !pagemap_scan_is_interesting_page(p->cur_vma_category, p))
> + return 0;
> +
> + ret = pagemap_scan_output(p->cur_vma_category, p, addr, &end);
> + if (addr == end)
> + return ret;
> +
> + if (~p->arg.flags & PM_SCAN_WP_MATCHING)
> + return ret;
> +
> + int err = uffd_wp_range(vma, addr, end - addr, true);
> + if (err < 0)
> + ret = err;
> +
> + 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,
> + .hugetlb_entry = pagemap_scan_hugetlb_entry,
> +};
> +
> +static int pagemap_scan_get_args(struct pm_scan_arg *arg,
> + unsigned long uarg)
> +{
> + unsigned long start, end, vec;
> +
> + if (copy_from_user(arg, (void __user *)uarg, sizeof(*arg)))
> + return -EFAULT;
> +
> + if (arg->size != sizeof(struct pm_scan_arg))
> + return -EINVAL;
> +
> + /* Validate requested features */
> + if (arg->flags & ~PM_SCAN_FLAGS)
> + return -EINVAL;
> + if ((arg->category_inverted | arg->category_mask |
> + arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
> + return -EINVAL;
> +
> + start = untagged_addr((unsigned long)arg->start);
> + end = untagged_addr((unsigned long)arg->end);
> + vec = untagged_addr((unsigned long)arg->vec);
> +
> + /* Validate memory pointers */
> + if (!IS_ALIGNED(start, PAGE_SIZE))
> + return -EINVAL;
> + if (!access_ok((void __user *)start, end - start))
> + return -EFAULT;
> + if (arg->vec_len && !vec)
> + return -EFAULT;
> + if (!access_ok((void __user *)vec,
> + arg->vec_len * sizeof(struct page_region)))
> + return -EFAULT;
> +
> + /* Fixup default values */
> + if (!arg->max_pages)
> + arg->max_pages = ULONG_MAX;
> +
> + return 0;
> +}
> +
> +static int pagemap_scan_writeback_args(struct pm_scan_arg *arg,
> + unsigned long uargl)
> +{
> + struct pm_scan_arg __user *uarg = (void __user *)uargl;
> +
> + if (copy_to_user(&uarg->start, &arg->start, sizeof(arg->start)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int pagemap_scan_init_bounce_buffer(struct pagemap_scan_private *p)
> +{
> + if (!p->arg.vec_len) {
> + /*
> + * An arbitrary non-page-aligned sentinel value for
> + * pagemap_scan_push_range().
> + */
> + p->cur_buf.start = p->cur_buf.end = ULLONG_MAX;
> + return 0;
> + }
> +
> + /*
> + * Allocate a smaller buffer to get output from inside the page
> + * walk functions and walk the range in PAGEMAP_WALK_SIZE chunks.
> + * The last range is always stored in p.cur_buf to allow coalescing
> + * consecutive ranges that have the same categories returned across
> + * walk_page_range() calls.
> + */
> + p->vec_buf_len = min_t(size_t, PAGEMAP_WALK_SIZE >> PAGE_SHIFT,
> + p->arg.vec_len - 1);
> + p->vec_buf = kmalloc_array(p->vec_buf_len, sizeof(*p->vec_buf),
> + GFP_KERNEL);
> + if (!p->vec_buf)
> + return -ENOMEM;
> +
> + p->vec_out = (void __user *)p->arg.vec;
> +
> + return 0;
> +}
> +
> +static int pagemap_scan_flush_buffer(struct pagemap_scan_private *p)
> +{
> + const struct page_region *buf = p->vec_buf;
> + int n = (int)p->vec_buf_index;
> +
> + if (!n)
> + return 0;
> +
> + if (copy_to_user(p->vec_out, buf, n * sizeof(*buf)))
> + return -EFAULT;
> +
> + p->arg.vec_len -= n;
> + p->vec_out += n;
> +
> + p->vec_buf_index = 0;
> + p->vec_buf_len = min_t(size_t, p->vec_buf_len, p->arg.vec_len - 1);
> +
> + return n;
> +}
> +
> +static long do_pagemap_scan(struct mm_struct *mm, unsigned long uarg)
> +{
> + unsigned long walk_start, walk_end;
> + struct mmu_notifier_range range;
> + struct pagemap_scan_private p;
> + size_t n_ranges_out = 0;
> + int ret;
> +
> + memset(&p, 0, sizeof(p));
> + ret = pagemap_scan_get_args(&p.arg, uarg);
> + if (ret)
> + return ret;
> +
> + ret = pagemap_scan_init_bounce_buffer(&p);
> + if (ret)
> + return ret;
> +
> + /* Protection change for the range is going to happen. */
> + if (p.arg.flags & PM_SCAN_WP_MATCHING) {
> + mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA, 0,
> + mm, p.arg.start, p.arg.end);
> + mmu_notifier_invalidate_range_start(&range);
> + }
> +
> + walk_start = walk_end = p.arg.start;
> + for (; walk_end != p.arg.end; walk_start = walk_end) {
> + int n_out;
> + walk_end = min_t(unsigned long,
> + (walk_start + PAGEMAP_WALK_SIZE) & PAGEMAP_WALK_MASK,
> + p.arg.end);
> +
> + ret = mmap_read_lock_killable(mm);
> + if (ret)
> + break;
> + ret = walk_page_range(mm, walk_start, walk_end,
> + &pagemap_scan_ops, &p);
> + mmap_read_unlock(mm);
> +
> + n_out = pagemap_scan_flush_buffer(&p);
> + if (n_out < 0)
> + ret = n_out;
> + else
> + n_ranges_out += n_out;
> +
> + if (ret)
> + break;
> + }
> +
> + if (p.cur_buf.start != p.cur_buf.end) {
> + if (copy_to_user(p.vec_out, &p.cur_buf, sizeof(p.cur_buf)))
> + ret = -EFAULT;
> + else
> + ++n_ranges_out;
> + }
> +
> + /* ENOSPC signifies early stop (buffer full) from the walk. */
> + if (!ret || ret == -ENOSPC)
> + ret = n_ranges_out;
> +
> + p.arg.start = p.end_addr ? p.end_addr : walk_start;
> + if (pagemap_scan_writeback_args(&p.arg, uarg))
> + ret = -EFAULT;
> +
> + if (p.arg.flags & PM_SCAN_WP_MATCHING)
> + mmu_notifier_invalidate_range_end(&range);
> +
> + kfree(p.vec_buf);
> + return ret;
> +}
> +
> +static long do_pagemap_cmd(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct mm_struct *mm = file->private_data;
> +
> + switch (cmd) {
> + case PAGEMAP_SCAN:
> + return do_pagemap_scan(mm, arg);
> +
> + default:
> + 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 = do_pagemap_cmd,
> + .compat_ioctl = do_pagemap_cmd,
> };
> #endif /* CONFIG_PROC_PAGE_MONITOR */
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 9f4bac3df59e..4f5fed605538 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -259,6 +259,7 @@ long hugetlb_change_protection(struct vm_area_struct *vma,
> unsigned long cp_flags);
>
> bool is_hugetlb_entry_migration(pte_t pte);
> +bool is_hugetlb_entry_hwpoisoned(pte_t pte);
> void hugetlb_unshare_all_pmds(struct vm_area_struct *vma);
>
> #else /* !CONFIG_HUGETLB_PAGE */
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index b7b56871029c..db39442befcb 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -305,4 +305,60 @@ 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 pm_scan_arg)
> +
> +/* Bits are set in flags of the page_region and masks in pm_scan_args */
> +#define PAGE_IS_WPASYNC (1 << 0)
> +#define PAGE_IS_WRITTEN (1 << 1)
> +#define PAGE_IS_FILE (1 << 2)
> +#define PAGE_IS_PRESENT (1 << 3)
> +#define PAGE_IS_SWAPPED (1 << 4)
> +#define PAGE_IS_PFNZERO (1 << 5)
> +
> +/*
> + * struct page_region - Page region with flags
> + * @start: Start of the region
> + * @end: End of the region (exclusive)
> + * @categories: PAGE_IS_* category bitmask for the region
> + */
> +struct page_region {
> + __u64 start;
> + __u64 end;
> + __u64 categories;
> +};
> +
> +/* Flags for PAGEMAP_SCAN ioctl */
> +#define PM_SCAN_WP_MATCHING (1 << 0) /* Write protect the pages matched. */
> +#define PM_SCAN_CHECK_WPASYNC (1 << 1) /* Abort the scan when a non-WP-enabled page is found. */
> +
> +/*
> + * struct pm_scan_arg - Pagemap ioctl argument
> + * @size: Size of the structure
> + * @flags: Flags for the IOCTL
> + * @start: Starting address of the region
> + * (Ending address of the walk is also returned in it)
> + * @end: Ending address of the region
> + * @vec: Address of page_region struct array for output
> + * @vec_len: Length of the page_region struct array
> + * @max_pages: Optional limit for number of returned pages (0 = disabled)
> + * @category_inverted: PAGE_IS_* categories which values match if 0 instead of 1
> + * @category_mask: Skip pages for which any category doesn't match
> + * @category_anyof_mask: Skip pages for which no category matches
> + * @return_mask: PAGE_IS_* categories that are to be reported in `page_region`s returned
> + */
> +struct pm_scan_arg {
> + __u64 size;
> + __u64 flags;
> + __u64 start;
> + __u64 end;
> + __u64 vec;
> + __u64 vec_len;
> + __u64 max_pages;
> + __u64 category_inverted;
> + __u64 category_mask;
> + __u64 category_anyof_mask;
> + __u64 return_mask;
> +};
> +
> #endif /* _UAPI_LINUX_FS_H */
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 751968eff92d..4bf8cc4aba2e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5008,7 +5008,7 @@ bool is_hugetlb_entry_migration(pte_t pte)
> return false;
> }
>
> -static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
> +bool is_hugetlb_entry_hwpoisoned(pte_t pte)
> {
> swp_entry_t swp;
>

--
BR,
Muhammad Usama Anjum

2023-07-21 12:03:41

by Michał Mirosław

[permalink] [raw]

Subject: Re: fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

2023-07-21 12:03:45

by Michał Mirosław

[permalink] [raw]

Subject: Re: fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On Fri, Jul 21, 2023 at 03:48:22PM +0500, Muhammad Usama Anjum wrote:
> On 7/21/23 12:28 AM, Michał Mirosław wrote:
> > This is a massaged version of patch by Muhammad Usama Anjum [1]
> > to illustrate my review comments and hopefully push the implementation
> > efforts closer to conclusion. The changes are:
> Thank you so much for this effort. I also want to reach conclusion. I'll
> agree with all the changes which don't affect me. But some requirements
> aren't being fulfilled with this current design.
>
> >
> > 1. the API:
[...]
> > b. rename match "flags" to 'page categories' everywhere - this makes
> > it easier to differentiate the ioctl()s categorisation of pages
> > from struct page flags;
> I've no problem with it.
>
> #define PAGE_IS_WPASYNC (1 << 0)
> #define PAGE_IS_WRITTEN (1 << 1)
> You have another new flag PAGE_IS_WPASYNC. But there is no application of
> PAGE_IS_WPASYNC. We must not add a flag which don't have any user.

Please see below.

> > c. change {required + excluded} to {inverted + required}. This was
> > rejected before, but I'd like to illustrate the difference.
> > Old interface can be translated to the new by:
> > categories_inverted = excluded_mask
> > categories_mask = required_mask | excluded_mask
> > categories_anyof_mask = anyof_mask
> > The new way allows filtering by: A & (B | !C)
> > categories_inverted = C
> > categories_mask = A
> > categories_anyof_mask = B | C
> I'm still unable to get the idea of inverted masks. IIRC Andei had also not
> supported/accepted this masking scheme. But I'll be okay with it if he
> supports this masking.

Please note that the masks are not inverted -- the values are. Masks
select which categories you want to filter on, and category_inverted
invert the meaning of a match (match 0 instead of 1).

> > d. change the ioctl to be a SCAN with optional WP. Addressing the
> > original use-case, GetWriteWatch() can be implemented as:
> As I've mentioned several times previously (without the name of
> ResetWriteWatch()) that we need exclusive WP without GET. This could be
> implemented with UFFDIO_WRITEPROTECT. But when we use UFFDIO_WRITEPROTECT,
> we hit some special case and performance is very slow. So with UFFD WP
> expert, Peter Xu we have decided to put exclusive WP in this IOCTL for
> implementation of ResetWriteWatch().
>
> A lot of simplification of the patch is made possible because of not
> keeping exclusive WP. (You have also written some quality code, more better.)
> >
> > memset(&args, 0, sizeof(args));
> > args.start = lpBaseAddress;
> > args.end = lpBaseAddress + dwRegionSize;
> > args.max_pages = *lpdwCount;
> > *lpdwGranularity = PAGE_SIZE;
> > args.flags = PM_SCAN_CHECK_WPASYNC;
> > if (dwFlags & WRITE_WATCH_FLAG_RESET)
> > args.flags |= PM_SCAN_WP_MATCHED;
> > args.categories_mask = PAGE_IS_WRITTEN;
> > args.return_mask = PAGE_IS_WRITTEN;

For ResetWriteWatch() you would:

args.flags = PM_SCAN_WP_MATCHING;
args.categories_mask = PAGE_IS_WPASYNC | PAGE_IS_WRITTEN;
args.return_mask = 0;

Or (if you want to error out if the range doesn't have WP enabled):

args.flags = PM_SCAN_WP_MATCHING | PM_SCAN_CHECK_WPASYNC;
args.categories_mask = PAGE_IS_WRITTEN;
args.return_mask = 0;

(PM_SCAN_CHECK_WPASYNC is effectively adding PAGE_IS_WPASYNC to the
required categories.)

[...]

> > 2. the implementation:
> > a. gather the page-categorising and write-protecting code in one place;
> Agreed.
>
> > b. optimization: add whole-vma skipping for WP usecase;
> I don't know who can benefit from it. Do you have any user in mind? When
> the user come of this optimization, this can be added later.

This is for users of WP that want to ignore WP for non-registered ranges
instead of erroring out on them. (I anticipate CRIU to use this.)

> > c. extracted output limiting code to pagemap_scan_output();
> If user passes half THP, current code wouldn't split huge page and WP the
> whole THP. We would loose the dirty state of other half huge page. This is
> bug. consoliding the output limiting code looks optimal, but we'll need to
> same limiting code to detect if full THP hasn't been passed in case of THP
> and HugeTLB.

For THP indeed - the code should check `end != start + HPAGE_SIZE`
instead of `ret == -ENOSPC`.

For HugeTLB there is a check that returns EINVAL when trying to WP
a partial page. I think I didn't change that part.

> > d. extracted range coalescing to pagemap_scan_push_range();
> My old pagemap_scan_output has become pagemap_scan_push_range().

Indeed. I did first push the max_pages check in, hence the 'extracting'
later.

> > e. extracted THP entry handling to pagemap_scan_thp_entry();
> Good. But I didn't found value in seperating it just like other historic
> pagemap code.

This is to avoid having to much indentation and long functions that do
many things at once.

> > f. added a shortcut for non-WP hugetlb scan; avoids conditional
> > locking;
> Yeah, some if conditions have been removed. But overall did couple of calls
> to is_interesting and scan_output functions instead of just one.

Yes, there are now two pairs instead of one. I see that I haven't pushed
the is_interesting calls into scan_output. This is now trivial:
if (!interesting...) {
*end = start;
return 0;
}
and could save some typing (but would need a different name for
scan_output as it would do filter & output), but I'm not sure about
readability.

Best Regards
Michał Mirosław

2023-07-21 18:14:54

by Muhammad Usama Anjum

[permalink] [raw]

Subject: Re: fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On 7/21/23 4:23 PM, Michał Mirosław wrote:
> On Fri, Jul 21, 2023 at 03:48:22PM +0500, Muhammad Usama Anjum wrote:
>> On 7/21/23 12:28 AM, Michał Mirosław wrote:
>>> This is a massaged version of patch by Muhammad Usama Anjum [1]
>>> to illustrate my review comments and hopefully push the implementation
>>> efforts closer to conclusion. The changes are:
>> Thank you so much for this effort. I also want to reach conclusion. I'll
>> agree with all the changes which don't affect me. But some requirements
>> aren't being fulfilled with this current design.
>>
>>>
>>> 1. the API:
> [...]
>>> b. rename match "flags" to 'page categories' everywhere - this makes
>>> it easier to differentiate the ioctl()s categorisation of pages
>>> from struct page flags;
>> I've no problem with it.
>>
>> #define PAGE_IS_WPASYNC (1 << 0)
>> #define PAGE_IS_WRITTEN (1 << 1)
>> You have another new flag PAGE_IS_WPASYNC. But there is no application of
>> PAGE_IS_WPASYNC. We must not add a flag which don't have any user.
>
> Please see below.
>
>>> c. change {required + excluded} to {inverted + required}. This was
>>> rejected before, but I'd like to illustrate the difference.
>>> Old interface can be translated to the new by:
>>> categories_inverted = excluded_mask
>>> categories_mask = required_mask | excluded_mask
>>> categories_anyof_mask = anyof_mask
>>> The new way allows filtering by: A & (B | !C)
>>> categories_inverted = C
>>> categories_mask = A
>>> categories_anyof_mask = B | C
>> I'm still unable to get the idea of inverted masks. IIRC Andei had also not
>> supported/accepted this masking scheme. But I'll be okay with it if he
>> supports this masking.
>
> Please note that the masks are not inverted -- the values are. Masks
> select which categories you want to filter on, and category_inverted
> invert the meaning of a match (match 0 instead of 1).
>
>>> d. change the ioctl to be a SCAN with optional WP. Addressing the
>>> original use-case, GetWriteWatch() can be implemented as:
>> As I've mentioned several times previously (without the name of
>> ResetWriteWatch()) that we need exclusive WP without GET. This could be
>> implemented with UFFDIO_WRITEPROTECT. But when we use UFFDIO_WRITEPROTECT,
>> we hit some special case and performance is very slow. So with UFFD WP
>> expert, Peter Xu we have decided to put exclusive WP in this IOCTL for
>> implementation of ResetWriteWatch().
>>
>> A lot of simplification of the patch is made possible because of not
>> keeping exclusive WP. (You have also written some quality code, more better.)
>>>
>>> memset(&args, 0, sizeof(args));
>>> args.start = lpBaseAddress;
>>> args.end = lpBaseAddress + dwRegionSize;
>>> args.max_pages = *lpdwCount;
>>> *lpdwGranularity = PAGE_SIZE;
>>> args.flags = PM_SCAN_CHECK_WPASYNC;
>>> if (dwFlags & WRITE_WATCH_FLAG_RESET)
>>> args.flags |= PM_SCAN_WP_MATCHED;
>>> args.categories_mask = PAGE_IS_WRITTEN;
>>> args.return_mask = PAGE_IS_WRITTEN;
>
> For ResetWriteWatch() you would:
>
> args.flags = PM_SCAN_WP_MATCHING;
> args.categories_mask = PAGE_IS_WPASYNC | PAGE_IS_WRITTEN;
> args.return_mask = 0;
>
> Or (if you want to error out if the range doesn't have WP enabled):
>
> args.flags = PM_SCAN_WP_MATCHING | PM_SCAN_CHECK_WPASYNC;
> args.categories_mask = PAGE_IS_WRITTEN;
> args.return_mask = 0;
>
> (PM_SCAN_CHECK_WPASYNC is effectively adding PAGE_IS_WPASYNC to the
> required categories.)
Right. But we don't want to perform GET in case of ResetWriteWatch(). It'll
be wasted effort to perform GET as well when we don't care about the dirty
status of the pages.

>
> [...]
>
>>> 2. the implementation:
>>> a. gather the page-categorising and write-protecting code in one place;
>> Agreed.
>>
>>> b. optimization: add whole-vma skipping for WP usecase;
>> I don't know who can benefit from it. Do you have any user in mind? When
>> the user come of this optimization, this can be added later.
>
> This is for users of WP that want to ignore WP for non-registered ranges
> instead of erroring out on them. (I anticipate CRIU to use this.)
>
>>> c. extracted output limiting code to pagemap_scan_output();
>> If user passes half THP, current code wouldn't split huge page and WP the
>> whole THP. We would loose the dirty state of other half huge page. This is
>> bug. consoliding the output limiting code looks optimal, but we'll need to
>> same limiting code to detect if full THP hasn't been passed in case of THP
>> and HugeTLB.
>
> For THP indeed - the code should check `end != start + HPAGE_SIZE`
> instead of `ret == -ENOSPC`.
Yeah, this need to be fixed.

>
> For HugeTLB there is a check that returns EINVAL when trying to WP
> a partial page. I think I didn't change that part.
>
>>> d. extracted range coalescing to pagemap_scan_push_range();
>> My old pagemap_scan_output has become pagemap_scan_push_range().
>
> Indeed. I did first push the max_pages check in, hence the 'extracting'
> later.
>
>>> e. extracted THP entry handling to pagemap_scan_thp_entry();
>> Good. But I didn't found value in seperating it just like other historic
>> pagemap code.
>
> This is to avoid having to much indentation and long functions that do
> many things at once.
>
>>> f. added a shortcut for non-WP hugetlb scan; avoids conditional
>>> locking;
>> Yeah, some if conditions have been removed. But overall did couple of calls
>> to is_interesting and scan_output functions instead of just one.
>
> Yes, there are now two pairs instead of one. I see that I haven't pushed
> the is_interesting calls into scan_output. This is now trivial:
> if (!interesting...) {
> *end = start;
> return 0;
> }
This can be the 3rd thing to fix.

Is it possible for you to fix the above mentioned 3 things and send the
patch for my testing:
1 Make GET optional
2 Detect partial THP WP operation and split
3 Optimization of moving this interesting logic to output() function

Please let me know if you cannot make the above fixes. I'll mix my patch
version and your patch and fix these things up.

> and could save some typing (but would need a different name for
> scan_output as it would do filter & output), but I'm not sure about
> readability.
>
> Best Regards
> Michał Mirosław

--
BR,
Muhammad Usama Anjum

2023-07-21 18:53:23

by Muhammad Usama Anjum

[permalink] [raw]

Subject: Re: fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On 7/21/23 4:29 PM, Michał Mirosław wrote:
> On Fri, Jul 21, 2023 at 03:48:22PM +0500, Muhammad Usama Anjum wrote:
>> On 7/21/23 12:28 AM, Michał Mirosław wrote:
>>> This is a massaged version of patch by Muhammad Usama Anjum [1]
>>> to illustrate my review comments and hopefully push the implementation
>>> efforts closer to conclusion. The changes are:
>> Thank you so much for this effort. I also want to reach conclusion. I'll
>> agree with all the changes which don't affect me. But some requirements
>> aren't being fulfilled with this current design.
> [...]
>> #define PAGE_IS_WPASYNC (1 << 0)
>> #define PAGE_IS_WRITTEN (1 << 1)
>> You have another new flag PAGE_IS_WPASYNC. But there is no application of
>> PAGE_IS_WPASYNC. We must not add a flag which don't have any user.
>
> BTW, I'm not sure I got the PAGE_IS_WPASYNC naming right - this is to
> designate pages that can be write-protected (are UFFD-registered I believe).
PAGE_IS_UFFDWP_REGISTERED or PAGE_IS_WP_REG?

>
> Best Regards
> Michał Mirosław

--
BR,
Muhammad Usama Anjum

2023-07-22 00:53:47

by Michał Mirosław

[permalink] [raw]

Subject: Re: fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On Fri, Jul 21, 2023 at 10:50:19PM +0500, Muhammad Usama Anjum wrote:
> On 7/21/23 4:23 PM, Michał Mirosław wrote:
> > On Fri, Jul 21, 2023 at 03:48:22PM +0500, Muhammad Usama Anjum wrote:
> >> On 7/21/23 12:28 AM, Michał Mirosław wrote:
[...]
> >>> d. change the ioctl to be a SCAN with optional WP. Addressing the
> >>> original use-case, GetWriteWatch() can be implemented as:
> >> As I've mentioned several times previously (without the name of
> >> ResetWriteWatch()) that we need exclusive WP without GET. This could be
> >> implemented with UFFDIO_WRITEPROTECT. But when we use UFFDIO_WRITEPROTECT,
> >> we hit some special case and performance is very slow. So with UFFD WP
> >> expert, Peter Xu we have decided to put exclusive WP in this IOCTL for
> >> implementation of ResetWriteWatch().
> >>
> >> A lot of simplification of the patch is made possible because of not
> >> keeping exclusive WP. (You have also written some quality code, more better.)
> >>>
> >>> memset(&args, 0, sizeof(args));
> >>> args.start = lpBaseAddress;
> >>> args.end = lpBaseAddress + dwRegionSize;
> >>> args.max_pages = *lpdwCount;
> >>> *lpdwGranularity = PAGE_SIZE;
> >>> args.flags = PM_SCAN_CHECK_WPASYNC;
> >>> if (dwFlags & WRITE_WATCH_FLAG_RESET)
> >>> args.flags |= PM_SCAN_WP_MATCHED;
> >>> args.categories_mask = PAGE_IS_WRITTEN;
> >>> args.return_mask = PAGE_IS_WRITTEN;
> >
> > For ResetWriteWatch() you would:
> >
> > args.flags = PM_SCAN_WP_MATCHING;
> > args.categories_mask = PAGE_IS_WPASYNC | PAGE_IS_WRITTEN;
> > args.return_mask = 0;
> >
> > Or (if you want to error out if the range doesn't have WP enabled):
> >
> > args.flags = PM_SCAN_WP_MATCHING | PM_SCAN_CHECK_WPASYNC;
> > args.categories_mask = PAGE_IS_WRITTEN;
> > args.return_mask = 0;
> >
> > (PM_SCAN_CHECK_WPASYNC is effectively adding PAGE_IS_WPASYNC to the
> > required categories.)
> Right. But we don't want to perform GET in case of ResetWriteWatch(). It'll
> be wasted effort to perform GET as well when we don't care about the dirty
> status of the pages.

This doesn't really do GET: return_mask == 0 means that there won't be any
ranges reported (and you can pass {NULL, 0} for arg.{vec, vec_len}). But
I've changed the no-GET criteria to vec == NULL (requires vec_len == 0).

> Is it possible for you to fix the above mentioned 3 things and send the
> patch for my testing:
> 1 Make GET optional
> 2 Detect partial THP WP operation and split
> 3 Optimization of moving this interesting logic to output() function
>
> Please let me know if you cannot make the above fixes. I'll mix my patch
> version and your patch and fix these things up.

Sending as a reply.

Best Regards
Michał Mirosław

2023-07-22 01:25:00

by Michał Mirosław

[permalink] [raw]

Subject: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

Signed-off-by: Muhammad Usama Anjum <[email protected]>
Signed-off-by: Micha? Miros?aw <[email protected]>

Changes from v1 [1]:

API:
1. Change no-GET operation condition from `arg.return_mask == 0` to
`arg.vec == NULL`. This will allow issuing the ioctl with
return_mask == 0 to gather matching ranges when the exact category
is not interesting. (Anticipated for CRIU scanning a large sparse
anonymous mapping).

Implementation:
1. Fixed splitting THPs if processing a non-full range.
2. Pushed ...is_interesting_page() call to ...output().

[1] https://lore.kernel.org/linux-mm/[email protected]/T/#mc8ad9ca249a8ec01980791539abcb8a221154e20

Best Regards
Micha? Miros?aw

Signed-off-by: Micha? Miros?aw <[email protected]>
---
fs/proc/task_mmu.c | 641 ++++++++++++++++++++++++++++++++++++++++
include/linux/hugetlb.h | 1 +
include/uapi/linux/fs.h | 56 ++++
mm/hugetlb.c | 2 +-
4 files changed, 699 insertions(+), 1 deletion(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index c1e6531cb02a..568e76f64824 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -19,6 +19,8 @@
#include <linux/shmem_fs.h>
#include <linux/uaccess.h>
#include <linux/pkeys.h>
+#include <linux/minmax.h>
+#include <linux/overflow.h>

#include <asm/elf.h>
#include <asm/tlb.h>
@@ -1749,11 +1751,650 @@ static int pagemap_release(struct inode *inode, struct file *file)
return 0;
}

+#define PM_SCAN_CATEGORIES (PAGE_IS_WPASYNC | PAGE_IS_WRITTEN | \
+ PAGE_IS_FILE | PAGE_IS_PRESENT | \
+ PAGE_IS_SWAPPED | PAGE_IS_PFNZERO)
+#define PM_SCAN_FLAGS (PM_SCAN_WP_MATCHING | PM_SCAN_CHECK_WPASYNC)
+
+struct pagemap_scan_private {
+ struct pm_scan_arg arg;
+ unsigned long cur_vma_category;
+ struct page_region *vec_buf, cur_buf;
+ unsigned long vec_buf_len, vec_buf_index, found_pages, end_addr;
+ struct page_region __user* vec_out;
+};
+
+static unsigned long pagemap_page_category(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
+{
+ unsigned long categories = 0;
+
+ if (pte_present(pte)) {
+ struct page *page = vm_normal_page(vma, addr, pte);
+
+ categories |= PAGE_IS_PRESENT;
+ if (!pte_uffd_wp(pte))
+ categories |= PAGE_IS_WRITTEN;
+ if (page && !PageAnon(page))
+ categories |= PAGE_IS_FILE;
+ if (is_zero_pfn(pte_pfn(pte)))
+ categories |= PAGE_IS_PFNZERO;
+ } else if (is_swap_pte(pte)) {
+ swp_entry_t swp = pte_to_swp_entry(pte);
+
+ categories |= PAGE_IS_SWAPPED;
+ if (!pte_swp_uffd_wp_any(pte))
+ categories |= PAGE_IS_WRITTEN;
+ if (is_pfn_swap_entry(swp) && !PageAnon(pfn_swap_entry_to_page(swp)))
+ categories |= PAGE_IS_FILE;
+ }
+
+ return categories;
+}
+
+static void make_uffd_wp_pte(struct vm_area_struct *vma,
+ unsigned long addr, pte_t *pte)
+{
+ pte_t ptent = ptep_get(pte);
+
+ if (pte_present(ptent)) {
+ pte_t old_pte;
+
+ old_pte = ptep_modify_prot_start(vma, addr, pte);
+ ptent = pte_mkuffd_wp(ptent);
+ ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent);
+ } else if (is_swap_pte(ptent)) {
+ ptent = pte_swp_mkuffd_wp(ptent);
+ set_pte_at(vma->vm_mm, addr, pte, ptent);
+ } else {
+ set_pte_at(vma->vm_mm, addr, pte,
+ make_pte_marker(PTE_MARKER_UFFD_WP));
+ }
+}
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+static unsigned long pagemap_thp_category(pmd_t pmd)
+{
+ unsigned long categories = 0;
+
+ if (pmd_present(pmd)) {
+ categories |= PAGE_IS_PRESENT;
+ if (!pmd_uffd_wp(pmd))
+ categories |= PAGE_IS_WRITTEN;
+ if (is_zero_pfn(pmd_pfn(pmd)))
+ categories |= PAGE_IS_PFNZERO;
+ } else if (is_swap_pmd(pmd)) {
+ categories |= PAGE_IS_SWAPPED;
+ if (!pmd_swp_uffd_wp(pmd))
+ categories |= PAGE_IS_WRITTEN;
+ }
+
+ return categories;
+}
+
+static void make_uffd_wp_pmd(struct vm_area_struct *vma,
+ unsigned long addr, pmd_t *pmdp)
+{
+ pmd_t old, pmd = *pmdp;
+
+ if (pmd_present(pmd)) {
+ old = pmdp_invalidate_ad(vma, addr, pmdp);
+ pmd = pmd_mkuffd_wp(old);
+ set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
+ } else if (is_migration_entry(pmd_to_swp_entry(pmd))) {
+ pmd = pmd_swp_mkuffd_wp(pmd);
+ set_pmd_at(vma->vm_mm, addr, pmdp, pmd);
+ }
+}
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
+
+#ifdef CONFIG_HUGETLB_PAGE
+static unsigned long pagemap_hugetlb_category(pte_t pte)
+{
+ unsigned long categories = 0;
+
+ if (pte_present(pte)) {
+ categories |= PAGE_IS_PRESENT;
+ if (!huge_pte_uffd_wp(pte))
+ categories |= PAGE_IS_WRITTEN;
+ if (!PageAnon(pte_page(pte)))
+ categories |= PAGE_IS_FILE;
+ if (is_zero_pfn(pte_pfn(pte)))
+ categories |= PAGE_IS_PFNZERO;
+ } else if (is_swap_pte(pte)) {
+ categories |= PAGE_IS_SWAPPED;
+ if (!pte_swp_uffd_wp_any(pte))
+ categories |= PAGE_IS_WRITTEN;
+ }
+
+ return categories;
+}
+
+static void make_uffd_wp_huge_pte(struct vm_area_struct *vma,
+ unsigned long addr, pte_t *ptep,
+ pte_t ptent)
+{
+ if (is_hugetlb_entry_hwpoisoned(ptent) || is_pte_marker(ptent))
+ return;
+
+ if (is_hugetlb_entry_migration(ptent))
+ set_huge_pte_at(vma->vm_mm, addr, ptep,
+ pte_swp_mkuffd_wp(ptent));
+ else if (!huge_pte_none(ptent))
+ huge_ptep_modify_prot_commit(vma, addr, ptep, ptent,
+ huge_pte_mkuffd_wp(ptent));
+ else
+ set_huge_pte_at(vma->vm_mm, addr, ptep,
+ make_pte_marker(PTE_MARKER_UFFD_WP));
+}
+#endif /* CONFIG_HUGETLB_PAGE */
+
+static bool pagemap_scan_is_interesting_page(unsigned long categories,
+ const struct pagemap_scan_private *p)
+{
+ categories ^= p->arg.category_inverted;
+ if ((categories & p->arg.category_mask) != p->arg.category_mask)
+ return false;
+ if (p->arg.category_anyof_mask && !(categories & p->arg.category_anyof_mask))
+ return false;
+
+ return true;
+}
+
+static bool pagemap_scan_is_interesting_vma(unsigned long categories,
+ const struct pagemap_scan_private *p)
+{
+ unsigned long required = p->arg.category_mask & PAGE_IS_WPASYNC;
+
+ categories ^= p->arg.category_inverted;
+ if ((categories & required) != required)
+ return false;
+ return true;
+}
+
+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;
+ unsigned long vma_category = 0;
+
+ if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
+ vma_category |= PAGE_IS_WPASYNC;
+ else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
+ return -EPERM;
+
+ if (vma->vm_flags & VM_PFNMAP)
+ return 1;
+
+ if (!pagemap_scan_is_interesting_vma(vma_category, p))
+ return 1;
+
+ p->cur_vma_category = vma_category;
+ return 0;
+}
+
+static bool pagemap_scan_push_range(unsigned long categories,
+ struct pagemap_scan_private *p,
+ unsigned long addr, unsigned long end)
+{
+ struct page_region *cur_buf = &p->cur_buf;
+
+ /*
+ * When there is no output buffer provided at all, the sentinel values
+ * won't match here. There is no other way for `cur_buf->end` to be
+ * non-zero other than it being non-empty.
+ */
+ if (addr == cur_buf->end && categories == cur_buf->categories) {
+ cur_buf->end = end;
+ return true;
+ }
+
+ if (cur_buf->end) {
+ if (p->vec_buf_index >= p->vec_buf_len)
+ return false;
+
+ memcpy(&p->vec_buf[p->vec_buf_index], cur_buf,
+ sizeof(*p->vec_buf));
+ ++p->vec_buf_index;
+ }
+
+ cur_buf->start = addr;
+ cur_buf->end = end;
+ cur_buf->categories = categories;
+ return true;
+}
+
+static void pagemap_scan_backout_range(struct pagemap_scan_private *p,
+ unsigned long addr, unsigned long end)
+{
+ struct page_region *cur_buf = &p->cur_buf;
+
+ if (cur_buf->start != addr)
+ cur_buf->end = addr;
+ else
+ cur_buf->start = cur_buf->end = 0;
+
+ p->end_addr = 0;
+ p->found_pages -= (end - addr) / PAGE_SIZE;
+}
+
+static int pagemap_scan_output(unsigned long categories,
+ struct pagemap_scan_private *p,
+ unsigned long addr, unsigned long *end)
+{
+ unsigned long n_pages, total_pages;
+ int ret = 0;
+
+ if (!pagemap_scan_is_interesting_page(categories, p)) {
+ *end = addr;
+ return 0;
+ }
+
+ if (!p->vec_buf)
+ return 0;
+
+ categories &= p->arg.return_mask;
+
+ n_pages = (*end - addr) / PAGE_SIZE;
+ if (check_add_overflow(p->found_pages, n_pages, &total_pages) || total_pages > p->arg.max_pages) {
+ size_t n_too_much = total_pages - p->arg.max_pages;
+ *end -= n_too_much * PAGE_SIZE;
+ n_pages -= n_too_much;
+ ret = -ENOSPC;
+ }
+
+ if (!pagemap_scan_push_range(categories, p, addr, *end)) {
+ *end = addr;
+ n_pages = 0;
+ ret = -ENOSPC;
+ }
+
+ p->found_pages += n_pages;
+ if (ret)
+ p->end_addr = *end;
+ return ret;
+}
+
+static int pagemap_scan_thp_entry(pmd_t *pmd, unsigned long start,
+ unsigned long end, struct mm_walk *walk)
+{
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+ struct pagemap_scan_private *p = walk->private;
+ struct vm_area_struct *vma = walk->vma;
+ unsigned long categories;
+ spinlock_t *ptl;
+ int ret = 0;
+
+ ptl = pmd_trans_huge_lock(pmd, vma);
+ if (!ptl)
+ return -ENOENT;
+
+ categories = p->cur_vma_category | pagemap_thp_category(*pmd);
+
+ ret = pagemap_scan_output(categories, p, start, &end);
+ if (start == end)
+ goto out_unlock;
+
+ if (~p->arg.flags & PM_SCAN_WP_MATCHING)
+ goto out_unlock;
+ if (~categories & PAGE_IS_WRITTEN)
+ goto out_unlock;
+
+ /*
+ * Break huge page into small pages if the WP operation
+ * need to be performed is on a portion of the huge page.
+ */
+ if (end != addr + HPAGE_SIZE) {
+ spin_unlock(ptl);
+ split_huge_pmd(vma, pmd, start);
+ pagemap_scan_backout_range(p, start, end);
+ return -ENOENT;
+ }
+
+ make_uffd_wp_pmd(vma, start, pmd);
+ flush_tlb_range(vma, start, end);
+out_unlock:
+ spin_unlock(ptl);
+ return ret;
+#else /* !CONFIG_TRANSPARENT_HUGEPAGE */
+ return -ENOENT;
+#endif
+}
+
+static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
+ unsigned long end, struct mm_walk *walk)
+{
+ struct pagemap_scan_private *p = walk->private;
+ struct vm_area_struct *vma = walk->vma;
+ pte_t *pte, *start_pte;
+ unsigned long addr;
+ bool flush = false;
+ spinlock_t *ptl;
+ int ret;
+
+ arch_enter_lazy_mmu_mode();
+
+ ret = pagemap_scan_thp_entry(pmd, start, end, walk);
+ if (ret != -ENOENT) {
+ arch_leave_lazy_mmu_mode();
+ return ret;
+ }
+
+ start_pte = pte = pte_offset_map_lock(vma->vm_mm, pmd, start, &ptl);
+ if (!pte) {
+ arch_leave_lazy_mmu_mode();
+ walk->action = ACTION_AGAIN;
+ return 0;
+ }
+
+ for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
+ unsigned long categories = p->cur_vma_category |
+ pagemap_page_category(vma, addr, ptep_get(pte));
+ unsigned long next = addr + PAGE_SIZE;
+
+ ret = pagemap_scan_output(categories, p, addr, &next);
+ if (next == addr)
+ break;
+
+ if (~p->arg.flags & PM_SCAN_WP_MATCHING)
+ continue;
+ if (~categories & PAGE_IS_WRITTEN)
+ continue;
+
+ make_uffd_wp_pte(vma, addr, pte);
+ if (!flush) {
+ start = addr;
+ flush = true;
+ }
+ }
+
+ if (flush)
+ flush_tlb_range(vma, start, addr);
+
+ pte_unmap_unlock(start_pte, ptl);
+ arch_leave_lazy_mmu_mode();
+
+ cond_resched();
+ return ret;
+}
+
+#ifdef CONFIG_HUGETLB_PAGE
+static int pagemap_scan_hugetlb_entry(pte_t *ptep, unsigned long hmask,
+ unsigned long start, unsigned long end,
+ struct mm_walk *walk)
+{
+ struct pagemap_scan_private *p = walk->private;
+ struct vm_area_struct *vma = walk->vma;
+ unsigned long categories;
+ spinlock_t *ptl;
+ int ret = 0;
+ pte_t pte;
+
+ if (~p->arg.flags & PM_SCAN_WP_MATCHING) {
+ /* Go the short route when not write-protecting pages. */
+
+ pte = huge_ptep_get(ptep);
+ categories = p->cur_vma_category | pagemap_hugetlb_category(pte);
+
+ return pagemap_scan_output(categories, p, start, &end);
+ }
+
+ if (end != start + HPAGE_SIZE) {
+ /* Partial HugeTLB page WP isn't possible. */
+ p->end_addr = start;
+ return -EINVAL;
+ }
+
+ i_mmap_lock_write(vma->vm_file->f_mapping);
+ ptl = huge_pte_lock(hstate_vma(vma), vma->vm_mm, ptep);
+
+ pte = huge_ptep_get(ptep);
+ categories = p->cur_vma_category | pagemap_hugetlb_category(pte);
+
+ ret = pagemap_scan_output(categories, p, start, &end);
+ if (start == end)
+ goto out_unlock;
+
+ if (categories & PAGE_IS_WRITTEN) {
+ make_uffd_wp_huge_pte(vma, start, ptep, pte);
+ flush_hugetlb_tlb_range(vma, start, end);
+ }
+
+out_unlock:
+ spin_unlock(ptl);
+ i_mmap_unlock_write(vma->vm_file->f_mapping);
+
+ return ret;
+}
+#else
+#define pagemap_scan_hugetlb_entry NULL
+#endif
+
+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;
+ int ret;
+
+ if (!vma)
+ return 0;
+
+ ret = pagemap_scan_output(p->cur_vma_category, p, addr, &end);
+ if (addr == end)
+ return ret;
+
+ if (~p->arg.flags & PM_SCAN_WP_MATCHING)
+ return ret;
+
+ int err = uffd_wp_range(vma, addr, end - addr, true);
+ if (err < 0)
+ ret = err;
+
+ 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,
+ .hugetlb_entry = pagemap_scan_hugetlb_entry,
+};
+
+static int pagemap_scan_get_args(struct pm_scan_arg *arg,
+ unsigned long uarg)
+{
+ unsigned long start, end, vec;
+
+ if (copy_from_user(arg, (void __user *)uarg, sizeof(*arg)))
+ return -EFAULT;
+
+ if (arg->size != sizeof(struct pm_scan_arg))
+ return -EINVAL;
+
+ /* Validate requested features */
+ if (arg->flags & ~PM_SCAN_FLAGS)
+ return -EINVAL;
+ if ((arg->category_inverted | arg->category_mask |
+ arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
+ return -EINVAL;
+
+ start = untagged_addr((unsigned long)arg->start);
+ end = untagged_addr((unsigned long)arg->end);
+ vec = untagged_addr((unsigned long)arg->vec);
+
+ /* Validate memory pointers */
+ if (!IS_ALIGNED(start, PAGE_SIZE))
+ return -EINVAL;
+ if (!access_ok((void __user *)start, end - start))
+ return -EFAULT;
+ if (!vec && arg->vec_len)
+ return -EFAULT;
+ if (vec && !access_ok((void __user *)vec,
+ arg->vec_len * sizeof(struct page_region)))
+ return -EFAULT;
+
+ /* Fixup default values */
+ if (!arg->max_pages)
+ arg->max_pages = ULONG_MAX;
+
+ return 0;
+}
+
+static int pagemap_scan_writeback_args(struct pm_scan_arg *arg,
+ unsigned long uargl)
+{
+ struct pm_scan_arg __user *uarg = (void __user *)uargl;
+
+ if (copy_to_user(&uarg->start, &arg->start, sizeof(arg->start)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int pagemap_scan_init_bounce_buffer(struct pagemap_scan_private *p)
+{
+ if (!p->arg.vec_len) {
+ /*
+ * An arbitrary non-page-aligned sentinel value for
+ * pagemap_scan_push_range().
+ */
+ p->cur_buf.start = p->cur_buf.end = ULLONG_MAX;
+ if (p->arg.vec)
+ p->vec_buf = ZERO_SIZE_PTR;
+ return 0;
+ }
+
+ /*
+ * Allocate a smaller buffer to get output from inside the page
+ * walk functions and walk the range in PAGEMAP_WALK_SIZE chunks.
+ * The last range is always stored in p.cur_buf to allow coalescing
+ * consecutive ranges that have the same categories returned across
+ * walk_page_range() calls.
+ */
+ p->vec_buf_len = min_t(size_t, PAGEMAP_WALK_SIZE >> PAGE_SHIFT,
+ p->arg.vec_len - 1);
+ p->vec_buf = kmalloc_array(p->vec_buf_len, sizeof(*p->vec_buf),
+ GFP_KERNEL);
+ if (!p->vec_buf)
+ return -ENOMEM;
+
+ p->vec_out = (void __user *)p->arg.vec;
+
+ return 0;
+}
+
+static int pagemap_scan_flush_buffer(struct pagemap_scan_private *p)
+{
+ const struct page_region *buf = p->vec_buf;
+ int n = (int)p->vec_buf_index;
+
+ if (!n)
+ return 0;
+
+ if (copy_to_user(p->vec_out, buf, n * sizeof(*buf)))
+ return -EFAULT;
+
+ p->arg.vec_len -= n;
+ p->vec_out += n;
+
+ p->vec_buf_index = 0;
+ p->vec_buf_len = min_t(size_t, p->vec_buf_len, p->arg.vec_len - 1);
+
+ return n;
+}
+
+static long do_pagemap_scan(struct mm_struct *mm, unsigned long uarg)
+{
+ unsigned long walk_start, walk_end;
+ struct mmu_notifier_range range;
+ struct pagemap_scan_private p;
+ size_t n_ranges_out = 0;
+ int ret;
+
+ memset(&p, 0, sizeof(p));
+ ret = pagemap_scan_get_args(&p.arg, uarg);
+ if (ret)
+ return ret;
+
+ ret = pagemap_scan_init_bounce_buffer(&p);
+ if (ret)
+ return ret;
+
+ /* Protection change for the range is going to happen. */
+ if (p.arg.flags & PM_SCAN_WP_MATCHING) {
+ mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA, 0,
+ mm, p.arg.start, p.arg.end);
+ mmu_notifier_invalidate_range_start(&range);
+ }
+
+ walk_start = walk_end = p.arg.start;
+ for (; walk_end != p.arg.end; walk_start = walk_end) {
+ int n_out;
+ walk_end = min_t(unsigned long,
+ (walk_start + PAGEMAP_WALK_SIZE) & PAGEMAP_WALK_MASK,
+ p.arg.end);
+
+ ret = mmap_read_lock_killable(mm);
+ if (ret)
+ break;
+ ret = walk_page_range(mm, walk_start, walk_end,
+ &pagemap_scan_ops, &p);
+ mmap_read_unlock(mm);
+
+ n_out = pagemap_scan_flush_buffer(&p);
+ if (n_out < 0)
+ ret = n_out;
+ else
+ n_ranges_out += n_out;
+
+ if (ret)
+ break;
+ }
+
+ if (p.cur_buf.start != p.cur_buf.end) {
+ if (copy_to_user(p.vec_out, &p.cur_buf, sizeof(p.cur_buf)))
+ ret = -EFAULT;
+ else
+ ++n_ranges_out;
+ }
+
+ /* ENOSPC signifies early stop (buffer full) from the walk. */
+ if (!ret || ret == -ENOSPC)
+ ret = n_ranges_out;
+
+ p.arg.start = p.end_addr ? p.end_addr : walk_start;
+ if (pagemap_scan_writeback_args(&p.arg, uarg))
+ ret = -EFAULT;
+
+ if (p.arg.flags & PM_SCAN_WP_MATCHING)
+ mmu_notifier_invalidate_range_end(&range);
+
+ kfree(p.vec_buf);
+ return ret;
+}
+
+static long do_pagemap_cmd(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct mm_struct *mm = file->private_data;
+
+ switch (cmd) {
+ case PAGEMAP_SCAN:
+ return do_pagemap_scan(mm, arg);
+
+ default:
+ 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 = do_pagemap_cmd,
+ .compat_ioctl = do_pagemap_cmd,
};
#endif /* CONFIG_PROC_PAGE_MONITOR */

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 9f4bac3df59e..4f5fed605538 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -259,6 +259,7 @@ long hugetlb_change_protection(struct vm_area_struct *vma,
unsigned long cp_flags);

bool is_hugetlb_entry_migration(pte_t pte);
+bool is_hugetlb_entry_hwpoisoned(pte_t pte);
void hugetlb_unshare_all_pmds(struct vm_area_struct *vma);

#else /* !CONFIG_HUGETLB_PAGE */
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index b7b56871029c..db39442befcb 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -305,4 +305,60 @@ 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 pm_scan_arg)
+
+/* Bits are set in flags of the page_region and masks in pm_scan_args */
+#define PAGE_IS_WPASYNC (1 << 0)
+#define PAGE_IS_WRITTEN (1 << 1)
+#define PAGE_IS_FILE (1 << 2)
+#define PAGE_IS_PRESENT (1 << 3)
+#define PAGE_IS_SWAPPED (1 << 4)
+#define PAGE_IS_PFNZERO (1 << 5)
+
+/*
+ * struct page_region - Page region with flags
+ * @start: Start of the region
+ * @end: End of the region (exclusive)
+ * @categories: PAGE_IS_* category bitmask for the region
+ */
+struct page_region {
+ __u64 start;
+ __u64 end;
+ __u64 categories;
+};
+
+/* Flags for PAGEMAP_SCAN ioctl */
+#define PM_SCAN_WP_MATCHING (1 << 0) /* Write protect the pages matched. */
+#define PM_SCAN_CHECK_WPASYNC (1 << 1) /* Abort the scan when a non-WP-enabled page is found. */
+
+/*
+ * struct pm_scan_arg - Pagemap ioctl argument
+ * @size: Size of the structure
+ * @flags: Flags for the IOCTL
+ * @start: Starting address of the region
+ * (Ending address of the walk is also returned in it)
+ * @end: Ending address of the region
+ * @vec: Address of page_region struct array for output
+ * @vec_len: Length of the page_region struct array
+ * @max_pages: Optional limit for number of returned pages (0 = disabled)
+ * @category_inverted: PAGE_IS_* categories which values match if 0 instead of 1
+ * @category_mask: Skip pages for which any category doesn't match
+ * @category_anyof_mask: Skip pages for which no category matches
+ * @return_mask: PAGE_IS_* categories that are to be reported in `page_region`s returned
+ */
+struct pm_scan_arg {
+ __u64 size;
+ __u64 flags;
+ __u64 start;
+ __u64 end;
+ __u64 vec;
+ __u64 vec_len;
+ __u64 max_pages;
+ __u64 category_inverted;
+ __u64 category_mask;
+ __u64 category_anyof_mask;
+ __u64 return_mask;
+};
+
#endif /* _UAPI_LINUX_FS_H */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 751968eff92d..4bf8cc4aba2e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5008,7 +5008,7 @@ bool is_hugetlb_entry_migration(pte_t pte)
return false;
}

-static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
+bool is_hugetlb_entry_hwpoisoned(pte_t pte)
{
swp_entry_t swp;

--
2.39.2

2023-07-22 14:43:23

by kernel test robot

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

Hi Michał,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master v6.5-rc2 next-20230721]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Micha-Miros-aw/fs-proc-task_mmu-Implement-IOCTL-for-efficient-page-table-scanning/20230722-082500
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/ZLshsAj5PbsEAHhP%40qmqm.qmqm.pl
patch subject: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20230722/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230722/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

fs/proc/task_mmu.c:1921:6: error: call to undeclared function 'userfaultfd_wp_async'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
^
>> fs/proc/task_mmu.c:2047:13: error: use of undeclared identifier 'addr'
if (end != addr + HPAGE_SIZE) {
^
2 errors generated.

vim +/addr +2047 fs/proc/task_mmu.c

1913
1914 static int pagemap_scan_test_walk(unsigned long start, unsigned long end,
1915 struct mm_walk *walk)
1916 {
1917 struct pagemap_scan_private *p = walk->private;
1918 struct vm_area_struct *vma = walk->vma;
1919 unsigned long vma_category = 0;
1920
> 1921 if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
1922 vma_category |= PAGE_IS_WPASYNC;
1923 else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
1924 return -EPERM;
1925
1926 if (vma->vm_flags & VM_PFNMAP)
1927 return 1;
1928
1929 if (!pagemap_scan_is_interesting_vma(vma_category, p))
1930 return 1;
1931
1932 p->cur_vma_category = vma_category;
1933 return 0;
1934 }
1935
1936 static bool pagemap_scan_push_range(unsigned long categories,
1937 struct pagemap_scan_private *p,
1938 unsigned long addr, unsigned long end)
1939 {
1940 struct page_region *cur_buf = &p->cur_buf;
1941
1942 /*
1943 * When there is no output buffer provided at all, the sentinel values
1944 * won't match here. There is no other way for `cur_buf->end` to be
1945 * non-zero other than it being non-empty.
1946 */
1947 if (addr == cur_buf->end && categories == cur_buf->categories) {
1948 cur_buf->end = end;
1949 return true;
1950 }
1951
1952 if (cur_buf->end) {
1953 if (p->vec_buf_index >= p->vec_buf_len)
1954 return false;
1955
1956 memcpy(&p->vec_buf[p->vec_buf_index], cur_buf,
1957 sizeof(*p->vec_buf));
1958 ++p->vec_buf_index;
1959 }
1960
1961 cur_buf->start = addr;
1962 cur_buf->end = end;
1963 cur_buf->categories = categories;
1964 return true;
1965 }
1966
1967 static void pagemap_scan_backout_range(struct pagemap_scan_private *p,
1968 unsigned long addr, unsigned long end)
1969 {
1970 struct page_region *cur_buf = &p->cur_buf;
1971
1972 if (cur_buf->start != addr)
1973 cur_buf->end = addr;
1974 else
1975 cur_buf->start = cur_buf->end = 0;
1976
1977 p->end_addr = 0;
1978 p->found_pages -= (end - addr) / PAGE_SIZE;
1979 }
1980
1981 static int pagemap_scan_output(unsigned long categories,
1982 struct pagemap_scan_private *p,
1983 unsigned long addr, unsigned long *end)
1984 {
1985 unsigned long n_pages, total_pages;
1986 int ret = 0;
1987
1988 if (!pagemap_scan_is_interesting_page(categories, p)) {
1989 *end = addr;
1990 return 0;
1991 }
1992
1993 if (!p->vec_buf)
1994 return 0;
1995
1996 categories &= p->arg.return_mask;
1997
1998 n_pages = (*end - addr) / PAGE_SIZE;
1999 if (check_add_overflow(p->found_pages, n_pages, &total_pages) || total_pages > p->arg.max_pages) {
2000 size_t n_too_much = total_pages - p->arg.max_pages;
2001 *end -= n_too_much * PAGE_SIZE;
2002 n_pages -= n_too_much;
2003 ret = -ENOSPC;
2004 }
2005
2006 if (!pagemap_scan_push_range(categories, p, addr, *end)) {
2007 *end = addr;
2008 n_pages = 0;
2009 ret = -ENOSPC;
2010 }
2011
2012 p->found_pages += n_pages;
2013 if (ret)
2014 p->end_addr = *end;
2015 return ret;
2016 }
2017
2018 static int pagemap_scan_thp_entry(pmd_t *pmd, unsigned long start,
2019 unsigned long end, struct mm_walk *walk)
2020 {
2021 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
2022 struct pagemap_scan_private *p = walk->private;
2023 struct vm_area_struct *vma = walk->vma;
2024 unsigned long categories;
2025 spinlock_t *ptl;
2026 int ret = 0;
2027
2028 ptl = pmd_trans_huge_lock(pmd, vma);
2029 if (!ptl)
2030 return -ENOENT;
2031
2032 categories = p->cur_vma_category | pagemap_thp_category(*pmd);
2033
2034 ret = pagemap_scan_output(categories, p, start, &end);
2035 if (start == end)
2036 goto out_unlock;
2037
2038 if (~p->arg.flags & PM_SCAN_WP_MATCHING)
2039 goto out_unlock;
2040 if (~categories & PAGE_IS_WRITTEN)
2041 goto out_unlock;
2042
2043 /*
2044 * Break huge page into small pages if the WP operation
2045 * need to be performed is on a portion of the huge page.
2046 */
> 2047 if (end != addr + HPAGE_SIZE) {
2048 spin_unlock(ptl);
2049 split_huge_pmd(vma, pmd, start);
2050 pagemap_scan_backout_range(p, start, end);
2051 return -ENOENT;
2052 }
2053
2054 make_uffd_wp_pmd(vma, start, pmd);
2055 flush_tlb_range(vma, start, end);
2056 out_unlock:
2057 spin_unlock(ptl);
2058 return ret;
2059 #else /* !CONFIG_TRANSPARENT_HUGEPAGE */
2060 return -ENOENT;
2061 #endif
2062 }
2063

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-07-22 14:44:35

by kernel test robot

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

2023-07-24 14:50:54

by Michał Mirosław

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On Mon, 24 Jul 2023 at 16:04, Muhammad Usama Anjum
<[email protected]> wrote:
>
> Fixed found bugs. Testing it further.
>
> - Split and backoff in case buffer full case as well
> - Fix the wrong breaking of loop if page isn't interesting, skip intead
> - Untag the address and save them into struct
> - Round off the end address to next page
>
> Signed-off-by: Muhammad Usama Anjum <[email protected]>
> ---
> fs/proc/task_mmu.c | 54 ++++++++++++++++++++++++++--------------------
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index add21fdf3c9a..64b326d0ec6d 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1985,18 +1989,19 @@ static int pagemap_scan_output(unsigned long
> categories,
> unsigned long n_pages, total_pages;
> int ret = 0;
>
> + if (!p->vec_buf)
> + return 0;
> +
> if (!pagemap_scan_is_interesting_page(categories, p)) {
> *end = addr;
> return 0;
> }
>
> - if (!p->vec_buf)
> - return 0;
> -
> categories &= p->arg.return_mask;

This is wrong - is_interesting() check must happen before output as
the `*end = addr` means the range should be skipped, but return 0
requests continuing of the walk.

> @@ -2044,7 +2050,7 @@ static int pagemap_scan_thp_entry(pmd_t *pmd,
> unsigned long start,
> * Break huge page into small pages if the WP operation
> * need to be performed is on a portion of the huge page.
> */
> - if (end != start + HPAGE_SIZE) {
> + if (end != start + HPAGE_SIZE || ret == -ENOSPC) {

Why is it needed? If `end == start + HPAGE_SIZE` then we're handling a
full hugepage anyway.

> @@ -2066,8 +2072,8 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
> unsigned long start,
> {
> struct pagemap_scan_private *p = walk->private;
> struct vm_area_struct *vma = walk->vma;
> + unsigned long addr, categories, next;
> pte_t *pte, *start_pte;
> - unsigned long addr;
> bool flush = false;
> spinlock_t *ptl;
> int ret;
> @@ -2088,12 +2094,14 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
> unsigned long start,
> }
>
> for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> - unsigned long categories = p->cur_vma_category |
> - pagemap_page_category(vma, addr, ptep_get(pte));
> - unsigned long next = addr + PAGE_SIZE;
> + categories = p->cur_vma_category |
> + pagemap_page_category(vma, addr, ptep_get(pte));
> + next = addr + PAGE_SIZE;

Why moving the variable declarations out of the loop?

>
> ret = pagemap_scan_output(categories, p, addr, &next);
> - if (next == addr)
> + if (ret == 0 && next == addr)
> + continue;
> + else if (next == addr)
> break;

Ah, this indeed was a bug. Nit:

if (next == addr) { if (!ret) continue; break; }

> @@ -2204,8 +2212,6 @@ static const struct mm_walk_ops pagemap_scan_ops = {
> static int pagemap_scan_get_args(struct pm_scan_arg *arg,
> unsigned long uarg)
> {
> - unsigned long start, end, vec;
> -
> if (copy_from_user(arg, (void __user *)uarg, sizeof(*arg)))
> return -EFAULT;
>
> @@ -2219,22 +2225,24 @@ static int pagemap_scan_get_args(struct pm_scan_arg
> *arg,
> arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
> return -EINVAL;
>
> - start = untagged_addr((unsigned long)arg->start);
> - end = untagged_addr((unsigned long)arg->end);
> - vec = untagged_addr((unsigned long)arg->vec);
> + arg->start = untagged_addr((unsigned long)arg->start);
> + arg->end = untagged_addr((unsigned long)arg->end);
> + arg->vec = untagged_addr((unsigned long)arg->vec);

BTW, We should we keep the tag in args writeback().

> /* Validate memory pointers */
> - if (!IS_ALIGNED(start, PAGE_SIZE))
> + if (!IS_ALIGNED(arg->start, PAGE_SIZE))
> return -EINVAL;
> - if (!access_ok((void __user *)start, end - start))
> + if (!access_ok((void __user *)arg->start, arg->end - arg->start))
> return -EFAULT;
> - if (!vec && arg->vec_len)
> + if (!arg->vec && arg->vec_len)
> return -EFAULT;
> - if (vec && !access_ok((void __user *)vec,
> + if (arg->vec && !access_ok((void __user *)arg->vec,
> arg->vec_len * sizeof(struct page_region)))
> return -EFAULT;
>
> /* Fixup default values */
> + arg->end = (arg->end & ~PAGE_MASK) ?
> + ((arg->end & PAGE_MASK) + PAGE_SIZE) : (arg->end);

arg->end = ALIGN(arg->end, PAGE_SIZE);

> if (!arg->max_pages)
> arg->max_pages = ULONG_MAX;
>

Best Regards
Michał Mirosław

2023-07-24 14:53:12

by Muhammad Usama Anjum

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

Fixed found bugs. Testing it further.

- Split and backoff in case buffer full case as well
- Fix the wrong breaking of loop if page isn't interesting, skip intead
- Untag the address and save them into struct
- Round off the end address to next page

Signed-off-by: Muhammad Usama Anjum <[email protected]>
---
fs/proc/task_mmu.c | 54 ++++++++++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index add21fdf3c9a..64b326d0ec6d 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1764,7 +1764,8 @@ struct pagemap_scan_private {
struct page_region __user* vec_out;
};

-static unsigned long pagemap_page_category(struct vm_area_struct *vma,
unsigned long addr, pte_t pte)
+static unsigned long pagemap_page_category(struct vm_area_struct *vma,
+ unsigned long addr, pte_t pte)
{
unsigned long categories = 0;

@@ -1908,6 +1909,7 @@ static bool pagemap_scan_is_interesting_vma(unsigned
long categories,
categories ^= p->arg.category_inverted;
if ((categories & required) != required)
return false;
+
return true;
}

@@ -1930,6 +1932,7 @@ static int pagemap_scan_test_walk(unsigned long
start, unsigned long end,
return 1;

p->cur_vma_category = vma_category;
+
return 0;
}

@@ -1961,6 +1964,7 @@ static bool pagemap_scan_push_range(unsigned long
categories,
cur_buf->start = addr;
cur_buf->end = end;
cur_buf->categories = categories;
+
return true;
}

@@ -1985,18 +1989,19 @@ static int pagemap_scan_output(unsigned long
categories,
unsigned long n_pages, total_pages;
int ret = 0;

+ if (!p->vec_buf)
+ return 0;
+
if (!pagemap_scan_is_interesting_page(categories, p)) {
*end = addr;
return 0;
}

- if (!p->vec_buf)
- return 0;
-
categories &= p->arg.return_mask;

n_pages = (*end - addr) / PAGE_SIZE;
- if (check_add_overflow(p->found_pages, n_pages, &total_pages) ||
total_pages > p->arg.max_pages) {
+ if (check_add_overflow(p->found_pages, n_pages, &total_pages) ||
+ total_pages > p->arg.max_pages) {
size_t n_too_much = total_pages - p->arg.max_pages;
*end -= n_too_much * PAGE_SIZE;
n_pages -= n_too_much;
@@ -2012,6 +2017,7 @@ static int pagemap_scan_output(unsigned long categories,
p->found_pages += n_pages;
if (ret)
p->end_addr = *end;
+
return ret;
}

@@ -2044,7 +2050,7 @@ static int pagemap_scan_thp_entry(pmd_t *pmd,
unsigned long start,
* Break huge page into small pages if the WP operation
* need to be performed is on a portion of the huge page.
*/
- if (end != start + HPAGE_SIZE) {
+ if (end != start + HPAGE_SIZE || ret == -ENOSPC) {
spin_unlock(ptl);
split_huge_pmd(vma, pmd, start);
pagemap_scan_backout_range(p, start, end);
@@ -2066,8 +2072,8 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
unsigned long start,
{
struct pagemap_scan_private *p = walk->private;
struct vm_area_struct *vma = walk->vma;
+ unsigned long addr, categories, next;
pte_t *pte, *start_pte;
- unsigned long addr;
bool flush = false;
spinlock_t *ptl;
int ret;
@@ -2088,12 +2094,14 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
unsigned long start,
}

for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
- unsigned long categories = p->cur_vma_category |
- pagemap_page_category(vma, addr, ptep_get(pte));
- unsigned long next = addr + PAGE_SIZE;
+ categories = p->cur_vma_category |
+ pagemap_page_category(vma, addr, ptep_get(pte));
+ next = addr + PAGE_SIZE;

ret = pagemap_scan_output(categories, p, addr, &next);
- if (next == addr)
+ if (ret == 0 && next == addr)
+ continue;
+ else if (next == addr)
break;

if (~p->arg.flags & PM_SCAN_WP_MATCHING)
@@ -2175,7 +2183,7 @@ static int pagemap_scan_pte_hole(unsigned long addr,
unsigned long end,
{
struct pagemap_scan_private *p = walk->private;
struct vm_area_struct *vma = walk->vma;
- int ret;
+ int ret, err;

if (!vma)
return 0;
@@ -2187,7 +2195,7 @@ static int pagemap_scan_pte_hole(unsigned long addr,
unsigned long end,
if (~p->arg.flags & PM_SCAN_WP_MATCHING)
return ret;

- int err = uffd_wp_range(vma, addr, end - addr, true);
+ err = uffd_wp_range(vma, addr, end - addr, true);
if (err < 0)
ret = err;

@@ -2204,8 +2212,6 @@ static const struct mm_walk_ops pagemap_scan_ops = {
static int pagemap_scan_get_args(struct pm_scan_arg *arg,
unsigned long uarg)
{
- unsigned long start, end, vec;
-
if (copy_from_user(arg, (void __user *)uarg, sizeof(*arg)))
return -EFAULT;

@@ -2219,22 +2225,24 @@ static int pagemap_scan_get_args(struct pm_scan_arg
*arg,
arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
return -EINVAL;

- start = untagged_addr((unsigned long)arg->start);
- end = untagged_addr((unsigned long)arg->end);
- vec = untagged_addr((unsigned long)arg->vec);
+ arg->start = untagged_addr((unsigned long)arg->start);
+ arg->end = untagged_addr((unsigned long)arg->end);
+ arg->vec = untagged_addr((unsigned long)arg->vec);

/* Validate memory pointers */
- if (!IS_ALIGNED(start, PAGE_SIZE))
+ if (!IS_ALIGNED(arg->start, PAGE_SIZE))
return -EINVAL;
- if (!access_ok((void __user *)start, end - start))
+ if (!access_ok((void __user *)arg->start, arg->end - arg->start))
return -EFAULT;
- if (!vec && arg->vec_len)
+ if (!arg->vec && arg->vec_len)
return -EFAULT;
- if (vec && !access_ok((void __user *)vec,
+ if (arg->vec && !access_ok((void __user *)arg->vec,
arg->vec_len * sizeof(struct page_region)))
return -EFAULT;

/* Fixup default values */
+ arg->end = (arg->end & ~PAGE_MASK) ?
+ ((arg->end & PAGE_MASK) + PAGE_SIZE) : (arg->end);
if (!arg->max_pages)
arg->max_pages = ULONG_MAX;

@@ -2279,7 +2287,7 @@ static int pagemap_scan_init_bounce_buffer(struct
pagemap_scan_private *p)
if (!p->vec_buf)
return -ENOMEM;

- p->vec_out = (void __user *)p->arg.vec;
+ p->vec_out = (struct page_region __user *)p->arg.vec;

return 0;
}
--
2.39.2

2023-07-24 15:40:42

by Muhammad Usama Anjum

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On 7/24/23 7:38 PM, Michał Mirosław wrote:
> On Mon, 24 Jul 2023 at 16:04, Muhammad Usama Anjum
> <[email protected]> wrote:
>>
>> Fixed found bugs. Testing it further.
>>
>> - Split and backoff in case buffer full case as well
>> - Fix the wrong breaking of loop if page isn't interesting, skip intead
>> - Untag the address and save them into struct
>> - Round off the end address to next page
>>
>> Signed-off-by: Muhammad Usama Anjum <[email protected]>
>> ---
>> fs/proc/task_mmu.c | 54 ++++++++++++++++++++++++++--------------------
>> 1 file changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index add21fdf3c9a..64b326d0ec6d 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1985,18 +1989,19 @@ static int pagemap_scan_output(unsigned long
>> categories,
>> unsigned long n_pages, total_pages;
>> int ret = 0;
>>
>> + if (!p->vec_buf)
>> + return 0;
>> +
>> if (!pagemap_scan_is_interesting_page(categories, p)) {
>> *end = addr;
>> return 0;
>> }
>>
>> - if (!p->vec_buf)
>> - return 0;
>> -
>> categories &= p->arg.return_mask;
>
> This is wrong - is_interesting() check must happen before output as
> the `*end = addr` means the range should be skipped, but return 0
> requests continuing of the walk.
Will revert.

>
>> @@ -2044,7 +2050,7 @@ static int pagemap_scan_thp_entry(pmd_t *pmd,
>> unsigned long start,
>> * Break huge page into small pages if the WP operation
>> * need to be performed is on a portion of the huge page.
>> */
>> - if (end != start + HPAGE_SIZE) {
>> + if (end != start + HPAGE_SIZE || ret == -ENOSPC) {
>
> Why is it needed? If `end == start + HPAGE_SIZE` then we're handling a
> full hugepage anyway.
If we weren't able to add the complete thp in the output buffer and we need
to perform WP on the entire page, we should split and rollback. Otherwise
we'll WP the entire thp and we'll lose the state on the remaining THP which
wasn't added to output.

Lets say max=100
only 100 pages would be added to output
we need to split and rollback otherwise other 412 pages would get WP

>
>> @@ -2066,8 +2072,8 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
>> unsigned long start,
>> {
>> struct pagemap_scan_private *p = walk->private;
>> struct vm_area_struct *vma = walk->vma;
>> + unsigned long addr, categories, next;
>> pte_t *pte, *start_pte;
>> - unsigned long addr;
>> bool flush = false;
>> spinlock_t *ptl;
>> int ret;
>> @@ -2088,12 +2094,14 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
>> unsigned long start,
>> }
>>
>> for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
>> - unsigned long categories = p->cur_vma_category |
>> - pagemap_page_category(vma, addr, ptep_get(pte));
>> - unsigned long next = addr + PAGE_SIZE;
>> + categories = p->cur_vma_category |
>> + pagemap_page_category(vma, addr, ptep_get(pte));
>> + next = addr + PAGE_SIZE;
>
> Why moving the variable declarations out of the loop?
Saving spaces inside loop. What are pros of declation of variable in loop?

>
>>
>> ret = pagemap_scan_output(categories, p, addr, &next);
>> - if (next == addr)
>> + if (ret == 0 && next == addr)
>> + continue;
>> + else if (next == addr)
>> break;
>
> Ah, this indeed was a bug. Nit:
>
> if (next == addr) { if (!ret) continue; break; }
>
>> @@ -2204,8 +2212,6 @@ static const struct mm_walk_ops pagemap_scan_ops = {
>> static int pagemap_scan_get_args(struct pm_scan_arg *arg,
>> unsigned long uarg)
>> {
>> - unsigned long start, end, vec;
>> -
>> if (copy_from_user(arg, (void __user *)uarg, sizeof(*arg)))
>> return -EFAULT;
>>
>> @@ -2219,22 +2225,24 @@ static int pagemap_scan_get_args(struct pm_scan_arg
>> *arg,
>> arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
>> return -EINVAL;
>>
>> - start = untagged_addr((unsigned long)arg->start);
>> - end = untagged_addr((unsigned long)arg->end);
>> - vec = untagged_addr((unsigned long)arg->vec);
>> + arg->start = untagged_addr((unsigned long)arg->start);
>> + arg->end = untagged_addr((unsigned long)arg->end);
>> + arg->vec = untagged_addr((unsigned long)arg->vec);
>
> BTW, We should we keep the tag in args writeback().
Sorry what?
After this function, the start, end and vec would be used. We need to make
sure that the address are untagged before that.

>
>> /* Validate memory pointers */
>> - if (!IS_ALIGNED(start, PAGE_SIZE))
>> + if (!IS_ALIGNED(arg->start, PAGE_SIZE))
>> return -EINVAL;
>> - if (!access_ok((void __user *)start, end - start))
>> + if (!access_ok((void __user *)arg->start, arg->end - arg->start))
>> return -EFAULT;
>> - if (!vec && arg->vec_len)
>> + if (!arg->vec && arg->vec_len)
>> return -EFAULT;
>> - if (vec && !access_ok((void __user *)vec,
>> + if (arg->vec && !access_ok((void __user *)arg->vec,
>> arg->vec_len * sizeof(struct page_region)))
>> return -EFAULT;
>>
>> /* Fixup default values */
>> + arg->end = (arg->end & ~PAGE_MASK) ?
>> + ((arg->end & PAGE_MASK) + PAGE_SIZE) : (arg->end);
>
> arg->end = ALIGN(arg->end, PAGE_SIZE);
>
>> if (!arg->max_pages)
>> arg->max_pages = ULONG_MAX;
>>
>
> Best Regards
> Michał Mirosław

--
BR,
Muhammad Usama Anjum

2023-07-24 16:47:51

by Michał Mirosław

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On Mon, 24 Jul 2023 at 17:22, Muhammad Usama Anjum
<[email protected]> wrote:
>
> On 7/24/23 7:38 PM, Michał Mirosław wrote:
> > On Mon, 24 Jul 2023 at 16:04, Muhammad Usama Anjum
> > <[email protected]> wrote:
> >>
> >> Fixed found bugs. Testing it further.
> >>
> >> - Split and backoff in case buffer full case as well
> >> - Fix the wrong breaking of loop if page isn't interesting, skip intead
> >> - Untag the address and save them into struct
> >> - Round off the end address to next page
> >>
> >> Signed-off-by: Muhammad Usama Anjum <[email protected]>
> >> ---
> >> fs/proc/task_mmu.c | 54 ++++++++++++++++++++++++++--------------------
> >> 1 file changed, 31 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> >> index add21fdf3c9a..64b326d0ec6d 100644
> >> --- a/fs/proc/task_mmu.c
> >> +++ b/fs/proc/task_mmu.c
> >> @@ -2044,7 +2050,7 @@ static int pagemap_scan_thp_entry(pmd_t *pmd,
> >> unsigned long start,
> >> * Break huge page into small pages if the WP operation
> >> * need to be performed is on a portion of the huge page.
> >> */
> >> - if (end != start + HPAGE_SIZE) {
> >> + if (end != start + HPAGE_SIZE || ret == -ENOSPC) {
> >
> > Why is it needed? If `end == start + HPAGE_SIZE` then we're handling a
> > full hugepage anyway.
> If we weren't able to add the complete thp in the output buffer and we need
> to perform WP on the entire page, we should split and rollback. Otherwise
> we'll WP the entire thp and we'll lose the state on the remaining THP which
> wasn't added to output.
>
> Lets say max=100
> only 100 pages would be added to output
> we need to split and rollback otherwise other 412 pages would get WP

In this case *end will be truncated by output() to match the number of
pages that fit.

> >> @@ -2066,8 +2072,8 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
> >> unsigned long start,
> >> {
> >> struct pagemap_scan_private *p = walk->private;
> >> struct vm_area_struct *vma = walk->vma;
> >> + unsigned long addr, categories, next;
> >> pte_t *pte, *start_pte;
> >> - unsigned long addr;
> >> bool flush = false;
> >> spinlock_t *ptl;
> >> int ret;
> >> @@ -2088,12 +2094,14 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
> >> unsigned long start,
> >> }
> >>
> >> for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> >> - unsigned long categories = p->cur_vma_category |
> >> - pagemap_page_category(vma, addr, ptep_get(pte));
> >> - unsigned long next = addr + PAGE_SIZE;
> >> + categories = p->cur_vma_category |
> >> + pagemap_page_category(vma, addr, ptep_get(pte));
> >> + next = addr + PAGE_SIZE;
> >
> > Why moving the variable declarations out of the loop?
> Saving spaces inside loop. What are pros of declation of variable in loop?

Informing the reader that the variables have scope limited to the loop body.

[...]
> >> @@ -2219,22 +2225,24 @@ static int pagemap_scan_get_args(struct pm_scan_arg
> >> *arg,
> >> arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
> >> return -EINVAL;
> >>
> >> - start = untagged_addr((unsigned long)arg->start);
> >> - end = untagged_addr((unsigned long)arg->end);
> >> - vec = untagged_addr((unsigned long)arg->vec);
> >> + arg->start = untagged_addr((unsigned long)arg->start);
> >> + arg->end = untagged_addr((unsigned long)arg->end);
> >> + arg->vec = untagged_addr((unsigned long)arg->vec);
> >
> > BTW, We should we keep the tag in args writeback().
> Sorry what?
> After this function, the start, end and vec would be used. We need to make
> sure that the address are untagged before that.

We do write back the address the walk ended at to arg->start in
userspace. This pointer I think needs the tag reconstructed so that
retrying the ioctl() will be possible.

Best Regards
Michał Mirosław

2023-07-25 07:37:11

by Muhammad Usama Anjum

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On 7/24/23 9:10 PM, Michał Mirosław wrote:
[...]>>>> @@ -2219,22 +2225,24 @@ static int pagemap_scan_get_args(struct
pm_scan_arg
>>>> *arg,
>>>> arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
>>>> return -EINVAL;
>>>>
>>>> - start = untagged_addr((unsigned long)arg->start);
>>>> - end = untagged_addr((unsigned long)arg->end);
>>>> - vec = untagged_addr((unsigned long)arg->vec);
>>>> + arg->start = untagged_addr((unsigned long)arg->start);
>>>> + arg->end = untagged_addr((unsigned long)arg->end);
>>>> + arg->vec = untagged_addr((unsigned long)arg->vec);
>>>
>>> BTW, We should we keep the tag in args writeback().
>> Sorry what?
>> After this function, the start, end and vec would be used. We need to make
>> sure that the address are untagged before that.
>
> We do write back the address the walk ended at to arg->start in
> userspace. This pointer I think needs the tag reconstructed so that
> retrying the ioctl() will be possible.
Even if we reconstruct the tag for end and vec, We need to update the start
address. Can we just put same tag as original start in it? I'm not sure.

--
BR,
Muhammad Usama Anjum

2023-07-25 09:27:58

by Muhammad Usama Anjum

[permalink] [raw]

Subject: Re: [v2] fs/proc/task_mmu: Implement IOCTL for efficient page table scanning

On 7/25/23 12:23 PM, Muhammad Usama Anjum wrote:
> On 7/24/23 9:10 PM, Michał Mirosław wrote:
> [...]>>>> @@ -2219,22 +2225,24 @@ static int pagemap_scan_get_args(struct
> pm_scan_arg
>>>>> *arg,
>>>>> arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
>>>>> return -EINVAL;
>>>>>
>>>>> - start = untagged_addr((unsigned long)arg->start);
>>>>> - end = untagged_addr((unsigned long)arg->end);
>>>>> - vec = untagged_addr((unsigned long)arg->vec);
>>>>> + arg->start = untagged_addr((unsigned long)arg->start);
>>>>> + arg->end = untagged_addr((unsigned long)arg->end);
>>>>> + arg->vec = untagged_addr((unsigned long)arg->vec);
>>>>
>>>> BTW, We should we keep the tag in args writeback().
>>> Sorry what?
>>> After this function, the start, end and vec would be used. We need to make
>>> sure that the address are untagged before that.
>>
>> We do write back the address the walk ended at to arg->start in
>> userspace. This pointer I think needs the tag reconstructed so that
>> retrying the ioctl() will be possible.
> Even if we reconstruct the tag for end and vec, We need to update the start
> address. Can we just put same tag as original start in it? I'm not sure.
The special users would use tags. If they are using it, they'll just re-add
the tag in next invocation. I think this implementation is correct.

>

--
BR,
Muhammad Usama Anjum