Re: [PATCH v2 1/6] mm/arm64: constify pte_get*() and contpte get logic

On Tue, Aug 04, 2026 at 01:31:04PM +0100, Pedro Falcato wrote:
> On Tue, Aug 04, 2026 at 11:55:09AM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Mon, Aug 03, 2026 at 05:43:55PM +0100, Pedro Falcato wrote:
> > > None of the contpte code needs write access to the PTEs.
> >
> > This seems like a very broad statement? Is that actually true? I see a bunch of
> > ptep's that aren't const-ified, so you should explain why those couldn't be
> > converted.
>
> ACK. FTR, I think it would've been far clearer with "None of the contpte get
> code". There is of course contpte code that needs write access (e.g
> contpte_clear_full_ptes).

yeah that sounds better. Maybe just something like 'specify const for read-only
users of pte *' or similar?

>
> >
> > Also you add a new contpte_align_down() macro, you should mention that it the
> > commit message, explain why it was needed.
> >
> > In general more needed here :) it'd be ok if it was a truly trivial change that
> > was all obvious but you're changing some pte_t *'s and not others so it's
> > clearly not.
> >
> > >
> > > Signed-off-by: Pedro Falcato <[email protected]>
> > > ---
> > >  arch/arm64/include/asm/pgtable.h | 10 +++++-----
> > >  arch/arm64/mm/contpte.c          | 11 ++++++++---
> > >  2 files changed, 13 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > > index a2681d755358..043bc0649cee 100644
> > > --- a/arch/arm64/include/asm/pgtable.h
> > > +++ b/arch/arm64/include/asm/pgtable.h
> > > @@ -378,7 +378,7 @@ static inline void __set_pte(pte_t *ptep, pte_t pte)
> > >  	__set_pte_complete(pte);
> > >  }
> > >
> > > -static inline pte_t __ptep_get(pte_t *ptep)
> > > +static inline pte_t __ptep_get(const pte_t *ptep)
> > >  {
> > >  	return READ_ONCE(*ptep);
> > >  }
> > > @@ -1652,8 +1652,8 @@ extern void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
> > >  				pte_t *ptep, pte_t pte);
> > >  extern void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
> > >  				pte_t *ptep, pte_t pte);
> > > -extern pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte);
> > > -extern pte_t contpte_ptep_get_lockless(pte_t *orig_ptep);
> > > +extern pte_t contpte_ptep_get(const pte_t *ptep, pte_t orig_pte);
> > > +extern pte_t contpte_ptep_get_lockless(const pte_t *orig_ptep);
> >
> > This is (very) nitty but - not sure on the policy on extern's (Will/Catalin?) -
> > but in mm we drop them when we touch the code since you don't need them these
> > days :)
>
> *nods*. For what it's worth, this is new code that never needed extern.

Yup, this isn't a big deal anyway :)

