On 12/16/21 4:55 PM, Huang, Ying wrote:
Li Xinhai <lixinhai.lxh@xxxxxxxxx> writes:
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@xxxxxxxxx>
---
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.
I think your code works. The patch description may be improved. If
!thp_migration_supported() and !pmd_present(), the original code may
dead loop in theory.
Thanks for review.
I will mention this potential dead loop in the commit message.
Best Regards,
Huang, Ying
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
