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

[Naoya Horiguchi]

On Sun, Apr 22, 2018 at 08:03:49PM -0700, Matthew Wilcox wrote:
> On Fri, Apr 06, 2018 at 03:07:11AM +, Naoya Horiguchi wrote:
> > Subject: [PATCH] mm: enable thp migration for shmem thp
> 
> This patch is buggy, but not in a significant way:
> 
> > @@ -524,13 +524,26 @@ int migrate_page_move_mapping(struct address_space 
> > *mapping,
> > }
> >  
> > radix_tree_replace_slot(>i_pages, pslot, newpage);
> 
> ^^^ this line should have been deleted
> 
> > +   if (PageTransHuge(page)) {
> > +   int i;
> > +   int index = page_index(page);
> > +
> > +   for (i = 0; i < HPAGE_PMD_NR; i++) {
> ^^^ or this iteration should start at 1
> > +   pslot = radix_tree_lookup_slot(>i_pages,
> > +  index + i);
> > +   radix_tree_replace_slot(>i_pages, pslot,
> > +   newpage + i);
> > +   }
> > +   } else {
> > +   radix_tree_replace_slot(>i_pages, pslot, newpage);
> ^^^ and if the second option, then we don't need this line
> > +   }
> 
> So either this:
> 
> - radix_tree_replace_slot(>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(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>i_pages, pslot, newpage);
> + }
> 
> Or this:
> 
>   radix_tree_replace_slot(>i_pages, pslot, newpage);
> + if (PageTransHuge(page)) {
> + int i;
> + int index = page_index(page);
> +
> + for (i = 1; i < HPAGE_PMD_NR; i++) {
> + pslot = radix_tree_lookup_slot(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + }
> 
> The second one is shorter and involves fewer lookups ...

Hi Matthew,

Thank you for poinitng out, I like the second one.
The original patch is now in upsteam, so I wrote a patch on it.

Thanks,
Naoya


From: Naoya Horiguchi 
Date: Sun, 22 Apr 2018 20:03:49 -0700
Subject: [PATCH] mm: migrate: fix double call of radix_tree_replace_slot()

radix_tree_replace_slot() is called twice for head page, it's
obviously a bug. Let's fix it.

Fixes: e71769ae5260 ("mm: enable thp migration for shmem thp")
Reported-by: Matthew Wilcox 
Signed-off-by: Naoya Horiguchi 
---
 mm/migrate.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 568433023831..8c0af0f7cab1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -528,14 +528,12 @@ int migrate_page_move_mapping(struct address_space 
*mapping,
int i;
int index = page_index(page);
 
-   for (i = 0; i < HPAGE_PMD_NR; i++) {
+   for (i = 1; i < HPAGE_PMD_NR; i++) {
pslot = radix_tree_lookup_slot(>i_pages,
   index + i);
radix_tree_replace_slot(>i_pages, pslot,
newpage + i);
}
-   } else {
-   radix_tree_replace_slot(>i_pages, pslot, newpage);
}
 
/*
-- 
2.7.4

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

[Naoya Horiguchi]

On Sun, Apr 22, 2018 at 08:03:49PM -0700, Matthew Wilcox wrote:
> On Fri, Apr 06, 2018 at 03:07:11AM +, Naoya Horiguchi wrote:
> > Subject: [PATCH] mm: enable thp migration for shmem thp
> 
> This patch is buggy, but not in a significant way:
> 
> > @@ -524,13 +524,26 @@ int migrate_page_move_mapping(struct address_space 
> > *mapping,
> > }
> >  
> > radix_tree_replace_slot(>i_pages, pslot, newpage);
> 
> ^^^ this line should have been deleted
> 
> > +   if (PageTransHuge(page)) {
> > +   int i;
> > +   int index = page_index(page);
> > +
> > +   for (i = 0; i < HPAGE_PMD_NR; i++) {
> ^^^ or this iteration should start at 1
> > +   pslot = radix_tree_lookup_slot(>i_pages,
> > +  index + i);
> > +   radix_tree_replace_slot(>i_pages, pslot,
> > +   newpage + i);
> > +   }
> > +   } else {
> > +   radix_tree_replace_slot(>i_pages, pslot, newpage);
> ^^^ and if the second option, then we don't need this line
> > +   }
> 
> So either this:
> 
> - radix_tree_replace_slot(>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(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>i_pages, pslot, newpage);
> + }
> 
> Or this:
> 
>   radix_tree_replace_slot(>i_pages, pslot, newpage);
> + if (PageTransHuge(page)) {
> + int i;
> + int index = page_index(page);
> +
> + for (i = 1; i < HPAGE_PMD_NR; i++) {
> + pslot = radix_tree_lookup_slot(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + }
> 
> The second one is shorter and involves fewer lookups ...

Hi Matthew,

Thank you for poinitng out, I like the second one.
The original patch is now in upsteam, so I wrote a patch on it.

Thanks,
Naoya


From: Naoya Horiguchi 
Date: Sun, 22 Apr 2018 20:03:49 -0700
Subject: [PATCH] mm: migrate: fix double call of radix_tree_replace_slot()

radix_tree_replace_slot() is called twice for head page, it's
obviously a bug. Let's fix it.

Fixes: e71769ae5260 ("mm: enable thp migration for shmem thp")
Reported-by: Matthew Wilcox 
Signed-off-by: Naoya Horiguchi 
---
 mm/migrate.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 568433023831..8c0af0f7cab1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -528,14 +528,12 @@ int migrate_page_move_mapping(struct address_space 
*mapping,
int i;
int index = page_index(page);
 
-   for (i = 0; i < HPAGE_PMD_NR; i++) {
+   for (i = 1; i < HPAGE_PMD_NR; i++) {
pslot = radix_tree_lookup_slot(>i_pages,
   index + i);
radix_tree_replace_slot(>i_pages, pslot,
newpage + i);
}
-   } else {
-   radix_tree_replace_slot(>i_pages, pslot, newpage);
}
 
/*
-- 
2.7.4

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

[Matthew Wilcox]

On Fri, Apr 06, 2018 at 03:07:11AM +, Naoya Horiguchi wrote:
> Subject: [PATCH] mm: enable thp migration for shmem thp

This patch is buggy, but not in a significant way:

> @@ -524,13 +524,26 @@ int migrate_page_move_mapping(struct address_space 
> *mapping,
>   }
>  
>   radix_tree_replace_slot(>i_pages, pslot, newpage);

^^^ this line should have been deleted

> + if (PageTransHuge(page)) {
> + int i;
> + int index = page_index(page);
> +
> + for (i = 0; i < HPAGE_PMD_NR; i++) {
^^^ or this iteration should start at 1
> + pslot = radix_tree_lookup_slot(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>i_pages, pslot, newpage);
^^^ and if the second option, then we don't need this line
> + }

So either this:

-   radix_tree_replace_slot(>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(>i_pages,
+  index + i);
+   radix_tree_replace_slot(>i_pages, pslot,
+   newpage + i);
+   }
+   } else {
+   radix_tree_replace_slot(>i_pages, pslot, newpage);
+   }

Or this:

radix_tree_replace_slot(>i_pages, pslot, newpage);
+   if (PageTransHuge(page)) {
+   int i;
+   int index = page_index(page);
+
+   for (i = 1; i < HPAGE_PMD_NR; i++) {
+   pslot = radix_tree_lookup_slot(>i_pages,
+  index + i);
+   radix_tree_replace_slot(>i_pages, pslot,
+   newpage + i);
+   }
+   }

The second one is shorter and involves fewer lookups ...

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

[Matthew Wilcox]

On Fri, Apr 06, 2018 at 03:07:11AM +, Naoya Horiguchi wrote:
> Subject: [PATCH] mm: enable thp migration for shmem thp

This patch is buggy, but not in a significant way:

> @@ -524,13 +524,26 @@ int migrate_page_move_mapping(struct address_space 
> *mapping,
>   }
>  
>   radix_tree_replace_slot(>i_pages, pslot, newpage);

^^^ this line should have been deleted

> + if (PageTransHuge(page)) {
> + int i;
> + int index = page_index(page);
> +
> + for (i = 0; i < HPAGE_PMD_NR; i++) {
^^^ or this iteration should start at 1
> + pslot = radix_tree_lookup_slot(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>i_pages, pslot, newpage);
^^^ and if the second option, then we don't need this line
> + }

So either this:

-   radix_tree_replace_slot(>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(>i_pages,
+  index + i);
+   radix_tree_replace_slot(>i_pages, pslot,
+   newpage + i);
+   }
+   } else {
+   radix_tree_replace_slot(>i_pages, pslot, newpage);
+   }

Or this:

radix_tree_replace_slot(>i_pages, pslot, newpage);
+   if (PageTransHuge(page)) {
+   int i;
+   int index = page_index(page);
+
+   for (i = 1; i < HPAGE_PMD_NR; i++) {
+   pslot = radix_tree_lookup_slot(>i_pages,
+  index + i);
+   radix_tree_replace_slot(>i_pages, pslot,
+   newpage + i);
+   }
+   }

The second one is shorter and involves fewer lookups ...

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

[Michal Hocko]

On Wed 11-04-18 12:27:39, Andrew Morton wrote:
> On Wed, 11 Apr 2018 11:26:11 +0200 Michal Hocko  wrote:
> 
> > On Fri 06-04-18 03:07:11, Naoya Horiguchi wrote:
> > > >From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > > From: Naoya Horiguchi 
> > > 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 
> > > Cc: sta...@vger.kernel.org # 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).
> > 
> 
> Sure.  What happened with "Michal's fix in another email"
> (https://lkml.kernel.org/r/20180406051452.gb23...@hori1.linux.bs1.fc.nec.co.jp)?

I guess you meant http://lkml.kernel.org/r/20180405190405.gs6...@dhcp22.suse.cz

Well, that would be a workaround in case we didn't have a proper fix. It
is much simpler but it wouldn't make backporting to older kernels any
easier because it depends on other non-trivial changes you already have
in your tree. So having a full THP pagecache migration support is
preferred of course.

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Wed 11-04-18 12:27:39, Andrew Morton wrote:
> On Wed, 11 Apr 2018 11:26:11 +0200 Michal Hocko  wrote:
> 
> > On Fri 06-04-18 03:07:11, Naoya Horiguchi wrote:
> > > >From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > > From: Naoya Horiguchi 
> > > 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 
> > > Cc: sta...@vger.kernel.org # 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).
> > 
> 
> Sure.  What happened with "Michal's fix in another email"
> (https://lkml.kernel.org/r/20180406051452.gb23...@hori1.linux.bs1.fc.nec.co.jp)?

I guess you meant http://lkml.kernel.org/r/20180405190405.gs6...@dhcp22.suse.cz

Well, that would be a workaround in case we didn't have a proper fix. It
is much simpler but it wouldn't make backporting to older kernels any
easier because it depends on other non-trivial changes you already have
in your tree. So having a full THP pagecache migration support is
preferred of course.

-- 
Michal Hocko
SUSE Labs

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

[Andrew Morton]

On Wed, 11 Apr 2018 11:26:11 +0200 Michal Hocko  wrote:

> On Fri 06-04-18 03:07:11, Naoya Horiguchi wrote:
> > >From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > From: Naoya Horiguchi 
> > 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 
> > Cc: sta...@vger.kernel.org # 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).
> 

Sure.  What happened with "Michal's fix in another email"
(https://lkml.kernel.org/r/20180406051452.gb23...@hori1.linux.bs1.fc.nec.co.jp)?

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

[Andrew Morton]

On Wed, 11 Apr 2018 11:26:11 +0200 Michal Hocko  wrote:

> On Fri 06-04-18 03:07:11, Naoya Horiguchi wrote:
> > >From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > From: Naoya Horiguchi 
> > 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 
> > Cc: sta...@vger.kernel.org # 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).
> 

Sure.  What happened with "Michal's fix in another email"
(https://lkml.kernel.org/r/20180406051452.gb23...@hori1.linux.bs1.fc.nec.co.jp)?

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

[Michal Hocko]

On Fri 06-04-18 03:07:11, Naoya Horiguchi wrote:
> >From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi 
> 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 
> Cc: sta...@vger.kernel.org # 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(>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,
>   >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(>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(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>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(>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 & 

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

[Michal Hocko]

On Fri 06-04-18 03:07:11, Naoya Horiguchi wrote:
> >From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi 
> 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 
> Cc: sta...@vger.kernel.org # 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(>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,
>   >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(>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(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>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(>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)) {
>   

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

[Kirill A. Shutemov]

On Fri, Apr 06, 2018 at 03:07:11AM +, Naoya Horiguchi wrote:
> Hi everyone,
> 
> On Thu, Apr 05, 2018 at 06:03:17PM +0200, Michal Hocko wrote:
> > On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> > > On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> > > > On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > > > > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> > > > [...]
> > > > > > RIght, I confused the two. What is the proper layer to fix that 
> > > > > > then?
> > > > > > rmap_walk_file?
> > > > > 
> > > > > Maybe something like this? Totally untested.
> > > > 
> > > > This looks way too complex. Why cannot we simply split THP page cache
> > > > during migration?
> > > 
> > > This way we unify the codepath for archictures that don't support THP
> > > migration and shmem THP.
> > 
> > But why? There shouldn't be really nothing to prevent THP (anon or
> > shemem) to be migratable. If we cannot migrate it at once we can always
> > split it. So why should we add another thp specific handling all over
> > the place?
> 
> If thp migration works fine for shmem, we can keep anon/shmem thp to
> be migratable and we don't need any ad-hoc workaround.
> So I wrote a patch to enable it.
> This patch does not change any shmem specific code, so I think that
> it works for file thp (not only shmem,) but I don't test it yet.
> 
> Thanks,
> Naoya Horiguchi
> -
> From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi 
> 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 
> Cc: sta...@vger.kernel.org # v4.15+

This looks sane to me.

Acked-by: Kirill A. Shutemov 

As, yeah, as you mentioned down the thread it's not a stable material

-- 
 Kirill A. Shutemov

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

[Kirill A. Shutemov]

On Fri, Apr 06, 2018 at 03:07:11AM +, Naoya Horiguchi wrote:
> Hi everyone,
> 
> On Thu, Apr 05, 2018 at 06:03:17PM +0200, Michal Hocko wrote:
> > On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> > > On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> > > > On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > > > > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> > > > [...]
> > > > > > RIght, I confused the two. What is the proper layer to fix that 
> > > > > > then?
> > > > > > rmap_walk_file?
> > > > > 
> > > > > Maybe something like this? Totally untested.
> > > > 
> > > > This looks way too complex. Why cannot we simply split THP page cache
> > > > during migration?
> > > 
> > > This way we unify the codepath for archictures that don't support THP
> > > migration and shmem THP.
> > 
> > But why? There shouldn't be really nothing to prevent THP (anon or
> > shemem) to be migratable. If we cannot migrate it at once we can always
> > split it. So why should we add another thp specific handling all over
> > the place?
> 
> If thp migration works fine for shmem, we can keep anon/shmem thp to
> be migratable and we don't need any ad-hoc workaround.
> So I wrote a patch to enable it.
> This patch does not change any shmem specific code, so I think that
> it works for file thp (not only shmem,) but I don't test it yet.
> 
> Thanks,
> Naoya Horiguchi
> -
> From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi 
> 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 
> Cc: sta...@vger.kernel.org # v4.15+

This looks sane to me.

Acked-by: Kirill A. Shutemov 

As, yeah, as you mentioned down the thread it's not a stable material

-- 
 Kirill A. Shutemov

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

[Naoya Horiguchi]

On Fri, Apr 06, 2018 at 09:08:15AM +0200, Michal Hocko wrote:
> On Fri 06-04-18 05:14:53, Naoya Horiguchi wrote:
> > On Fri, Apr 06, 2018 at 03:07:11AM +, Horiguchi Naoya(堀口 直也) wrote:
> > ...
> > > -
> > > From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > > From: Naoya Horiguchi 
> > > 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 
> > > Cc: sta...@vger.kernel.org # v4.15+
> > 
> > ... oh, I don't think this is suitable for stable.
> > Michal's fix in another email can come first with "CC: stable",
> > then this one.
> > Anyway I want to get some feedback on the change of this patch.
> 
> My patch is indeed much simpler but it depends on [1] and that doesn't
> sound like a stable material as well because it depends on onether 2
> patches. Maybe we need some other hack for 4.15 if we really care enough.
> 
> [1] http://lkml.kernel.org/r/20180103082555.14592-4-mho...@kernel.org

OK, so I like just giving up sending to stable.

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

[Naoya Horiguchi]

On Fri, Apr 06, 2018 at 09:08:15AM +0200, Michal Hocko wrote:
> On Fri 06-04-18 05:14:53, Naoya Horiguchi wrote:
> > On Fri, Apr 06, 2018 at 03:07:11AM +, Horiguchi Naoya(堀口 直也) wrote:
> > ...
> > > -
> > > From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > > From: Naoya Horiguchi 
> > > 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 
> > > Cc: sta...@vger.kernel.org # v4.15+
> > 
> > ... oh, I don't think this is suitable for stable.
> > Michal's fix in another email can come first with "CC: stable",
> > then this one.
> > Anyway I want to get some feedback on the change of this patch.
> 
> My patch is indeed much simpler but it depends on [1] and that doesn't
> sound like a stable material as well because it depends on onether 2
> patches. Maybe we need some other hack for 4.15 if we really care enough.
> 
> [1] http://lkml.kernel.org/r/20180103082555.14592-4-mho...@kernel.org

OK, so I like just giving up sending to stable.

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

[Michal Hocko]

On Fri 06-04-18 05:14:53, Naoya Horiguchi wrote:
> On Fri, Apr 06, 2018 at 03:07:11AM +, Horiguchi Naoya(堀口 直也) wrote:
> ...
> > -
> > From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > From: Naoya Horiguchi 
> > 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 
> > Cc: sta...@vger.kernel.org # v4.15+
> 
> ... oh, I don't think this is suitable for stable.
> Michal's fix in another email can come first with "CC: stable",
> then this one.
> Anyway I want to get some feedback on the change of this patch.

My patch is indeed much simpler but it depends on [1] and that doesn't
sound like a stable material as well because it depends on onether 2
patches. Maybe we need some other hack for 4.15 if we really care enough.

[1] http://lkml.kernel.org/r/20180103082555.14592-4-mho...@kernel.org
-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Fri 06-04-18 05:14:53, Naoya Horiguchi wrote:
> On Fri, Apr 06, 2018 at 03:07:11AM +, Horiguchi Naoya(堀口 直也) wrote:
> ...
> > -
> > From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> > From: Naoya Horiguchi 
> > 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 
> > Cc: sta...@vger.kernel.org # v4.15+
> 
> ... oh, I don't think this is suitable for stable.
> Michal's fix in another email can come first with "CC: stable",
> then this one.
> Anyway I want to get some feedback on the change of this patch.

My patch is indeed much simpler but it depends on [1] and that doesn't
sound like a stable material as well because it depends on onether 2
patches. Maybe we need some other hack for 4.15 if we really care enough.

[1] http://lkml.kernel.org/r/20180103082555.14592-4-mho...@kernel.org
-- 
Michal Hocko
SUSE Labs

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

[Naoya Horiguchi]

On Fri, Apr 06, 2018 at 03:07:11AM +, Horiguchi Naoya(堀口 直也) wrote:
...
> -
> From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi 
> 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 
> Cc: sta...@vger.kernel.org # v4.15+

... oh, I don't think this is suitable for stable.
Michal's fix in another email can come first with "CC: stable",
then this one.
Anyway I want to get some feedback on the change of this patch.

Thanks,
Naoya Horiguchi

> ---
>  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(>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,
>   >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(>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(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>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(>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 & 

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

[Naoya Horiguchi]

On Fri, Apr 06, 2018 at 03:07:11AM +, Horiguchi Naoya(堀口 直也) wrote:
...
> -
> From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
> From: Naoya Horiguchi 
> 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 
> Cc: sta...@vger.kernel.org # v4.15+

... oh, I don't think this is suitable for stable.
Michal's fix in another email can come first with "CC: stable",
then this one.
Anyway I want to get some feedback on the change of this patch.

Thanks,
Naoya Horiguchi

> ---
>  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(>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,
>   >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(>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(>i_pages,
> +index + i);
> + radix_tree_replace_slot(>i_pages, pslot,
> + newpage + i);
> + }
> + } else {
> + radix_tree_replace_slot(>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(>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)) {
>   

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

[Naoya Horiguchi]

Hi everyone,

On Thu, Apr 05, 2018 at 06:03:17PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> > On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> > > On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > > > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> > > [...]
> > > > > RIght, I confused the two. What is the proper layer to fix that then?
> > > > > rmap_walk_file?
> > > > 
> > > > Maybe something like this? Totally untested.
> > > 
> > > This looks way too complex. Why cannot we simply split THP page cache
> > > during migration?
> > 
> > This way we unify the codepath for archictures that don't support THP
> > migration and shmem THP.
> 
> But why? There shouldn't be really nothing to prevent THP (anon or
> shemem) to be migratable. If we cannot migrate it at once we can always
> split it. So why should we add another thp specific handling all over
> the place?

If thp migration works fine for shmem, we can keep anon/shmem thp to
be migratable and we don't need any ad-hoc workaround.
So I wrote a patch to enable it.
This patch does not change any shmem specific code, so I think that
it works for file thp (not only shmem,) but I don't test it yet.

Thanks,
Naoya Horiguchi
-
>From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi 
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 
Cc: sta...@vger.kernel.org # v4.15+
---
 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(>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,
>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(>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(>i_pages,
+  index + i);
+   

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

[Naoya Horiguchi]

Hi everyone,

On Thu, Apr 05, 2018 at 06:03:17PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> > On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> > > On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > > > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> > > [...]
> > > > > RIght, I confused the two. What is the proper layer to fix that then?
> > > > > rmap_walk_file?
> > > > 
> > > > Maybe something like this? Totally untested.
> > > 
> > > This looks way too complex. Why cannot we simply split THP page cache
> > > during migration?
> > 
> > This way we unify the codepath for archictures that don't support THP
> > migration and shmem THP.
> 
> But why? There shouldn't be really nothing to prevent THP (anon or
> shemem) to be migratable. If we cannot migrate it at once we can always
> split it. So why should we add another thp specific handling all over
> the place?

If thp migration works fine for shmem, we can keep anon/shmem thp to
be migratable and we don't need any ad-hoc workaround.
So I wrote a patch to enable it.
This patch does not change any shmem specific code, so I think that
it works for file thp (not only shmem,) but I don't test it yet.

Thanks,
Naoya Horiguchi
-
>From e31ec037701d1cc76b26226e4b66d8c783d40889 Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi 
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 
Cc: sta...@vger.kernel.org # v4.15+
---
 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(>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,
>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(>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(>i_pages,
+  index + i);
+   radix_tree_replace_slot(>i_pages, pslot,
+  

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Zi Yan]

On 5 Apr 2018, at 15:04, Michal Hocko wrote:

> On Thu 05-04-18 13:58:43, Zi Yan wrote:
>> On 5 Apr 2018, at 12:03, Michal Hocko wrote:
>>
>>> On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
 On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
>> On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> [...]
>>> RIght, I confused the two. What is the proper layer to fix that then?
>>> rmap_walk_file?
>>
>> Maybe something like this? Totally untested.
>
> This looks way too complex. Why cannot we simply split THP page cache
> during migration?

 This way we unify the codepath for archictures that don't support THP
 migration and shmem THP.
>>>
>>> But why? There shouldn't be really nothing to prevent THP (anon or
>>> shemem) to be migratable. If we cannot migrate it at once we can always
>>> split it. So why should we add another thp specific handling all over
>>> the place?
>>
>> Then, it would be much easier if your "unclutter thp migration" patches is 
>> merged,
>> plus the patch below:
>
> Good point. Except I would prefer a less convoluted condition
>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 60531108021a..b4087aa890f5 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1138,7 +1138,9 @@ static ICE_noinline int unmap_and_move(new_page_t 
>> get_new_page,
>> int rc = MIGRATEPAGE_SUCCESS;
>> struct page *newpage;
>>
>> -   if (!thp_migration_supported() && PageTransHuge(page))
>> +   if ((!thp_migration_supported() ||
>> +(thp_migration_supported() && !PageAnon(page))) &&
>> +   PageTransHuge(page))
>> return -ENOMEM;
>
> What about this?
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5d0dc7b85f90..cd02e2bdf37c 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1138,7 +1138,11 @@ static ICE_noinline int unmap_and_move(new_page_t 
> get_new_page,
>   int rc = MIGRATEPAGE_SUCCESS;
>   struct page *newpage;
>
> - if (!thp_migration_supported() && PageTransHuge(page))
> + /*
> +  * THP pagecache or generally non-migrateable THP need to be split
> +  * up before migration
> +  */
> + if (PageTransHuge(page) && (!thp_migration_supported() || 
> !PageAnon(page)))
>   return -ENOMEM;
>
>   newpage = get_new_page(page, private);

I think it works and is better than mine.

Reviewed-by: Zi Yan 

--
Best Regards
Yan Zi


signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Zi Yan]

On 5 Apr 2018, at 15:04, Michal Hocko wrote:

> On Thu 05-04-18 13:58:43, Zi Yan wrote:
>> On 5 Apr 2018, at 12:03, Michal Hocko wrote:
>>
>>> On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
 On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
>> On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> [...]
>>> RIght, I confused the two. What is the proper layer to fix that then?
>>> rmap_walk_file?
>>
>> Maybe something like this? Totally untested.
>
> This looks way too complex. Why cannot we simply split THP page cache
> during migration?

 This way we unify the codepath for archictures that don't support THP
 migration and shmem THP.
>>>
>>> But why? There shouldn't be really nothing to prevent THP (anon or
>>> shemem) to be migratable. If we cannot migrate it at once we can always
>>> split it. So why should we add another thp specific handling all over
>>> the place?
>>
>> Then, it would be much easier if your "unclutter thp migration" patches is 
>> merged,
>> plus the patch below:
>
> Good point. Except I would prefer a less convoluted condition
>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 60531108021a..b4087aa890f5 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1138,7 +1138,9 @@ static ICE_noinline int unmap_and_move(new_page_t 
>> get_new_page,
>> int rc = MIGRATEPAGE_SUCCESS;
>> struct page *newpage;
>>
>> -   if (!thp_migration_supported() && PageTransHuge(page))
>> +   if ((!thp_migration_supported() ||
>> +(thp_migration_supported() && !PageAnon(page))) &&
>> +   PageTransHuge(page))
>> return -ENOMEM;
>
> What about this?
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5d0dc7b85f90..cd02e2bdf37c 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1138,7 +1138,11 @@ static ICE_noinline int unmap_and_move(new_page_t 
> get_new_page,
>   int rc = MIGRATEPAGE_SUCCESS;
>   struct page *newpage;
>
> - if (!thp_migration_supported() && PageTransHuge(page))
> + /*
> +  * THP pagecache or generally non-migrateable THP need to be split
> +  * up before migration
> +  */
> + if (PageTransHuge(page) && (!thp_migration_supported() || 
> !PageAnon(page)))
>   return -ENOMEM;
>
>   newpage = get_new_page(page, private);

I think it works and is better than mine.

Reviewed-by: Zi Yan 

--
Best Regards
Yan Zi


signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 13:58:43, Zi Yan wrote:
> On 5 Apr 2018, at 12:03, Michal Hocko wrote:
> 
> > On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> >> On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> >>> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
>  On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> >>> [...]
> > RIght, I confused the two. What is the proper layer to fix that then?
> > rmap_walk_file?
> 
>  Maybe something like this? Totally untested.
> >>>
> >>> This looks way too complex. Why cannot we simply split THP page cache
> >>> during migration?
> >>
> >> This way we unify the codepath for archictures that don't support THP
> >> migration and shmem THP.
> >
> > But why? There shouldn't be really nothing to prevent THP (anon or
> > shemem) to be migratable. If we cannot migrate it at once we can always
> > split it. So why should we add another thp specific handling all over
> > the place?
> 
> Then, it would be much easier if your "unclutter thp migration" patches is 
> merged,
> plus the patch below:

Good point. Except I would prefer a less convoluted condition

> diff --git a/mm/migrate.c b/mm/migrate.c
> index 60531108021a..b4087aa890f5 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1138,7 +1138,9 @@ static ICE_noinline int unmap_and_move(new_page_t 
> get_new_page,
> int rc = MIGRATEPAGE_SUCCESS;
> struct page *newpage;
> 
> -   if (!thp_migration_supported() && PageTransHuge(page))
> +   if ((!thp_migration_supported() ||
> +(thp_migration_supported() && !PageAnon(page))) &&
> +   PageTransHuge(page))
> return -ENOMEM;

What about this?
diff --git a/mm/migrate.c b/mm/migrate.c
index 5d0dc7b85f90..cd02e2bdf37c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1138,7 +1138,11 @@ static ICE_noinline int unmap_and_move(new_page_t 
get_new_page,
int rc = MIGRATEPAGE_SUCCESS;
struct page *newpage;
 
-   if (!thp_migration_supported() && PageTransHuge(page))
+   /*
+* THP pagecache or generally non-migrateable THP need to be split
+* up before migration
+*/
+   if (PageTransHuge(page) && (!thp_migration_supported() || 
!PageAnon(page)))
return -ENOMEM;
 
newpage = get_new_page(page, private);


-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 13:58:43, Zi Yan wrote:
> On 5 Apr 2018, at 12:03, Michal Hocko wrote:
> 
> > On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> >> On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> >>> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
>  On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> >>> [...]
> > RIght, I confused the two. What is the proper layer to fix that then?
> > rmap_walk_file?
> 
>  Maybe something like this? Totally untested.
> >>>
> >>> This looks way too complex. Why cannot we simply split THP page cache
> >>> during migration?
> >>
> >> This way we unify the codepath for archictures that don't support THP
> >> migration and shmem THP.
> >
> > But why? There shouldn't be really nothing to prevent THP (anon or
> > shemem) to be migratable. If we cannot migrate it at once we can always
> > split it. So why should we add another thp specific handling all over
> > the place?
> 
> Then, it would be much easier if your "unclutter thp migration" patches is 
> merged,
> plus the patch below:

Good point. Except I would prefer a less convoluted condition

> diff --git a/mm/migrate.c b/mm/migrate.c
> index 60531108021a..b4087aa890f5 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1138,7 +1138,9 @@ static ICE_noinline int unmap_and_move(new_page_t 
> get_new_page,
> int rc = MIGRATEPAGE_SUCCESS;
> struct page *newpage;
> 
> -   if (!thp_migration_supported() && PageTransHuge(page))
> +   if ((!thp_migration_supported() ||
> +(thp_migration_supported() && !PageAnon(page))) &&
> +   PageTransHuge(page))
> return -ENOMEM;

What about this?
diff --git a/mm/migrate.c b/mm/migrate.c
index 5d0dc7b85f90..cd02e2bdf37c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1138,7 +1138,11 @@ static ICE_noinline int unmap_and_move(new_page_t 
get_new_page,
int rc = MIGRATEPAGE_SUCCESS;
struct page *newpage;
 
-   if (!thp_migration_supported() && PageTransHuge(page))
+   /*
+* THP pagecache or generally non-migrateable THP need to be split
+* up before migration
+*/
+   if (PageTransHuge(page) && (!thp_migration_supported() || 
!PageAnon(page)))
return -ENOMEM;
 
newpage = get_new_page(page, private);


-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Zi Yan]

On 5 Apr 2018, at 12:03, Michal Hocko wrote:

> On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
>> On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
>>> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
 On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
>>> [...]
> RIght, I confused the two. What is the proper layer to fix that then?
> rmap_walk_file?

 Maybe something like this? Totally untested.
>>>
>>> This looks way too complex. Why cannot we simply split THP page cache
>>> during migration?
>>
>> This way we unify the codepath for archictures that don't support THP
>> migration and shmem THP.
>
> But why? There shouldn't be really nothing to prevent THP (anon or
> shemem) to be migratable. If we cannot migrate it at once we can always
> split it. So why should we add another thp specific handling all over
> the place?

Then, it would be much easier if your "unclutter thp migration" patches is 
merged,
plus the patch below:

diff --git a/mm/migrate.c b/mm/migrate.c
index 60531108021a..b4087aa890f5 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1138,7 +1138,9 @@ static ICE_noinline int unmap_and_move(new_page_t 
get_new_page,
int rc = MIGRATEPAGE_SUCCESS;
struct page *newpage;

-   if (!thp_migration_supported() && PageTransHuge(page))
+   if ((!thp_migration_supported() ||
+(thp_migration_supported() && !PageAnon(page))) &&
+   PageTransHuge(page))
return -ENOMEM;

newpage = get_new_page(page, private)

--
Best Regards
Yan Zi


signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Zi Yan]

On 5 Apr 2018, at 12:03, Michal Hocko wrote:

> On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
>> On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
>>> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
 On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
>>> [...]
> RIght, I confused the two. What is the proper layer to fix that then?
> rmap_walk_file?

 Maybe something like this? Totally untested.
>>>
>>> This looks way too complex. Why cannot we simply split THP page cache
>>> during migration?
>>
>> This way we unify the codepath for archictures that don't support THP
>> migration and shmem THP.
>
> But why? There shouldn't be really nothing to prevent THP (anon or
> shemem) to be migratable. If we cannot migrate it at once we can always
> split it. So why should we add another thp specific handling all over
> the place?

Then, it would be much easier if your "unclutter thp migration" patches is 
merged,
plus the patch below:

diff --git a/mm/migrate.c b/mm/migrate.c
index 60531108021a..b4087aa890f5 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1138,7 +1138,9 @@ static ICE_noinline int unmap_and_move(new_page_t 
get_new_page,
int rc = MIGRATEPAGE_SUCCESS;
struct page *newpage;

-   if (!thp_migration_supported() && PageTransHuge(page))
+   if ((!thp_migration_supported() ||
+(thp_migration_supported() && !PageAnon(page))) &&
+   PageTransHuge(page))
return -ENOMEM;

newpage = get_new_page(page, private)

--
Best Regards
Yan Zi


signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> > On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> > [...]
> > > > RIght, I confused the two. What is the proper layer to fix that then?
> > > > rmap_walk_file?
> > > 
> > > Maybe something like this? Totally untested.
> > 
> > This looks way too complex. Why cannot we simply split THP page cache
> > during migration?
> 
> This way we unify the codepath for archictures that don't support THP
> migration and shmem THP.

But why? There shouldn't be really nothing to prevent THP (anon or
shemem) to be migratable. If we cannot migrate it at once we can always
split it. So why should we add another thp specific handling all over
the place?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 18:55:51, Kirill A. Shutemov wrote:
> On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> > On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> > [...]
> > > > RIght, I confused the two. What is the proper layer to fix that then?
> > > > rmap_walk_file?
> > > 
> > > Maybe something like this? Totally untested.
> > 
> > This looks way too complex. Why cannot we simply split THP page cache
> > during migration?
> 
> This way we unify the codepath for archictures that don't support THP
> migration and shmem THP.

But why? There shouldn't be really nothing to prevent THP (anon or
shemem) to be migratable. If we cannot migrate it at once we can always
split it. So why should we add another thp specific handling all over
the place?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> [...]
> > > RIght, I confused the two. What is the proper layer to fix that then?
> > > rmap_walk_file?
> > 
> > Maybe something like this? Totally untested.
> 
> This looks way too complex. Why cannot we simply split THP page cache
> during migration?

This way we unify the codepath for archictures that don't support THP
migration and shmem THP.

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Thu, Apr 05, 2018 at 05:05:47PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> > On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> [...]
> > > RIght, I confused the two. What is the proper layer to fix that then?
> > > rmap_walk_file?
> > 
> > Maybe something like this? Totally untested.
> 
> This looks way too complex. Why cannot we simply split THP page cache
> during migration?

This way we unify the codepath for archictures that don't support THP
migration and shmem THP.

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
[...]
> > RIght, I confused the two. What is the proper layer to fix that then?
> > rmap_walk_file?
> 
> Maybe something like this? Totally untested.

This looks way too complex. Why cannot we simply split THP page cache
during migration?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 16:40:45, Kirill A. Shutemov wrote:
> On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
[...]
> > RIght, I confused the two. What is the proper layer to fix that then?
> > rmap_walk_file?
> 
> Maybe something like this? Totally untested.

This looks way too complex. Why cannot we simply split THP page cache
during migration?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 15:28:38, Kirill A. Shutemov wrote:
> > On Thu, Apr 05, 2018 at 10:59:27AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > > > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > > > My testing for the latest kernel supporting thp migration found 
> > > > > > > > out 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.
> > > > > > >
> > > > > > > Why does it fail? Shmem pages should be movable without any 
> > > > > > > issues.
> > > > > > 
> > > > > > .. because try_to_unmap_one() explicitly skips unmapping for 
> > > > > > migration.
> > > > > > 
> > > > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > > > >   /* PMD-mapped THP migration entry */
> > > > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > > > !PageTransCompound(page), page);
> > > > > >   
> > > > > >   if (!PageAnon(page))
> > > > > >   continue;
> > > > > >   
> > > > > >   set_pmd_migration_entry(, page);
> > > > > >   continue;
> > > > > >   }
> > > > > >   #endif
> > > > > > 
> > > > > > When I implemented this code, I felt hard to work on both of anon 
> > > > > > thp
> > > > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > > > steps.
> > > > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > > > testing)
> > > > > > to extend thp migration for shmem. But I think it's a reasonable 
> > > > > > next step.
> > > > > 
> > > > > OK, I see. I have forgot about this part. Please be explicit about 
> > > > > that
> > > > > in the changelog. Also the proper fix is to not use movable zone for
> > > > > shmem page THP rather than hack around it in the hotplug specific code
> > > > > IMHO.
> > > > 
> > > > No. We should just split the page before running
> > > > try_to_unmap(TTU_MIGRATION) on the page.
> > > 
> > > Something like this or it is completely broken. I completely forgot the
> > > whole page_vma_mapped_walk business.
> > 
> > No, this wouldn't work. We need to split page, not pmd to make migration
> > work.
> 
> RIght, I confused the two. What is the proper layer to fix that then?
> rmap_walk_file?

