Re: [PATCH 2/4] mm: memory_hotplug: check hwpoisoned page firstly in do_migrate_range()

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

Hi David, I have some question,

On 2024/8/2 16:02, Kefeng Wang wrote:
> 
...
>>>            */
>>> -        if (PageHWPoison(page)) {
>>> +        if (unlikely(PageHWPoison(page))) {
>>> +            folio = page_folio(page);
>>> +
>>>               if (WARN_ON(folio_test_lru(folio)))
>>>                   folio_isolate_lru(folio);
>>> +
>>>               if (folio_mapped(folio))
>>> -                try_to_unmap(folio, TTU_IGNORE_MLOCK);
>>> +                unmap_posioned_folio(folio, TTU_IGNORE_MLOCK);
>>> +
>>> +            if (folio_test_large(folio))
>>> +                pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>>               continue;
>>>           }
>>> +        if (PageHuge(page)) {
>>> +            pfn = page_to_pfn(head) + compound_nr(head) - 1;
>>> +            isolate_hugetlb(folio, &source);
>>> +            continue;
>>> +        } else if (PageTransHuge(page))

If the page is a tail page, we will BUG_ON(DEBUG_VM enabled) here, but
it seems that we don't guarantee the page won't be a tail page.

>>> +            pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;

thp_nr_pages() need a head page, I think it should use head here, so we
can directly use folio_nr_pages().

>>
>> If we can use a folio in the PageHWPoison() case, can we use one here 
>> as well? I know that it's all unreliable when not holding a folio 
>> reference, and we have to be a bit careful.
> 
> Using a folio here is part of patch4, I want to unify hugetlb/thp(or 
> large folio) with "pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1" 
> when large folio after get a ref.

Think it again, even the folio don't hold a ref(splitting concurrently
or something else), folio_nr_pages return incorrect, it won't cause 
issue since we will loop and find movable pages again in 
scan_movable_pages() and try to isolate pages, so directly use

if (folio_test_large(folio)) {
	pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
	if (folio_test_hugetlb(folio))
		isolate_hugetlb(folio, &source);
}

Correct me if I am wrong, thanks.

> 
>>
>> It feels like using folios here would mostly be fine, because things 
>> like PageHuge() already use folios internally.
>>
>> And using it in the PageHWPoison() but not here looks a bit odd.
> 
> We will convert to use folio in the following patch.
> 
>>
>> The important part is that we don't segfault if we'd overshoot our 
>> target.
>>
>