Re: [PATCH v5] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE

[Barry Song <21cnbao@gmail.com>]

On Thu, Aug 14, 2025 at 7:30 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> for moving present pages. Mitigate this cost by opportunistically
> batching present contiguous pages for TLB flushing.
>
> Without batching, in our testing on an arm64 Android device with UFFD GC,
> which uses MOVE ioctl for compaction, we observed that out of the total
> time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> ~20% in vm_normal_folio().
>
> With batching, the proportion of vm_normal_folio() increases to over
> 70% of move_pages_pte() without any changes to vm_normal_folio().
> Furthermore, time spent within move_pages_pte() is only ~20%, which
> includes TLB-flush overhead.
>
> When the GC intensive benchmark, which was used to gather the above
> numbers, is run on cuttlefish (qemu android instance on x86_64), the
> completion time of the benchmark went down from ~45mins to ~20mins.
>
> Furthermore, system_server, one of the most performance critical system
> processes on android, saw over 50% reduction in GC compaction time on an
> arm64 android device.
>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Barry Song <v-songbaohua@oppo.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>

Reviewed-by: Barry Song <baohua@kernel.org>

[...]
> +static long move_present_ptes(struct mm_struct *mm,
> +                             struct vm_area_struct *dst_vma,
> +                             struct vm_area_struct *src_vma,
> +                             unsigned long dst_addr, unsigned long src_addr,
> +                             pte_t *dst_pte, pte_t *src_pte,
> +                             pte_t orig_dst_pte, pte_t orig_src_pte,
> +                             pmd_t *dst_pmd, pmd_t dst_pmdval,
> +                             spinlock_t *dst_ptl, spinlock_t *src_ptl,
> +                             struct folio **first_src_folio, unsigned long len,
> +                             struct anon_vma *src_anon_vma)
> +{
> +       int err = 0;
> +       struct folio *src_folio = *first_src_folio;
> +       unsigned long src_start = src_addr;
> +       unsigned long src_end;
> +
> +       if (len > PAGE_SIZE) {
> +               len = pmd_addr_end(dst_addr, dst_addr + len) - dst_addr;
> +               src_end = pmd_addr_end(src_addr, src_addr + len);
> +       } else
> +               src_end = src_addr + len;

Nit:

Look at Documentation/process/coding-style.rst.

This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

.. code-block:: c

    if (condition) {
        do_this();
        do_that();
    } else {
        otherwise();
    }

By the way, what about the following for both cases? Would it impact
performance in the `PAGE_SIZE` cases?

len = pmd_addr_end(dst_addr, dst_addr + len) - dst_addr;
src_end = pmd_addr_end(src_addr, src_addr + len);

Thanks
Barry