Maybe something like this? Totally untested.

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index a8a126259bc4..9da8fbd1eb6b 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -359,4 +359,22 @@ static inline bool thp_migration_supported(void)
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
+static inline bool PageTransHugeMigratable(struct page *page)
+{
+   return thp_migration_supported() &&
+   PageTransHuge(page) && PageAnon(page);
+}
+
+static inline bool PageTransMigratable(struct page *page)
+{
+   return thp_migration_supported() &&
+   PageTransCompound(page) && PageAnon(page);
+}
+
+static inline bool PageTransNonMigratable(struct page *page)
+{
+   return thp_migration_supported() &&
+   PageTransCompound(page) && !PageAnon(page);
+}
+
 #endif /* _LINUX_HUGE_MM_H */
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index a2246cf670ba..dd66dfe6d198 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -43,7 +43,7 @@ static inline struct page *new_page_nodemask(struct page 
*page,
return 
alloc_huge_page_nodemask(page_hstate(compound_head(page)),
preferred_nid, nodemask);
 
-   if (thp_migration_supported() && PageTransHuge(page)) {
+   if (PageTransHugeMigratable(page)) {
order = HPAGE_PMD_ORDER;
gfp_mask |= GFP_TRANSHUGE;
}
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b2bd52ff7605..0672938abf44 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1381,9 +1381,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long 
end_pfn)
if (isolate_huge_page(page, ))
move_pages -= 1 << compound_order(head);
continue;
-   } else if 

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Thu, Apr 05, 2018 at 02:48:30PM +0200, Michal Hocko wrote:
> On Thu 05-04-18 15:28:38, Kirill A. Shutemov wrote:
> > On Thu, Apr 05, 2018 at 10:59:27AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > > > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > > > My testing for the latest kernel supporting thp migration found 
> > > > > > > > out 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.
> > > > > > >
> > > > > > > Why does it fail? Shmem pages should be movable without any 
> > > > > > > issues.
> > > > > > 
> > > > > > .. because try_to_unmap_one() explicitly skips unmapping for 
> > > > > > migration.
> > > > > > 
> > > > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > > > >   /* PMD-mapped THP migration entry */
> > > > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > > > !PageTransCompound(page), page);
> > > > > >   
> > > > > >   if (!PageAnon(page))
> > > > > >   continue;
> > > > > >   
> > > > > >   set_pmd_migration_entry(, page);
> > > > > >   continue;
> > > > > >   }
> > > > > >   #endif
> > > > > > 
> > > > > > When I implemented this code, I felt hard to work on both of anon 
> > > > > > thp
> > > > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > > > steps.
> > > > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > > > testing)
> > > > > > to extend thp migration for shmem. But I think it's a reasonable 
> > > > > > next step.
> > > > > 
> > > > > OK, I see. I have forgot about this part. Please be explicit about 
> > > > > that
> > > > > in the changelog. Also the proper fix is to not use movable zone for
> > > > > shmem page THP rather than hack around it in the hotplug specific code
> > > > > IMHO.
> > > > 
> > > > No. We should just split the page before running
> > > > try_to_unmap(TTU_MIGRATION) on the page.
> > > 
> > > Something like this or it is completely broken. I completely forgot the
> > > whole page_vma_mapped_walk business.
> > 
> > No, this wouldn't work. We need to split page, not pmd to make migration
> > work.
> 
> RIght, I confused the two. What is the proper layer to fix that then?
> rmap_walk_file?

