| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <888a8314-b200-40b7-ab52-f30f0af93374@lucifer.local>
Date: Tue, 27 May 2025 17:29:26 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Dev Jain <dev.jain@....com>
Cc: akpm@...ux-foundation.org, 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 v3 2/2] mm: Optimize mremap() by PTE batching
On Tue, May 27, 2025 at 09:52:47PM +0530, Dev Jain wrote:
>
> On 27/05/25 4:15 pm, Lorenzo Stoakes wrote:
> > On Tue, May 27, 2025 at 01:20:49PM +0530, Dev Jain wrote:
> > > 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.
> > But you're also making this applicable to non-contpte cases?
> >
> > See below, but the commit message shoud clearly point out this is general
> > for page table split large folios (unless I've missed something of course!
> > :)
> >
> > > Signed-off-by: Dev Jain <dev.jain@....com>
> > > ---
> > > mm/mremap.c | 40 +++++++++++++++++++++++++++++++++-------
> > > 1 file changed, 33 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > index 0163e02e5aa8..580b41f8d169 100644
> > > --- a/mm/mremap.c
> > > +++ b/mm/mremap.c
> > > @@ -170,6 +170,24 @@ static pte_t move_soft_dirty_pte(pte_t pte)
> > > return pte;
> > > }
> > >
> > > +/* mremap a batch of PTEs mapping the same large folio */
> > I think this comment is fairly useless, it basically spells out the function
> > name.
> >
> > I'd prefer something like 'determine if a PTE contains physically contiguous
> > entries which map the same large folio'.
>
> I'd rather prefer dropping the comment altogether, the function is fairly obvious : )
Sure fine.
>
>
> > > +static int mremap_folio_pte_batch(struct vm_area_struct *vma, unsigned long addr,
> > > + pte_t *ptep, pte_t pte, int max_nr)
> > > +{
> > > + const fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> > > + struct folio *folio;
> > > +
> > > + if (max_nr == 1)
> > > + return 1;
> > > +
> > > + folio = vm_normal_folio(vma, addr, pte);
> > > + if (!folio || !folio_test_large(folio))
> > > + return 1;
> > > +
> > > + return folio_pte_batch(folio, addr, ptep, pte, max_nr, flags, NULL,
> > > + NULL, NULL);
> > > +}
> > The code is much better however! :)
> >
> > > +
> > > static int move_ptes(struct pagetable_move_control *pmc,
> > > unsigned long extent, pmd_t *old_pmd, pmd_t *new_pmd)
> > > {
> > > @@ -177,7 +195,7 @@ static int move_ptes(struct pagetable_move_control *pmc,
> > > bool need_clear_uffd_wp = vma_has_uffd_without_event_remap(vma);
> > > struct mm_struct *mm = vma->vm_mm;
> > > pte_t *old_ptep, *new_ptep;
> > > - pte_t pte;
> > > + pte_t old_pte, pte;
> > > pmd_t dummy_pmdval;
> > > spinlock_t *old_ptl, *new_ptl;
> > > bool force_flush = false;
> > > @@ -185,6 +203,8 @@ static int move_ptes(struct pagetable_move_control *pmc,
> > > unsigned long new_addr = pmc->new_addr;
> > > unsigned long old_end = old_addr + extent;
> > > unsigned long len = old_end - old_addr;
> > > + int max_nr_ptes;
> > > + int nr_ptes;
> > > int err = 0;
> > >
> > > /*
> > > @@ -236,12 +256,14 @@ static int move_ptes(struct pagetable_move_control *pmc,
> > > flush_tlb_batched_pending(vma->vm_mm);
> > > arch_enter_lazy_mmu_mode();
> > >
> > > - for (; old_addr < old_end; old_ptep++, old_addr += PAGE_SIZE,
> > > - new_ptep++, new_addr += PAGE_SIZE) {
> > > - if (pte_none(ptep_get(old_ptep)))
> > > + for (; old_addr < old_end; old_ptep += nr_ptes, old_addr += nr_ptes * PAGE_SIZE,
> > > + new_ptep += nr_ptes, new_addr += nr_ptes * PAGE_SIZE) {
> > > + nr_ptes = 1;
> > > + max_nr_ptes = (old_end - old_addr) >> PAGE_SHIFT;
> > > + old_pte = ptep_get(old_ptep);
> > > + if (pte_none(old_pte))
> > > continue;
> > >
> > > - pte = ptep_get_and_clear(mm, old_addr, old_ptep);
> > > /*
> > > * If we are remapping a valid PTE, make sure
> > > * to flush TLB before we drop the PTL for the
> > > @@ -253,8 +275,12 @@ static int move_ptes(struct pagetable_move_control *pmc,
> > > * the TLB entry for the old mapping has been
> > > * flushed.
> > > */
> > > - if (pte_present(pte))
> > > + if (pte_present(old_pte)) {
> > > + nr_ptes = mremap_folio_pte_batch(vma, old_addr, old_ptep,
> > > + old_pte, max_nr_ptes);
> > > force_flush = true;
> > > + }
> > > + pte = get_and_clear_full_ptes(mm, old_addr, old_ptep, nr_ptes, 0);
> > Just to clarify, in the previous revision you said:
> >
> > "Split THPs won't be batched; you can use pte_batch() (from David's refactoring)
> > and figure the split THP batch out, but then get_and_clear_full_ptes() will be
> > gathering a/d bits and smearing them across the batch, which will be incorrect."
> >
> > But... this will be triggered for page table split large folio no?
> >
> > So is there something wrong here or not?
>
> Since I am using folio_pte_batch (and not the hypothetical pte_batch() I was
> saying in the other email), the batch must belong to the same folio. Since split
> THP means a small folio, nr_ptes will be 1.
I'm not sure I follow - keep in mind there's two kinds of splitting - folio
splitting and page table splitting.
If I invoke split_huge_pmd(), I end up with a bunch of PTEs mapping the same
large folio. The folio itself is not split, so nr_ptes surely will be equal to
something >1 here right?
I hit this in my MREMAP_RELOCATE_ANON work - where I had to take special care to
differentiate between these cases.
And the comment for folio_pte_batch() states 'Detect a PTE batch: consecutive
(present) PTEs that map consecutive pages of the same large folio.' - so I don't
see why this would not hit this case?
I may be missing something however!
>
>
>
> >
> > > pte = move_pte(pte, old_addr, new_addr);
> > > pte = move_soft_dirty_pte(pte);
> > >
> > > @@ -267,7 +293,7 @@ static int move_ptes(struct pagetable_move_control *pmc,
> > > else if (is_swap_pte(pte))
> > > pte = pte_swp_clear_uffd_wp(pte);
> > > }
> > > - set_pte_at(mm, new_addr, new_ptep, pte);
> > > + set_ptes(mm, new_addr, new_ptep, pte, nr_ptes);
> > The code looks much better here after refactoring, however!
> >
> > > }
> > > }
> > >
> > > --
> > > 2.30.2
> > >
Powered by blists - more mailing lists