Re: [PATCH 6/8] mm/memory-failure: Convert memory_failure() to use a folio

[Jane Chu <jane.chu@oracle.com>]

On 3/13/2024 7:34 PM, Miaohe Lin wrote:

> On 2024/3/13 9:23, Jane Chu wrote:
>> On 3/12/2024 7:14 AM, Matthew Wilcox wrote:
>>
>>> On Tue, Mar 12, 2024 at 03:07:39PM +0800, Miaohe Lin wrote:
>>>> On 2024/3/11 20:31, Matthew Wilcox wrote:
>>>>> Assuming we have a refcount on this page so it can't be simultaneously
>>>>> split/freed/whatever, these three sequences are equivalent:
>>>> If page is stable after page refcnt is held, I agree below three sequences are equivalent.
>>>>
>>>>> 1    if (PageCompound(p))
>>>>>
>>>>> 2    struct page *head = compound_head(p);
>>>>> 2    if (PageHead(head))
>>>>>
>>>>> 3    struct folio *folio = page_folio(p);
>>>>> 3    if (folio_test_large(folio))
>>>>>
>>>>> .
>>>>>
>>>> But please see below commit:
>>>>
>>>> """
>>>> commit f37d4298aa7f8b74395aa13c728677e2ed86fdaf
>>>> Author: Andi Kleen <ak@linux.intel.com>
>>>> Date:   Wed Aug 6 16:06:49 2014 -0700
>>>>
>>>>       hwpoison: fix race with changing page during offlining
>>>>
>>>>       When a hwpoison page is locked it could change state due to parallel
>>>>       modifications.  The original compound page can be torn down and then
>>>>       this 4k page becomes part of a differently-size compound page is is a
>>>>       standalone regular page.
>>>>
>>>>       Check after the lock if the page is still the same compound page.
>>> I can't speak to what the rules were ten years ago, but this is not
>>> true now.  Compound pages cannot be split if you hold a refcount.
>>> Since we don't track a per-page refcount, we wouldn't know which of
>>> the split pages to give the excess refcount to.
>> I noticed this recently
>>
>>   * GUP pin and PG_locked transferred to @page. Rest subpages can be freed if
>>   * they are not mapped.
>>   *
>>   * Returns 0 if the hugepage is split successfully.
>>   * Returns -EBUSY if the page is pinned or if anon_vma disappeared from under
>>   * us.
>>   */
>> int split_huge_page_to_list(struct page *page, struct list_head *list)
>> {
>>
>> I have a test case with poisoned shmem THP page that was mlocked and
>>
>> GUP pinned (FOLL_LONGTERM|FOLL_WRITE), but the split succeeded.
> Thanks for points this out. Compound pages can be split even if extra refcnt is held. So folio_test_large
> check is not stable if we hold a refcnt now? Will it introduce some obscure races?
>
> Except from that, I think a page cannot become a subpage of a THP when extra refcnt is held now. So below code can be removed.
> Any thought?
>
> 	/*
> 	 * We're only intended to deal with the non-Compound page here.
> 	 * However, the page could have changed compound pages due to
> 	 * race window. If this happens, we could try again to hopefully
> 	 * handle the page next round.
> 	 */
> 	if (PageCompound(p)) {
> 		if (retry) {
> 			ClearPageHWPoison(p);
> 			unlock_page(p);
> 			put_page(p);
> 			flags &= ~MF_COUNT_INCREASED;
> 			retry = false;
> 			goto try_again;
> 		}
> 		res = action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
> 		goto unlock_page;
> 	}
Not sure of what scenario it was meant to deal with.  How about adding a 
warning instead of removal? It'll be interesting to see how the warning 
got triggered. But if after a while nothing happens, then remove it.

thanks!

-jane

>
> Thanks.
>
>> thanks,
>>
>> -jane
>>
>> .