Maybe something like this? Totally untested.

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index a8a126259bc4..9da8fbd1eb6b 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -359,4 +359,22 @@ static inline bool thp_migration_supported(void)
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
+static inline bool PageTransHugeMigratable(struct page *page)
+{
+   return thp_migration_supported() &&
+   PageTransHuge(page) && PageAnon(page);
+}
+
+static inline bool PageTransMigratable(struct page *page)
+{
+   return thp_migration_supported() &&
+   PageTransCompound(page) && PageAnon(page);
+}
+
+static inline bool PageTransNonMigratable(struct page *page)
+{
+   return thp_migration_supported() &&
+   PageTransCompound(page) && !PageAnon(page);
+}
+
 #endif /* _LINUX_HUGE_MM_H */
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index a2246cf670ba..dd66dfe6d198 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -43,7 +43,7 @@ static inline struct page *new_page_nodemask(struct page 
*page,
return 
alloc_huge_page_nodemask(page_hstate(compound_head(page)),
preferred_nid, nodemask);
 
-   if (thp_migration_supported() && PageTransHuge(page)) {
+   if (PageTransHugeMigratable(page)) {
order = HPAGE_PMD_ORDER;
gfp_mask |= GFP_TRANSHUGE;
}
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b2bd52ff7605..0672938abf44 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1381,9 +1381,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long 
end_pfn)
if (isolate_huge_page(page, ))
move_pages -= 1 << compound_order(head);
continue;
-   } else if 

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 15:28:38, Kirill A. Shutemov wrote:
> On Thu, Apr 05, 2018 at 10:59:27AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > > My testing for the latest kernel supporting thp migration found 
> > > > > > > out 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.
> > > > > >
> > > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > > 
> > > > > .. because try_to_unmap_one() explicitly skips unmapping for 
> > > > > migration.
> > > > > 
> > > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > > >   /* PMD-mapped THP migration entry */
> > > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > > !PageTransCompound(page), page);
> > > > >   
> > > > >   if (!PageAnon(page))
> > > > >   continue;
> > > > >   
> > > > >   set_pmd_migration_entry(, page);
> > > > >   continue;
> > > > >   }
> > > > >   #endif
> > > > > 
> > > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > > steps.
> > > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > > testing)
> > > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > > step.
> > > > 
> > > > OK, I see. I have forgot about this part. Please be explicit about that
> > > > in the changelog. Also the proper fix is to not use movable zone for
> > > > shmem page THP rather than hack around it in the hotplug specific code
> > > > IMHO.
> > > 
> > > No. We should just split the page before running
> > > try_to_unmap(TTU_MIGRATION) on the page.
> > 
> > Something like this or it is completely broken. I completely forgot the
> > whole page_vma_mapped_walk business.
> 
> No, this wouldn't work. We need to split page, not pmd to make migration
> work.

