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

[Zi Yan <ziy@nvidia.com>]

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? 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?

Best Regards,
Yan, Zi