Re: [PATCH 4/4] mm: memory_hotplug: unify Huge/LRU/non-LRU movable folio isolation

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/8/2 4:23, David Hildenbrand wrote:
> On 25.07.24 03:16, Kefeng Wang wrote:
>> Move isolate_hugetlb() after grab a reference, and use the
>> isolate_folio_to_list() to unify hugetlb/LRU/non-LRU folio
>> isolation, which cleanup code a bit and save a few calls to
>> compound_head().
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/memory_hotplug.c | 48 +++++++++++++++------------------------------
>>   1 file changed, 16 insertions(+), 32 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index ccaf4c480aed..057037766efa 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1773,20 +1773,17 @@ static int scan_movable_pages(unsigned long 
>> start, unsigned long end,
>>   static void do_migrate_range(unsigned long start_pfn, unsigned long 
>> end_pfn)
>>   {
>>       unsigned long pfn;
>> -    struct page *page, *head;
>>       LIST_HEAD(source);
>> +    struct folio *folio;
>>       static DEFINE_RATELIMIT_STATE(migrate_rs, 
>> DEFAULT_RATELIMIT_INTERVAL,
>>                         DEFAULT_RATELIMIT_BURST);
>>       for (pfn = start_pfn; pfn < end_pfn; pfn++) {
>> -        struct folio *folio;
>> -        bool isolated;
>> +        struct page *page;
>>           if (!pfn_valid(pfn))
>>               continue;
>>           page = pfn_to_page(pfn);
>> -        folio = page_folio(page);
>> -        head = &folio->page;
>>           /*
>>            * HWPoison pages have elevated reference counts so the 
>> migration would
>> @@ -1808,36 +1805,22 @@ static void do_migrate_range(unsigned long 
>> start_pfn, unsigned long end_pfn)
>>               continue;
>>           }
>> -        if (PageHuge(page)) {
>> -            pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> -            isolate_hugetlb(folio, &source);
>> +        folio = folio_get_nontail_page(page);
>> +        if (!folio)
>>               continue;
> 
> There is one interesting case: 1 GiB hugetlb folios can span multiple 
> memory blocks (e.g., 128 MiB). Offlining individual blocks must work.
> 
> If you do the folio_get_nontail_page() we'd not be able to offline a 
> memory block in the middle anymore, because we'd never try even 
> isolating it.

Indeed, will test this case.

> 
> So likely we have to try getting the head page of a large folio instead 
> (as a fallback if this fails?) and continue from there.
> 
> In case of an free hugetlb tail page we would now iterate each 
> individual page instead of simply jumping forward like the old code 
> would have done. I think we want to maintain that behavior as well?

Yes, only can occur when the first hugetlb page if start_pfn is not head 
page, will reconsider of this part

> 
>> -        } else if (PageTransHuge(page))
>> -            pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>> -
>> -        if (!get_page_unless_zero(page))
>> -            continue;
>> -        /*
>> -         * We can skip free pages. And we can deal with pages on
>> -         * LRU and non-lru movable pages.
>> -         */
>> -        if (PageLRU(page))
>> -            isolated = isolate_lru_page(page);
>> -        else
>> -            isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
>> -        if (isolated) {
>> -            list_add_tail(&page->lru, &source);
>> -            if (!__PageMovable(page))
>> -                inc_node_page_state(page, NR_ISOLATED_ANON +
>> -                            page_is_file_lru(page));
>> -        } else {
>> +        /* Skip free folios, deal with hugetlb, LRU and non-lru 
>> movable folios. */
> 
> Can you clarify what "skip free folios" means? For free folios the 
> folio_get_nontail_page() shouldn't have succeeded. Did you mean if the 
> folio got freed in the meantime?

I think we could drop the comments here, the original comment is added
by commit 0c0e61958965 ("memory unplug: page offline"), and there is no
increase the reference to page, but with commit 700c2a46e882 ("mem-
hotplug: call isolate_lru_page with elevated refcount"), the comment
could be dropped since the folio can't be freed here.


> 
>> +        if (!isolate_folio_to_list(folio, &source)) {
>>               if (__ratelimit(&migrate_rs)) {
>>                   pr_warn("failed to isolate pfn %lx\n", pfn);
>>                   dump_page(page, "isolation failed");
>>               }
>>           }
> 
>