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

[David Hildenbrand <david@redhat.com>]

On 06.08.24 05:44, Kefeng Wang wrote:
> 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.

Maybe at some point we might want to remove these sanity checks or have 
explicit, expected-to-be-racy folio functions.

Like folio_test_hugetlb_racy(), folio_test_large_racy(), 
folio_nr_pages_racy().

Because the VM_DEBUG checks for folio_test_large() etc. actually make 
sense in other context where we know that concurrent splitting is 
impossible.

But maybe part of the puzzle will be in the future that we want to do a 
RCU read lock here and perform freeing/splitting under RCU, when we'll 
also have to alloc/free the "struct folio".

> 
>>>> +            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);
> }

Likely we should add a comment here that a large folio might get split 
concurrently and that folio_nr_pages() might read garbage. But out loop 
should handle that and we would revisit the split folio later.


-- 
Cheers,

David / dhildenb