Re: WIP: Performance improvements

[Andrei Vagin <avagin@gmail.com>]

On Fri, Jul 28, 2023 at 4:02 AM Muhammad Usama Anjum
<usama.anjum@collabora.com> 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
>