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

[Ankur Arora <ankur.a.arora@oracle.com>]

Christophe Leroy (CS GROUP) <chleroy@kernel.org> writes:

> 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:

Yeah I was debating whether to do that or not.

> #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

On second thoughts after the discussion below it's less confusing if I
didn't tie these two together:

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

 #ifndef clear_user_highpage
 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 ?

clear_user_page[s]() is semantically different enough from clear_user_highpages()
that I don't think it makes sense to fuse the two.

For one thing, we would have to disentangle them again if/once CONFIG_HIGHMEM
goes away.

> 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

The generic clear_user_page() only exists if the generic clear_user_highpage()
is defined. The arch might provide a clear_user_page() of its own (ex.
arm/sparc etc).

> 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() ?

For !HIGHMEM, the clear_user_pages() might be just clear_pages() or a
loop around an architecture specific clear_user_page().

--
ankur