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

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

Ankur Arora <ankur.a.arora@oracle.com> writes:

> David Hildenbrand (Red Hat) <david@kernel.org> writes:
>
>> On 11/23/25 12:53, Christophe Leroy (CS GROUP) wrote:
>>>
>>> Le 21/11/2025 à 21:23, Ankur Arora a écrit :
>>>> From: David Hildenbrand <david@redhat.com>
>>>>
>>>> Let's drop all variants that effectively map to clear_page() and
>>>> provide it in a generic variant instead.
>>>>
>>>> We'll use the macro clear_user_page to indicate whether an architecture
>>>> provides it's own variant.
>>>>
>>>> We have to be a bit careful if an architecture provides a custom
>>>> clear_user_highpage(), because then it's very likely that some special
>>>> flushing magic is happening behind the scenes.
>>>>
>>>> Maybe at some point these should be CONFIG_ options.
>>>>
>>>> Note that for parisc, clear_page() and clear_user_page() map to
>>>> clear_page_asm(), so we can just get rid of the custom clear_user_page()
>>>> implementation. There is a clear_user_page_asm() function on parisc,
>>>> that seems to be unused. Not sure what's up with that.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>>> ...
>>>
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index 7c79b3369b82..6fa6c188f99a 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -3879,6 +3879,28 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
>>>>    				unsigned int order) {}
>>>>    #endif	/* CONFIG_DEBUG_PAGEALLOC */
>>>>    +#ifndef clear_user_page
>>>> +/**
>>>> + * 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
>>>> + */
>>>> +static inline void clear_user_page(void *addr, unsigned long vaddr, struct page *page)
>>>> +{
>>>> +#ifdef clear_user_highpage
>>>> +	/*
>>>> +	 * If an architecture defines its own clear_user_highpage() variant,
>>>> +	 * then we have to be a bit more careful here and cannot simply
>>>> +	 * rely on clear_page().
>>>> +	 */
>>>> +	clear_user_highpage(page, vaddr);
>>>> +#else
>>>> +	clear_page(addr);
>>>> +#endif
>>>> +}
>>>> +#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);
>>>
>>> Isn't it chicken and egg with clear_user_highpage() in linux/highmem.h ? :
>>
>> No really, because we make use of clear_user_highpage() only when the arch
>> defines it, so the highmem.h variant is ignored?
>>
>> Not that I particularly enjoy this way of handling it, so something cleaner
>> would be nice :)
>>
>> (in particular, relying on highmem.h defines in mm.h is a bit suboptimal)
>>
>>> #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
>>> And at the end this function is the only caller of clear_user_page() so
>>> there is apparently no need for a generic clear_user_page(), at least
>>> not when clear_user_highpage() is defined.
>>> I think is would be simpler and cleaner to instead add the following in
>>> linux/highmem.c:
>>
>> I assume you mean highmem.h
>>
>> It's not really highmem.h material, but if it makes things cleaner, sure.
>>
>> Might be that the compiler will not be happy about that.
>>
>> @Ankur can you play with that and see if we can make compilers happy one way or
>> the other?
>
> This looks like a good change (and from my tests on a couple of
> configs seems to build fine.)
>
> Though clear_user_pages() is also only called from clear_user_highpages().
> Would be good to treat that similarly.
>
> (I don't think it can be done because how the arch code treats
> both quite differently.)
>
> Anyway I'll play with that a bit more.

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
+
 /* 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
+}
+

Tree with changes: github.com/terminus/linux clear-pages.v10-rc2

This seems to be fine on a variety of configurations.

David, could you take a quick look at it?

Thanks
--
ankur