| Message ID | 20230103191340.116536-9-sidhartha.kumar@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | continue hugetlb folio conversions | expand |
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.
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?
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.
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
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.
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,
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 --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); } }
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(-)