Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   David Hildenbrand <david@redhat.com>
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC PATCH 2/2] mm,page_alloc: Make alloc_contig_range handle free hugetlb pages
Date:   Thu, 25 Feb 2021 23:15:15 +0100
Message-Id: <1C808F10-158D-4DB8-A393-01829A398B17@redhat.com>
References: <b4bf7fcf-449d-daed-e490-994b1072652a@oracle.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Oscar Salvador <osalvador@suse.de>,
        Muchun Song <songmuchun@bytedance.com>,
        Michal Hocko <mhocko@kernel.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
In-Reply-To: <b4bf7fcf-449d-daed-e490-994b1072652a@oracle.com>
To:     Mike Kravetz <mike.kravetz@oracle.com>
Precedence: bulk


> Am 25.02.2021 um 22:43 schrieb Mike Kravetz <mike.kravetz@oracle.com>:
>=20
> =EF=BB=BFOn 2/10/21 12:23 AM, David Hildenbrand wrote:
>>> On 08.02.21 11:38, Oscar Salvador wrote:
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -952,6 +952,17 @@ isolate_migratepages_block(struct compact_control *=
cc, unsigned long low_pfn,
>>>                      low_pfn +=3D compound_nr(page) - 1;
>>>                      goto isolate_success_no_list;
>>>                  }
>>> +            } else {
>>=20
>> } else if (alloc_and_dissolve_huge_page(page))) {
>>=20
>> ...
>>=20
>>> +                /*
>>> +                 * Free hugetlb page. Allocate a new one and
>>> +                 * dissolve this is if succeed.
>>> +                 */
>>> +                if (alloc_and_dissolve_huge_page(page)) {
>>> +                    unsigned long order =3D buddy_order_unsafe(page);
>>> +
>>> +                    low_pfn +=3D (1UL << order) - 1;
>>> +                    continue;
>>> +                }
>>=20
>>=20
>>=20
>> Note that there is a very ugly corner case we will have to handle gracefu=
lly (I think also in patch #1):
>>=20
>> Assume you allocated a gigantic page (and assume that we are not using CM=
A for gigantic pages for simplicity). Assume you want to allocate another on=
e. alloc_pool_huge_page()->...->alloc_contig_pages() will stumble over the f=
irst allocated page. It will try to alloc_and_dissolve_huge_page() the exist=
ing gigantic page. To do that, it will alloc_pool_huge_page()->...->alloc_co=
ntig_pages() ... and so on. Bad.
>>=20
>=20
> Sorry for resurrecting an old thread.
> While looking at V3 of these patches, I was exploring all the calling
> sequences looking for races and other issues.  It 'may' be that the
> issue about infinitely allocating and freeing gigantic pages may not be
> an issue.  Of course, I could be mistaken.  Here is my reasoning:
>=20
> alloc_and_dissolve_huge_page (now isolate_or_dissolve_huge_page) will be
> called from __alloc_contig_migrate_range() within alloc_contig_range().
> Before calling __alloc_contig_migrate_range, we call start_isolate_page_ra=
nge
> to isolate all page blocks in the range.  Because all the page blocks in
> the range are isolated, another invocation of alloc_contig_range will
> not operate on any part of that range.  See the comments for
> start_isolate_page_range or commit 2c7452a075d4.  So, when
> start_isolate_page_range goes to allocate another gigantic page it will
> never notice/operate on the existing gigantic page.
>=20
> Again, this is confusing and I might be missing something.

I think you are right that the endless loop is blocked. But I think the whol=
e thing could cascade once we have multiple gigantic pages allocated.

Try allocating a new gpage. We find an existing gpage, isolate it and try to=
 migrate it. To do that, we try allocating a new gpage. We find yet another e=
xisting gpage, isolate and try to migrate it ... until we isolated all gpage=
s on out way to an actual usable area. Then we have to actually migrate all t=
hese in reverse order ...

Of course this only works if we can actually isolate a gigantic page - which=
 should be the case I think (they are migratable and should be marked as mov=
able).

>=20
> In any case, I agree that gigantic pages are tricky and we should leave
> them out of the discussion for now.  We can rethink this later if
> necessary.

Yes, it=E2=80=98s tricky and not strictly required right now because we neve=
r place them on ZONE_MOVABLE. And as I said, actual use cases might be rare.=