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

[David Hildenbrand <david@redhat.com>]

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.

-- 
Cheers,

David / dhildenb