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

[Jinjiang Tu <tujinjiang@huawei.com>]

在 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);
                         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.

Skip if it's a large folio before we isolate from lru.