RIght, I confused the two. What is the proper layer to fix that then?
rmap_walk_file?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Thu 05-04-18 15:28:38, Kirill A. Shutemov wrote:
> On Thu, Apr 05, 2018 at 10:59:27AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > > My testing for the latest kernel supporting thp migration found 
> > > > > > > out 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.
> > > > > >
> > > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > > 
> > > > > .. because try_to_unmap_one() explicitly skips unmapping for 
> > > > > migration.
> > > > > 
> > > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > > >   /* PMD-mapped THP migration entry */
> > > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > > !PageTransCompound(page), page);
> > > > >   
> > > > >   if (!PageAnon(page))
> > > > >   continue;
> > > > >   
> > > > >   set_pmd_migration_entry(, page);
> > > > >   continue;
> > > > >   }
> > > > >   #endif
> > > > > 
> > > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > > steps.
> > > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > > testing)
> > > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > > step.
> > > > 
> > > > OK, I see. I have forgot about this part. Please be explicit about that
> > > > in the changelog. Also the proper fix is to not use movable zone for
> > > > shmem page THP rather than hack around it in the hotplug specific code
> > > > IMHO.
> > > 
> > > No. We should just split the page before running
> > > try_to_unmap(TTU_MIGRATION) on the page.
> > 
> > Something like this or it is completely broken. I completely forgot the
> > whole page_vma_mapped_walk business.
> 
> No, this wouldn't work. We need to split page, not pmd to make migration
> work.

