On 2024/3/15 2:15, Jane Chu wrote:
> 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@xxxxxxxxxxxxxxx>
>>>>> 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.
This sounds like a good alternative. Thanks.
