linux-kernel - Re: [PATCH] hugetlb: freeze allocated pages before creating hugetlb pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ywf4Kx0isZai48As@monkey>
Date:   Thu, 25 Aug 2022 15:31:07 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Joao Martins <joao.m.martins@...cle.com>
Cc:     Muchun Song <muchun.song@...ux.dev>, Linux MM <linux-mm@...ck.org>,
        linux-kernel@...r.kernel.org,
        Muchun Song <songmuchun@...edance.com>,
        Matthew Wilcox <willy@...radead.org>,
        Michal Hocko <mhocko@...e.com>, Peter Xu <peterx@...hat.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] hugetlb: freeze allocated pages before creating hugetlb
 pages

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@...cle.com> wrote:
> >> On 08/10/22 14:20, Muchun Song wrote:
> >>>> On Aug 9, 2022, at 05:28, Mike Kravetz <mike.kravetz@...cle.com> 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@infradead.org/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?
-- 
Mike Kravetz