RIght, I confused the two. What is the proper layer to fix that then?
rmap_walk_file?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Thu, Apr 05, 2018 at 10:59:27AM +0200, Michal Hocko wrote:
> On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > My testing for the latest kernel supporting thp migration found out 
> > > > > > 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.
> > > > >
> > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > 
> > > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > > 
> > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > >   /* PMD-mapped THP migration entry */
> > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > !PageTransCompound(page), page);
> > > >   
> > > >   if (!PageAnon(page))
> > > >   continue;
> > > >   
> > > >   set_pmd_migration_entry(, page);
> > > >   continue;
> > > >   }
> > > >   #endif
> > > > 
> > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > steps.
> > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > testing)
> > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > step.
> > > 
> > > OK, I see. I have forgot about this part. Please be explicit about that
> > > in the changelog. Also the proper fix is to not use movable zone for
> > > shmem page THP rather than hack around it in the hotplug specific code
> > > IMHO.
> > 
> > No. We should just split the page before running
> > try_to_unmap(TTU_MIGRATION) on the page.
> 
> Something like this or it is completely broken. I completely forgot the
> whole page_vma_mapped_walk business.

No, this wouldn't work. We need to split page, not pmd to make migration
work.

> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9eaa6354fe70..cbbfbcb08b83 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1356,6 +1356,7 @@ static bool try_to_unmap_one(struct page *page, struct 
> vm_area_struct *vma,
>   return true;
>  
>   if (flags & TTU_SPLIT_HUGE_PMD) {
> +split:
>   split_huge_pmd_address(vma, address,
>   flags & TTU_SPLIT_FREEZE, page);
>   }
> @@ -1375,7 +1376,7 @@ static bool try_to_unmap_one(struct page *page, struct 
> vm_area_struct *vma,
>   VM_BUG_ON_PAGE(PageHuge(page) || 
> !PageTransCompound(page), page);
>  
>   if (!PageAnon(page))
> - continue;
> + goto split;
>  
>   set_pmd_migration_entry(, page);
>   continue;
> -- 
> Michal Hocko
> SUSE Labs

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Thu, Apr 05, 2018 at 10:59:27AM +0200, Michal Hocko wrote:
> On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > My testing for the latest kernel supporting thp migration found out 
> > > > > > 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.
> > > > >
> > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > 
> > > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > > 
> > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > >   /* PMD-mapped THP migration entry */
> > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > !PageTransCompound(page), page);
> > > >   
> > > >   if (!PageAnon(page))
> > > >   continue;
> > > >   
> > > >   set_pmd_migration_entry(, page);
> > > >   continue;
> > > >   }
> > > >   #endif
> > > > 
> > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > steps.
> > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > testing)
> > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > step.
> > > 
> > > OK, I see. I have forgot about this part. Please be explicit about that
> > > in the changelog. Also the proper fix is to not use movable zone for
> > > shmem page THP rather than hack around it in the hotplug specific code
> > > IMHO.
> > 
> > No. We should just split the page before running
> > try_to_unmap(TTU_MIGRATION) on the page.
> 
> Something like this or it is completely broken. I completely forgot the
> whole page_vma_mapped_walk business.