>
> >
> > >  extern void contpte_set_ptes(struct mm_struct *mm, unsigned long addr,
> > >  				pte_t *ptep, pte_t pte, unsigned int nr);
> > >  extern void contpte_clear_full_ptes(struct mm_struct *mm, unsigned long addr,
> > > @@ -1732,7 +1732,7 @@ static inline unsigned int pte_batch_hint(pte_t *ptep, pte_t pte)
> > >   */
> > >
> > >  #define ptep_get ptep_get
> > > -static inline pte_t ptep_get(pte_t *ptep)
> > > +static inline pte_t ptep_get(const pte_t *ptep)
> > >  {
> > >  	pte_t pte = __ptep_get(ptep);
> > >
> > > @@ -1743,7 +1743,7 @@ static inline pte_t ptep_get(pte_t *ptep)
> > >  }
> > >
> > >  #define ptep_get_lockless ptep_get_lockless
> > > -static inline pte_t ptep_get_lockless(pte_t *ptep)
> > > +static inline pte_t ptep_get_lockless(const pte_t *ptep)
> > >  {
> > >  	pte_t pte = __ptep_get(ptep);
> > >
> > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> > > index 2de12656b4d8..3a5d6937fb51 100644
> > > --- a/arch/arm64/mm/contpte.c
> > > +++ b/arch/arm64/mm/contpte.c
> > > @@ -26,6 +26,11 @@ static inline pte_t *contpte_align_down(pte_t *ptep)
> > >  	return PTR_ALIGN_DOWN(ptep, sizeof(*ptep) * CONT_PTES);
> > >  }
> > >
> > > +#define contpte_align_down(ptep)                                                       \
> > > +	_Generic((ptep),                                                               \
> > > +		 const pte_t *: (const pte_t *) contpte_align_down((pte_t *) (ptep)),  \
> > > +		 pte_t *: contpte_align_down((pte_t *) ptep))
> >
> > I really hate these _Generic() helper things. So ugly. And it's a pretty horrid
> > cast now :(
>
> Me too!
>
> >
> > Was it not possible to const-ify further to just be able to constify
> > contpte_align_down itself?
> >
>
> You can't do that because some callers want a pte_t* out of align_down,
> others want a const pte_t* out of align_down, depending on the param.
>
> In A More Civilized Language(TM):
>
> template <typename T>
> T contpte_align_down(T ptr);

Now do it in rust ;)

>
> :P

Well as per below you can just avoid the whole issue with:

#define contpte_align_down(ptep) \
 	PTR_ALIGN_DOWN(ptep, sizeof(*(ptep)) * CONT_PTES)

And avoid this mess? You're ultimately calling into a macro anyway.

The generic thing Seems like a lot of hassle for an align-down, and then you're
forcing a macro and nasty casts anyway so I don't really see the downside of
just making it a macro in general?

>
> > It also seems to contradict the claim that contpte doesn't need write-access to
> > pte's since you're going to lengths to allow non-const pte_t * here.
>
> Yep, I'll admit the commit message is confusing and crap. I'll flesh it out
> here.

Thanks!

>
> >
> > You should cover off why this was necessary in the commit message as above.
> >
> > Anyway PTR_ALIGN_DOWN() is already const-safe so couldn't you anyway just
> > collapse this to:
> >
> > #define contpte_align_down(ptep) \
> > 	PTR_ALIGN_DOWN(ptep, sizeof(*(ptep)) * CONT_PTES)
> >
> > Then describe in the commit message why you need to handle both cases?
> >
> > > +
> > >  static inline pte_t *contpte_align_addr_ptep(unsigned long *start,
> > >  					     unsigned long *end, pte_t *ptep,
> > >  					     unsigned int nr)
> > > @@ -310,7 +315,7 @@ void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
> > >  }
> > >  EXPORT_SYMBOL_GPL(__contpte_try_unfold);
> > >
> > > -pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte)
> > > +pte_t contpte_ptep_get(const pte_t *ptep, pte_t orig_pte)
> > >  {
> > >  	/*
> > >  	 * Gather access/dirty bits, which may be populated in any of the ptes
> > > @@ -367,7 +372,7 @@ static inline bool contpte_is_consistent(pte_t pte, unsigned long pfn,
> > >  			pgprot_val(prot) == pgprot_val(orig_prot);
> > >  }
> > >
> > > -pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
> > > +pte_t contpte_ptep_get_lockless(const pte_t *orig_ptep)
> > >  {
> > >  	/*
> > >  	 * The ptep_get_lockless() API requires us to read and return *orig_ptep
> > > @@ -386,10 +391,10 @@ pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
> > >  	 * because it is not part of a contpte range.
> > >  	 */
> > >
> > > +	const pte_t *ptep;
> >
> > Nit but this is breaking the reverse xmas tree isn't it?
>
> Yep, seems like I mistakenly broke the coding style here and in the
> contpte_align_down macro above (the \ is misaligned). I'll fix it up.
>
> (I think Andrew isn't taking more material for next cycle, and while
> this should have no functional effect, it is very late and you have
> pushback, so probably no rush here...)

Yeah I assumed this was for 7.4 :) I mean the series is fine for 7.3 too AFAIC
(with feedback addressed obviously).

>
> --
> Pedro

--
Cheers, Lorenzo