| Message ID | 20211211151014.650778-1-lixinhai.lxh@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] mm/gup.c: stricter check on THP migration entry during follow_pmd_mask | expand |
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. > 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)); > + > 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 >
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)); >> + >> 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 >>
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). >>> 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
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 >
On 12/16/21 4:55 PM, Huang, Ying wrote: > Li Xinhai <lixinhai.lxh@gmail.com> 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@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. > > 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 >>>
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)); + 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
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> --- mm/gup.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)