Re: [PATCH] mm: migration :shared anonymous migration test is failing

[Dev Jain <dev.jain@arm.com>]

On 20/12/24 9:02 am, Baolin Wang wrote:
>
>
> On 2024/12/20 11:12, Donet Tom wrote:
>>
>> On 12/20/24 08:01, Baolin Wang wrote:
>>>
>>>
>>> On 2024/12/19 20:47, Donet Tom wrote:
>>>> The migration selftest is currently failing for shared anonymous
>>>> mappings due to a race condition.
>>>>
>>>> During migration, the source folio's PTE is unmapped by nuking the
>>>> PTE, flushing the TLB,and then marking the page for migration
>>>> (by creating the swap entries). The issue arises when, immediately
>>>> after the PTE is nuked and the TLB is flushed, but before the page
>>>> is marked for migration, another thread accesses the page. This
>>>> triggers a page fault, and the page fault handler invokes
>>>> do_pte_missing() instead of do_swap_page(), as the page is not yet
>>>> marked for migration.
>>>>
>>>> In the fault handling path, do_pte_missing() calls __do_fault()
>>>> ->shmem_fault() -> shmem_get_folio_gfp() -> filemap_get_entry().
>>>> This eventually calls folio_try_get(), incrementing the reference
>>>> count of the folio undergoing migration. The thread then blocks
>>>> on folio_lock(), as the migration path holds the lock. This
>>>> results in the migration failing in __migrate_folio(), which expects
>>>> the folio's reference count to be 2. However, the reference count is
>>>> incremented by the fault handler, leading to the failure.
>>>>
>>>> The issue arises because, after nuking the PTE and before marking the
>>>> page for migration, the page is accessed. To address this, we have
>>>> updated the logic to first nuke the PTE, then mark the page for
>>>> migration, and only then flush the TLB. With this patch, If the 
>>>> page is
>>>> accessed immediately after nuking the PTE, the TLB entry is still
>>>> valid, so no fault occurs. After marking the page for migration,
>>>
>>> IMO, I don't think this assumption is correct. At this point, the 
>>> TLB entry might also be evicted, so a page fault could still occur. 
>>> It's just a matter of probability.
>> In this patch, we mark the page for migration before flushing the TLB.
>> This ensures that if someone accesses the page after the TLB flush,
>> the page fault will occur and in the page fault handler will wait for 
>> the
>> migration to complete. So migration will not fail
>>
>> Without this patch, if someone accesses the page after the TLB flush
>> but before it is marked for migration, the migration will fail.
>
> Actually my concern is the same as David's (I did not see David's 
> reply before sending my comments), which is that your patch does not 
> "rules out all cases".


I like this solution but really the proper solution for this one was to 
atomically set the migration entry IMHO.

>
>>> Additionally, IIUC, if another thread is accessing the shmem folio 
>>> causing the migration to fail, I think this is expected, and 
>>> migration failure is not a vital issue?
>>>
>> In my case, the shmem migration test is always failing,
>> even after retries. Would it be correct to consider this
>> as expected behavior?
>
> IMHO I think your test case is too aggressive and unlikely to occur in 
> real-world scenarios. Additionally, as I mentioned, migration failure 
> is not a vital issue in the system, and some temporary refcnt can also 
> lead to migration failure if you want to create such test cases. So 
> personally, I don't think it is worthy doing.

Agreed, AFAIR the test case starts faulting exactly on those pages which 
we want to migrate, making this a very artificial scenario.