[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7567c594-7588-49e0-8b09-2a591181b24d@redhat.com>
Date: Wed, 6 Aug 2025 10:08:33 +0200
From: David Hildenbrand <david@...hat.com>
To: Dev Jain <dev.jain@....com>, akpm@...ux-foundation.org
Cc: ryan.roberts@....com, willy@...radead.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, catalin.marinas@....com, will@...nel.org,
Liam.Howlett@...cle.com, lorenzo.stoakes@...cle.com, vbabka@...e.cz,
jannh@...gle.com, anshuman.khandual@....com, peterx@...hat.com,
joey.gouly@....com, ioworker0@...il.com, baohua@...nel.org,
kevin.brodsky@....com, quic_zhenhuah@...cinc.com,
christophe.leroy@...roup.eu, yangyicong@...ilicon.com,
linux-arm-kernel@...ts.infradead.org, hughd@...gle.com,
yang@...amperecomputing.com, ziy@...dia.com
Subject: Re: [PATCH v5 6/7] mm: Optimize mprotect() by PTE batching
On 18.07.25 11:02, Dev Jain wrote:
> Use folio_pte_batch to batch process a large folio. Note that, PTE
> batching here will save a few function calls, and this strategy in certain
> cases (not this one) batches atomic operations in general, so we have
> a performance win for all arches. This patch paves the way for patch 7
> which will help us elide the TLBI per contig block on arm64.
>
> The correctness of this patch lies on the correctness of setting the
> new ptes based upon information only from the first pte of the batch
> (which may also have accumulated a/d bits via modify_prot_start_ptes()).
>
> Observe that the flag combination we pass to mprotect_folio_pte_batch()
> guarantees that the batch is uniform w.r.t the soft-dirty bit and the
> writable bit. Therefore, the only bits which may differ are the a/d bits.
> So we only need to worry about code which is concerned about the a/d bits
> of the PTEs.
>
> Setting extra a/d bits on the new ptes where previously they were not set,
> is fine - setting access bit when it was not set is not an incorrectness
> problem but will only possibly delay the reclaim of the page mapped by
> the pte (which is in fact intended because the kernel just operated on this
> region via mprotect()!). Setting dirty bit when it was not set is again
> not an incorrectness problem but will only possibly force an unnecessary
> writeback.
>
> So now we need to reason whether something can go wrong via
> can_change_pte_writable(). The pte_protnone, pte_needs_soft_dirty_wp,
> and userfaultfd_pte_wp cases are solved due to uniformity in the
> corresponding bits guaranteed by the flag combination. The ptes all
> belong to the same VMA (since callers guarantee that [start, end) will
> lie within the VMA) therefore the conditional based on the VMA is also
> safe to batch around.
>
> Since the dirty bit on the PTE really is just an indication that the folio
> got written to - even if the PTE is not actually dirty but one of the PTEs
> in the batch is, the wp-fault optimization can be made. Therefore, it is
> safe to batch around pte_dirty() in can_change_shared_pte_writable()
> (in fact this is better since without batching, it may happen that
> some ptes aren't changed to writable just because they are not dirty,
> even though the other ptes mapping the same large folio are dirty).
>
> To batch around the PageAnonExclusive case, we must check the corresponding
> condition for every single page. Therefore, from the large folio batch,
> we process sub batches of ptes mapping pages with the same
> PageAnonExclusive condition, and process that sub batch, then determine
> and process the next sub batch, and so on. Note that this does not cause
> any extra overhead; if suppose the size of the folio batch is 512, then
> the sub batch processing in total will take 512 iterations, which is the
> same as what we would have done before.
>
> For pte_needs_flush():
>
> ppc does not care about the a/d bits.
>
> For x86, PAGE_SAVED_DIRTY is ignored. We will flush only when a/d bits
> get cleared; since we can only have extra a/d bits due to batching,
> we will only have an extra flush, not a case where we elide a flush due
> to batching when we shouldn't have.
>
> Signed-off-by: Dev Jain <dev.jain@....com>
I wanted to review this, but looks like it's already upstream and I
suspect it's buggy (see the upstream report I cc'ed you on)
[...]
> +
> +/*
> + * This function is a result of trying our very best to retain the
> + * "avoid the write-fault handler" optimization. In can_change_pte_writable(),
> + * if the vma is a private vma, and we cannot determine whether to change
> + * the pte to writable just from the vma and the pte, we then need to look
> + * at the actual page pointed to by the pte. Unfortunately, if we have a
> + * batch of ptes pointing to consecutive pages of the same anon large folio,
> + * the anon-exclusivity (or the negation) of the first page does not guarantee
> + * the anon-exclusivity (or the negation) of the other pages corresponding to
> + * the pte batch; hence in this case it is incorrect to decide to change or
> + * not change the ptes to writable just by using information from the first
> + * pte of the batch. Therefore, we must individually check all pages and
> + * retrieve sub-batches.
> + */
> +static void commit_anon_folio_batch(struct vm_area_struct *vma,
> + struct folio *folio, unsigned long addr, pte_t *ptep,
> + pte_t oldpte, pte_t ptent, int nr_ptes, struct mmu_gather *tlb)
> +{
> + struct page *first_page = folio_page(folio, 0);
Who says that we have the first page of the folio mapped into the first
PTE of the batch?
> + bool expected_anon_exclusive;
> + int sub_batch_idx = 0;
> + int len;
> +
> + while (nr_ptes) {
> + expected_anon_exclusive = PageAnonExclusive(first_page + sub_batch_idx);
> + len = page_anon_exclusive_sub_batch(sub_batch_idx, nr_ptes,
> + first_page, expected_anon_exclusive);
> + prot_commit_flush_ptes(vma, addr, ptep, oldpte, ptent, len,
> + sub_batch_idx, expected_anon_exclusive, tlb);
> + sub_batch_idx += len;
> + nr_ptes -= len;
> + }
> +}
> +
> +static void set_write_prot_commit_flush_ptes(struct vm_area_struct *vma,
> + struct folio *folio, unsigned long addr, pte_t *ptep,
> + pte_t oldpte, pte_t ptent, int nr_ptes, struct mmu_gather *tlb)
> +{
> + bool set_write;
> +
> + if (vma->vm_flags & VM_SHARED) {
> + set_write = can_change_shared_pte_writable(vma, ptent);
> + prot_commit_flush_ptes(vma, addr, ptep, oldpte, ptent, nr_ptes,
> + /* idx = */ 0, set_write, tlb);
> + return;
> + }
> +
> + set_write = maybe_change_pte_writable(vma, ptent) &&
> + (folio && folio_test_anon(folio));
> + if (!set_write) {
> + prot_commit_flush_ptes(vma, addr, ptep, oldpte, ptent, nr_ptes,
> + /* idx = */ 0, set_write, tlb);
> + return;
> + }
> + commit_anon_folio_batch(vma, folio, addr, ptep, oldpte, ptent, nr_ptes, tlb);
> +}
> +
> static long change_pte_range(struct mmu_gather *tlb,
> struct vm_area_struct *vma, pmd_t *pmd, unsigned long addr,
> unsigned long end, pgprot_t newprot, unsigned long cp_flags)
> @@ -206,8 +302,9 @@ static long change_pte_range(struct mmu_gather *tlb,
> nr_ptes = 1;
> oldpte = ptep_get(pte);
> if (pte_present(oldpte)) {
> + const fpb_t flags = FPB_RESPECT_SOFT_DIRTY | FPB_RESPECT_WRITE;
> int max_nr_ptes = (end - addr) >> PAGE_SHIFT;
> - struct folio *folio;
> + struct folio *folio = NULL;
> pte_t ptent;
>
> /*
> @@ -221,11 +318,16 @@ static long change_pte_range(struct mmu_gather *tlb,
>
> /* determine batch to skip */
> nr_ptes = mprotect_folio_pte_batch(folio,
> - pte, oldpte, max_nr_ptes);
> + pte, oldpte, max_nr_ptes, /* flags = */ 0);
> continue;
> }
> }
>
> + if (!folio)
> + folio = vm_normal_folio(vma, addr, oldpte);
> +
> + nr_ptes = mprotect_folio_pte_batch(folio, pte, oldpte, max_nr_ptes, flags);
> +
> oldpte = modify_prot_start_ptes(vma, addr, pte, nr_ptes);
> ptent = pte_modify(oldpte, newprot);
>
> @@ -248,14 +350,13 @@ static long change_pte_range(struct mmu_gather *tlb,
> * COW or special handling is required.
> */
> if ((cp_flags & MM_CP_TRY_CHANGE_WRITABLE) &&
> - !pte_write(ptent) &&
> - can_change_pte_writable(vma, addr, ptent))
> - ptent = pte_mkwrite(ptent, vma);
> -
> - modify_prot_commit_ptes(vma, addr, pte, oldpte, ptent, nr_ptes);
> - if (pte_needs_flush(oldpte, ptent))
> - tlb_flush_pte_range(tlb, addr, PAGE_SIZE);
> - pages++;
> + !pte_write(ptent))
> + set_write_prot_commit_flush_ptes(vma, folio,
> + addr, pte, oldpte, ptent, nr_ptes, tlb);
While staring at this:
Very broken indentation.
> + else
> + prot_commit_flush_ptes(vma, addr, pte, oldpte, ptent,
> + nr_ptes, /* idx = */ 0, /* set_write = */ false, tlb);
Semi-broken intendation.
> + pages += nr_ptes;
> } else if (is_swap_pte(oldpte)) {
> swp_entry_t entry = pte_to_swp_entry(oldpte);
> pte_t newpte;
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists