Re: [PATCH] mm/migrate_device: fix double unlock and remove dead code

David Hildenbrand (Arm)
2026-04-14T09:51:14Z

On 4/13/26 22:03, Zi Yan wrote:
> On 13 Apr 2026, at 15:38, David Hildenbrand (Arm) wrote:
>
>> On 4/13/26 15:09, Sunny Patel wrote:
>>> Fix two bugs in device migration paths:
>>>
>>> 1) migrate_vma_collect_huge_pmd() calls spin_unlock after
>>> softleaf_entry_wait_on_locked(), which already releases the ptl.
>>>
>>> 2) migrate_vma_insert_huge_pmd_page() has a dead else-if branch and this
>>> branch is always unreachable.
>>>
>>
>> Can you move 1) into a separate patch, add a Fixes: tag an CC stable?
>>
>> I think it is
>>
>> Fixes: a30b48bf1b24 ("mm/migrate_device: implement THP migration of zone
>> device pages")
>>
>> 2) will then be a pure cleanup patch.
>>
>> Thanks!
>>
>>> Signed-off-by: Sunny Patel <nueralspacetech@gmail.com>
>>> ---
>>> mm/migrate_device.c | 4 +---
>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>>> index 8079676c8f1f..0e005c26ee88 100644
>>> --- a/mm/migrate_device.c
>>> +++ b/mm/migrate_device.c
>>> @@ -177,7 +177,6 @@ static int migrate_vma_collect_huge_pmd(pmd_t *pmdp, unsigned long start,
>>>
>>> if (softleaf_is_migration(entry)) {
>>> softleaf_entry_wait_on_locked(entry, ptl);
>>> - spin_unlock(ptl);
>>
>>
>> Yes, that looks correct to me.
>>
>>> return -EAGAIN;
>>> }
>>>
>>> @@ -869,8 +868,7 @@ static int migrate_vma_insert_huge_pmd_page(struct migrate_vma *migrate,
>>> if (!is_huge_zero_pmd(*pmdp))
>>> goto unlock_abort;
>>> flush = true;
>>> - } else if (!pmd_none(*pmdp))
>>> - goto unlock_abort;
>>> + }
>>
>> Huh, how did that happen. I hope that it's not a typo and we wanted to
>> check for something else.
>>
>
>
> Looking at the function and trying to figure this out, but find
> VM_WARN_ON_FOLIO(!folio, folio) at the top, where folio is from page_folio(page).
> It is either a nop or a zero dereferencing. It should be removed.

Heh, VM_WARN_ON_FOLIO(!folio, folio) itself is completely odd. Dump NULL
if NULL.

Agreed that this should be removed. Or replaced by a

VM_WARN_ON_ONCE(page);

>
> VM_WARN_ON_ONCE(!pmd_none(*pmdp) && !is_huge_zero_pmd(*pmdp)) can be removed
> too, since the above ifs takes !pmd_none(*pmdp) && !is_huge_zero_pmd(*pmdp)
> to unlock_abort.

Right, and if that's an unexpected case, we should warn on that path
instead. Like

if (WARN_ON_ONCE(!is_huge_zero_pmd(*pmdp)))
goto unlock_abort;

>
> Back to the above code:
>
> if (!pmd_none(*pmdp)) {
> if (!is_huge_zero_pmd(*pmdp))
> goto unlock_abort;
> flush = true;
> } else if (!pmd_none(*pmdp))
> goto unlock_abort;
>
> It seems to me that the first if should be removed, since if pmdp is
> not pmd_none(), others filled pmd entry before us, so no further
> action should be taken, otherwise, the function will overwrite
> some valid pmd entry.
>
> OK, look at migrate_vma_insert_page(), which does PTE level work,
> the above code might be intended to do:
>
> if (pmd_present(*pmdp)) {
> if (!is_huge_zero_pmd(*pmdp))
> goto unlock_abort;
> flush = true;
> } else if (!pmd_none(*pmdp))
> goto unlock_abort;

Agreed, that makes more sense. Thanks!

--
Cheers,

David