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

[Lokesh Gidra <lokeshgidra@google.com>]

On Fri, Aug 15, 2025 at 3:11 AM Barry Song <21cnbao@gmail.com> wrote:
>
> On Fri, Aug 15, 2025 at 9:44 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > 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>
Thanks :-)
> >
> > [...]
> > > +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();
> >     }
Sorry for missing that. I can fix this in v6.
> >
> > By the way, what about the following for both cases? Would it impact
> > performance in the `PAGE_SIZE` cases?

I just wanted to avoid a bunch of instructions in two pmd_addr_end
invocations for the (len == PAGE_SIZE) case, which is not going to be
uncommon. But I guess overall, it is not big enough to matter so can
be removed.
> >
> > len = pmd_addr_end(dst_addr, dst_addr + len) - dst_addr;
> > src_end = pmd_addr_end(src_addr, src_addr + len);
>
> By the way, do src and dst always have the same offset within a
> single PMD? I don’t think so. If not, how can we verify that if
> src’s PMD is not overflowing, dst is safe as well?
>
> Have you only checked src? And for src, since you are already using
> pmd_addr_end(), is src_end = src_addr + len fine? Why are you calling
> pmd_addr_end twice after your first pmd_addr_end has already limited
> the range?

Effectively, we have to calculate min(len, extent in src pmd, extent
in dst pmd). That's the max that can be batched within a single
critical section of src_ptl and dst_ptl. The first pmd_addr_end() is
calculating min(len, extent of dst pmd). The second pmd_addr_end() is
calculating min(result of previous pmd_addr_end, extent of src pmd). I
don't think I'm missing any overflow check. But please correct me if
I'm mistaken.
>
> Thanks
> Barry