No, this wouldn't work. We need to split page, not pmd to make migration
work.

> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9eaa6354fe70..cbbfbcb08b83 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1356,6 +1356,7 @@ static bool try_to_unmap_one(struct page *page, struct 
> vm_area_struct *vma,
>   return true;
>  
>   if (flags & TTU_SPLIT_HUGE_PMD) {
> +split:
>   split_huge_pmd_address(vma, address,
>   flags & TTU_SPLIT_FREEZE, page);
>   }
> @@ -1375,7 +1376,7 @@ static bool try_to_unmap_one(struct page *page, struct 
> vm_area_struct *vma,
>   VM_BUG_ON_PAGE(PageHuge(page) || 
> !PageTransCompound(page), page);
>  
>   if (!PageAnon(page))
> - continue;
> + goto split;
>  
>   set_pmd_migration_entry(, page);
>   continue;
> -- 
> Michal Hocko
> SUSE Labs

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > My testing for the latest kernel supporting thp migration found out 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.
> > > >
> > > > Why does it fail? Shmem pages should be movable without any issues.
> > > 
> > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > 
> > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > >   /* PMD-mapped THP migration entry */
> > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > !PageTransCompound(page), page);
> > >   
> > >   if (!PageAnon(page))
> > >   continue;
> > >   
> > >   set_pmd_migration_entry(, page);
> > >   continue;
> > >   }
> > >   #endif
> > > 
> > > When I implemented this code, I felt hard to work on both of anon thp
> > > and shmem thp at one time, so I separated the proposal into smaller steps.
> > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > testing)
> > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > step.
> > 
> > OK, I see. I have forgot about this part. Please be explicit about that
> > in the changelog. Also the proper fix is to not use movable zone for
> > shmem page THP rather than hack around it in the hotplug specific code
> > IMHO.
> 
> No. We should just split the page before running
> try_to_unmap(TTU_MIGRATION) on the page.

Something like this or it is completely broken. I completely forgot the
whole page_vma_mapped_walk business.

diff --git a/mm/rmap.c b/mm/rmap.c
index 9eaa6354fe70..cbbfbcb08b83 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1356,6 +1356,7 @@ static bool try_to_unmap_one(struct page *page, struct 
vm_area_struct *vma,
return true;
 
if (flags & TTU_SPLIT_HUGE_PMD) {
+split:
split_huge_pmd_address(vma, address,
flags & TTU_SPLIT_FREEZE, page);
}
@@ -1375,7 +1376,7 @@ static bool try_to_unmap_one(struct page *page, struct 
vm_area_struct *vma,
VM_BUG_ON_PAGE(PageHuge(page) || 
!PageTransCompound(page), page);
 
if (!PageAnon(page))
-   continue;
+   goto split;
 
set_pmd_migration_entry(, page);
continue;
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > My testing for the latest kernel supporting thp migration found out 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.
> > > >
> > > > Why does it fail? Shmem pages should be movable without any issues.
> > > 
> > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > 
> > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > >   /* PMD-mapped THP migration entry */
> > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > !PageTransCompound(page), page);
> > >   
> > >   if (!PageAnon(page))
> > >   continue;
> > >   
> > >   set_pmd_migration_entry(, page);
> > >   continue;
> > >   }
> > >   #endif
> > > 
> > > When I implemented this code, I felt hard to work on both of anon thp
> > > and shmem thp at one time, so I separated the proposal into smaller steps.
> > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > testing)
> > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > step.
> > 
> > OK, I see. I have forgot about this part. Please be explicit about that
> > in the changelog. Also the proper fix is to not use movable zone for
> > shmem page THP rather than hack around it in the hotplug specific code
> > IMHO.
> 
> No. We should just split the page before running
> try_to_unmap(TTU_MIGRATION) on the page.

Something like this or it is completely broken. I completely forgot the
whole page_vma_mapped_walk business.

