| Message ID | 20220425132723.34824-4-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup and fixup patches for migration | expand |
On 25.04.22 15:27, Miaohe Lin wrote: > We might fail to isolate huge page due to e.g. the page is under migration > which cleared HPageMigratable. So we should return -EBUSY in this case > rather than always return 1 which could confuse the user. Also we make > the prototype of isolate_huge_page consistent with isolate_lru_page to > improve the readability. > > Fixes: e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") If this is a fix, what's the runtime effect of it? You state "could confuse", which doesn't indicate an actual BUG to me.
On Mon, Apr 25, 2022 at 09:27:22PM +0800, Miaohe Lin wrote: > We might fail to isolate huge page due to e.g. the page is under migration > which cleared HPageMigratable. So we should return -EBUSY in this case > rather than always return 1 which could confuse the user. Also we make > the prototype of isolate_huge_page consistent with isolate_lru_page to > improve the readability. > > Fixes: e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") > Suggested-by: Huang Ying <ying.huang@intel.com> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > include/linux/hugetlb.h | 6 +++--- > mm/gup.c | 2 +- > mm/hugetlb.c | 11 +++++------ > mm/memory-failure.c | 2 +- > mm/mempolicy.c | 2 +- > mm/migrate.c | 5 +++-- > 6 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index 04f0186b089b..306d6ef3fa22 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -170,7 +170,7 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to, > vm_flags_t vm_flags); > long hugetlb_unreserve_pages(struct inode *inode, long start, long end, > long freed); > -bool isolate_huge_page(struct page *page, struct list_head *list); > +int isolate_huge_page(struct page *page, struct list_head *list); > int get_hwpoison_huge_page(struct page *page, bool *hugetlb); > int get_huge_page_for_hwpoison(unsigned long pfn, int flags); > void putback_active_hugepage(struct page *page); > @@ -376,9 +376,9 @@ static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr, > return NULL; > } > > -static inline bool isolate_huge_page(struct page *page, struct list_head *list) > +static inline int isolate_huge_page(struct page *page, struct list_head *list) Since you already touched all the call sites, how about renaming this to hugetlb_isolate()? I've always felt that huge_page is not a straightforward and clear name since we also have another type of huge page (THP). I think hugetlb is more specific. Thanks.
On 2022/4/29 19:36, Muchun Song wrote: > On Mon, Apr 25, 2022 at 09:27:22PM +0800, Miaohe Lin wrote: >> We might fail to isolate huge page due to e.g. the page is under migration >> which cleared HPageMigratable. So we should return -EBUSY in this case >> rather than always return 1 which could confuse the user. Also we make >> the prototype of isolate_huge_page consistent with isolate_lru_page to >> improve the readability. >> >> Fixes: e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") >> Suggested-by: Huang Ying <ying.huang@intel.com> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> include/linux/hugetlb.h | 6 +++--- >> mm/gup.c | 2 +- >> mm/hugetlb.c | 11 +++++------ >> mm/memory-failure.c | 2 +- >> mm/mempolicy.c | 2 +- >> mm/migrate.c | 5 +++-- >> 6 files changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >> index 04f0186b089b..306d6ef3fa22 100644 >> --- a/include/linux/hugetlb.h >> +++ b/include/linux/hugetlb.h >> @@ -170,7 +170,7 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to, >> vm_flags_t vm_flags); >> long hugetlb_unreserve_pages(struct inode *inode, long start, long end, >> long freed); >> -bool isolate_huge_page(struct page *page, struct list_head *list); >> +int isolate_huge_page(struct page *page, struct list_head *list); >> int get_hwpoison_huge_page(struct page *page, bool *hugetlb); >> int get_huge_page_for_hwpoison(unsigned long pfn, int flags); >> void putback_active_hugepage(struct page *page); >> @@ -376,9 +376,9 @@ static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr, >> return NULL; >> } >> >> -static inline bool isolate_huge_page(struct page *page, struct list_head *list) >> +static inline int isolate_huge_page(struct page *page, struct list_head *list) > > Since you already touched all the call sites, how about renaming this > to hugetlb_isolate()? I've always felt that huge_page is not a > straightforward and clear name since we also have another type of > huge page (THP). I think hugetlb is more specific. > Sorry for late respond. This suggestion looks good to me. But is isolate_hugetlb more suitable? This could make it more consistent with isolate_lru_page? What do you think? Thanks! > Thanks. > > . >
On Mon, May 9, 2022 at 11:24 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > > On 2022/4/29 19:36, Muchun Song wrote: > > On Mon, Apr 25, 2022 at 09:27:22PM +0800, Miaohe Lin wrote: > >> We might fail to isolate huge page due to e.g. the page is under migration > >> which cleared HPageMigratable. So we should return -EBUSY in this case > >> rather than always return 1 which could confuse the user. Also we make > >> the prototype of isolate_huge_page consistent with isolate_lru_page to > >> improve the readability. > >> > >> Fixes: e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") > >> Suggested-by: Huang Ying <ying.huang@intel.com> > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > >> --- > >> include/linux/hugetlb.h | 6 +++--- > >> mm/gup.c | 2 +- > >> mm/hugetlb.c | 11 +++++------ > >> mm/memory-failure.c | 2 +- > >> mm/mempolicy.c | 2 +- > >> mm/migrate.c | 5 +++-- > >> 6 files changed, 14 insertions(+), 14 deletions(-) > >> > >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > >> index 04f0186b089b..306d6ef3fa22 100644 > >> --- a/include/linux/hugetlb.h > >> +++ b/include/linux/hugetlb.h > >> @@ -170,7 +170,7 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to, > >> vm_flags_t vm_flags); > >> long hugetlb_unreserve_pages(struct inode *inode, long start, long end, > >> long freed); > >> -bool isolate_huge_page(struct page *page, struct list_head *list); > >> +int isolate_huge_page(struct page *page, struct list_head *list); > >> int get_hwpoison_huge_page(struct page *page, bool *hugetlb); > >> int get_huge_page_for_hwpoison(unsigned long pfn, int flags); > >> void putback_active_hugepage(struct page *page); > >> @@ -376,9 +376,9 @@ static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr, > >> return NULL; > >> } > >> > >> -static inline bool isolate_huge_page(struct page *page, struct list_head *list) > >> +static inline int isolate_huge_page(struct page *page, struct list_head *list) > > > > Since you already touched all the call sites, how about renaming this > > to hugetlb_isolate()? I've always felt that huge_page is not a > > straightforward and clear name since we also have another type of > > huge page (THP). I think hugetlb is more specific. > > > > Sorry for late respond. This suggestion looks good to me. But is isolate_hugetlb more suitable? > This could make it more consistent with isolate_lru_page? What do you think? > There is also a function named folio_isolate_lru(). My initial consideration was making it consistent with folio_isolate_lru(). isolate_hugetlb looks good to me as well. Thanks.
On 2022/5/9 12:21, Muchun Song wrote: > On Mon, May 9, 2022 at 11:24 AM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> On 2022/4/29 19:36, Muchun Song wrote: >>> On Mon, Apr 25, 2022 at 09:27:22PM +0800, Miaohe Lin wrote: >>>> We might fail to isolate huge page due to e.g. the page is under migration >>>> which cleared HPageMigratable. So we should return -EBUSY in this case >>>> rather than always return 1 which could confuse the user. Also we make >>>> the prototype of isolate_huge_page consistent with isolate_lru_page to >>>> improve the readability. >>>> >>>> Fixes: e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") >>>> Suggested-by: Huang Ying <ying.huang@intel.com> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> include/linux/hugetlb.h | 6 +++--- >>>> mm/gup.c | 2 +- >>>> mm/hugetlb.c | 11 +++++------ >>>> mm/memory-failure.c | 2 +- >>>> mm/mempolicy.c | 2 +- >>>> mm/migrate.c | 5 +++-- >>>> 6 files changed, 14 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >>>> index 04f0186b089b..306d6ef3fa22 100644 >>>> --- a/include/linux/hugetlb.h >>>> +++ b/include/linux/hugetlb.h >>>> @@ -170,7 +170,7 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to, >>>> vm_flags_t vm_flags); >>>> long hugetlb_unreserve_pages(struct inode *inode, long start, long end, >>>> long freed); >>>> -bool isolate_huge_page(struct page *page, struct list_head *list); >>>> +int isolate_huge_page(struct page *page, struct list_head *list); >>>> int get_hwpoison_huge_page(struct page *page, bool *hugetlb); >>>> int get_huge_page_for_hwpoison(unsigned long pfn, int flags); >>>> void putback_active_hugepage(struct page *page); >>>> @@ -376,9 +376,9 @@ static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr, >>>> return NULL; >>>> } >>>> >>>> -static inline bool isolate_huge_page(struct page *page, struct list_head *list) >>>> +static inline int isolate_huge_page(struct page *page, struct list_head *list) >>> >>> Since you already touched all the call sites, how about renaming this >>> to hugetlb_isolate()? I've always felt that huge_page is not a >>> straightforward and clear name since we also have another type of >>> huge page (THP). I think hugetlb is more specific. >>> >> >> Sorry for late respond. This suggestion looks good to me. But is isolate_hugetlb more suitable? >> This could make it more consistent with isolate_lru_page? What do you think? >> > > There is also a function named folio_isolate_lru(). My initial consideration was > making it consistent with folio_isolate_lru(). isolate_hugetlb looks good to me > as well. I see. Many thanks for your explanation. :) > > Thanks. > . >
On 2022/4/29 18:08, David Hildenbrand wrote: > On 25.04.22 15:27, Miaohe Lin wrote: >> We might fail to isolate huge page due to e.g. the page is under migration >> which cleared HPageMigratable. So we should return -EBUSY in this case >> rather than always return 1 which could confuse the user. Also we make >> the prototype of isolate_huge_page consistent with isolate_lru_page to >> improve the readability. >> >> Fixes: e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") > > If this is a fix, what's the runtime effect of it? > > You state "could confuse", which doesn't indicate an actual BUG to me. The hugetlb page might not be migrated due to error while it's not reported in the __user *status. So the caller might think all of the memory is migrated and thus does not retry to migrate the hugetlb page in the next round. Is this too trival to bother adding a Fixes tag? Thanks! > >
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 04f0186b089b..306d6ef3fa22 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -170,7 +170,7 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to, vm_flags_t vm_flags); long hugetlb_unreserve_pages(struct inode *inode, long start, long end, long freed); -bool isolate_huge_page(struct page *page, struct list_head *list); +int isolate_huge_page(struct page *page, struct list_head *list); int get_hwpoison_huge_page(struct page *page, bool *hugetlb); int get_huge_page_for_hwpoison(unsigned long pfn, int flags); void putback_active_hugepage(struct page *page); @@ -376,9 +376,9 @@ static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr, return NULL; } -static inline bool isolate_huge_page(struct page *page, struct list_head *list) +static inline int isolate_huge_page(struct page *page, struct list_head *list) { - return false; + return -EBUSY; } static inline int get_hwpoison_huge_page(struct page *page, bool *hugetlb) diff --git a/mm/gup.c b/mm/gup.c index 5c17d4816441..c15d41636e8e 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1869,7 +1869,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages, * Try to move out any movable page before pinning the range. */ if (folio_test_hugetlb(folio)) { - if (!isolate_huge_page(&folio->page, + if (isolate_huge_page(&folio->page, &movable_page_list)) isolation_error_count++; continue; diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 74c9964c1b11..098f81e8550d 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2766,8 +2766,7 @@ static int alloc_and_dissolve_huge_page(struct hstate *h, struct page *old_page, * Fail with -EBUSY if not possible. */ spin_unlock_irq(&hugetlb_lock); - if (!isolate_huge_page(old_page, list)) - ret = -EBUSY; + ret = isolate_huge_page(old_page, list); spin_lock_irq(&hugetlb_lock); goto free_new; } else if (!HPageFreed(old_page)) { @@ -2843,7 +2842,7 @@ int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list) if (hstate_is_gigantic(h)) return -ENOMEM; - if (page_count(head) && isolate_huge_page(head, list)) + if (page_count(head) && !isolate_huge_page(head, list)) ret = 0; else if (!page_count(head)) ret = alloc_and_dissolve_huge_page(h, head, list); @@ -6940,15 +6939,15 @@ follow_huge_pgd(struct mm_struct *mm, unsigned long address, pgd_t *pgd, int fla return pte_page(*(pte_t *)pgd) + ((address & ~PGDIR_MASK) >> PAGE_SHIFT); } -bool isolate_huge_page(struct page *page, struct list_head *list) +int isolate_huge_page(struct page *page, struct list_head *list) { - bool ret = true; + int ret = 0; spin_lock_irq(&hugetlb_lock); if (!PageHeadHuge(page) || !HPageMigratable(page) || !get_page_unless_zero(page)) { - ret = false; + ret = -EBUSY; goto unlock; } ClearHPageMigratable(page); diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 1d117190c350..a83d32bbc567 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -2203,7 +2203,7 @@ static bool isolate_page(struct page *page, struct list_head *pagelist) bool lru = PageLRU(page); if (PageHuge(page)) { - isolated = isolate_huge_page(page, pagelist); + isolated = !isolate_huge_page(page, pagelist); } else { if (lru) isolated = !isolate_lru_page(page); diff --git a/mm/mempolicy.c b/mm/mempolicy.c index e4f125e48cc4..a4467c4e9f8d 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -602,7 +602,7 @@ static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask, /* With MPOL_MF_MOVE, we migrate only unshared hugepage. */ if (flags & (MPOL_MF_MOVE_ALL) || (flags & MPOL_MF_MOVE && page_mapcount(page) == 1)) { - if (!isolate_huge_page(page, qp->pagelist) && + if (isolate_huge_page(page, qp->pagelist) && (flags & MPOL_MF_STRICT)) /* * Failed to isolate page but allow migrating pages diff --git a/mm/migrate.c b/mm/migrate.c index 0fc4651b3e39..c937a496239b 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1628,8 +1628,9 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, if (PageHuge(page)) { if (PageHead(page)) { - isolate_huge_page(page, pagelist); - err = 1; + err = isolate_huge_page(page, pagelist); + if (!err) + err = 1; } } else { struct page *head;
We might fail to isolate huge page due to e.g. the page is under migration which cleared HPageMigratable. So we should return -EBUSY in this case rather than always return 1 which could confuse the user. Also we make the prototype of isolate_huge_page consistent with isolate_lru_page to improve the readability. Fixes: e8db67eb0ded ("mm: migrate: move_pages() supports thp migration") Suggested-by: Huang Ying <ying.huang@intel.com> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- include/linux/hugetlb.h | 6 +++--- mm/gup.c | 2 +- mm/hugetlb.c | 11 +++++------ mm/memory-failure.c | 2 +- mm/mempolicy.c | 2 +- mm/migrate.c | 5 +++-- 6 files changed, 14 insertions(+), 14 deletions(-)