[v1] add STATIC_PMD_ZERO_PAGE config option

[PATCH 4/5] mm: add mm_get_static_huge_zero_folio() routine

Posted by Pankaj Raghav 4 months ago

Add mm_get_static_huge_zero_folio() routine so that huge_zero_folio can be
used without the need to pass any mm struct. This will return ZERO_PAGE
folio if CONFIG_STATIC_PMD_ZERO_PAGE is disabled.

This routine can also be called even if THP is disabled.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 include/linux/mm.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index b20d60d68b3c..c8805480ff21 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4021,6 +4021,22 @@ static inline bool vma_is_special_huge(const struct vm_area_struct *vma)
 extern struct folio *huge_zero_folio;
 extern unsigned long huge_zero_pfn;
 
+/*
+ * mm_get_static_huge_zero_folio - Get a PMD sized zero folio
+ *
+ * This function will return a PMD sized zero folio if CONFIG_STATIC_PMD_ZERO_PAGE
+ * is enabled. Otherwise, a ZERO_PAGE folio is returned.
+ *
+ * Deduce the size of the folio with folio_size instead of assuming the
+ * folio size.
+ */
+static inline struct folio *mm_get_static_huge_zero_folio(void)
+{
+	if(IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
+		return READ_ONCE(huge_zero_folio);
+	return page_folio(ZERO_PAGE(0));
+}
+
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 static inline bool is_huge_zero_folio(const struct folio *folio)
 {
-- 
2.49.0

Re: [PATCH 4/5] mm: add mm_get_static_huge_zero_folio() routine

Posted by Dave Hansen 4 months ago

On 6/12/25 03:50, Pankaj Raghav wrote:
> +/*
> + * mm_get_static_huge_zero_folio - Get a PMD sized zero folio

Isn't that a rather inaccurate function name and comment?

The third line of the function literally returns a non-PMD-sized zero folio.

> + * This function will return a PMD sized zero folio if CONFIG_STATIC_PMD_ZERO_PAGE
> + * is enabled. Otherwise, a ZERO_PAGE folio is returned.
> + *
> + * Deduce the size of the folio with folio_size instead of assuming the
> + * folio size.
> + */
> +static inline struct folio *mm_get_static_huge_zero_folio(void)
> +{
> +	if(IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
> +		return READ_ONCE(huge_zero_folio);
> +	return page_folio(ZERO_PAGE(0));
> +}

This doesn't tell us very much about when I should use:

	mm_get_static_huge_zero_folio()
vs.
	mm_get_huge_zero_folio(mm)
vs.
	page_folio(ZERO_PAGE(0))

What's with the "mm_" in the name? Usually "mm" means "mm_struct" not
Memory Management. It's really weird to prefix something that doesn't
take an "mm_struct" with "mm_"

Isn't the "get_" also a bad idea since mm_get_huge_zero_folio() does its
own refcounting but this interface does not?

Shouldn't this be something more along the lines of:

/*
 * pick_zero_folio() - Pick and return the largest available zero folio
 *
 * mm_get_huge_zero_folio() is preferred over this function. It is more
 * flexible and can provide a larger zero page under wider
 * circumstances.
 *
 * Only use this when there is no mm available.
 *
 * ... then other comments
 */
static inline struct folio *pick_zero_folio(void)
{
	if (IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
		return READ_ONCE(huge_zero_folio);
	return page_folio(ZERO_PAGE(0));
}

Or, maybe even name it _just_: zero_folio()

Re: [PATCH 4/5] mm: add mm_get_static_huge_zero_folio() routine

Posted by Pankaj Raghav (Samsung) 4 months ago

On Thu, Jun 12, 2025 at 07:09:34AM -0700, Dave Hansen wrote:
> On 6/12/25 03:50, Pankaj Raghav wrote:
> > +/*
> > + * mm_get_static_huge_zero_folio - Get a PMD sized zero folio
> 
> Isn't that a rather inaccurate function name and comment?
I agree. I also felt it was not a good name for the function.

> 
> The third line of the function literally returns a non-PMD-sized zero folio.
> 
> > + * This function will return a PMD sized zero folio if CONFIG_STATIC_PMD_ZERO_PAGE
> > + * is enabled. Otherwise, a ZERO_PAGE folio is returned.
> > + *
> > + * Deduce the size of the folio with folio_size instead of assuming the
> > + * folio size.
> > + */
> > +static inline struct folio *mm_get_static_huge_zero_folio(void)
> > +{
> > +	if(IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
> > +		return READ_ONCE(huge_zero_folio);
> > +	return page_folio(ZERO_PAGE(0));
> > +}
> 
> This doesn't tell us very much about when I should use:
> 
> 	mm_get_static_huge_zero_folio()
> vs.
> 	mm_get_huge_zero_folio(mm)
> vs.
> 	page_folio(ZERO_PAGE(0))
> 
> What's with the "mm_" in the name? Usually "mm" means "mm_struct" not
> Memory Management. It's really weird to prefix something that doesn't
> take an "mm_struct" with "mm_"

Got it. Actually, I was not aware of this one.

> 
> Isn't the "get_" also a bad idea since mm_get_huge_zero_folio() does its
> own refcounting but this interface does not?
> 

Agree.

> Shouldn't this be something more along the lines of:
> 
> /*
>  * pick_zero_folio() - Pick and return the largest available zero folio
>  *
>  * mm_get_huge_zero_folio() is preferred over this function. It is more
>  * flexible and can provide a larger zero page under wider
>  * circumstances.
>  *
>  * Only use this when there is no mm available.
>  *
>  * ... then other comments
>  */
> static inline struct folio *pick_zero_folio(void)
> {
> 	if (IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
> 		return READ_ONCE(huge_zero_folio);
> 	return page_folio(ZERO_PAGE(0));
> }
> 
> Or, maybe even name it _just_: zero_folio()

I think zero_folio() sounds like a good and straightforward name. In
most cases it will return a ZERO_PAGE() folio. If
CONFIG_STATIC_PMD_ZERO_PAGE is enabled, then we return a PMD page.

Thanks for all your comments Dave.

--
Pankaj

Re: [PATCH 4/5] mm: add mm_get_static_huge_zero_folio() routine

Posted by David Hildenbrand 3 months, 3 weeks ago

On 12.06.25 22:54, Pankaj Raghav (Samsung) wrote:
> On Thu, Jun 12, 2025 at 07:09:34AM -0700, Dave Hansen wrote:
>> On 6/12/25 03:50, Pankaj Raghav wrote:
>>> +/*
>>> + * mm_get_static_huge_zero_folio - Get a PMD sized zero folio
>>
>> Isn't that a rather inaccurate function name and comment?
> I agree. I also felt it was not a good name for the function.
> 
>>
>> The third line of the function literally returns a non-PMD-sized zero folio.
>>
>>> + * This function will return a PMD sized zero folio if CONFIG_STATIC_PMD_ZERO_PAGE
>>> + * is enabled. Otherwise, a ZERO_PAGE folio is returned.
>>> + *
>>> + * Deduce the size of the folio with folio_size instead of assuming the
>>> + * folio size.
>>> + */
>>> +static inline struct folio *mm_get_static_huge_zero_folio(void)
>>> +{
>>> +	if(IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
>>> +		return READ_ONCE(huge_zero_folio);
>>> +	return page_folio(ZERO_PAGE(0));
>>> +}
>>
>> This doesn't tell us very much about when I should use:
>>
>> 	mm_get_static_huge_zero_folio()
>> vs.
>> 	mm_get_huge_zero_folio(mm)
>> vs.
>> 	page_folio(ZERO_PAGE(0))
>>
>> What's with the "mm_" in the name? Usually "mm" means "mm_struct" not
>> Memory Management. It's really weird to prefix something that doesn't
>> take an "mm_struct" with "mm_"
> 
> Got it. Actually, I was not aware of this one.
> 
>>
>> Isn't the "get_" also a bad idea since mm_get_huge_zero_folio() does its
>> own refcounting but this interface does not?
>>
> 
> Agree.
> 
>> Shouldn't this be something more along the lines of:
>>
>> /*
>>   * pick_zero_folio() - Pick and return the largest available zero folio
>>   *
>>   * mm_get_huge_zero_folio() is preferred over this function. It is more
>>   * flexible and can provide a larger zero page under wider
>>   * circumstances.
>>   *
>>   * Only use this when there is no mm available.
>>   *
>>   * ... then other comments
>>   */
>> static inline struct folio *pick_zero_folio(void)
>> {
>> 	if (IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
>> 		return READ_ONCE(huge_zero_folio);
>> 	return page_folio(ZERO_PAGE(0));
>> }
>>
>> Or, maybe even name it _just_: zero_folio()
> 
> I think zero_folio() sounds like a good and straightforward name. In
> most cases it will return a ZERO_PAGE() folio. If
> CONFIG_STATIC_PMD_ZERO_PAGE is enabled, then we return a PMD page.

"zero_folio" would be confusing I'm afraid.

At least with current "is_zero_folio" etc.

"largest_zero_folio" or sth. like that might make it clearer that the 
size we are getting back might actually differ.

-- 
Cheers,

David / dhildenb

Re: [PATCH 4/5] mm: add mm_get_static_huge_zero_folio() routine

Posted by Pankaj Raghav (Samsung) 3 months, 3 weeks ago

On Mon, Jun 16, 2025 at 11:14:07AM +0200, David Hildenbrand wrote:
> On 12.06.25 22:54, Pankaj Raghav (Samsung) wrote:
> > On Thu, Jun 12, 2025 at 07:09:34AM -0700, Dave Hansen wrote:
> > > On 6/12/25 03:50, Pankaj Raghav wrote:
> > > > +/*
> > > > + * mm_get_static_huge_zero_folio - Get a PMD sized zero folio
> > > 
> > > Isn't that a rather inaccurate function name and comment?
> > I agree. I also felt it was not a good name for the function.
> > 
> > > 
> > > The third line of the function literally returns a non-PMD-sized zero folio.
> > > 
> > > > + * This function will return a PMD sized zero folio if CONFIG_STATIC_PMD_ZERO_PAGE
> > > > + * is enabled. Otherwise, a ZERO_PAGE folio is returned.
> > > > + *
> > > > + * Deduce the size of the folio with folio_size instead of assuming the
> > > > + * folio size.
> > > > + */
> > > > +static inline struct folio *mm_get_static_huge_zero_folio(void)
> > > > +{
> > > > +	if(IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
> > > > +		return READ_ONCE(huge_zero_folio);
> > > > +	return page_folio(ZERO_PAGE(0));
> > > > +}
> > > 
> > > This doesn't tell us very much about when I should use:
> > > 
> > > 	mm_get_static_huge_zero_folio()
> > > vs.
> > > 	mm_get_huge_zero_folio(mm)
> > > vs.
> > > 	page_folio(ZERO_PAGE(0))
> > > 
> > > What's with the "mm_" in the name? Usually "mm" means "mm_struct" not
> > > Memory Management. It's really weird to prefix something that doesn't
> > > take an "mm_struct" with "mm_"
> > 
> > Got it. Actually, I was not aware of this one.
> > 
> > > 
> > > Isn't the "get_" also a bad idea since mm_get_huge_zero_folio() does its
> > > own refcounting but this interface does not?
> > > 
> > 
> > Agree.
> > 
> > > Shouldn't this be something more along the lines of:
> > > 
> > > /*
> > >   * pick_zero_folio() - Pick and return the largest available zero folio
> > >   *
> > >   * mm_get_huge_zero_folio() is preferred over this function. It is more
> > >   * flexible and can provide a larger zero page under wider
> > >   * circumstances.
> > >   *
> > >   * Only use this when there is no mm available.
> > >   *
> > >   * ... then other comments
> > >   */
> > > static inline struct folio *pick_zero_folio(void)
> > > {
> > > 	if (IS_ENABLED(CONFIG_STATIC_PMD_ZERO_PAGE))
> > > 		return READ_ONCE(huge_zero_folio);
> > > 	return page_folio(ZERO_PAGE(0));
> > > }
> > > 
> > > Or, maybe even name it _just_: zero_folio()
> > 
> > I think zero_folio() sounds like a good and straightforward name. In
> > most cases it will return a ZERO_PAGE() folio. If
> > CONFIG_STATIC_PMD_ZERO_PAGE is enabled, then we return a PMD page.
> 
> "zero_folio" would be confusing I'm afraid.
> 
> At least with current "is_zero_folio" etc.
> 
> "largest_zero_folio" or sth. like that might make it clearer that the size
> we are getting back might actually differ.
> 

That makes sense. I can change that in the next revision.

--
Pankaj