Message-ID: <4500541c-dde1-41a7-81c8-ac1573d05419@lucifer.local>
Date: Mon, 30 Jun 2025 12:08:21 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Mike Rapoport <rppt@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
        Zi Yan <ziy@...dia.com>, Matthew Brost <matthew.brost@...el.com>,
        Joshua Hahn <joshua.hahnjy@...il.com>, Rakie Kim <rakie.kim@...com>,
        Byungchul Park <byungchul@...com>, Gregory Price <gourry@...rry.net>,
        Ying Huang <ying.huang@...ux.alibaba.com>,
        Alistair Popple <apopple@...dia.com>, Pedro Falcato <pfalcato@...e.de>,
        Rik van Riel <riel@...riel.com>, Harry Yoo <harry.yoo@...cle.com>
Subject: Re: [PATCH v1 4/4] mm: remove boolean output parameters from
 folio_pte_batch_ext()

On Mon, Jun 30, 2025 at 11:32:40AM +0200, David Hildenbrand wrote:
> On 27.06.25 21:04, Lorenzo Stoakes wrote:
> > On Fri, Jun 27, 2025 at 01:55:10PM +0200, 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@...hat.com>
> >
> > Overall a really nice cleanup! Just some comments below.
> >
> > > ---
> > >   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.
> >
> > This seems weird to me, I know it's a pointer to a copy of the PTE, essentially
> > replacing the pte param from before, but now it's also an output value?
> > Shouldn't this be made clear?
>
> As you spotted, I make that clear below and for each and every flag that
> someone would set that would affect it.

I still think this needs to be made clearer. As a reviewer I had no idea what on
earth this parameter was for honestly without having to think quite a bit (and I
try to avoid that these days :P).

And as a user of this function I'd be like 'weird it needs a copy'.

See below...

>
> >
> > I know it's a pain and churn but if this is now meant to be an output var we
> > should probably make it the last param too.
> >
> > At least needs an (output) or something here.
>
> Well, it's an input+output parameter.
>
> "Pointer at a copy of the first page table entry that might be modified
> depending on @flags." is a bit mouthful, but maybe clearer than just
> "output".

Yeah but even then it's not clear :)

So yeah it is both, and we get into vague realms here, actually normally
'output' means we never read it either... ugh god haha.

So maybe:

@ptentp: Pointer to a COPY of the first page table entry whose flags we update
         if appropriate.

And then update the description of folio_pte_batch_flags() both the brief one to
add 'updates ptentp to set flags if appropriate' and maybe in the larger
description bit.

Then we're as clear as we can be I think.

>
> [...]
>
> > >   	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)));
> >
> > Hm so if !virt_addr_valid(ptentp) we're ok? :P
>
> I had the same question when writing that. Obviously,
> PageTable(virt_to_page(ptentp)) faulted when called on something on the
> stack. (ran into that ... :) )
>
> Maybe "VM_WARN_ON(virt_addr_valid(ptentp));" would work as well, but I am
> not sure how that function behaves on all architectures ...

Well you wouldn't want to limit callers to only working on stack values...

I guess just add a comment, or rather update the the one below to something like:

	/*
	 * Ensure this is a pointer to a copy not a pointer into a page table.
	 * If this is a stack value, it won't be a valid virtual address, but that's
	 * fine because it also cannot be pointing into the page table.
	 */

Which would clarify.

>
> > I also think a quick comment here
> > would help, the commit message explains it but glancing at this I'd be confused.
> >
> > Something like:
> >
> > /* Ensure this is a pointer to a copy not a pointer into a page table. */
>
> Yes, makes sense.
>
>
> --
> Cheers,
>
> David / dhildenb
>

Powered by blists - more mailing lists