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

[David Hildenbrand <david@redhat.com>]

On 10.10.25 12:37, David Hildenbrand wrote:
> On 25.09.25 07:25, Ankur Arora wrote:
>>
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 23.09.25 22:26, Ankur Arora wrote:
>>>> David Hildenbrand <david@redhat.com> 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@oracle.com>
>>>>>> ---
>>>>>>      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@intel.com/
>>>> 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@redhat.com>
> 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@redhat.com>
> 
> 
> 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