Message-ID: <21642319-daf6-4104-8fef-f6e13925d9a3@lucifer.local>
Date: Thu, 8 May 2025 05:53:40 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Dev Jain <dev.jain@....com>, Liam.Howlett@...cle.com, vbabka@...e.cz,
        jannh@...gle.com, pfalcato@...e.de, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, david@...hat.com, peterx@...hat.com,
        ryan.roberts@....com, mingo@...nel.org, libang.li@...group.com,
        maobibo@...ngson.cn, zhengqi.arch@...edance.com, baohua@...nel.org,
        anshuman.khandual@....com, willy@...radead.org, ioworker0@...il.com,
        yang@...amperecomputing.com, baolin.wang@...ux.alibaba.com,
        ziy@...dia.com, hughd@...gle.com
Subject: Re: [PATCH v2 2/2] mm: Optimize mremap() by PTE batching

On Wed, May 07, 2025 at 06:37:03PM -0700, Andrew Morton wrote:
> On Wed,  7 May 2025 11:32:56 +0530 Dev Jain <dev.jain@....com> wrote:
>
> > To use PTE batching, we want to determine whether the folio mapped by
> > the PTE is large, thus requiring the use of vm_normal_folio(). We want
> > to avoid the cost of vm_normal_folio() if the code path doesn't already
> > require the folio. For arm64, pte_batch_hint() does the job. To generalize
> > this hint, add a helper which will determine whether two consecutive PTEs
> > point to consecutive PFNs, in which case there is a high probability that
> > the underlying folio is large.
> > Next, use folio_pte_batch() to optimize move_ptes(). On arm64, if the ptes
> > are painted with the contig bit, then ptep_get() will iterate through all 16
> > entries to collect a/d bits. Hence this optimization will result in a 16x
> > reduction in the number of ptep_get() calls. Next, ptep_get_and_clear()
> > will eventually call contpte_try_unfold() on every contig block, thus
> > flushing the TLB for the complete large folio range. Instead, use
> > get_and_clear_full_ptes() so as to elide TLBIs on each contig block, and only
> > do them on the starting and ending contig block.
> >
> > ...
> >
> > --- a/include/linux/pgtable.h
> > +++ b/include/linux/pgtable.h
> > @@ -369,6 +369,35 @@ static inline pgd_t pgdp_get(pgd_t *pgdp)
> >  }
> >  #endif
> >
> > +/**
> > + * maybe_contiguous_pte_pfns - Hint whether the page mapped by the pte belongs
> > + * to a large folio.
> > + * @ptep: Pointer to the page table entry.
> > + * @pte: The page table entry.
> > + *
> > + * This helper is invoked when the caller wants to batch over a set of ptes
> > + * mapping a large folio, but the concerned code path does not already have
> > + * the folio. We want to avoid the cost of vm_normal_folio() only to find that
> > + * the underlying folio was small; i.e keep the small folio case as fast as
> > + * possible.
> > + *
> > + * The caller must ensure that ptep + 1 exists.
> > + */
>
> Gee, that isn't the prettiest interface we've ever invented.
>
> Is there any prospect that this function will get another caller?  If
> not then it's probably better to just open-code all this in the caller
> and fill it up with comments.
>
>
> Anyway, review of this series is scanty.  I'd normally enter
> wait-and-see mode, but this:
>
> > the average execution time reduces from 1.9 to
> > 1.2 seconds, giving a 37% performance optimization, on Apple M3 (arm64).
>
> prompts me into push-it-along mode.
>
>

There's been a ton of review on the v1 (see [0]), I will get to this one
soon, but as you can see from the v1 review there's lots we need to check
for correctness.

So it's coming :)

[0]: https://lore.kernel.org/linux-mm/20250506050056.59250-1-dev.jain@arm.com/

Let's not be too hasty to merge this though until we're sure it's safe
(this is delicate code). But I'll be giving R-b tags on a finalised version
anyway.

Powered by blists - more mailing lists