by Andrei Vagin

[permalink] [raw]

Subject: Re: WIP: Performance improvements

On Fri, Jul 28, 2023 at 4:02 AM Muhammad Usama Anjum
<[email protected]> wrote:
>
> We are optimizing for more performance. Please find the change-set below
> for easy review before next revision and post your comments:
>
> - Replace memcpy() with direct copy as it proving to be very expensive
> - Don't check if PAGE_IS_FILE if no mask needs it as it is very
> expensive to check per pte
> - Add question in comment for discussion purpose
> - Add fast path for exclusive WP for ptes
> ---
> fs/proc/task_mmu.c | 54 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 7e92c33635cab..879baf896ed0b 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1757,37 +1757,51 @@ static int pagemap_release(struct inode *inode,
> struct file *file)
> PAGE_IS_HUGE)
> #define PM_SCAN_FLAGS (PM_SCAN_WP_MATCHING | PM_SCAN_CHECK_WPASYNC)
>
> +#define MASKS_OF_INTEREST(a) (a.category_inverted | a.category_mask | \
> + a.category_anyof_mask | a.return_mask)
> +
> struct pagemap_scan_private {
> struct pm_scan_arg arg;
> + unsigned long masks_of_interest;
> 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,
> +static unsigned long pagemap_page_category(struct pagemap_scan_private *p,
> + 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);
> + struct page *page;
>
> categories |= PAGE_IS_PRESENT;
> if (!pte_uffd_wp(pte))
> categories |= PAGE_IS_WRITTEN;
> - if (page && !PageAnon(page))
> - categories |= PAGE_IS_FILE;
> +
> + if (p->masks_of_interest & PAGE_IS_FILE) {
> + page = vm_normal_page(vma, addr, pte);
> + 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);
> + swp_entry_t swp;
>
> 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;
> +
> + if (p->masks_of_interest & PAGE_IS_FILE) {
> + swp = pte_to_swp_entry(pte);
> + if (is_pfn_swap_entry(swp) && !PageAnon(pfn_swap_entry_to_page(swp)))
> + categories |= PAGE_IS_FILE;
> + }
> }
>
> return categories;
> @@ -1957,9 +1971,7 @@ static bool pagemap_scan_push_range(unsigned long
> categories,
> 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;
> + p->vec_buf[p->vec_buf_index++] = *cur_buf;
> }
>
> cur_buf->start = addr;
> @@ -2095,9 +2107,24 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
> unsigned long start,
> return 0;
> }
>
> + if (!p->vec_buf) {
> + /* Fast path for performing exclusive WP */
> + for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> + if (pte_uffd_wp(ptep_get(pte)))
> + continue;
> + make_uffd_wp_pte(vma, addr, pte);
> + if (!flush) {
> + start = addr;
> + flush = true;
> + }
> + }
> + ret = 0;
> + goto flush_and_return;
> + }
> +
> for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> unsigned long categories = p->cur_vma_category |
> - pagemap_page_category(vma, addr, ptep_get(pte));
> + pagemap_page_category(p, vma, addr, ptep_get(pte));
> unsigned long next = addr + PAGE_SIZE;
>
> ret = pagemap_scan_output(categories, p, addr, &next);
> @@ -2119,6 +2146,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd,
> unsigned long start,
> }
> }
>
> +flush_and_return:
> if (flush)
> flush_tlb_range(vma, start, addr);
>
> @@ -2284,6 +2312,9 @@ static int pagemap_scan_init_bounce_buffer(struct
> pagemap_scan_private *p)
> * consecutive ranges that have the same categories returned across
> * walk_page_range() calls.
> */
> + // Question: Increasing the vec_buf_len increases the execution speed.
> + // But it'll increase the memory needed to run the IOCTL. Can we do
> something here?
> + // Right now only have space for 512 entries of page_region

The main problem here is that walk_page_range is executed for 512 pages.
I think we need to execute it for the whole range and interrupt it
when we need to
drain the bounce buffer.

For a trivial program that scans its address space the time is reduced from 20
seconds to 0.001 seconds. The test program and perf data are here:
https://gist.github.com/avagin/c5a22f3c78f8cb34281602dfe9c43d10

> 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),
> @@ -2329,6 +2360,7 @@ static long do_pagemap_scan(struct mm_struct *mm,
> unsigned long uarg)
> if (ret)
> return ret;
>
> + p.masks_of_interest = MASKS_OF_INTEREST(p.arg);
> ret = pagemap_scan_init_bounce_buffer(&p);
> if (ret)
> return ret;
> --
> 2.39.2
>

2023-08-07 05:20:17

by Muhammad Usama Anjum

[permalink] [raw]

Subject: Re: [PATCH v26 2/5] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs

On 8/5/23 2:53 AM, Andrei Vagin wrote:
> On Thu, Jul 27, 2023 at 2:37 AM Muhammad Usama Anjum
> <[email protected]> wrote:
>>
>
> <snip>
>
>> +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);
>
> This approach has performance implications. The basic program that scans
> its address space takes around 20-30 seconds, but it has just a few
> small mappings. The first optimization that comes to mind is to remove
> the PAGEMAP_WALK_SIZE limit and instead halt walk_page_range when the
> bounce buffer is full. After draining the buffer, the walk_page_range
> function can be restarted.
Yeah, I've this implemented in WIP and will be posting in next revision.

>
> The test program and perf data can be found here:
> https://gist.github.com/avagin/c5a22f3c78f8cb34281602dfe9c43d10
>
>> +
>> + 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;
>> + }
>> +
>
> Thanks,
> Andrei

--
BR,
Muhammad Usama Anjum