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

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

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?

--
ankur