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

Ankur Arora posted 16 patches 2 weeks ago
[PATCH v7 10/16] mm: define clear_pages(), clear_user_pages()
Posted by Ankur Arora 2 weeks ago
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.
+ * @addr: start address
+ * @npages: number of pages
+ *
+ * Assumes that (@addr, +@npages) references a kernel region.
+ * 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.
+ * @addr: kernel mapped address
+ * @vaddr: user mapped address
+ * @pg: start page
+ * @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
+
 #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);
-- 
2.43.5
Re: [PATCH v7 10/16] mm: define clear_pages(), clear_user_pages()
Posted by David Hildenbrand 1 week, 2 days ago
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".

> + * 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.

> + * @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. Any reason this is not a static inline 
function?

-- 
Cheers

David / dhildenb
Re: [PATCH v7 10/16] mm: define clear_pages(), clear_user_pages()
Posted by Ankur Arora 1 week, 1 day ago
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
Re: [PATCH v7 10/16] mm: define clear_pages(), clear_user_pages()
Posted by David Hildenbrand 1 week ago
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?


-- 
Cheers

David / dhildenb