| 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: <bc25dd02-6ace-45e6-9d3b-50f9c06aef98@lucifer.local>
Date: Fri, 13 Jun 2025 13:32:43 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Dev Jain <dev.jain@....com>, 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,
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 v4 2/2] mm: Optimize mremap() by PTE batching
On Wed, Jun 11, 2025 at 04:00:41PM +0200, David Hildenbrand wrote:
> On 10.06.25 05:50, 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.
> >
> > For split folios, there will be no pte batching; nr_ptes will be 1. For
> > pagetable splitting, the ptes will still point to the same large folio;
> > for arm64, this results in the optimization described above, and for other
> > arches (including the general case), a minor improvement is expected due to
> > a reduction in the number of function calls.
> >
> > Signed-off-by: Dev Jain <dev.jain@....com>
> > ---
> > mm/mremap.c | 39 ++++++++++++++++++++++++++++++++-------
> > 1 file changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index 180b12225368..18b215521ada 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -170,6 +170,23 @@ static pte_t move_soft_dirty_pte(pte_t pte)
> > return pte;
> > }
> > +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);
> > +}
> > +
> > static int move_ptes(struct pagetable_move_control *pmc,
> > unsigned long extent, pmd_t *old_pmd, pmd_t *new_pmd)
> > {
> > @@ -177,7 +194,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 +202,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,14 +255,16 @@ 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) {
> > + 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) {
> > VM_WARN_ON_ONCE(!pte_none(*new_ptep));
> > - if (pte_none(ptep_get(old_ptep)))
> > + 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
> > @@ -255,8 +276,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);
> > pte = move_pte(pte, old_addr, new_addr);
> > pte = move_soft_dirty_pte(pte);
> > @@ -269,7 +294,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);
>
>
> What I dislike is that some paths work on a single PTE, and we implicitly have to know
> that they don't apply for !pte_present.
I hate any kind of implicit knowledge like this.
>
> Like
> if (need_clear_uffd_wp && pte_marker_uffd_wp(pte))
I also despise [with words I cannot use on-list] how uffd is implemented.
It's _nothing but_ ad-hoc stuff like this spawned all around the place.
It's hateful.
>
> Will not get batched yet. And that is hidden inside the pte_marker_uffd_wp check ...
>
> Should we properly separate both paths (present vs. !present), and while at it, do
> some more cleanups? I'm thinking of the following on top (only compile-tested)
I'd like to see that, but I think maybe better as a follow up series?
On the other hand, this does improve this quite a bit. Could also be another
patch in the series.
>
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 18b215521adae..b88abf02b34e0 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -155,21 +155,6 @@ static void drop_rmap_locks(struct vm_area_struct *vma)
> i_mmap_unlock_write(vma->vm_file->f_mapping);
> }
> -static pte_t move_soft_dirty_pte(pte_t pte)
> -{
> - /*
> - * Set soft dirty bit so we can notice
> - * in userspace the ptes were moved.
> - */
> -#ifdef CONFIG_MEM_SOFT_DIRTY
> - if (pte_present(pte))
> - pte = pte_mksoft_dirty(pte);
> - else if (is_swap_pte(pte))
> - pte = pte_swp_mksoft_dirty(pte);
> -#endif
> - return pte;
> -}
> -
> static int mremap_folio_pte_batch(struct vm_area_struct *vma, unsigned long addr,
> pte_t *ptep, pte_t pte, int max_nr)
> {
> @@ -260,7 +245,6 @@ static int move_ptes(struct pagetable_move_control *pmc,
> VM_WARN_ON_ONCE(!pte_none(*new_ptep));
> nr_ptes = 1;
> - max_nr_ptes = (old_end - old_addr) >> PAGE_SHIFT;
> old_pte = ptep_get(old_ptep);
> if (pte_none(old_pte))
> continue;
> @@ -277,24 +261,34 @@ static int move_ptes(struct pagetable_move_control *pmc,
> * flushed.
> */
> if (pte_present(old_pte)) {
> + max_nr_ptes = (old_end - old_addr) >> PAGE_SHIFT;
> 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);
> - pte = move_pte(pte, old_addr, new_addr);
> - pte = move_soft_dirty_pte(pte);
> -
> - if (need_clear_uffd_wp && pte_marker_uffd_wp(pte))
> - pte_clear(mm, new_addr, new_ptep);
> - else {
> - if (need_clear_uffd_wp) {
> - if (pte_present(pte))
> - pte = pte_clear_uffd_wp(pte);
> - else if (is_swap_pte(pte))
> +
> + pte = get_and_clear_full_ptes(mm, old_addr, old_ptep,
> + nr_ptes, 0);
> + /*
> + * Moving present PTEs requires special care on some
> + * archs.
> + */
> + pte = move_pte(pte, old_addr, new_addr);
I guess we're good with only doing this in pte_present() case because the only
arch that implements this, sparc, does a present check anyway.
> + /* make userspace aware that this pte moved. */
> + pte = pte_mksoft_dirty(pte);
> + if (need_clear_uffd_wp)
> + pte = pte_clear_uffd_wp(pte);
> + set_ptes(mm, new_addr, new_ptep, pte, nr_ptes);
> + } else if (need_clear_uffd_wp && pte_marker_uffd_wp(pte)) {
> + pte_clear(mm, old_addr, old_ptep);
Same comment as below re: pte_clear().
I see you've dropped pte_clear(mm, new_addr, new_ptep) which I guess is
purposefully?
I do think that it is pointless yes.
> + } else {
> + pte_clear(mm, old_addr, old_ptep);
I guess this is intended to replace ptep_get_and_clear_full_ptes() above in the
single PTE case... no? Is this sufficient?
In the original code we'd always do ptep_get_and_clear().
I think the key difference is page_table_check_pte_clear().
I notice, hilariously, that there is a ptep_clear() that _does_ call this. What
a mess.
> + if (is_swap_pte(pte)) {
> + if (need_clear_uffd_wp)
> pte = pte_swp_clear_uffd_wp(pte);
> + /* make userspace aware that this pte moved. */
> + pte = pte_swp_mksoft_dirty(pte);
> }
> - set_ptes(mm, new_addr, new_ptep, pte, nr_ptes);
> + set_pte_at(mm, new_addr, new_ptep, pte);
> }
> }
>
>
> Note that I don't know why we had the existing
>
> - if (need_clear_uffd_wp && pte_marker_uffd_wp(pte))
> - pte_clear(mm, new_addr, new_ptep);
>
>
> I thought we would always expect that the destination pte is already pte_none() ?
I think this is because we already did the move_pte() call in the original code
before checking this:
pte = get_and_clear_full_ptes(mm, old_addr, old_ptep, nr_ptes, 0);
pte = move_pte(pte, old_addr, new_addr);
pte = move_soft_dirty_pte(pte);
if (need_clear_uffd_wp && pte_marker_uffd_wp(pte))
pte_clear(mm, new_addr, new_ptep);
But maybe it's because there was a presumption move_pte() would like you know,
move a PTE entry? Which it doesn't, it - only on SPARC - does a hook to flush
the dcache.
>
> --
> Cheers,
>
> David / dhildenb
>
Powered by blists - more mailing lists