Re: [PATCH] hugetlb: freeze allocated pages before creating hugetlb pages

[Mike Kravetz]

On 08/25/22 15:31, Mike Kravetz wrote:
> On 08/25/22 15:35, Joao Martins wrote:
> > On 8/12/22 06:36, Muchun Song wrote:
> > >> On Aug 11, 2022, at 06:38, Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
> > >> On 08/10/22 14:20, Muchun Song wrote:
> > >>>> On Aug 9, 2022, at 05:28, Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
> <snip>
> > >>>> There have been proposals to change at least the buddy allocator to
> > >>>> return frozen pages as described at [3].  If such a change is made, it
> > >>>> can be employed by the hugetlb code.  However, as mentioned above
> > >>>> hugetlb uses several low level allocators so each would need to be
> > >>>> modified to return frozen pages.  For now, we can manually freeze the
> > >>>> returned pages.  This is done in two places:
> > >>>> 1) alloc_buddy_huge_page, only the returned head page is ref counted.
> > >>>>  We freeze the head page, retrying once in the VERY rare case where
> > >>>>  there may be an inflated ref count.
> > >>>> 2) prep_compound_gigantic_page, for gigantic pages the current code
> > >>>>  freezes all pages except the head page.  New code will simply freeze
> > >>>>  the head page as well.
> <snip>
> > >>>> 	/*
> > >>>> 	 * By default we always try hard to allocate the page with
> > >>>> @@ -1933,7 +1934,21 @@ static struct page *alloc_buddy_huge_page(struct hstate *h,
> > >>>> 		gfp_mask |= __GFP_RETRY_MAYFAIL;
> > >>>> 	if (nid == NUMA_NO_NODE)
> > >>>> 		nid = numa_mem_id();
> > >>>> +retry:
> > >>>> 	page = __alloc_pages(gfp_mask, order, nid, nmask);
> > >>>> +
> > >>>> +	/* Freeze head page */
> > >>>> +	if (!page_ref_freeze(page, 1)) {
> > >>>
> > >>> Hi Mike,
> > >>>
> > >>> I saw Mattew has introduced a new helper alloc_frozen_pages() in thread [1] to allocate a
> > >>> frozen page. Then we do not need to handle an unexpected refcount case, which
> > >>> should be easy. Is there any consideration why we do not choose alloc_frozen_pages()?
> > >>
> > >> I asked Matthew about these efforts before creating this patch.  At the
> > >> time, there were issues with the first version of his patch series and
> > >> he wasn't sure when he would get around to looking into those issues.
> > >>
> > >> I then decided to proceed with manually freezing pages after allocation.
> > >> The thought was that alloc_frozen_pages() could be added when it became
> > >> available.  The 'downstream changes' to deal with pages that have zero
> > >> ref count should remain the same.  IMO, these downstream changes are the
> > >> more important parts of this patch.
> > >>
> > >> Shortly after sending this patch, Matthew took another look at his
> > >> series and discovered the source of issues.  He then sent this v2
> > >> series.  Matthew will correct me if this is not accurate.
> > > 
> > > Thanks Mike, it is really clear to me.
> > > 
> > >>
> > >>>
> > >>> [1] https://lore.kernel.org/linux-mm/20220809171854.3725722-15-willy@xxxxxxxxxxxxx/T/#u
> > >>>
> > >>
> > >> I am happy to wait until Matthew's series moves forward, and then use
> > >> the new interface.
> > > 
> > > I agree. Let’s wait together.
> > 
> > Maybe this is a bit of bad suggestion, but considering that the enterity of kernels
> > supporting hugetlb vmemmap optimization are using the old interface (the new one isn't yet
> > settled it seems?) would it be better to go with something this patch, and then have a
> > second patch (simpler one) that just switches to the new interface?
> > 
> 
> My thoughts.
> Right now, the earliest any of this code would be merged is in 6.1.  If
> the alloc_frozen_pages interface gors into 6.1, then it would make sense
> to just make this patch/change use it.  If not, we can manually freeze as
> done here, and switch when alloc_frozen_pages becomes available.  In either
> case, this could/should go into 6.1.
> 
> We still have a bit of time to see if alloc_frozen_pages will make 6.1.
> Ideally, we would wait and see.  Ideally, I (we) would help with Matthew's
> series.  However, I am a bit concerned that we may forget about pushing this
> forward and miss the window.  And, since Joao's optimization depends on this,
> that would miss the window as well.
> 
> Matthew, any thoughts on the likelihood of alloc_frozen_pages going into 6.1?

Unless someone objects, my plan is to send an updated verstion of this
patch (fixing __prep_compound_gigantic_page issue, thanks Miaohe!).  We
can then switch to using alloc_frozen_pages when it becomes available.

With this patch in place, Joao should be able to send an updated version of
the patch "mm/hugetlb_vmemmap: remap head page to newly allocated page".
-- 
Mike Kravetz