Re: [PATCH v1 4/4] mm: remove boolean output parameters from folio_pte_batch_ext()

[David Hildenbrand]

On 02.07.25 11:08, David Hildenbrand wrote:
> On 30.06.25 19:59, Zi Yan wrote:
> > On 27 Jun 2025, at 7:55, David Hildenbrand wrote:
> >
> > > Instead, let's just allow for specifying through flags whether we want
> > > to have bits merged into the original PTE.
> > >
> > > For the madvise() case, simplify by having only a single parameter for
> > > merging young+dirty. For madvise_cold_or_pageout_pte_range() merging the
> > > dirty bit is not required, but also not harmful. This code is not that
> > > performance critical after all to really force all micro-optimizations.
> > >
> > > As we now have two pte_t * parameters, use PageTable() to make sure we
> > > are actually given a pointer at a copy of the PTE, not a pointer into
> > > an actual page table.
> > >
> > > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> > > ---
> > >    mm/internal.h | 58 +++++++++++++++++++++++++++++++--------------------
> > >    mm/madvise.c  | 26 +++++------------------
> > >    mm/memory.c   |  8 ++-----
> > >    mm/util.c     |  2 +-
> > >    4 files changed, 43 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/mm/internal.h b/mm/internal.h
> > > index 6000b683f68ee..fe69e21b34a24 100644
> > > --- a/mm/internal.h
> > > +++ b/mm/internal.h
> > > @@ -208,6 +208,18 @@ typedef int __bitwise fpb_t;
> > >    /* Compare PTEs honoring the soft-dirty bit. */
> > >    #define FPB_HONOR_SOFT_DIRTY		((__force fpb_t)BIT(1))
> > >
> > > +/*
> > > + * Merge PTE write bits: if any PTE in the batch is writable, modify the
> > > + * PTE at @ptentp to be writable.
> > > + */
> > > +#define FPB_MERGE_WRITE			((__force fpb_t)BIT(2))
> > > +
> > > +/*
> > > + * Merge PTE young and dirty bits: if any PTE in the batch is young or dirty,
> > > + * modify the PTE at @ptentp to be young or dirty, respectively.
> > > + */
> > > +#define FPB_MERGE_YOUNG_DIRTY		((__force fpb_t)BIT(3))
> > > +
> > >    static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
> > >    {
> > >    	if (!(flags & FPB_HONOR_DIRTY))
> > > @@ -220,16 +232,11 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
> > >    /**
> > >     * folio_pte_batch_ext - detect a PTE batch for a large folio
> > >     * @folio: The large folio to detect a PTE batch for.
> > > + * @vma: The VMA. Only relevant with FPB_MERGE_WRITE, otherwise can be NULL.
> > >     * @ptep: Page table pointer for the first entry.
> > > - * @pte: Page table entry for the first page.
> > > + * @ptentp: Pointer at a copy of the first page table entry.
> > >     * @max_nr: The maximum number of table entries to consider.
> > >     * @flags: Flags to modify the PTE batch semantics.
> > > - * @any_writable: Optional pointer to indicate whether any entry except the
> > > - *		  first one is writable.
> > > - * @any_young: Optional pointer to indicate whether any entry except the
> > > - *		  first one is young.
> > > - * @any_dirty: Optional pointer to indicate whether any entry except the
> > > - *		  first one is dirty.
> > >     *
> > >     * Detect a PTE batch: consecutive (present) PTEs that map consecutive
> > >     * pages of the same large folio in a single VMA and a single page table.
> > > @@ -242,28 +249,26 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
> > >     * must be limited by the caller so scanning cannot exceed a single VMA and
> > >     * a single page table.
> > >     *
> > > + * Depending on the FPB_MERGE_* flags, the pte stored at @ptentp will
> > > + * be modified.
> > > + *
> > >     * This function will be inlined to optimize based on the input parameters;
> > >     * consider using folio_pte_batch() instead if applicable.
> > >     *
> > >     * Return: the number of table entries in the batch.
> > >     */
> > >    static inline unsigned int folio_pte_batch_ext(struct folio *folio,
> > > -		pte_t *ptep, pte_t pte, unsigned int max_nr, fpb_t flags,
> > > -		bool *any_writable, bool *any_young, bool *any_dirty)
> > > +		struct vm_area_struct *vma, pte_t *ptep, pte_t *ptentp,
> > > +		unsigned int max_nr, fpb_t flags)
> > >    {
> > > +	bool any_writable = false, any_young = false, any_dirty = false;
> > > +	pte_t expected_pte, pte = *ptentp;
> > >    	unsigned int nr, cur_nr;
> > > -	pte_t expected_pte;
> > > -
> > > -	if (any_writable)
> > > -		*any_writable = false;
> > > -	if (any_young)
> > > -		*any_young = false;
> > > -	if (any_dirty)
> > > -		*any_dirty = false;
> > >
> > >    	VM_WARN_ON_FOLIO(!pte_present(pte), folio);
> > >    	VM_WARN_ON_FOLIO(!folio_test_large(folio) || max_nr < 1, folio);
> > >    	VM_WARN_ON_FOLIO(page_folio(pfn_to_page(pte_pfn(pte))) != folio, folio);
> > > +	VM_WARN_ON(virt_addr_valid(ptentp) && PageTable(virt_to_page(ptentp)));
> >
> > Why not just VM_WARN_ON(!pte_same(*ptep, *ptentp)) ?
> >
> > ptep points to the first page table entry and ptentp points to the copy of it.
> > I assume ptep should point to a valid page table entry to begin with.
>
> That would also work, only miss the case where someone would by accident
> flip both pointers :)

... or pass the same pointer twice.

--
Cheers,

David / dhildenb