Re: [PATCH] mm: shmem: enable thp migration (Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page)

From: Michal Hocko
Date: Wed Apr 11 2018 - 05:26:21 EST


On Fri 06-04-18 03:07:11, Naoya Horiguchi wrote:
> >From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> Date: Fri, 6 Apr 2018 10:58:35 +0900
> Subject: [PATCH] mm: enable thp migration for shmem thp
>
> My testing for the latest kernel supporting thp migration showed an
> infinite loop in offlining the memory block that is filled with shmem
> thps. We can get out of the loop with a signal, but kernel should
> return with failure in this case.
>
> What happens in the loop is that scan_movable_pages() repeats returning
> the same pfn without any progress. That's because page migration always
> fails for shmem thps.
>
> In memory offline code, memory blocks containing unmovable pages should
> be prevented from being offline targets by has_unmovable_pages() inside
> start_isolate_page_range().
>
> So it's possible to change migratability
> for non-anonymous thps to avoid the issue, but it introduces more complex
> and thp-specific handling in migration code, so it might not good.
>
> So this patch is suggesting to fix the issue by enabling thp migration
> for shmem thp. Both of anon/shmem thp are migratable so we don't need
> precheck about the type of thps.
>
> Fixes: commit 72b39cfc4d75 ("mm, memory_hotplug: do not fail offlining too early")
> Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v4.15+

I do not really feel qualified to give my ack but this is the right
approach for the fix. We simply do expect that LRU pages are migrateable
as well as zone_movable pages.

Andrew, do you plan to take it (with Kirill's ack).

Thanks!

> ---
> mm/huge_memory.c | 5 ++++-
> mm/migrate.c | 19 ++++++++++++++++---
> mm/rmap.c | 3 ---
> 3 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 2aff58624886..933c1bbd3464 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2926,7 +2926,10 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
> pmde = maybe_pmd_mkwrite(pmde, vma);
>
> flush_cache_range(vma, mmun_start, mmun_start + HPAGE_PMD_SIZE);
> - page_add_anon_rmap(new, vma, mmun_start, true);
> + if (PageAnon(new))
> + page_add_anon_rmap(new, vma, mmun_start, true);
> + else
> + page_add_file_rmap(new, true);
> set_pmd_at(mm, mmun_start, pvmw->pmd, pmde);
> if (vma->vm_flags & VM_LOCKED)
> mlock_vma_page(new);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index bdef905b1737..f92dd9f50981 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -472,7 +472,7 @@ int migrate_page_move_mapping(struct address_space *mapping,
> pslot = radix_tree_lookup_slot(&mapping->i_pages,
> page_index(page));
>
> - expected_count += 1 + page_has_private(page);
> + expected_count += hpage_nr_pages(page) + page_has_private(page);
> if (page_count(page) != expected_count ||
> radix_tree_deref_slot_protected(pslot,
> &mapping->i_pages.xa_lock) != page) {
> @@ -505,7 +505,7 @@ int migrate_page_move_mapping(struct address_space *mapping,
> */
> newpage->index = page->index;
> newpage->mapping = page->mapping;
> - get_page(newpage); /* add cache reference */
> + page_ref_add(newpage, hpage_nr_pages(page)); /* add cache reference */
> if (PageSwapBacked(page)) {
> __SetPageSwapBacked(newpage);
> if (PageSwapCache(page)) {
> @@ -524,13 +524,26 @@ int migrate_page_move_mapping(struct address_space *mapping,
> }
>
> radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
> + if (PageTransHuge(page)) {
> + int i;
> + int index = page_index(page);
> +
> + for (i = 0; i < HPAGE_PMD_NR; i++) {
> + pslot = radix_tree_lookup_slot(&mapping->i_pages,
> + index + i);
> + radix_tree_replace_slot(&mapping->i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(&mapping->i_pages, pslot, newpage);
> + }
>
> /*
> * Drop cache reference from old page by unfreezing
> * to one less reference.
> * We know this isn't the last reference.
> */
> - page_ref_unfreeze(page, expected_count - 1);
> + page_ref_unfreeze(page, expected_count - hpage_nr_pages(page));
>
> xa_unlock(&mapping->i_pages);
> /* Leave irq disabled to prevent preemption while updating stats */
> diff --git a/mm/rmap.c b/mm/rmap.c
> index f0dd4e4565bc..8d5337fed37b 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1374,9 +1374,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> VM_BUG_ON_PAGE(PageHuge(page) || !PageTransCompound(page), page);
>
> - if (!PageAnon(page))
> - continue;
> -
> set_pmd_migration_entry(&pvmw, page);
> continue;
> }
> --
> 2.7.4

--
Michal Hocko
SUSE Labs