Re: [PATCH] mm/vmscan: fix hwpoisoned large folio handling in shrink_folio_list

[Jinjiang Tu <tujinjiang@huawei.com>]

在 2025/6/17 3:27, David Hildenbrand 写道:
> On 16.06.25 13:33, Jinjiang Tu wrote:
>>
>> 在 2025/6/12 23:50, David Hildenbrand 写道:
>>> On 12.06.25 17:35, Zi Yan wrote:
>>>> On 12 Jun 2025, at 3:53, David Hildenbrand wrote:
>>>>
>>>>> On 11.06.25 19:52, Zi Yan wrote:
>>>>>> On 11 Jun 2025, at 13:34, David Hildenbrand wrote:
>>>>>>
>>>>>>>> So __folio_split() has an implicit rule that:
>>>>>>>> 1. if the given list is not NULL, the folio cannot be on LRU;
>>>>>>>> 2. if the given list is NULL, the folio is on LRU.
>>>>>>>>
>>>>>>>> And the rule is buried deeply in lru_add_split_folio().
>>>>>>>>
>>>>>>>> Should we add some checks in __folio_split()?
>>>>>>>>
>>>>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>>>>> index d3e66136e41a..8ce2734c9ca0 100644
>>>>>>>> --- a/mm/huge_memory.c
>>>>>>>> +++ b/mm/huge_memory.c
>>>>>>>> @@ -3732,6 +3732,11 @@ static int __folio_split(struct folio
>>>>>>>> *folio, unsigned int new_order,
>>>>>>>>          VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>>>>>>>>          VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
>>>>>>>>
>>>>>>>> +    if (list && folio_test_lru(folio))
>>>>>>>> +        return -EINVAL;
>>>>>>>> +    if (!list && !folio_test_lru(folio))
>>>>>>>> +        return -EINVAL;
>>>>>>>> +
>>>>>>>
>>>>>>> I guess we currently don't run into that, because whenever a folio
>>>>>>> is otherwise isolated, there is an additional reference or a page
>>>>>>> table mapping, so it cannot get split either way (e.g., freezing
>>>>>>> the refcount fails).
>>>>>>>
>>>>>>> So maybe these checks would be too early and they should happen
>>>>>>> after we froze the refcount?
>>>>>>
>>>>>> But if the caller does the isolation, the additional refcount is OK
>>>>>> and
>>>>>> can_split_folio() will return true. In addition, __folio_split()
>>>>>> does not
>>>>>> change folio LRU state, so these two checks are orthogonal to 
>>>>>> refcount
>>>>>> check, right? The placement of them does not matter, but earlier
>>>>>> the better
>>>>>> to avoid unnecessary work. I see these are sanity checks for 
>>>>>> callers.
>>>>>
>>>>> In light of the discussion in this thread, if you have someone that
>>>>> takes the folio off the LRU concurrently, I think we could still run
>>>>> into a race here. Because that could happen just after we passed the
>>>>> test in __folio_split().
>>>>>
>>>>> That's why I think the test would have to happen when there are no
>>>>> such races possible anymore.
>>>>
>>>> Make sense. Thanks for the explanation.
>>>>
>>>>>
>>>>> But the real question is if it is okay to remove the folio from the
>>>>> LRU as done in the patch discussed here ...
>>>>
>>>> I just read through the email thread. IIUC, when
>>>> deferred_split_scan() split
>>>> a THP, it expects the THP is on LRU list. I think it makes sense since
>>>> all these THPs are in both the deferred_split_queue and LRU list.
>>>> And deferred_split_scan() uses split_folio() without providing a list
>>>> to store the after-split folios.
>>>>
>>>> In terms of the patch, since unmap_poisoned_folio() does not handle
>>>> large
>>>> folios, why not just split the large folios and add the after-split
>>>> folios
>>>> to folio_list?
>>>
>>> That's what I raised, but apparently it might not be worth it for that
>>> corner case (splitting might fail).
>>>
>>> Then, the while loop will go over all the after-split folios
>>>> one by one.
>>>>
>>>> BTW, unmap_poisoned_folio() is also used in do_migrate_range() from
>>>> memory_hotplug.c and there is no guard for large folios either. That
>>>> also needs a fix?
>>>
>>> Yes, that was mentioned, and I was hoping we could let
>>> unmap_poisoned_folio() check+fail in that case.
>>
>> Maybe we could fix do_migrate_range() like below:
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 8305483de38b..5a6d869e6b56 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1823,7 +1823,10 @@ static void do_migrate_range(unsigned long
>> start_pfn, unsigned long end_pfn)
>>                           pfn = folio_pfn(folio) + 
>> folio_nr_pages(folio) - 1;
>>
>>                   if (folio_contain_hwpoisoned_page(folio)) {
>> -                       if (WARN_ON(folio_test_lru(folio)))
>> +                       if (folio_test_large(folio))
>> +                               goto put_folio;
>> +
>> +                       if (folio_test_lru(folio))
> >                                   folio_isolate_lru(folio);
>
> Hm, what is supposed to happen if we fail folio_isolate_lru()?

unmap_poisoned_folio() seems not to be assumed with lru flag cleared.

But other calls of unmap_poisoned_folio() guarantee the folio is 
ioslated, maybe we should be consistent with them?

>
>>                           if (folio_mapped(folio)) {
>>                                   folio_lock(folio);
>>
>> The folio may be on lru, if folio_test_lru() check happens between
>> setting hwposion flag and isolating from lru in memory_failure().
>> So, I remove the WARN_ON.
>
> I guess this would work, although this special-casing on large folios 
> in the caller of unmap_poisoned_folio() is rather weird.
>
> What is supposed to happen if unmap_poisoned_folio() failed for small 
> folios? Why are we okay with having the LRU flag cleared and the folio 
> isolated?

In hwpoison_user_mappings(), if unmap_poisoned_folio() fails, the small 
folio is kept with isolated too.

IIUC, after the folio is kept with ioslated, other subsystems could not 
operate on this folio, to avoid introduing issues.