diff --git a/mm/rmap.c b/mm/rmap.c
index 9eaa6354fe70..cbbfbcb08b83 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1356,6 +1356,7 @@ static bool try_to_unmap_one(struct page *page, struct 
vm_area_struct *vma,
return true;
 
if (flags & TTU_SPLIT_HUGE_PMD) {
+split:
split_huge_pmd_address(vma, address,
flags & TTU_SPLIT_FREEZE, page);
}
@@ -1375,7 +1376,7 @@ static bool try_to_unmap_one(struct page *page, struct 
vm_area_struct *vma,
VM_BUG_ON_PAGE(PageHuge(page) || 
!PageTransCompound(page), page);
 
if (!PageAnon(page))
-   continue;
+   goto split;
 
set_pmd_migration_entry(, page);
continue;
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 14:16:18, Kirill A. Shutemov wrote:
> On Tue, Apr 03, 2018 at 12:58:15PM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > > My testing for the latest kernel supporting thp migration found 
> > > > > > > out 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.
> > > > > >
> > > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > > 
> > > > > .. because try_to_unmap_one() explicitly skips unmapping for 
> > > > > migration.
> > > > > 
> > > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > > >   /* PMD-mapped THP migration entry */
> > > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > > !PageTransCompound(page), page);
> > > > >   
> > > > >   if (!PageAnon(page))
> > > > >   continue;
> > > > >   
> > > > >   set_pmd_migration_entry(, page);
> > > > >   continue;
> > > > >   }
> > > > >   #endif
> > > > > 
> > > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > > steps.
> > > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > > testing)
> > > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > > step.
> > > > 
> > > > OK, I see. I have forgot about this part. Please be explicit about that
> > > > in the changelog. Also the proper fix is to not use movable zone for
> > > > shmem page THP rather than hack around it in the hotplug specific code
> > > > IMHO.
> > > 
> > > No. We should just split the page before running
> > > try_to_unmap(TTU_MIGRATION) on the page.
> > 
> > If splitting is a preffered way then I do not have any objection. We
> > just cannot keep unmovable objects in the zone movable.
> 
> We had anon-thp in movable zone for ages, long before THP migration was
> implemented.

Yeah, and it was a bug and kind of less serious before we made zone
movable kinda more serious. CMA wants to use it for its allocations and
the memory hotplug really depends on migrateability these days.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 14:16:18, Kirill A. Shutemov wrote:
> On Tue, Apr 03, 2018 at 12:58:15PM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > > My testing for the latest kernel supporting thp migration found 
> > > > > > > out 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.
> > > > > >
> > > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > > 
> > > > > .. because try_to_unmap_one() explicitly skips unmapping for 
> > > > > migration.
> > > > > 
> > > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > > >   /* PMD-mapped THP migration entry */
> > > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > > !PageTransCompound(page), page);
> > > > >   
> > > > >   if (!PageAnon(page))
> > > > >   continue;
> > > > >   
> > > > >   set_pmd_migration_entry(, page);
> > > > >   continue;
> > > > >   }
> > > > >   #endif
> > > > > 
> > > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > > steps.
> > > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > > testing)
> > > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > > step.
> > > > 
> > > > OK, I see. I have forgot about this part. Please be explicit about that
> > > > in the changelog. Also the proper fix is to not use movable zone for
> > > > shmem page THP rather than hack around it in the hotplug specific code
> > > > IMHO.
> > > 
> > > No. We should just split the page before running
> > > try_to_unmap(TTU_MIGRATION) on the page.
> > 
> > If splitting is a preffered way then I do not have any objection. We
> > just cannot keep unmovable objects in the zone movable.
> 
> We had anon-thp in movable zone for ages, long before THP migration was
> implemented.

Yeah, and it was a bug and kind of less serious before we made zone
movable kinda more serious. CMA wants to use it for its allocations and
the memory hotplug really depends on migrateability these days.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Tue, Apr 03, 2018 at 12:58:15PM +0200, Michal Hocko wrote:
> On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > My testing for the latest kernel supporting thp migration found out 
> > > > > > 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.
> > > > >
> > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > 
> > > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > > 
> > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > >   /* PMD-mapped THP migration entry */
> > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > !PageTransCompound(page), page);
> > > >   
> > > >   if (!PageAnon(page))
> > > >   continue;
> > > >   
> > > >   set_pmd_migration_entry(, page);
> > > >   continue;
> > > >   }
> > > >   #endif
> > > > 
> > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > steps.
> > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > testing)
> > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > step.
> > > 
> > > OK, I see. I have forgot about this part. Please be explicit about that
> > > in the changelog. Also the proper fix is to not use movable zone for
> > > shmem page THP rather than hack around it in the hotplug specific code
> > > IMHO.
> > 
> > No. We should just split the page before running
> > try_to_unmap(TTU_MIGRATION) on the page.
> 
> If splitting is a preffered way then I do not have any objection. We
> just cannot keep unmovable objects in the zone movable.

We had anon-thp in movable zone for ages, long before THP migration was
implemented.

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Tue, Apr 03, 2018 at 12:58:15PM +0200, Michal Hocko wrote:
> On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> > On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > > My testing for the latest kernel supporting thp migration found out 
> > > > > > 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.
> > > > >
> > > > > Why does it fail? Shmem pages should be movable without any issues.
> > > > 
> > > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > > 
> > > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > > >   /* PMD-mapped THP migration entry */
> > > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > > !PageTransCompound(page), page);
> > > >   
> > > >   if (!PageAnon(page))
> > > >   continue;
> > > >   
> > > >   set_pmd_migration_entry(, page);
> > > >   continue;
> > > >   }
> > > >   #endif
> > > > 
> > > > When I implemented this code, I felt hard to work on both of anon thp
> > > > and shmem thp at one time, so I separated the proposal into smaller 
> > > > steps.
> > > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > > testing)
> > > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > > step.
> > > 
> > > OK, I see. I have forgot about this part. Please be explicit about that
> > > in the changelog. Also the proper fix is to not use movable zone for
> > > shmem page THP rather than hack around it in the hotplug specific code
> > > IMHO.
> > 
> > No. We should just split the page before running
> > try_to_unmap(TTU_MIGRATION) on the page.
> 
> If splitting is a preffered way then I do not have any objection. We
> just cannot keep unmovable objects in the zone movable.

We had anon-thp in movable zone for ages, long before THP migration was
implemented.

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > My testing for the latest kernel supporting thp migration found out 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.
> > > >
> > > > Why does it fail? Shmem pages should be movable without any issues.
> > > 
> > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > 
> > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > >   /* PMD-mapped THP migration entry */
> > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > !PageTransCompound(page), page);
> > >   
> > >   if (!PageAnon(page))
> > >   continue;
> > >   
> > >   set_pmd_migration_entry(, page);
> > >   continue;
> > >   }
> > >   #endif
> > > 
> > > When I implemented this code, I felt hard to work on both of anon thp
> > > and shmem thp at one time, so I separated the proposal into smaller steps.
> > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > testing)
> > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > step.
> > 
> > OK, I see. I have forgot about this part. Please be explicit about that
> > in the changelog. Also the proper fix is to not use movable zone for
> > shmem page THP rather than hack around it in the hotplug specific code
> > IMHO.
> 
> No. We should just split the page before running
> try_to_unmap(TTU_MIGRATION) on the page.

If splitting is a preffered way then I do not have any objection. We
just cannot keep unmovable objects in the zone movable.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 13:54:11, Kirill A. Shutemov wrote:
> On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > > My testing for the latest kernel supporting thp migration found out 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.
> > > >
> > > > Why does it fail? Shmem pages should be movable without any issues.
> > > 
> > > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > > 
> > >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> > >   /* PMD-mapped THP migration entry */
> > >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> > >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > > !PageTransCompound(page), page);
> > >   
> > >   if (!PageAnon(page))
> > >   continue;
> > >   
> > >   set_pmd_migration_entry(, page);
> > >   continue;
> > >   }
> > >   #endif
> > > 
> > > When I implemented this code, I felt hard to work on both of anon thp
> > > and shmem thp at one time, so I separated the proposal into smaller steps.
> > > Shmem uses pagecache so we need some non-trivial effort (including 
> > > testing)
> > > to extend thp migration for shmem. But I think it's a reasonable next 
> > > step.
> > 
> > OK, I see. I have forgot about this part. Please be explicit about that
> > in the changelog. Also the proper fix is to not use movable zone for
> > shmem page THP rather than hack around it in the hotplug specific code
> > IMHO.
> 
> No. We should just split the page before running
> try_to_unmap(TTU_MIGRATION) on the page.

If splitting is a preffered way then I do not have any objection. We
just cannot keep unmovable objects in the zone movable.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > My testing for the latest kernel supporting thp migration found out 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.
> > >
> > > Why does it fail? Shmem pages should be movable without any issues.
> > 
> > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > 
> >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> >   /* PMD-mapped THP migration entry */
> >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > !PageTransCompound(page), page);
> >   
> >   if (!PageAnon(page))
> >   continue;
> >   
> >   set_pmd_migration_entry(, page);
> >   continue;
> >   }
> >   #endif
> > 
> > When I implemented this code, I felt hard to work on both of anon thp
> > and shmem thp at one time, so I separated the proposal into smaller steps.
> > Shmem uses pagecache so we need some non-trivial effort (including testing)
> > to extend thp migration for shmem. But I think it's a reasonable next step.
> 
> OK, I see. I have forgot about this part. Please be explicit about that
> in the changelog. Also the proper fix is to not use movable zone for
> shmem page THP rather than hack around it in the hotplug specific code
> IMHO.

No. We should just split the page before running
try_to_unmap(TTU_MIGRATION) on the page.

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Kirill A. Shutemov]

