Re: [PATCH V2] mm/gup.c: stricter check on THP migration entry during follow_pmd_mask

[Li Xinhai <lixinhai.lxh@gmail.com>]

On 12/15/21 10:46 PM, Zi Yan wrote:
> On 15 Dec 2021, at 7:47, Li Xinhai wrote:
> 
>> On 12/11/21 11:16 PM, Li Xinhai wrote:
>>>
>>>
>>> On 12/11/21 11:10 PM, Li Xinhai wrote:
>>>> When BUG_ON check for THP migration entry, the exsiting code only check
>>>> thp_migration_supported case, but not for !thp_migration_supported case.
>>>> To make the BUG_ON check consistent, we need catch both cases.
>>>>
>>>> Move the BUG_ON check one step eariler, because if the bug happen we
>>>> should know it instead of depend on FOLL_MIGRATION been used by caller.
>>>>
>>>> Because pmdval instead of *pmd is read by the is_pmd_migration_entry()
>>>> check, the existing code don't help to avoid useless locking within
>>>> pmd_migration_entry_wait(), so remove that check.
>>>>
>>>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>>>> ---
>>> V1->V2:
>>> Move the BUG_ON() check before if(!(flags & FOLL_MIGRATION)); and add comments
>>> for it.
>>>
>> Yan, Ying and Kirill have been worked on this part of code, may help to review.
>>
>> This change was based on my code review, no real bug has been observed.
>>
>>
>>>>    mm/gup.c | 13 +++++++++----
>>>>    1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>> index 2c51e9748a6a..94d0e586ca0b 100644
>>>> --- a/mm/gup.c
>>>> +++ b/mm/gup.c
>>>> @@ -642,12 +642,17 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
>>>>        }
>>>>    retry:
>>>>        if (!pmd_present(pmdval)) {
>>>> +        /*
>>>> +         * Should never reach here, if thp migration is not supported;
>>>> +         * Otherwise, it must be a thp miration entry.
>>>> +         */
>>>> +        VM_BUG_ON(!thp_migration_supported() ||
>>>> +                  !is_pmd_migration_entry(pmdval));
>>>> +
> 
> This means VM_BUG_ON will be triggered when pmdval is not present and THP migration
> is not enabled. This can happen when it is pmd_none(). That is not a bug and should
> just return no_page_table(vma, flags).
> 

Thanks for review!
The pmd_none() has been checked at the beginning of follow_pmd_mask() and before
the 'retry' loop start again, so that possibility is excluded.

We will have VM_BUG_ON(1) if thp_migration_supported() is false, or
VM_BUG_ON(!is_pmd_migration_entry(pmdval)) if thp_migration_supported() is true, by
compiler.

If we left !thp_migration_supported() case for the VM_BUG_ON(), it will cause
misunderstanding that that case is not a bug at the code context.

>>>>            if (likely(!(flags & FOLL_MIGRATION)))
>>>>                return no_page_table(vma, flags);
>>>> -        VM_BUG_ON(thp_migration_supported() &&
>>>> -                  !is_pmd_migration_entry(pmdval));
>>>> -        if (is_pmd_migration_entry(pmdval))
>>>> -            pmd_migration_entry_wait(mm, pmd);
>>>> +
>>>> +        pmd_migration_entry_wait(mm, pmd);
>>>>            pmdval = READ_ONCE(*pmd);
>>>>            /*
>>>>             * MADV_DONTNEED may convert the pmd to null because
>>>>
> 
> --
> Best Regards,
> Yan, Zi
>