| Message ID | 20210122195231.324857-4-mike.kravetz@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | create hugetlb flags to consolidate state | expand |
On Fri, Jan 22, 2021 at 11:52:29AM -0800, Mike Kravetz wrote: > The HP_Migratable flag indicates a page is a candidate for migration. > Only set the flag if the page's hstate supports migration. This allows > the migration paths to detect non-migratable pages earlier. If migration > is not supported for the hstate, HP_Migratable will not be set, the page > will not be isolated and no attempt will be made to migrate. We should > never get to unmap_and_move_huge_page for a page where migration is not > supported, so throw a warning if we do. > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> I wish there was a better way to do this like opencode it at fault path, but as you mentioned that is troublesome. I am not a big fan of the name either, too long for my taste but I cannot come up with anything better so: Reviewed-by: Oscar Salvador <osalvador@suse.de> > --- > fs/hugetlbfs/inode.c | 2 +- > include/linux/hugetlb.h | 9 +++++++++ > mm/hugetlb.c | 8 ++++---- > mm/migrate.c | 9 ++++----- > 4 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index e1d7ed2a53a9..93f7b8d3c5fd 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > > mutex_unlock(&hugetlb_fault_mutex_table[hash]); > > - SetHPageMigratable(page); > + SetHPageMigratableIfSupported(page); > /* > * unlock_page because locked by add_to_page_cache() > * put_page() due to reference from alloc_huge_page() > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index 58be44a915d1..cd1960541f2a 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h) > return arch_hugetlb_migration_supported(h); > } > > +/* > + * Only set HPageMigratable if migration supported for page > + */ > +static inline void SetHPageMigratableIfSupported(struct page *page) > +{ > + if (hugepage_migration_supported(page_hstate(page))) > + SetHPageMigratable(page); > +} > + > /* > * Movability check is different as compared to migration check. > * It determines whether or not a huge page should be placed on > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index f1a3c8230dbf..4da1a29ac5e2 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -4194,7 +4194,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, > make_huge_pte(vma, new_page, 1)); > page_remove_rmap(old_page, true); > hugepage_add_new_anon_rmap(new_page, vma, haddr); > - SetHPageMigratable(new_page); > + SetHPageMigratableIfSupported(new_page); > /* Make the old page be freed below */ > new_page = old_page; > } > @@ -4436,7 +4436,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, > * been isolated for migration. > */ > if (new_page) > - SetHPageMigratable(page); > + SetHPageMigratableIfSupported(page); > > unlock_page(page); > out: > @@ -4747,7 +4747,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > update_mmu_cache(dst_vma, dst_addr, dst_pte); > > spin_unlock(ptl); > - SetHPageMigratable(page); > + SetHPageMigratableIfSupported(page); > if (vm_shared) > unlock_page(page); > ret = 0; > @@ -5589,7 +5589,7 @@ void putback_active_hugepage(struct page *page) > { > VM_BUG_ON_PAGE(!PageHead(page), page); > spin_lock(&hugetlb_lock); > - SetHPageMigratable(page); > + SetHPageMigratableIfSupported(page); > list_move_tail(&page->lru, &(page_hstate(page))->hugepage_activelist); > spin_unlock(&hugetlb_lock); > put_page(page); > diff --git a/mm/migrate.c b/mm/migrate.c > index a3e1acc72ad7..c8d19e83f372 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1275,13 +1275,12 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, > struct address_space *mapping = NULL; > > /* > - * Migratability of hugepages depends on architectures and their size. > - * This check is necessary because some callers of hugepage migration > - * like soft offline and memory hotremove don't walk through page > - * tables or check whether the hugepage is pmd-based or not before > - * kicking migration. > + * Support for migration should be checked at isolation time. > + * Therefore, we should never get here if migration is not supported > + * for the page. > */ > if (!hugepage_migration_supported(page_hstate(hpage))) { > + VM_WARN_ON(1); > list_move_tail(&hpage->lru, ret); > return -ENOSYS; > } > -- > 2.29.2 > >
On Fri 22-01-21 11:52:29, Mike Kravetz wrote: > The HP_Migratable flag indicates a page is a candidate for migration. > Only set the flag if the page's hstate supports migration. This allows > the migration paths to detect non-migratable pages earlier. If migration > is not supported for the hstate, HP_Migratable will not be set, the page > will not be isolated and no attempt will be made to migrate. We should > never get to unmap_and_move_huge_page for a page where migration is not > supported, so throw a warning if we do. > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > fs/hugetlbfs/inode.c | 2 +- > include/linux/hugetlb.h | 9 +++++++++ > mm/hugetlb.c | 8 ++++---- > mm/migrate.c | 9 ++++----- > 4 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index e1d7ed2a53a9..93f7b8d3c5fd 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > > mutex_unlock(&hugetlb_fault_mutex_table[hash]); > > - SetHPageMigratable(page); > + SetHPageMigratableIfSupported(page); > /* > * unlock_page because locked by add_to_page_cache() > * put_page() due to reference from alloc_huge_page() > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index 58be44a915d1..cd1960541f2a 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h) > return arch_hugetlb_migration_supported(h); > } > > +/* > + * Only set HPageMigratable if migration supported for page > + */ > +static inline void SetHPageMigratableIfSupported(struct page *page) This is really mouthful... > +{ > + if (hugepage_migration_supported(page_hstate(page))) > + SetHPageMigratable(page); and it is really a trivial wrapper. I do understand why you want to prevent from the code duplication and potentially a missing check but this all is just an internal hugetlb code. Even if the flag is set on non-migrateable hugetlb page then this will not be fatal. The migration can fail even on those pages for which migration is supported right? So I am not really sure this is an improvement in the end. But up to you I do not really have a strong opinion here.
On 1/27/21 2:35 AM, Michal Hocko wrote: > On Fri 22-01-21 11:52:29, Mike Kravetz wrote: >> The HP_Migratable flag indicates a page is a candidate for migration. >> Only set the flag if the page's hstate supports migration. This allows >> the migration paths to detect non-migratable pages earlier. If migration >> is not supported for the hstate, HP_Migratable will not be set, the page >> will not be isolated and no attempt will be made to migrate. We should >> never get to unmap_and_move_huge_page for a page where migration is not >> supported, so throw a warning if we do. >> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> >> --- >> fs/hugetlbfs/inode.c | 2 +- >> include/linux/hugetlb.h | 9 +++++++++ >> mm/hugetlb.c | 8 ++++---- >> mm/migrate.c | 9 ++++----- >> 4 files changed, 18 insertions(+), 10 deletions(-) >> >> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c >> index e1d7ed2a53a9..93f7b8d3c5fd 100644 >> --- a/fs/hugetlbfs/inode.c >> +++ b/fs/hugetlbfs/inode.c >> @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, >> >> mutex_unlock(&hugetlb_fault_mutex_table[hash]); >> >> - SetHPageMigratable(page); >> + SetHPageMigratableIfSupported(page); >> /* >> * unlock_page because locked by add_to_page_cache() >> * put_page() due to reference from alloc_huge_page() >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >> index 58be44a915d1..cd1960541f2a 100644 >> --- a/include/linux/hugetlb.h >> +++ b/include/linux/hugetlb.h >> @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h) >> return arch_hugetlb_migration_supported(h); >> } >> >> +/* >> + * Only set HPageMigratable if migration supported for page >> + */ >> +static inline void SetHPageMigratableIfSupported(struct page *page) > > This is really mouthful... > >> +{ >> + if (hugepage_migration_supported(page_hstate(page))) >> + SetHPageMigratable(page); > > and it is really a trivial wrapper. I do understand why you want to > prevent from the code duplication and potentially a missing check but > this all is just an internal hugetlb code. Even if the flag is set on > non-migrateable hugetlb page then this will not be fatal. The migration > can fail even on those pages for which migration is supported right? > > So I am not really sure this is an improvement in the end. But up to you > I do not really have a strong opinion here. Yes, this patch is somewhat optional. It should be a minor improvement in cases where we are dealing with hpages in a non-migratable hstate. Although, I do not believe this is the common case. The real reason for even looking into this was a comment by Oscar. With the name change to HPageMigratable, it implies that the page is migratable. However, this is not the case if the page's hstate does not support migration. So, if we check the hstate when setting the flag we can eliminate those cases where the page is certainly not migratable. I don't really love this patch. It has minimal functional value. Oscar, what do you think about dropping this?
On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote: > Yes, this patch is somewhat optional. It should be a minor improvement > in cases where we are dealing with hpages in a non-migratable hstate. > Although, I do not believe this is the common case. > > The real reason for even looking into this was a comment by Oscar. With > the name change to HPageMigratable, it implies that the page is migratable. > However, this is not the case if the page's hstate does not support migration. > So, if we check the hstate when setting the flag we can eliminate those > cases where the page is certainly not migratable. > > I don't really love this patch. It has minimal functional value. > > Oscar, what do you think about dropping this? Yeah, I remember this topic arose during a discussion of patch#2 in the early versions, about whether the renaming to HPageMigratable made sense. Back then I thought that we could have this in one place at fault-path [1], which should have made this prettier, but it is not the case. True is that the optimization is little, so I am fine with dropping this patch. unmap_and_move_huge_page() fences off pages belonging to non-migratable hstates. [1] https://patchwork.kernel.org/project/linux-mm/patch/20210120013049.311822-3-mike.kravetz@oracle.com/#23914033 Thanks
On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote: > On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote: > > Yes, this patch is somewhat optional. It should be a minor improvement > > in cases where we are dealing with hpages in a non-migratable hstate. > > Although, I do not believe this is the common case. > > > > The real reason for even looking into this was a comment by Oscar. With > > the name change to HPageMigratable, it implies that the page is migratable. > > However, this is not the case if the page's hstate does not support migration. > > So, if we check the hstate when setting the flag we can eliminate those > > cases where the page is certainly not migratable. > > > > I don't really love this patch. It has minimal functional value. > > > > Oscar, what do you think about dropping this? > > Yeah, I remember this topic arose during a discussion of patch#2 in the > early versions, about whether the renaming to HPageMigratable made > sense. > > Back then I thought that we could have this in one place at fault-path [1], > which should have made this prettier, but it is not the case. > True is that the optimization is little, so I am fine with dropping this > patch. I've dropped it. > unmap_and_move_huge_page() fences off pages belonging to non-migratable > hstates. > > [1] https://patchwork.kernel.org/project/linux-mm/patch/20210120013049.311822-3-mike.kravetz@oracle.com/#23914033
On 1/28/21 1:37 PM, Andrew Morton wrote: > On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote: > >> On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote: >>> Yes, this patch is somewhat optional. It should be a minor improvement >>> in cases where we are dealing with hpages in a non-migratable hstate. >>> Although, I do not believe this is the common case. >>> >>> The real reason for even looking into this was a comment by Oscar. With >>> the name change to HPageMigratable, it implies that the page is migratable. >>> However, this is not the case if the page's hstate does not support migration. >>> So, if we check the hstate when setting the flag we can eliminate those >>> cases where the page is certainly not migratable. >>> >>> I don't really love this patch. It has minimal functional value. >>> >>> Oscar, what do you think about dropping this? >> >> Yeah, I remember this topic arose during a discussion of patch#2 in the >> early versions, about whether the renaming to HPageMigratable made >> sense. >> >> Back then I thought that we could have this in one place at fault-path [1], >> which should have made this prettier, but it is not the case. >> True is that the optimization is little, so I am fine with dropping this >> patch. > > I've dropped it. Thanks Andrew. Michal suggested that comments describing synchronization be added for each flag. Since I did 'one patch per flag', that would be an update to each patch. Or, I could simply add a patch to update the comment block based on what you already have. Let me know what is best/easiest for you.
On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: > On 1/28/21 1:37 PM, Andrew Morton wrote: > > On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote: > > > >> On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote: > >>> Yes, this patch is somewhat optional. It should be a minor improvement > >>> in cases where we are dealing with hpages in a non-migratable hstate. > >>> Although, I do not believe this is the common case. > >>> > >>> The real reason for even looking into this was a comment by Oscar. With > >>> the name change to HPageMigratable, it implies that the page is migratable. > >>> However, this is not the case if the page's hstate does not support migration. > >>> So, if we check the hstate when setting the flag we can eliminate those > >>> cases where the page is certainly not migratable. > >>> > >>> I don't really love this patch. It has minimal functional value. > >>> > >>> Oscar, what do you think about dropping this? > >> > >> Yeah, I remember this topic arose during a discussion of patch#2 in the > >> early versions, about whether the renaming to HPageMigratable made > >> sense. > >> > >> Back then I thought that we could have this in one place at fault-path [1], > >> which should have made this prettier, but it is not the case. > >> True is that the optimization is little, so I am fine with dropping this > >> patch. > > > > I've dropped it. > > Thanks Andrew. > > Michal suggested that comments describing synchronization be added for each > flag. Since I did 'one patch per flag', that would be an update to each patch. > Or, I could simply add a patch to update the comment block based on what you > already have. > > Let me know what is best/easiest for you. I guess just one patch is best for reviewers. Then I'll split up into a sprinkle of -fix patches if I'm feeling energetic ;)
On Thu 28-01-21 14:15:31, Andrew Morton wrote: > On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: > > > On 1/28/21 1:37 PM, Andrew Morton wrote: > > > On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote: > > > > > >> On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote: > > >>> Yes, this patch is somewhat optional. It should be a minor improvement > > >>> in cases where we are dealing with hpages in a non-migratable hstate. > > >>> Although, I do not believe this is the common case. > > >>> > > >>> The real reason for even looking into this was a comment by Oscar. With > > >>> the name change to HPageMigratable, it implies that the page is migratable. > > >>> However, this is not the case if the page's hstate does not support migration. > > >>> So, if we check the hstate when setting the flag we can eliminate those > > >>> cases where the page is certainly not migratable. > > >>> > > >>> I don't really love this patch. It has minimal functional value. > > >>> > > >>> Oscar, what do you think about dropping this? > > >> > > >> Yeah, I remember this topic arose during a discussion of patch#2 in the > > >> early versions, about whether the renaming to HPageMigratable made > > >> sense. > > >> > > >> Back then I thought that we could have this in one place at fault-path [1], > > >> which should have made this prettier, but it is not the case. > > >> True is that the optimization is little, so I am fine with dropping this > > >> patch. > > > > > > I've dropped it. > > > > Thanks Andrew. > > > > Michal suggested that comments describing synchronization be added for each > > flag. Since I did 'one patch per flag', that would be an update to each patch. > > Or, I could simply add a patch to update the comment block based on what you > > already have. > > > > Let me know what is best/easiest for you. > > I guess just one patch is best for reviewers. Then I'll split up into > a sprinkle of -fix patches if I'm feeling energetic ;) A single patch on top should be just fine. It is not like this would be a bisectability breaker.
On 1/28/21 2:15 PM, Andrew Morton wrote: > On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: >> >> Michal suggested that comments describing synchronization be added for each >> flag. Since I did 'one patch per flag', that would be an update to each patch. >> Or, I could simply add a patch to update the comment block based on what you >> already have. >> >> Let me know what is best/easiest for you. > > I guess just one patch is best for reviewers. Then I'll split up into > a sprinkle of -fix patches if I'm feeling energetic ;) Here is a patch to update the comments for all those flags. It should apply on top of what is in your tree. From: Mike Kravetz <mike.kravetz@oracle.com> Date: Fri, 29 Jan 2021 10:36:12 -0800 Subject: [PATCH] huegtlb: add synchronization information for new hugetlb specific flags Adding comments, no functional change. Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- include/linux/hugetlb.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index e5e6ffd55392..cf70795d2209 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -480,14 +480,24 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr, * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at * allocation time. Cleared when page is fully instantiated. Free * routine checks flag to restore a reservation on error paths. + * Synchronization: Examined or modified by code that knows it has + * the only reference to page. i.e. After allocation but before use + * or when the page is being freed. * HPG_migratable - Set after a newly allocated page is added to the page * cache and/or page tables. Indicates the page is a candidate for * migration. + * Synchronization: Initially set after new page allocation with no + * locking. When examined and modified during migration processing + * (isolate, migrate, putback) the hugetlb_lock is held. * HPG_temporary - - Set on a page that is temporarily allocated from the buddy * allocator. Typically used for migration target pages when no pages * are available in the pool. The hugetlb free page path will * immediately free pages with this flag set to the buddy allocator. + * Synchronization: Can be set after huge page allocation from buddy when + * code knows it has only reference. All other examinations and + * modifications require hugetlb_lock. * HPG_freed - Set when page is on the free lists. + * Synchronization: hugetlb_lock held for examination and modification. */ enum hugetlb_page_flags { HPG_restore_reserve = 0,
On Fri 29-01-21 10:46:15, Mike Kravetz wrote: > On 1/28/21 2:15 PM, Andrew Morton wrote: > > On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: > >> > >> Michal suggested that comments describing synchronization be added for each > >> flag. Since I did 'one patch per flag', that would be an update to each patch. > >> Or, I could simply add a patch to update the comment block based on what you > >> already have. > >> > >> Let me know what is best/easiest for you. > > > > I guess just one patch is best for reviewers. Then I'll split up into > > a sprinkle of -fix patches if I'm feeling energetic ;) > > Here is a patch to update the comments for all those flags. It should > apply on top of what is in your tree. > > From: Mike Kravetz <mike.kravetz@oracle.com> > Date: Fri, 29 Jan 2021 10:36:12 -0800 > Subject: [PATCH] huegtlb: add synchronization information for new hugetlb > specific flags > > Adding comments, no functional change. > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> Acked-by: Michal Hocko <mhocko@suse.com> Thanks Mike! > --- > include/linux/hugetlb.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index e5e6ffd55392..cf70795d2209 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -480,14 +480,24 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr, > * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at > * allocation time. Cleared when page is fully instantiated. Free > * routine checks flag to restore a reservation on error paths. > + * Synchronization: Examined or modified by code that knows it has > + * the only reference to page. i.e. After allocation but before use > + * or when the page is being freed. > * HPG_migratable - Set after a newly allocated page is added to the page > * cache and/or page tables. Indicates the page is a candidate for > * migration. > + * Synchronization: Initially set after new page allocation with no > + * locking. When examined and modified during migration processing > + * (isolate, migrate, putback) the hugetlb_lock is held. > * HPG_temporary - - Set on a page that is temporarily allocated from the buddy > * allocator. Typically used for migration target pages when no pages > * are available in the pool. The hugetlb free page path will > * immediately free pages with this flag set to the buddy allocator. > + * Synchronization: Can be set after huge page allocation from buddy when > + * code knows it has only reference. All other examinations and > + * modifications require hugetlb_lock. > * HPG_freed - Set when page is on the free lists. > + * Synchronization: hugetlb_lock held for examination and modification. > */ > enum hugetlb_page_flags { > HPG_restore_reserve = 0, > -- > 2.29.2 >
On 2/1/21 3:49 AM, Michal Hocko wrote: > On Fri 29-01-21 10:46:15, Mike Kravetz wrote: >> On 1/28/21 2:15 PM, Andrew Morton wrote: >>> On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote: >>>> >>>> Michal suggested that comments describing synchronization be added for each >>>> flag. Since I did 'one patch per flag', that would be an update to each patch. >>>> Or, I could simply add a patch to update the comment block based on what you >>>> already have. >>>> >>>> Let me know what is best/easiest for you. >>> >>> I guess just one patch is best for reviewers. Then I'll split up into >>> a sprinkle of -fix patches if I'm feeling energetic ;) >> >> Here is a patch to update the comments for all those flags. It should >> apply on top of what is in your tree. >> >> From: Mike Kravetz <mike.kravetz@oracle.com> >> Date: Fri, 29 Jan 2021 10:36:12 -0800 >> Subject: [PATCH] huegtlb: add synchronization information for new hugetlb >> specific flags >> >> Adding comments, no functional change. >> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > > Acked-by: Michal Hocko <mhocko@suse.com> > > Thanks Mike! Hi Andrew, You may have missed this due to the depth of email thread. Can you pick this up, or would you like me to resend? Thanks,
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index e1d7ed2a53a9..93f7b8d3c5fd 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, mutex_unlock(&hugetlb_fault_mutex_table[hash]); - SetHPageMigratable(page); + SetHPageMigratableIfSupported(page); /* * unlock_page because locked by add_to_page_cache() * put_page() due to reference from alloc_huge_page() diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 58be44a915d1..cd1960541f2a 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h) return arch_hugetlb_migration_supported(h); } +/* + * Only set HPageMigratable if migration supported for page + */ +static inline void SetHPageMigratableIfSupported(struct page *page) +{ + if (hugepage_migration_supported(page_hstate(page))) + SetHPageMigratable(page); +} + /* * Movability check is different as compared to migration check. * It determines whether or not a huge page should be placed on diff --git a/mm/hugetlb.c b/mm/hugetlb.c index f1a3c8230dbf..4da1a29ac5e2 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4194,7 +4194,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma, make_huge_pte(vma, new_page, 1)); page_remove_rmap(old_page, true); hugepage_add_new_anon_rmap(new_page, vma, haddr); - SetHPageMigratable(new_page); + SetHPageMigratableIfSupported(new_page); /* Make the old page be freed below */ new_page = old_page; } @@ -4436,7 +4436,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, * been isolated for migration. */ if (new_page) - SetHPageMigratable(page); + SetHPageMigratableIfSupported(page); unlock_page(page); out: @@ -4747,7 +4747,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, update_mmu_cache(dst_vma, dst_addr, dst_pte); spin_unlock(ptl); - SetHPageMigratable(page); + SetHPageMigratableIfSupported(page); if (vm_shared) unlock_page(page); ret = 0; @@ -5589,7 +5589,7 @@ void putback_active_hugepage(struct page *page) { VM_BUG_ON_PAGE(!PageHead(page), page); spin_lock(&hugetlb_lock); - SetHPageMigratable(page); + SetHPageMigratableIfSupported(page); list_move_tail(&page->lru, &(page_hstate(page))->hugepage_activelist); spin_unlock(&hugetlb_lock); put_page(page); diff --git a/mm/migrate.c b/mm/migrate.c index a3e1acc72ad7..c8d19e83f372 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1275,13 +1275,12 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, struct address_space *mapping = NULL; /* - * Migratability of hugepages depends on architectures and their size. - * This check is necessary because some callers of hugepage migration - * like soft offline and memory hotremove don't walk through page - * tables or check whether the hugepage is pmd-based or not before - * kicking migration. + * Support for migration should be checked at isolation time. + * Therefore, we should never get here if migration is not supported + * for the page. */ if (!hugepage_migration_supported(page_hstate(hpage))) { + VM_WARN_ON(1); list_move_tail(&hpage->lru, ret); return -ENOSYS; }
The HP_Migratable flag indicates a page is a candidate for migration. Only set the flag if the page's hstate supports migration. This allows the migration paths to detect non-migratable pages earlier. If migration is not supported for the hstate, HP_Migratable will not be set, the page will not be isolated and no attempt will be made to migrate. We should never get to unmap_and_move_huge_page for a page where migration is not supported, so throw a warning if we do. Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- fs/hugetlbfs/inode.c | 2 +- include/linux/hugetlb.h | 9 +++++++++ mm/hugetlb.c | 8 ++++---- mm/migrate.c | 9 ++++----- 4 files changed, 18 insertions(+), 10 deletions(-)