Re: [PATCH v9 2/7] mm: introduce clear_pages() and clear_user_pages()

[Lance Yang <ioworker0@gmail.com>]

From: Lance Yang <lance.yang@linux.dev>


On Mon, 24 Nov 2025 11:26:56 +0100, David Hildenbrand (Red Hat) wrote:
> Replying here while I am already at it.
> 
> >> +#ifndef clear_pages
> >> +/**
> >> + * clear_pages() - clear a page range for kernel-internal use.
> >> + * @addr: start address
> >> + * @npages: number of pages
> >> + *
> >> + * Use clear_user_pages() instead when clearing a page range to be
> >> + * mapped to user space.
> >> + *
> >> + * Does absolutely no exception handling.
> >> + */
> >> +static inline void clear_pages(void *addr, unsigned int npages)
> >> +{
> >> +	do {
> >> +		clear_page(addr);
> >> +		addr += PAGE_SIZE;
> >> +	} while (--npages);
> > 
> > Why a 'do while' instead of a 'while' ?
> 
> More efficient when we know that npages > 0.
> 
> > 
> > Are you certain that this function will never ever be called with a nul
> > npages ?
> 
> That is the expectation here, yes. We should probably document that 
> expectation.
> 
> > 
> >> +}
> >> +#endif
> >> +
> >>    #ifndef clear_user_page
> >>    /**
> >>     * clear_user_page() - clear a page to be mapped to user space
> >> @@ -3901,6 +3921,27 @@ static inline void clear_user_page(void *addr, unsigned long vaddr, struct page
> >>    }
> >>    #endif
> >>    
> >> +/**
> >> + * clear_user_pages() - clear a page range to be mapped to user space
> >> + * @addr: start address
> >> + * @vaddr: start address of the user mapping
> >> + * @page: start page
> >> + * @npages: number of pages
> >> + *
> >> + * Assumes that the region (@addr, +@npages) has been validated
> >> + * already so this does no exception handling.
> >> + */
> >> +#ifdef clear_user_pages
> >> +void clear_user_pages(void *addr, unsigned long vaddr,
> >> +		struct page *page, unsigned int npages);
> > 
> > By doing this you forbid architectures to define it as a static inline,
> > is that wanted ?
> 
> Note that this is not the intention. The intention is to either use a 
> direct mapping to clear_pages(), or fallback to the variant in mm/util.c.
> 
> The architecture is currently never expected to provide clear_user_pages().
> 
> Wondering if we can make that cleaner.
> 
> I'm wondering if the dependency on highmem.h here in mm.h is rather the 
> problem.
> 
> How I hate this macro crap with arch overrides.
> 
> > 
> >> +#else
> >> +static inline void clear_user_pages(void *addr, unsigned long vaddr,
> >> +		struct page *page, unsigned int npages)
> >> +{
> >> +	clear_pages(addr, npages);
> >> +}
> >> +#endif
> >> +
> >>    #ifdef __HAVE_ARCH_GATE_AREA
> >>    extern struct vm_area_struct *get_gate_vma(struct mm_struct *mm);
> >>    extern int in_gate_area_no_mm(unsigned long addr);
> >> diff --git a/mm/util.c b/mm/util.c
> >> index 8989d5767528..3c6cd44db1bd 100644
> >> --- a/mm/util.c
> >> +++ b/mm/util.c
> >> @@ -1344,3 +1344,16 @@ bool page_range_contiguous(const struct page *page, unsigned long nr_pages)
> >>    }
> >>    EXPORT_SYMBOL(page_range_contiguous);
> >>    #endif
> >> +
> >> +#ifdef clear_user_page
> >> +void clear_user_pages(void *addr,
> > 
> > What happens if clear_user_page is defined but not clear_user_pages ? In
> > that case it seems like the definition in linux/mm.h will conflict.
> 
> The generic mm.h variant will not set clear_user_page() and consequently 
> we map directly to clear_pages().

Hmm, I suspect there might be a subtle issue with the build flow on SPARC ...

Inside include/linux/mm.h, the guard checks for clear_user_pages (plural).
Since SPARC doesn't define that, the header provides the static inline
fallback.

However, mm/util.c includes that header. And since SPARC does define
clear_user_page (singular), the .c file proceeds to compile the non-static
definition as well.

Wouldn't that result in the compiler seeing both a static inline and a
non-static definition in the same translation unit? It seems like this
would trigger a redefinition error ...

Thanks,
Lance