Re: [PATCH v9 1/7] treewide: provide a generic clear_user_page() variant

["Christophe Leroy (CS GROUP)" <chleroy@kernel.org>]

Hi Ankur,

Le 28/11/2025 à 00:57, Ankur Arora a écrit :
> 
> Ankur Arora <ankur.a.arora@oracle.com> writes:
> 
> 
> How about something like this for clear_user_page() (though maybe I
> should be always defining clear_user_page() and not conditioning it on
> the existence of the generic clear_user_highpage()):
> 
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index abc20f9810fd..ca9d28aa29b2 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -197,6 +197,22 @@ static inline void invalidate_kernel_vmap_range(void *vaddr, int size)
>   }
>   #endif
> 
> +#if !defined(clear_user_page) && !defined(clear_user_highpage)
> +/**
> + * clear_user_page() - clear a page to be mapped to user space
> + * @addr: the address of the page
> + * @vaddr: the address of the user mapping
> + * @page: the page
> + *
> + * The sole user of clear_user_page() is clear_user_highpage().
> + * Define it if the arch does not and only if needed.
> + */
> +static inline void clear_user_page(void *addr, unsigned long vaddr, struct page *page)
> +{
> +       clear_page(addr);
> +}
> +#endif

WOuld be more obvious if you enclose that inside the same #ifdef as 
clear_user_highpage(), something like:

#ifndef clear_user_highpage

#ifndef clear_user_page
static inline void clear_user_page(void *addr, unsigned long vaddr, 
struct page *page)
{
	clear_page(addr);
}
#endif

static inline void clear_user_highpage(struct page *page, unsigned long 
vaddr)
{
	void *addr = kmap_local_page(page);
	clear_user_page(addr, vaddr, page);
	kunmap_local(addr);
}
#endif

> +
>   /* when CONFIG_HIGHMEM is not set these will be plain clear/copy_page */
>   #ifndef clear_user_highpage
>   static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
> 
> And for clear_user_pages():
> 
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index ca9d28aa29b2..b9b3cc76a91a 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -223,6 +223,35 @@ static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
>   }
>   #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.
> + */
> +static inline void clear_user_pages(void *addr, unsigned long vaddr,
> +               struct page *page, unsigned int npages)
> +{
> +#ifdef clear_user_page
> +       do {
> +               clear_user_page(addr, vaddr, page);
> +               addr += PAGE_SIZE;
> +               vaddr += PAGE_SIZE;
> +               page++;
> +       } while (--npages);
> +#else
> +       /*
> +        * Prefer clear_pages() to allow for architectural optimizations
> +        * when operations on contiguous page ranges.
> +        */
> +       clear_pages(addr, npages);
> +#endif
> +}

Not sure to understand the logic. You say this is not expected to be 
overriden by architectures in the near future, then why do we need that 
? Can't we do everything inside clear_user_highpages() for clarity ?

At the time being clear_user_page() is used exclusively by 
clear_user_highpage() so I expect clear_user_page() to only exist when 
CONFIG_HIGHMEM is enabled. And in that case clear_user_highpages() 
doesn't call clear_user_pages() so at the end only the else part 
remains, which is a simple call to clear_pages(). Why not just call 
clear_pages() directly from clear_user_highpages() and drop 
clear_user_pages() ?

Christophe