Re: [PATCH v7 10/16] mm: define clear_pages(), clear_user_pages()

[David Hildenbrand]

On 10.10.25 12:37, David Hildenbrand wrote:
> On 25.09.25 07:25, Ankur Arora wrote:
> > David Hildenbrand <david@xxxxxxxxxx> writes:
> >
> > > On 23.09.25 22:26, Ankur Arora wrote:
> > > > David Hildenbrand <david@xxxxxxxxxx> writes:
> > > >
> > > > > On 17.09.25 17:24, Ankur Arora wrote:
> > > > > > Define fallback versions of clear_pages(), clear_user_pages().
> > > > > > In absence of architectural primitives, we just clear pages
> > > > > > sequentially.
> > > > > > Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> > > > > > ---
> > > > > >      include/linux/mm.h | 38 ++++++++++++++++++++++++++++++++++++++
> > > > > >      1 file changed, 38 insertions(+)
> > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > > index 1ae97a0b8ec7..0cde9b01da5e 100644
> > > > > > --- a/include/linux/mm.h
> > > > > > +++ b/include/linux/mm.h
> > > > > > @@ -3768,6 +3768,44 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
> > > > > >      				unsigned int order) {}
> > > > > >      #endif	/* CONFIG_DEBUG_PAGEALLOC */
> > > > > >      +#ifndef clear_pages
> > > > > > +/**
> > > > > > + * clear_pages() - clear a page range using a kernel virtual address.
> > > > >
> > > > > I'd just call this "clear a page range for kernel-internal use"
> > > > >
> > > > > > + * @addr: start address
> > > > > > + * @npages: number of pages
> > > > > > + *
> > > > > > + * Assumes that (@addr, +@npages) references a kernel region.
> > > > >
> > > > > And say here simply that "Use clear_user_pages() instead for clearing a page
> > > > > range to be mapped to user space".
> > > > So, comments that actually speak to the use instead of technically
> > > > correct but unhelpful generalities :). Thanks, good lesson.
> > > >
> > > > > > + * Does absolutely no exception handling.
> > > > > > + */
> > > > > > +static inline void clear_pages(void *addr, unsigned int npages)
> > > > > > +{
> > > > > > +	do {
> > > > > > +		clear_page(addr);
> > > > > > +		addr += PAGE_SIZE;
> > > > > > +	} while (--npages);
> > > > > > +}
> > > > > > +#endif
> > > > > > +
> > > > > > +#ifndef clear_user_pages
> > > > > > +/**
> > > > > > + * clear_user_pages() - clear a page range mapped by the user.
> > > > >
> > > > > I'd call this then "clear a page range to be mapped to user space"
> > > > >
> > > > > Because it's usually called before we actually map it and it will properly flush
> > > > > the dcache if required.
> > > > Makes sense.
> > > >
> > > > > > + * @addr: kernel mapped address
> > > > >
> > > > > "start address"
> > > > >
> > > > > > + * @vaddr: user mapped address
> > > > >
> > > > > "start address of the user mapping" ?
> > > > >
> > > > > > + * @pg: start page
> > > > >
> > > > > Please just call it "page". I know, clear_user_page() has this weird page vs. pg
> > > > > thingy, but let's do it better here.
> > > > >
> > > > > > + * @npages: number of pages
> > > > > > + *
> > > > > > + * Assumes that the region (@addr, +@npages) has been validated
> > > > > > + * already so this does no exception handling.
> > > > > > + */
> > > > > > +#define clear_user_pages(addr, vaddr, pg, npages)	\
> > > > > > +do {							\
> > > > > > +	clear_user_page(addr, vaddr, pg);		\
> > > > > > +	addr += PAGE_SIZE;				\
> > > > > > +	vaddr += PAGE_SIZE;				\
> > > > > > +	pg++;						\
> > > > > > +} while (--npages)
> > > > > > +#endif
> > > > >
> > > > > Should indent with one tab.
> > > > Will do. Also acking to the ones above.
> > > >
> > > > > Any reason this is not a static inline function?
> > > > Alas yes. Most architecture code defines clear_user_page() as a macro
> > > > where, if they need a to flush the dcache or otherwise do something
> > > > special, they need access to some external primitive. And this primitive
> > > > which might not be visible in contexts that we include this header.
> > > > For instance this one on sparc:
> > > >      https://lore.kernel.org/lkml/202509030338.DlQJTxIk-lkp@xxxxxxxxx/
> > > > Defining as a macro to get around that. But maybe there's a better
> > > > way?
> > >
> > > Can we just move it to mm/utils.c and not have it be an inline function?
> >
> > Thanks. Yeah, that's a good place for it.
>
> So, I'm looking into this and I think we should fixup the arch if possible.
>
> I now have
>
> commit 0f90e18abec6b6080af9ee5583cbba28d483a87d (HEAD)
> Author: David Hildenbrand <david@xxxxxxxxxx>
> Date:   Fri Oct 10 06:09:51 2025 -0400
>
>       treewide: provide a generic clear_user_page() variant
>
>       Let's drop all variants that effectively map to clear_page() and
>       provide it in a generic variant instead.
>
>       We'll use __HAVE_ARCH_CLEAR_USER_PAGE, similar to
>       __HAVE_ARCH_COPY_USER_HIGHPAGE, to indicate whether an architecture
>       provides it's own variant.
>
>       Maybe at some point these should be CONFIG_ options.
>
>       Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>
>
> To get started. Did you only run into the issue with sparc or was there
> another one problematic?

Okay, m68k is nasty as well. Change of plans, let's keep a 
clear_user_pages() variant in mm/utils.c when the arch has special 
clear_user_page() needs.

--
Cheers

David / dhildenb