Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ac30dbee-59d4-1b65-5a88-a8f19f0601c4@gmail.com>
Date:   Thu, 22 Sep 2022 16:14:27 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.2
Subject: Re: [PATCH 0/3] mm/hugetlb: hugepage migration enhancements
Content-Language: en-US
To:     Mike Kravetz <mike.kravetz@oracle.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Muchun Song <songmuchun@bytedance.com>,
        Oscar Salvador <osalvador@suse.de>,
        Michal Hocko <mhocko@suse.com>,
        David Hildenbrand <david@redhat.com>,
        Florian Fainelli <f.fainelli@gmail.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20220921223639.1152392-1-opendmb@gmail.com>
 <YyzEz4snl2x51iTY@monkey>
From:   Doug Berger <opendmb@gmail.com>
In-Reply-To: <YyzEz4snl2x51iTY@monkey>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/22/2022 1:25 PM, Mike Kravetz wrote:
> On 09/21/22 15:36, Doug Berger wrote:
>> This patch set was included as patches [04/21-06/21] in my
>> previous patch set to introduce Designated Movable Blocks [1].
>> They were included there for context, but they have value on
>> their own and are being resubmitted here for consideration on
>> their own merits.
>>
>> The alloc_contig_range() function attempts to isolate the free
>> pages within the range and migrate the data from non-free pages
>> within the range to allow those pages to become isolated free
>> pages. If all of the pages in the range are on isolated free
>> page lists they can be allocated to the caller.
>>
>> When free hugepages are encountered in the range an attempt is
>> made to allocate a new compound page to become a replacement
>> hugepage and to dissolve the free hugepage so that its pages
>> within isolated pageblocks can be added to the isolated free
>> page lists. Hugepages that are not free and encountered within
>> the range must be migrated out of the range and dissolved to
>> allow the underlying pages to be added to the isolated free
>> page lists.
>>
>> Moving the data from hugepages within the range and freeing the
>> hugepages is not sufficient since the allocation of migration
>> target hugepages is allowed to come from free hugepages that may
>> contain isolated pageblocks and freed hugepages will not be on
>> isolated free page lists so the alloc_contig_range() will fail.
> 
> Thanks for looking into this!  I am adding Oscar, Michal and David on
> Cc: as they have looked at similar issues in the past.
Thanks for helping to get the right eyeballs looking at this.

> 
> Before jumping into the details of your changes, I just want to make one
> observation.  When migrating (or dissolving) hugetlb pages that are in a
> range operated on by alloc_contig_range(), we should not change the number
> of hugetlb pages available as noted here.  This includes the number of
> total huge pages and the number of huge pages on the node.  Therefore,
> we should allocate another huge page from buddy either as the migration
> target or to take the place of the dissolved page.
> 
> For free huge pages, we do this via alloc_and_dissolve_huge_page.  IIUC,
> there are no issues with this code path?
Yes, I did not observe any issue with the the free hugepage handling 
except that your recent improvements with freezing allocated pages 
(thanks for those) will make merging patch 1 more difficult ;).

> 
> As noted above, for pages to be migrated we first try to use an existing
> free huge page as the target.  Quite some time ago, Michal added code to
> allocate a new page from buddy as the target if no free huge pages were
> available.  This change also included a special flag to dissolve the
> source huge page when it is freed.  It seems like this is the exact
> behavior we want here?  I wonder if it might be easier just to use this
> existing code?
Yes, the temporary page flag can be made to work here and I did 
experiment with it, but it is dependent on the current design decisions.
I decided to move to the approach suggested here because I believe it 
could conceivably scale if support for migrating gigantic pages is 
desired in the future. The circular dependency on alloc_contig_range 
will likely keep that from happening, but that was my thinking.

Thanks for taking the time,
-Doug