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

[Lokesh Gidra <lokeshgidra@google.com>]

On Fri, Aug 15, 2025 at 11:38 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Sat, Aug 16, 2025 at 12:27 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> >
> > 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.
>
> Reducing the number of instructions doesn’t necessarily improve
> performance—in fact, it can often have the opposite effect. It may lead
> to increased branch mispredictions or make the code more memory-bound.
> In this particular case, could branch misprediction be the real issue?
>
That's true. I didn't even consider branch misprediction. I will
upload v6 which removes the condition, thereby also fixing the nit.
> > > >
> > > > 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.
>
> You are right. I misunderstood your code yesterday.
>
> Thanks
> Barry