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

[Jinjiang Tu <tujinjiang@huawei.com>]

在 2025/6/13 0:48, Zi Yan 写道:
> On 12 Jun 2025, at 11:50, David Hildenbrand wrote:
>
>> 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).
> OK, the reason to not split is that handling split failures is too much work
> and it is better to be done in memory_failure().
>
> In terms of this patch, returning large poisoned folio back to LRU list
> seems to be the right thing to do. My thought is that the reclaim code
> is trying to free this folio, but it cannot reclaim a large poisoned folio,
> so it puts the folio back like it cannot reclaim an in-use folio.
>
>> 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.
> For this case, if unmap_poisoned_folio() fails, the whole range cannot
> be offline?

If we 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);


If memory_failure calls try_to_split_thp_page() after do_migrate_range() 
puts the folio, the THP can be handled by memory_failure(),
and the memory can be offlined after memory_failure() handling.

However, If memory_failure() fails to split due to the extra reference, 
the whole memory can't be offlined forever.
>
> Best Regards,
> Yan, Zi