Re: [PATCH v4 01/12] mm/rmap: convert page -> folio for hwpoison checks

[Dev Jain]

On 09/06/26 7:14 pm, David Hildenbrand (Arm) wrote:
> On 5/26/26 08:36, Dev Jain wrote:
>> If any page of a folio is poisoned, try_to_unmap() is called with the
>> intent to remove any mappings pointing to any part of the folio.
>>
>> Therefore, an hwpoison check on a single page does not make sense
>> (although everything works out because the memory-failure path sets
>> hwpoison on the head page too). Convert these checks to check whether
>> the folio has at least one hwpoisoned page.
> 
> I think the reasoning is different and should better be described:
> 
> 1) For hugetlb, we will always get subpage == head page (single PTE -> first PFN).
> 
> try_memory_failure_hugetlb()->try_memory_failure_hugetlb()->hugetlb_update_hwpoison()
> will do the folio_test_set_hwpoison().
> 
> folio_test_set_hwpoison() is the same as setting it on the head page.
> 
> For hugetlb, subpage==headpage
> 
> That's why PageHWPoison(subpage) used to work.
> 
> 
> 2) try_to_unmap() cannot handle large folios (besides hugetlb folios).
> 
> unmap_poisoned_folio() spells that out. That's why subpage==headpage right now.

Should have mentioned: subpage != headpage for contpte mapped hugetlb folios
on arm64.

But I think even in that case, hwpoison is not set on the specific pfn, it is
set only on the head page. So I think folio_test_hwpoison() would suffice.

> 
> 
> That's why PageHWPoison(subpage) used to work.
> 
>>
>> While at it, convert VM_BUG_* to VM_WARN_*.
>>
>> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
>> ---
>>  mm/rmap.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index fb3c351f8c458..430c91c8fe2ae 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -2116,10 +2116,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>  			bool anon = folio_test_anon(folio);
>>  
>>  			/*
>> -			 * The try_to_unmap() is only passed a hugetlb page
>> -			 * in the case where the hugetlb page is poisoned.
>> +			 * The try_to_unmap() is only passed a hugetlb folio
>> +			 * in the case where the hugetlb folio contains a
>> +			 * poisoned page.
>>  			 */
>> -			VM_BUG_ON_PAGE(!PageHWPoison(subpage), subpage);
>> +			VM_WARN_ON_FOLIO(!folio_contain_hwpoisoned_page(folio), folio);
> 
> I think we could also just use folio_test_hwpoison() here.
> 
>>  			/*
>>  			 * huge_pmd_unshare may unmap an entire PMD page.
>>  			 * There is no way of knowing exactly which PMDs may
>> @@ -2198,7 +2199,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>  		/* Update high watermark before we lower rss */
>>  		update_hiwater_rss(mm);
>>  
>> -		if (PageHWPoison(subpage) && (flags & TTU_HWPOISON)) {
>> +		if (folio_contain_hwpoisoned_page(folio) && (flags & TTU_HWPOISON)) {
>>  			pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
>>  			if (folio_test_hugetlb(folio)) {
>>  				hugetlb_count_sub(folio_nr_pages(folio), mm);
> 
> And here. Which might be cleaner, because folio_contain_hwpoisoned_page() is so
> far never called on hugetlb folios.
> 
> Because folio_set_has_hwpoisoned() is only ever used on THPs, not hugetlb. It
> would currently work, but the expectation so far is that these primitives are
> for small folios+thp, not hugetlb.
> 
> You could add a comment here, that with TTU_HWPOISON, we only ever expect small
> folios or hugetlb folios for now.
>