On Tue, Apr 03, 2018 at 10:34:51AM +0200, Michal Hocko wrote:
> On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> > On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > > My testing for the latest kernel supporting thp migration found out 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.
> > >
> > > Why does it fail? Shmem pages should be movable without any issues.
> > 
> > .. because try_to_unmap_one() explicitly skips unmapping for migration.
> > 
> >   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> >   /* PMD-mapped THP migration entry */
> >   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
> >   VM_BUG_ON_PAGE(PageHuge(page) || 
> > !PageTransCompound(page), page);
> >   
> >   if (!PageAnon(page))
> >   continue;
> >   
> >   set_pmd_migration_entry(, page);
> >   continue;
> >   }
> >   #endif
> > 
> > When I implemented this code, I felt hard to work on both of anon thp
> > and shmem thp at one time, so I separated the proposal into smaller steps.
> > Shmem uses pagecache so we need some non-trivial effort (including testing)
> > to extend thp migration for shmem. But I think it's a reasonable next step.
> 
> OK, I see. I have forgot about this part. Please be explicit about that
> in the changelog. Also the proper fix is to not use movable zone for
> shmem page THP rather than hack around it in the hotplug specific code
> IMHO.

No. We should just split the page before running
try_to_unmap(TTU_MIGRATION) on the page.

-- 
 Kirill A. Shutemov

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > My testing for the latest kernel supporting thp migration found out 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.
> >
> > Why does it fail? Shmem pages should be movable without any issues.
> 
> .. because try_to_unmap_one() explicitly skips unmapping for migration.
> 
>   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>   /* PMD-mapped THP migration entry */
>   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
>   VM_BUG_ON_PAGE(PageHuge(page) || 
> !PageTransCompound(page), page);
>   
>   if (!PageAnon(page))
>   continue;
>   
>   set_pmd_migration_entry(, page);
>   continue;
>   }
>   #endif
> 
> When I implemented this code, I felt hard to work on both of anon thp
> and shmem thp at one time, so I separated the proposal into smaller steps.
> Shmem uses pagecache so we need some non-trivial effort (including testing)
> to extend thp migration for shmem. But I think it's a reasonable next step.

OK, I see. I have forgot about this part. Please be explicit about that
in the changelog. Also the proper fix is to not use movable zone for
shmem page THP rather than hack around it in the hotplug specific code
IMHO.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 08:24:06, Naoya Horiguchi wrote:
> On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> > On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > > My testing for the latest kernel supporting thp migration found out 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.
> >
> > Why does it fail? Shmem pages should be movable without any issues.
> 
> .. because try_to_unmap_one() explicitly skips unmapping for migration.
> 
>   #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>   /* PMD-mapped THP migration entry */
>   if (!pvmw.pte && (flags & TTU_MIGRATION)) {
>   VM_BUG_ON_PAGE(PageHuge(page) || 
> !PageTransCompound(page), page);
>   
>   if (!PageAnon(page))
>   continue;
>   
>   set_pmd_migration_entry(, page);
>   continue;
>   }
>   #endif
> 
> When I implemented this code, I felt hard to work on both of anon thp
> and shmem thp at one time, so I separated the proposal into smaller steps.
> Shmem uses pagecache so we need some non-trivial effort (including testing)
> to extend thp migration for shmem. But I think it's a reasonable next step.

OK, I see. I have forgot about this part. Please be explicit about that
in the changelog. Also the proper fix is to not use movable zone for
shmem page THP rather than hack around it in the hotplug specific code
IMHO.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Naoya Horiguchi]

On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > My testing for the latest kernel supporting thp migration found out 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.
>
> Why does it fail? Shmem pages should be movable without any issues.

.. because try_to_unmap_one() explicitly skips unmapping for migration.

  #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
  /* PMD-mapped THP migration entry */
  if (!pvmw.pte && (flags & TTU_MIGRATION)) {
  VM_BUG_ON_PAGE(PageHuge(page) || 
!PageTransCompound(page), page);
  
  if (!PageAnon(page))
  continue;
  
  set_pmd_migration_entry(, page);
  continue;
  }
  #endif

When I implemented this code, I felt hard to work on both of anon thp
and shmem thp at one time, so I separated the proposal into smaller steps.
Shmem uses pagecache so we need some non-trivial effort (including testing)
to extend thp migration for shmem. But I think it's a reasonable next step.

Thanks,
Naoya Horiguchi

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Naoya Horiguchi]

On Tue, Apr 03, 2018 at 09:59:28AM +0200, Michal Hocko wrote:
> On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> > My testing for the latest kernel supporting thp migration found out 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.
>
> Why does it fail? Shmem pages should be movable without any issues.

.. because try_to_unmap_one() explicitly skips unmapping for migration.

  #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
  /* PMD-mapped THP migration entry */
  if (!pvmw.pte && (flags & TTU_MIGRATION)) {
  VM_BUG_ON_PAGE(PageHuge(page) || 
!PageTransCompound(page), page);
  
  if (!PageAnon(page))
  continue;
  
  set_pmd_migration_entry(, page);
  continue;
  }
  #endif

When I implemented this code, I felt hard to work on both of anon thp
and shmem thp at one time, so I separated the proposal into smaller steps.
Shmem uses pagecache so we need some non-trivial effort (including testing)
to extend thp migration for shmem. But I think it's a reasonable next step.

Thanks,
Naoya Horiguchi

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> My testing for the latest kernel supporting thp migration found out 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.

Why does it fail? Shmem pages should be movable without any issues.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm: consider non-anonymous thp as unmovable page

[Michal Hocko]

On Tue 03-04-18 13:46:28, Naoya Horiguchi wrote:
> My testing for the latest kernel supporting thp migration found out 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.

Why does it fail? Shmem pages should be movable without any issues.

-- 
Michal Hocko
SUSE Labs

[PATCH v1] mm: consider non-anonymous thp as unmovable page

[Naoya Horiguchi]

My testing for the latest kernel supporting thp migration found out 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 this patch simply does it for
non-anonymous thps.

Fixes: commit 72b39cfc4d75 ("mm, memory_hotplug: do not fail offlining too 
early")
Signed-off-by: Naoya Horiguchi 
Cc: sta...@vger.kernel.org # v4.15+
---
Actually I'm not sure which commit we should set to "Fixes" tag.
Commit 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports thp
migration") failed to introduce the code that this patch is suggesting.
But the infinite loop became visible by commit 72b39cfc4d75 ("mm,
memory_hotplug: do not fail offlining too early") where retry code was
removed.
---
 mm/page_alloc.c | 12 
 1 file changed, 12 insertions(+)

diff --git v4.16-rc7-mmotm-2018-03-28-16-05/mm/page_alloc.c 
v4.16-rc7-mmotm-2018-03-28-16-05_patched/mm/page_alloc.c
index 905db9d..dbbe8fa 100644
--- v4.16-rc7-mmotm-2018-03-28-16-05/mm/page_alloc.c
+++ v4.16-rc7-mmotm-2018-03-28-16-05_patched/mm/page_alloc.c
@@ -7682,6 +7682,18 @@ bool has_unmovable_pages(struct zone *zone, struct page 
*page, int count,
 
if (!PageLRU(page))
found++;
+
+   /*
+* Thp migration is available only for anonymous thps for now.
+* So let's consider non-anonymous thps as unmovable pages.
+*/
+   if (PageTransCompound(page)) {
+   if (PageAnon(page))
+   iter += (1 << page_order(page)) - 1;
+   else
+   found++;
+   }
+
/*
 * If there are RECLAIMABLE pages, we need to check
 * it.  But now, memory offline itself doesn't call
-- 
2.7.0

[PATCH v1] mm: consider non-anonymous thp as unmovable page

[Naoya Horiguchi]

My testing for the latest kernel supporting thp migration found out 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 this patch simply does it for
non-anonymous thps.

Fixes: commit 72b39cfc4d75 ("mm, memory_hotplug: do not fail offlining too 
early")
Signed-off-by: Naoya Horiguchi 
Cc: sta...@vger.kernel.org # v4.15+
---
Actually I'm not sure which commit we should set to "Fixes" tag.
Commit 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports thp
migration") failed to introduce the code that this patch is suggesting.
But the infinite loop became visible by commit 72b39cfc4d75 ("mm,
memory_hotplug: do not fail offlining too early") where retry code was
removed.
---
 mm/page_alloc.c | 12 
 1 file changed, 12 insertions(+)

diff --git v4.16-rc7-mmotm-2018-03-28-16-05/mm/page_alloc.c 
v4.16-rc7-mmotm-2018-03-28-16-05_patched/mm/page_alloc.c
index 905db9d..dbbe8fa 100644
--- v4.16-rc7-mmotm-2018-03-28-16-05/mm/page_alloc.c
+++ v4.16-rc7-mmotm-2018-03-28-16-05_patched/mm/page_alloc.c
@@ -7682,6 +7682,18 @@ bool has_unmovable_pages(struct zone *zone, struct page 
*page, int count,
 
if (!PageLRU(page))
found++;
+
+   /*
+* Thp migration is available only for anonymous thps for now.
+* So let's consider non-anonymous thps as unmovable pages.
+*/
+   if (PageTransCompound(page)) {
+   if (PageAnon(page))
+   iter += (1 << page_order(page)) - 1;
+   else
+   found++;
+   }
+
/*
 * If there are RECLAIMABLE pages, we need to check
 * it.  But now, memory offline itself doesn't call
-- 
2.7.0