Re: [PATCH v2 1/4] mm,hwpoison,hugetlb,memory_hotplug: hotremove memory section with hwpoisoned hugepage

[Miaohe Lin <linmiaohe@huawei.com>]

On 2022/9/6 14:14, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Tue, Sep 06, 2022 at 10:59:58AM +0800, Miaohe Lin wrote:
>> On 2022/9/5 14:21, Naoya Horiguchi wrote:
>>> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
>>>
>>> HWPoisoned page is not supposed to be accessed once marked, but currently
>>> such accesses can happen during memory hotremove because do_migrate_range()
>>> can be called before dissolve_free_huge_pages() is called.
>>>
>>> Move dissolve_free_huge_pages() before scan_movable_pages(). Recently
>>> delayed dissolve has been implemented, so the dissolving can turn
>>> a hwpoisoned hugepage into 4kB hwpoison page, which memory hotplug can
>>> handle safely.
>>
>> Yes, thanks for your work, Naoya. ;)
>>
>>>
>>> Reported-by: Miaohe Lin <linmiaohe@huawei.com>
>>> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
>>> ---
>>>  mm/memory_hotplug.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index fad6d1f2262a..c24735d63b25 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1880,6 +1880,17 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>>  
>>>  			cond_resched();
>>>  
>>> +			/*
>>> +			 * Dissolve free hugepages in the memory block before doing
>>> +			 * offlining actually in order to make hugetlbfs's object
>>> +			 * counting consistent.
>>> +			 */
>>> +			ret = dissolve_free_huge_pages(start_pfn, end_pfn);
>>> +			if (ret) {
>>> +				reason = "failure to dissolve huge pages";
>>> +				goto failed_removal_isolated;
>>> +			}
>>
>> This change has a side-effect. If hugetlb pages are in-use, dissolve_free_huge_pages() will always return -EBUSY
>> even if those pages can be migrated. So we fail to hotremove the memory even if they could be offlined.
>> Or am I miss something?
> 
> Thank you for the comment, you're right.  (Taking a look over my test result
> carefully, it showed failures for the related cases, I somehow overlooked
> them, really sorry.)  So my second thought is that we keep offline_pages()
> as is, and insert a few line in do_migrate_range() to handle the case of
> hwpoisoned hugepage like below:
> 
> @@ -1642,6 +1642,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> 
>                 if (PageHuge(page)) {
>                         pfn = page_to_pfn(head) + compound_nr(head) - 1;
> +                       if (PageHWPoison(head))
> +                               continue;

Thanks for your update. But it seems this is not enough. With the above code change, HWPoisoned
hugetlb pages will always be ignored in do_migrate_range(). And if these pages are HPageMigratable,
they will be returned in scan_movable_pages() then passed into the do_migrate_range() again. Thus
a possible deadloop will occur until these pages become un-movable?

>                         isolate_hugetlb(head, &source);
>                         continue;
>                 } else if (PageTransHuge(page))
> 
> This is slightly different from your original suggestion
> https://lore.kernel.org/linux-mm/20220421135129.19767-1-linmiaohe@huawei.com/T
> , as discussed in the thread existing "if (PageHWPoison(page))" branch in
> this function can't be used for hugetlb.  We could adjust them to handle
> hugetlb, but maybe separating code for hugetlb first from the others looks
> less compicated to me.

It might be better to do something, e.g. unmap the hugetlb pages to prevent accessing from process if mapped,
even try truncating the error page from pagecache. But I have no strong opinion as handling memory failure
would always be a best try. ;)

Thanks,
Miaohe Lin


> 
> If you have any suggestion on this, please let me know.
> 
> Thanks,
> Naoya Horiguchi
>