diff mbox series

[mm-unstable,8/8] mm/hugetlb: convert demote_free_huge_page to folios

Message ID 20230103191340.116536-9-sidhartha.kumar@oracle.com
State New
Headers
Series continue hugetlb folio conversions |

Commit Message

Sidhartha Kumar Jan. 3, 2023, 7:13 p.m. UTC 
  Change demote_free_huge_page to demote_free_hugetlb_folio() and change
demote_pool_huge_page() pass in a folio.

Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 mm/hugetlb.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

Comments

Mike Kravetz Jan. 7, 2023, 1:11 a.m. UTC | #1 
On 01/03/23 13:13, Sidhartha Kumar wrote:
> Change demote_free_huge_page to demote_free_hugetlb_folio() and change
> demote_pool_huge_page() pass in a folio.
> 
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  mm/hugetlb.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 2bb69b098117..a89728c6987d 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3438,12 +3438,12 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
>  	return 0;
>  }
>  
> -static int demote_free_huge_page(struct hstate *h, struct page *page)
> +static int demote_free_hugetlb_folio(struct hstate *h, struct folio *folio)
>  {
> -	int i, nid = page_to_nid(page);
> +	int i, nid = folio_nid(folio);
>  	struct hstate *target_hstate;
> -	struct folio *folio = page_folio(page);
>  	struct page *subpage;
> +	struct folio *subfolio;
>  	int rc = 0;
>  
>  	target_hstate = size_to_hstate(PAGE_SIZE << h->demote_order);
> @@ -3451,18 +3451,18 @@ static int demote_free_huge_page(struct hstate *h, struct page *page)
>  	remove_hugetlb_folio_for_demote(h, folio, false);
>  	spin_unlock_irq(&hugetlb_lock);
>  
> -	rc = hugetlb_vmemmap_restore(h, page);
> +	rc = hugetlb_vmemmap_restore(h, &folio->page);
>  	if (rc) {
> -		/* Allocation of vmemmmap failed, we can not demote page */
> +		/* Allocation of vmemmmap failed, we can not demote folio */
>  		spin_lock_irq(&hugetlb_lock);
> -		set_page_refcounted(page);
> -		add_hugetlb_folio(h, page_folio(page), false);
> +		folio_ref_unfreeze(folio, 1);
> +		add_hugetlb_folio(h, folio, false);
>  		return rc;
>  	}
>  
>  	/*
>  	 * Use destroy_compound_hugetlb_folio_for_demote for all huge page
> -	 * sizes as it will not ref count pages.
> +	 * sizes as it will not ref count folios.
>  	 */
>  	destroy_compound_hugetlb_folio_for_demote(folio, huge_page_order(h));
>  
> @@ -3477,15 +3477,15 @@ static int demote_free_huge_page(struct hstate *h, struct page *page)
>  	mutex_lock(&target_hstate->resize_lock);
>  	for (i = 0; i < pages_per_huge_page(h);
>  				i += pages_per_huge_page(target_hstate)) {
> -		subpage = nth_page(page, i);
> -		folio = page_folio(subpage);
> +		subpage = folio_page(folio, i);
> +		subfolio = page_folio(subpage);

No problems with the code, but I am not in love with the name subfolio.
I know it is patterned after 'subpage'.  For better or worse, the term
subpage is used throughout the kernel.  This would be the first usage of
the term 'subfolio'.

Matthew do you have any comments on the naming?  It is local to hugetlb,
but I would hate to see use of the term subfolio based on its introduction
here.
Matthew Wilcox Jan. 7, 2023, 1:31 a.m. UTC | #2 
On Fri, Jan 06, 2023 at 05:11:36PM -0800, Mike Kravetz wrote:
> On 01/03/23 13:13, Sidhartha Kumar wrote:
> > @@ -3477,15 +3477,15 @@ static int demote_free_huge_page(struct hstate *h, struct page *page)
> >  	mutex_lock(&target_hstate->resize_lock);
> >  	for (i = 0; i < pages_per_huge_page(h);
> >  				i += pages_per_huge_page(target_hstate)) {
> > -		subpage = nth_page(page, i);
> > -		folio = page_folio(subpage);
> > +		subpage = folio_page(folio, i);
> > +		subfolio = page_folio(subpage);
> 
> No problems with the code, but I am not in love with the name subfolio.
> I know it is patterned after 'subpage'.  For better or worse, the term
> subpage is used throughout the kernel.  This would be the first usage of
> the term 'subfolio'.
> 
> Matthew do you have any comments on the naming?  It is local to hugetlb,
> but I would hate to see use of the term subfolio based on its introduction
> here.

I'm really not a fan of it either.  I intended to dive into this patch
and understand the function it's modifying, in the hopes of suggesting
a better name and/or method.

Since I haven't done that yet, maybe "new" or "dest" names work?
Mike Kravetz Jan. 7, 2023, 8:55 p.m. UTC | #3 
On 01/07/23 01:31, Matthew Wilcox wrote:
> On Fri, Jan 06, 2023 at 05:11:36PM -0800, Mike Kravetz wrote:
> > On 01/03/23 13:13, Sidhartha Kumar wrote:
> > > @@ -3477,15 +3477,15 @@ static int demote_free_huge_page(struct hstate *h, struct page *page)
> > >  	mutex_lock(&target_hstate->resize_lock);
> > >  	for (i = 0; i < pages_per_huge_page(h);
> > >  				i += pages_per_huge_page(target_hstate)) {
> > > -		subpage = nth_page(page, i);
> > > -		folio = page_folio(subpage);
> > > +		subpage = folio_page(folio, i);
> > > +		subfolio = page_folio(subpage);
> > 
> > No problems with the code, but I am not in love with the name subfolio.
> > I know it is patterned after 'subpage'.  For better or worse, the term
> > subpage is used throughout the kernel.  This would be the first usage of
> > the term 'subfolio'.
> > 
> > Matthew do you have any comments on the naming?  It is local to hugetlb,
> > but I would hate to see use of the term subfolio based on its introduction
> > here.
> 
> I'm really not a fan of it either.  I intended to dive into this patch
> and understand the function it's modifying, in the hopes of suggesting
> a better name and/or method.

At a high level, this routine is splitting a very large folio (1G for
example) into multiple large folios of a smaller size (512 2M folios for
example).  The loop is iterating through the very large folio at
increments of the smaller large folio.  subfolio (previously subpage) is
used to point to the smaller large folio within the loop.
Sidhartha Kumar Jan. 9, 2023, 4:36 p.m. UTC | #4 
On 1/7/23 2:55 PM, Mike Kravetz wrote:
> On 01/07/23 01:31, Matthew Wilcox wrote:
>> On Fri, Jan 06, 2023 at 05:11:36PM -0800, Mike Kravetz wrote:
>>> On 01/03/23 13:13, Sidhartha Kumar wrote:
>>>> @@ -3477,15 +3477,15 @@ static int demote_free_huge_page(struct hstate *h, struct page *page)
>>>>   	mutex_lock(&target_hstate->resize_lock);
>>>>   	for (i = 0; i < pages_per_huge_page(h);
>>>>   				i += pages_per_huge_page(target_hstate)) {
>>>> -		subpage = nth_page(page, i);
>>>> -		folio = page_folio(subpage);
>>>> +		subpage = folio_page(folio, i);
>>>> +		subfolio = page_folio(subpage);
>>>
>>> No problems with the code, but I am not in love with the name subfolio.
>>> I know it is patterned after 'subpage'.  For better or worse, the term
>>> subpage is used throughout the kernel.  This would be the first usage of
>>> the term 'subfolio'.
>>>
>>> Matthew do you have any comments on the naming?  It is local to hugetlb,
>>> but I would hate to see use of the term subfolio based on its introduction
>>> here.
>>
>> I'm really not a fan of it either.  I intended to dive into this patch
>> and understand the function it's modifying, in the hopes of suggesting
>> a better name and/or method.
> 
> At a high level, this routine is splitting a very large folio (1G for
> example) into multiple large folios of a smaller size (512 2M folios for
> example).  The loop is iterating through the very large folio at
> increments of the smaller large folio.  subfolio (previously subpage) is
> used to point to the smaller large folio within the loop.
> 
If folio does not need to be part of the variable name, how about 
something like 'demote_target'? The prep call inside the loop would then 
look like:

prep_new_hugetlb_folio(target_hstate, demote_target, nid);

so it is still clear that demote_target is a folio. A more concise 
version could also be 'demote_dst' but that seems more ambiguous than 
target.

Thanks,
Sidhartha Kumar
Mike Kravetz Jan. 9, 2023, 6:23 p.m. UTC | #5 
On 01/09/23 10:36, Sidhartha Kumar wrote:
> On 1/7/23 2:55 PM, Mike Kravetz wrote:
> > On 01/07/23 01:31, Matthew Wilcox wrote:
> > > On Fri, Jan 06, 2023 at 05:11:36PM -0800, Mike Kravetz wrote:
> > > > On 01/03/23 13:13, Sidhartha Kumar wrote:
> > > > > @@ -3477,15 +3477,15 @@ static int demote_free_huge_page(struct hstate *h, struct page *page)
> > > > >   	mutex_lock(&target_hstate->resize_lock);
> > > > >   	for (i = 0; i < pages_per_huge_page(h);
> > > > >   				i += pages_per_huge_page(target_hstate)) {
> > > > > -		subpage = nth_page(page, i);
> > > > > -		folio = page_folio(subpage);
> > > > > +		subpage = folio_page(folio, i);
> > > > > +		subfolio = page_folio(subpage);
> > > > 
> > > > No problems with the code, but I am not in love with the name subfolio.
> > > > I know it is patterned after 'subpage'.  For better or worse, the term
> > > > subpage is used throughout the kernel.  This would be the first usage of
> > > > the term 'subfolio'.
> > > > 
> > > > Matthew do you have any comments on the naming?  It is local to hugetlb,
> > > > but I would hate to see use of the term subfolio based on its introduction
> > > > here.
> > > 
> > > I'm really not a fan of it either.  I intended to dive into this patch
> > > and understand the function it's modifying, in the hopes of suggesting
> > > a better name and/or method.
> > 
> > At a high level, this routine is splitting a very large folio (1G for
> > example) into multiple large folios of a smaller size (512 2M folios for
> > example).  The loop is iterating through the very large folio at
> > increments of the smaller large folio.  subfolio (previously subpage) is
> > used to point to the smaller large folio within the loop.
> > 
> If folio does not need to be part of the variable name, how about something
> like 'demote_target'? The prep call inside the loop would then look like:
> 
> prep_new_hugetlb_folio(target_hstate, demote_target, nid);
> 
> so it is still clear that demote_target is a folio. A more concise version
> could also be 'demote_dst' but that seems more ambiguous than target.

I am OK with that naming.  Primary concern was the introduction of the
term subfolio.
John Hubbard Jan. 9, 2023, 8:01 p.m. UTC | #6 
On 1/9/23 10:23, Mike Kravetz wrote:
>>>>> No problems with the code, but I am not in love with the name subfolio.
>>>>> I know it is patterned after 'subpage'.  For better or worse, the term
>>>>> subpage is used throughout the kernel.  This would be the first usage of
>>>>> the term 'subfolio'.
>>>>>
>>>>> Matthew do you have any comments on the naming?  It is local to hugetlb,
>>>>> but I would hate to see use of the term subfolio based on its introduction
>>>>> here.
>>>>
>>>> I'm really not a fan of it either.  I intended to dive into this patch
>>>> and understand the function it's modifying, in the hopes of suggesting
>>>> a better name and/or method.
>>>
>>> At a high level, this routine is splitting a very large folio (1G for
>>> example) into multiple large folios of a smaller size (512 2M folios for
>>> example).  The loop is iterating through the very large folio at
>>> increments of the smaller large folio.  subfolio (previously subpage) is
>>> used to point to the smaller large folio within the loop.
>>>
>> If folio does not need to be part of the variable name, how about something
>> like 'demote_target'? The prep call inside the loop would then look like:
>>
>> prep_new_hugetlb_folio(target_hstate, demote_target, nid);
>>
>> so it is still clear that demote_target is a folio. A more concise version
>> could also be 'demote_dst' but that seems more ambiguous than target.
> 
> I am OK with that naming.  Primary concern was the introduction of the
> term subfolio.

How about one of these:

     smaller_folio
     inner_folio

Those are more self-explanatory, while still avoiding "subfolio".

thanks,
Sidhartha Kumar Jan. 9, 2023, 8:53 p.m. UTC | #7 
On 1/9/23 2:01 PM, John Hubbard wrote:
> On 1/9/23 10:23, Mike Kravetz wrote:
>>>>>> No problems with the code, but I am not in love with the name 
>>>>>> subfolio.
>>>>>> I know it is patterned after 'subpage'.  For better or worse, the 
>>>>>> term
>>>>>> subpage is used throughout the kernel.  This would be the first 
>>>>>> usage of
>>>>>> the term 'subfolio'.
>>>>>>
>>>>>> Matthew do you have any comments on the naming?  It is local to 
>>>>>> hugetlb,
>>>>>> but I would hate to see use of the term subfolio based on its 
>>>>>> introduction
>>>>>> here.
>>>>>
>>>>> I'm really not a fan of it either.  I intended to dive into this patch
>>>>> and understand the function it's modifying, in the hopes of suggesting
>>>>> a better name and/or method.
>>>>
>>>> At a high level, this routine is splitting a very large folio (1G for
>>>> example) into multiple large folios of a smaller size (512 2M folios 
>>>> for
>>>> example).  The loop is iterating through the very large folio at
>>>> increments of the smaller large folio.  subfolio (previously 
>>>> subpage) is
>>>> used to point to the smaller large folio within the loop.
>>>>
>>> If folio does not need to be part of the variable name, how about 
>>> something
>>> like 'demote_target'? The prep call inside the loop would then look 
>>> like:
>>>
>>> prep_new_hugetlb_folio(target_hstate, demote_target, nid);
>>>
>>> so it is still clear that demote_target is a folio. A more concise 
>>> version
>>> could also be 'demote_dst' but that seems more ambiguous than target.
>>
>> I am OK with that naming.  Primary concern was the introduction of the
>> term subfolio.
> 
> How about one of these:
> 
>      smaller_folio
>      inner_folio
> 
> Those are more self-explanatory, while still avoiding "subfolio".
> 
I would be fine with inner_folio.

Thanks,
Sidhartha Kumar

> thanks,
diff mbox series

Patch

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 2bb69b098117..a89728c6987d 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3438,12 +3438,12 @@  static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	return 0;
 }
 
-static int demote_free_huge_page(struct hstate *h, struct page *page)
+static int demote_free_hugetlb_folio(struct hstate *h, struct folio *folio)
 {
-	int i, nid = page_to_nid(page);
+	int i, nid = folio_nid(folio);
 	struct hstate *target_hstate;
-	struct folio *folio = page_folio(page);
 	struct page *subpage;
+	struct folio *subfolio;
 	int rc = 0;
 
 	target_hstate = size_to_hstate(PAGE_SIZE << h->demote_order);
@@ -3451,18 +3451,18 @@  static int demote_free_huge_page(struct hstate *h, struct page *page)
 	remove_hugetlb_folio_for_demote(h, folio, false);
 	spin_unlock_irq(&hugetlb_lock);
 
-	rc = hugetlb_vmemmap_restore(h, page);
+	rc = hugetlb_vmemmap_restore(h, &folio->page);
 	if (rc) {
-		/* Allocation of vmemmmap failed, we can not demote page */
+		/* Allocation of vmemmmap failed, we can not demote folio */
 		spin_lock_irq(&hugetlb_lock);
-		set_page_refcounted(page);
-		add_hugetlb_folio(h, page_folio(page), false);
+		folio_ref_unfreeze(folio, 1);
+		add_hugetlb_folio(h, folio, false);
 		return rc;
 	}
 
 	/*
 	 * Use destroy_compound_hugetlb_folio_for_demote for all huge page
-	 * sizes as it will not ref count pages.
+	 * sizes as it will not ref count folios.
 	 */
 	destroy_compound_hugetlb_folio_for_demote(folio, huge_page_order(h));
 
@@ -3477,15 +3477,15 @@  static int demote_free_huge_page(struct hstate *h, struct page *page)
 	mutex_lock(&target_hstate->resize_lock);
 	for (i = 0; i < pages_per_huge_page(h);
 				i += pages_per_huge_page(target_hstate)) {
-		subpage = nth_page(page, i);
-		folio = page_folio(subpage);
+		subpage = folio_page(folio, i);
+		subfolio = page_folio(subpage);
 		if (hstate_is_gigantic(target_hstate))
-			prep_compound_gigantic_folio_for_demote(folio,
+			prep_compound_gigantic_folio_for_demote(subfolio,
 							target_hstate->order);
 		else
 			prep_compound_page(subpage, target_hstate->order);
-		set_page_private(subpage, 0);
-		prep_new_hugetlb_folio(target_hstate, folio, nid);
+		folio_change_private(subfolio, NULL);
+		prep_new_hugetlb_folio(target_hstate, subfolio, nid);
 		free_huge_page(subpage);
 	}
 	mutex_unlock(&target_hstate->resize_lock);
@@ -3508,6 +3508,7 @@  static int demote_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
 {
 	int nr_nodes, node;
 	struct page *page;
+	struct folio *folio;
 
 	lockdep_assert_held(&hugetlb_lock);
 
@@ -3521,8 +3522,8 @@  static int demote_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
 		list_for_each_entry(page, &h->hugepage_freelists[node], lru) {
 			if (PageHWPoison(page))
 				continue;
-
-			return demote_free_huge_page(h, page);
+			folio = page_folio(page);
+			return demote_free_hugetlb_folio(h, folio);
 		}
 	}