| Message ID | 20250411132359.312708-1-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio | expand |
On 4/11/25 15:23, Oscar Salvador wrote: > alloc_buddy_hugetlb_folio() allocates a rmappable folio, then strips the > rmappable part and freezes it. > We can simplify all that by allocating frozen pages directly. > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Oscar Salvador <osalvador@suse.de> Much nice. Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
On 11.04.25 15:23, Oscar Salvador wrote: > alloc_buddy_hugetlb_folio() allocates a rmappable folio, then strips the > rmappable part and freezes it. > We can simplify all that by allocating frozen pages directly. > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Oscar Salvador <osalvador@suse.de> > --- > mm/hugetlb.c | 17 +---------------- > 1 file changed, 1 insertion(+), 16 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index e3e6ac991b9c..83fa2b9f6fc4 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -1950,7 +1950,6 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h, > int order = huge_page_order(h); > struct folio *folio; > bool alloc_try_hard = true; > - bool retry = true; > > /* > * By default we always try hard to allocate the folio with > @@ -1965,22 +1964,8 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h, > gfp_mask |= __GFP_RETRY_MAYFAIL; > if (nid == NUMA_NO_NODE) > nid = numa_mem_id(); > -retry: > - folio = __folio_alloc(gfp_mask, order, nid, nmask); > - /* Ensure hugetlb folio won't have large_rmappable flag set. */ > - if (folio) > - folio_clear_large_rmappable(folio); > > - if (folio && !folio_ref_freeze(folio, 1)) { > - folio_put(folio); > - if (retry) { /* retry once */ > - retry = false; > - goto retry; > - } > - /* WOW! twice in a row. */ > - pr_warn("HugeTLB unexpected inflated folio ref count\n"); > - folio = NULL; > - } > + folio = (struct folio *)__alloc_frozen_pages(gfp_mask, order, nid, nmask); > > /* > * If we did not specify __GFP_RETRY_MAYFAIL, but still got a I assume htlb_alloc_mask() will always include _GFP_COMP. But semantically, it might be wrong: __folio_alloc() will in the memdesc world also make sure to allocate the memdesc, __alloc_frozen_pages() not. Maybe one would want a __alloc_frozen_folio() .... @willy?
On Fri, Apr 11, 2025 at 03:44:31PM +0200, David Hildenbrand wrote: > I assume htlb_alloc_mask() will always include _GFP_COMP. static inline gfp_t htlb_alloc_mask(struct hstate *h) { gfp_t gfp = __GFP_COMP | __GFP_NOWARN; > But semantically, it might be wrong: __folio_alloc() will in the memdesc > world also make sure to allocate the memdesc, __alloc_frozen_pages() not. > > Maybe one would want a __alloc_frozen_folio() .... @willy? This is fine. Yes, it'll need to be modified when we get to the separately allocated memdesc, but there's a number of places that cast the freshly allocated page to a folio, and I'll have to come up with a way to catch them all. Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> Oscar, if you want to take on the gigantic allocation next ... - I don't think we need folio_alloc_gigantic() to be wrapped in alloc_hooks - folio_alloc_gigantic() should return a frozen folio - as should hugetlb_cma_alloc_folio()
On 11.04.25 16:21, Matthew Wilcox wrote: > On Fri, Apr 11, 2025 at 03:44:31PM +0200, David Hildenbrand wrote: >> I assume htlb_alloc_mask() will always include _GFP_COMP. > > static inline gfp_t htlb_alloc_mask(struct hstate *h) > { > gfp_t gfp = __GFP_COMP | __GFP_NOWARN; > I checked that as well and actually meant to say the I assume that we will here always have __GFP_COMP because the gfp_mask we get here will always be derived from htlb_alloc_mask(). >> But semantically, it might be wrong: __folio_alloc() will in the memdesc >> world also make sure to allocate the memdesc, __alloc_frozen_pages() not. >> >> Maybe one would want a __alloc_frozen_folio() .... @willy? > > This is fine. Yes, it'll need to be modified when we get to the > separately allocated memdesc, but there's a number of places that > cast the freshly allocated page to a folio, and I'll have to come up > with a way to catch them all. Okay, thanks for clarifying. Reviewed-by: David Hildenbrand <david@redhat.com>
On Fri, Apr 11, 2025 at 03:21:42PM +0100, Matthew Wilcox wrote: > Oscar, if you want to take on the gigantic allocation next ... > > - I don't think we need folio_alloc_gigantic() to be wrapped in > alloc_hooks > - folio_alloc_gigantic() should return a frozen folio > - as should hugetlb_cma_alloc_folio() Yeah, I was taking a look at that as well. I plan to tackle it in a few days.
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index e3e6ac991b9c..83fa2b9f6fc4 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -1950,7 +1950,6 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h, int order = huge_page_order(h); struct folio *folio; bool alloc_try_hard = true; - bool retry = true; /* * By default we always try hard to allocate the folio with @@ -1965,22 +1964,8 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h, gfp_mask |= __GFP_RETRY_MAYFAIL; if (nid == NUMA_NO_NODE) nid = numa_mem_id(); -retry: - folio = __folio_alloc(gfp_mask, order, nid, nmask); - /* Ensure hugetlb folio won't have large_rmappable flag set. */ - if (folio) - folio_clear_large_rmappable(folio); - if (folio && !folio_ref_freeze(folio, 1)) { - folio_put(folio); - if (retry) { /* retry once */ - retry = false; - goto retry; - } - /* WOW! twice in a row. */ - pr_warn("HugeTLB unexpected inflated folio ref count\n"); - folio = NULL; - } + folio = (struct folio *)__alloc_frozen_pages(gfp_mask, order, nid, nmask); /* * If we did not specify __GFP_RETRY_MAYFAIL, but still got a
alloc_buddy_hugetlb_folio() allocates a rmappable folio, then strips the rmappable part and freezes it. We can simplify all that by allocating frozen pages directly. Suggested-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Oscar Salvador <osalvador@suse.de> --- mm/hugetlb.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-)