[PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Zi Yan]

From: Zi Yan <zi.yan@cs.rutgers.edu>

In [1], Andrea reported that during memory hotplug/hot remove
prep_transhuge_page() is called incorrectly on non-THP pages for
migration, when THP is on but THP migration is not enabled.
This leads to a bad state of target pages for migration.

This patch fixes it by only calling prep_transhuge_page() when we are
certain that the target page is THP.

[1] https://lkml.org/lkml/2017/11/20/411

Cc: stable@vger.kernel.org # v4.14
Fixes: 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports thp migration")
Reported-by: Andrea Reale <ar@linux.vnet.ibm.com>
Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: "JA(C)rA'me Glisse" <jglisse@redhat.com>
---
 include/linux/migrate.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 895ec0c4942e..a2246cf670ba 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -54,7 +54,7 @@ static inline struct page *new_page_nodemask(struct page *page,
 	new_page = __alloc_pages_nodemask(gfp_mask, order,
 				preferred_nid, nodemask);
 
-	if (new_page && PageTransHuge(page))
+	if (new_page && PageTransHuge(new_page))
 		prep_transhuge_page(new_page);
 
 	return new_page;
-- 
2.14.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Andrew Morton]

On Mon, 20 Nov 2017 21:18:55 -0500 Zi Yan <zi.yan@sent.com> wrote:

> In [1], Andrea reported that during memory hotplug/hot remove
> prep_transhuge_page() is called incorrectly on non-THP pages for
> migration, when THP is on but THP migration is not enabled.
> This leads to a bad state of target pages for migration.
> 
> This patch fixes it by only calling prep_transhuge_page() when we are
> certain that the target page is THP.

What are the user-visible effects of the bug?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

On 21 Nov 2017, at 17:12, Andrew Morton wrote:

> On Mon, 20 Nov 2017 21:18:55 -0500 Zi Yan <zi.yan@sent.com> wrote:
>
>> In [1], Andrea reported that during memory hotplug/hot remove
>> prep_transhuge_page() is called incorrectly on non-THP pages for
>> migration, when THP is on but THP migration is not enabled.
>> This leads to a bad state of target pages for migration.
>>
>> This patch fixes it by only calling prep_transhuge_page() when we are
>> certain that the target page is THP.
>
> What are the user-visible effects of the bug?

By inspecting the code, if called on a non-THP, prep_transhuge_page() will
1) change the value of the mapping of (page + 2), since it is used for THP deferred list;
2) change the lru value of (page + 1), since it is used for THP’s dtor.

Both can lead to data corruption of these two pages.

—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 557 bytes --]

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Andrea Reale]

On Tue 21 Nov 2017, 17:35, Zi Yan wrote:
> On 21 Nov 2017, at 17:12, Andrew Morton wrote:
> 
> > On Mon, 20 Nov 2017 21:18:55 -0500 Zi Yan <zi.yan@sent.com> wrote:
> >
> >> This patch fixes it by only calling prep_transhuge_page() when we are
> >> certain that the target page is THP.
> >
> > What are the user-visible effects of the bug?
> 
> By inspecting the code, if called on a non-THP, prep_transhuge_page() will
> 1) change the value of the mapping of (page + 2), since it is used for THP deferred list;
> 2) change the lru value of (page + 1), since it is used for THPa??s dtor.
> 
> Both can lead to data corruption of these two pages.

Pragmatically and from the point of view of the memory_hotplug subsys,
the effect is a kernel crash when pages are being migrated during a memory
hot remove offline and migration target pages are found in a bad state.

Best,
Andrea

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Michal Hocko]

On Mon 20-11-17 21:18:55, Zi Yan wrote:
> From: Zi Yan <zi.yan@cs.rutgers.edu>
> 
> In [1], Andrea reported that during memory hotplug/hot remove
> prep_transhuge_page() is called incorrectly on non-THP pages for
> migration, when THP is on but THP migration is not enabled.
> This leads to a bad state of target pages for migration.
> 
> This patch fixes it by only calling prep_transhuge_page() when we are
> certain that the target page is THP.
> 
> [1] https://lkml.org/lkml/2017/11/20/411

lkml.org tends to be quite unstable so a
http://lkml.kernel.org/r/$msg-id is usually a preferred way.

> 
> Cc: stable@vger.kernel.org # v4.14
> Fixes: 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports thp migration")
> Reported-by: Andrea Reale <ar@linux.vnet.ibm.com>
> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: "Jerome Glisse" <jglisse@redhat.com>
> ---
>  include/linux/migrate.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 895ec0c4942e..a2246cf670ba 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -54,7 +54,7 @@ static inline struct page *new_page_nodemask(struct page *page,
>  	new_page = __alloc_pages_nodemask(gfp_mask, order,
>  				preferred_nid, nodemask);
>  
> -	if (new_page && PageTransHuge(page))
> +	if (new_page && PageTransHuge(new_page))
>  		prep_transhuge_page(new_page);

I would keep the two checks consistent. But that leads to a more
interesting question. new_page_nodemask does

	if (thp_migration_supported() && PageTransHuge(page)) {
		order = HPAGE_PMD_ORDER;
		gfp_mask |= GFP_TRANSHUGE;
	}

How come it is safe to allocate an order-0 page if
!thp_migration_supported() when we are about to migrate THP? This
doesn't make any sense to me. Are we working around this somewhere else?
Why shouldn't we simply return NULL here?

Nayoa, could you explain please? 8135d8926c08 ("mm: memory_hotplug:
memory hotremove supports thp migration") changelog is less than
satisfactory.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]

On 22 Nov 2017, at 3:54, Michal Hocko wrote:

> On Mon 20-11-17 21:18:55, Zi Yan wrote:
>> From: Zi Yan <zi.yan@cs.rutgers.edu>
>>
>> In [1], Andrea reported that during memory hotplug/hot remove
>> prep_transhuge_page() is called incorrectly on non-THP pages for
>> migration, when THP is on but THP migration is not enabled.
>> This leads to a bad state of target pages for migration.
>>
>> This patch fixes it by only calling prep_transhuge_page() when we are
>> certain that the target page is THP.
>>
>> [1] https://lkml.org/lkml/2017/11/20/411
>
> lkml.org tends to be quite unstable so a
> http://lkml.kernel.org/r/$msg-id is usually a preferred way.

Got it. Thanks.

>
>>
>> Cc: stable@vger.kernel.org # v4.14
>> Fixes: 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports thp migration")
>> Reported-by: Andrea Reale <ar@linux.vnet.ibm.com>
>> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>> ---
>>  include/linux/migrate.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
>> index 895ec0c4942e..a2246cf670ba 100644
>> --- a/include/linux/migrate.h
>> +++ b/include/linux/migrate.h
>> @@ -54,7 +54,7 @@ static inline struct page *new_page_nodemask(struct page *page,
>>  	new_page = __alloc_pages_nodemask(gfp_mask, order,
>>  				preferred_nid, nodemask);
>>
>> -	if (new_page && PageTransHuge(page))
>> +	if (new_page && PageTransHuge(new_page))
>>  		prep_transhuge_page(new_page);
>
> I would keep the two checks consistent. But that leads to a more
> interesting question. new_page_nodemask does
>
> 	if (thp_migration_supported() && PageTransHuge(page)) {
> 		order = HPAGE_PMD_ORDER;
> 		gfp_mask |= GFP_TRANSHUGE;
> 	}
>
> How come it is safe to allocate an order-0 page if
> !thp_migration_supported() when we are about to migrate THP? This
> doesn't make any sense to me. Are we working around this somewhere else?
> Why shouldn't we simply return NULL here?

If !thp_migration_supported(), we will first split a THP and migrate its head page. This process
is done in unmap_and_move() after get_new_page() (the function pointer to this new_page_nodemask())
is called. The situation can be PageTransHuge(page) is true here, but the page is split
in unmap_and_move(), so we want to return a order-0 page here.

I think the confusion comes from that there is no guarantee of THP allocation when we are
doing THP migration. If we can allocate a THP during THP migration, we are good. Otherwise, we want to
fallback to the old way, splitting the original THP and migrating the head page, to preserve
the original code behavior.

Does it clarify your confusion?


—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 557 bytes --]

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Michal Hocko]

On Wed 22-11-17 04:18:35, Zi Yan wrote:
> On 22 Nov 2017, at 3:54, Michal Hocko wrote:
[...]
> > I would keep the two checks consistent. But that leads to a more
> > interesting question. new_page_nodemask does
> >
> > 	if (thp_migration_supported() && PageTransHuge(page)) {
> > 		order = HPAGE_PMD_ORDER;
> > 		gfp_mask |= GFP_TRANSHUGE;
> > 	}
> >
> > How come it is safe to allocate an order-0 page if
> > !thp_migration_supported() when we are about to migrate THP? This
> > doesn't make any sense to me. Are we working around this somewhere else?
> > Why shouldn't we simply return NULL here?
> 
> If !thp_migration_supported(), we will first split a THP and migrate
> its head page. This process is done in unmap_and_move() after
> get_new_page() (the function pointer to this new_page_nodemask()) is
> called. The situation can be PageTransHuge(page) is true here, but the
> page is split in unmap_and_move(), so we want to return a order-0 page
> here.

This deserves a big fat comment in the code because this is not clear
from the code!

> I think the confusion comes from that there is no guarantee of THP
> allocation when we are doing THP migration. If we can allocate a THP
> during THP migration, we are good. Otherwise, we want to fallback to
> the old way, splitting the original THP and migrating the head page,
> to preserve the original code behavior.

I understand that but that should be done explicitly rather than relying
on two functions doing the right thing because this is just too fragile.

Moreover I am not really sure this is really working properly. Just look
at the split_huge_page. It moves all the tail pages to the LRU list
while migrate_pages has a list of pages to migrate. So we will migrate
the head page and all the rest will get back to the LRU list. What
guarantees that they will get migrated as well.

This all looks like a mess!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Michal Hocko]

On Wed 22-11-17 10:35:10, Michal Hocko wrote:
[...]
> Moreover I am not really sure this is really working properly. Just look
> at the split_huge_page. It moves all the tail pages to the LRU list
> while migrate_pages has a list of pages to migrate. So we will migrate
> the head page and all the rest will get back to the LRU list. What
> guarantees that they will get migrated as well.

OK, so this is as I've expected. It doesn't work! Some pfn walker based
migration will just skip tail pages see madvise_inject_error.
__alloc_contig_migrate_range will simply fail on THP page see
isolate_migratepages_block so we even do not try to migrate it.
do_move_page_to_node_array will simply migrate head and do not care
about tail pages. do_mbind splits the page and then fall back to pte
walk when thp migration is not supported but it doesn't handle tail
pages if the THP migration path is not able to allocate a fresh THP
AFAICS. Memory hotplug should be safe because it doesn't skip the whole
THP when doing pfn walk.

Unless I am missing something here this looks like a huge mess to me.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On 22 Nov 2017, at 5:14, Michal Hocko wrote:

> On Wed 22-11-17 10:35:10, Michal Hocko wrote:
> [...]
>> Moreover I am not really sure this is really working properly. Just look
>> at the split_huge_page. It moves all the tail pages to the LRU list
>> while migrate_pages has a list of pages to migrate. So we will migrate
>> the head page and all the rest will get back to the LRU list. What
>> guarantees that they will get migrated as well.
>
> OK, so this is as I've expected. It doesn't work! Some pfn walker based
> migration will just skip tail pages see madvise_inject_error.
> __alloc_contig_migrate_range will simply fail on THP page see
> isolate_migratepages_block so we even do not try to migrate it.
> do_move_page_to_node_array will simply migrate head and do not care
> about tail pages. do_mbind splits the page and then fall back to pte
> walk when thp migration is not supported but it doesn't handle tail
> pages if the THP migration path is not able to allocate a fresh THP
> AFAICS. Memory hotplug should be safe because it doesn't skip the whole
> THP when doing pfn walk.
>
> Unless I am missing something here this looks like a huge mess to me.

+Kirill

First, I agree with you that splitting a THP and only migrating its head page
is a mess. But what you describe is also the behavior of migrate_page()
_before_ THP migration support is added. I thought that was intended.

Look at http://elixir.free-electrons.com/linux/v4.13.15/source/mm/migrate.c#L1091,
unmap_and_move() splits THPs and only migrates the head page in v4.13 before THP
migration is added. I think the behavior was introduced since v4.5 (I just skimmed
v4.0 to v4.13 code and did not have time to use git blame), before that THPs are
not migrated but shown as successfully migrated (at least from v4.4’s code).

Naoya and I had a discussion on this ‘splitting a THP and migrating its head page’ before.
We think we should try to spilt the THP and migrate all its subpages. I did not have
time to get the code out yet.

I am traveling today, so I may not be able to do anything useful. I will be on a break
for a month and will not have good accesses to any machines. I can try to fix
this ‘splitting a THP and migrating its head page’ after that.

—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 557 bytes --]

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]

On 22 Nov 2017, at 7:13, Zi Yan wrote:

> On 22 Nov 2017, at 5:14, Michal Hocko wrote:
>
>> On Wed 22-11-17 10:35:10, Michal Hocko wrote:
>> [...]
>>> Moreover I am not really sure this is really working properly. Just look
>>> at the split_huge_page. It moves all the tail pages to the LRU list
>>> while migrate_pages has a list of pages to migrate. So we will migrate
>>> the head page and all the rest will get back to the LRU list. What
>>> guarantees that they will get migrated as well.
>>
>> OK, so this is as I've expected. It doesn't work! Some pfn walker based
>> migration will just skip tail pages see madvise_inject_error.
>> __alloc_contig_migrate_range will simply fail on THP page see
>> isolate_migratepages_block so we even do not try to migrate it.
>> do_move_page_to_node_array will simply migrate head and do not care
>> about tail pages. do_mbind splits the page and then fall back to pte
>> walk when thp migration is not supported but it doesn't handle tail
>> pages if the THP migration path is not able to allocate a fresh THP
>> AFAICS. Memory hotplug should be safe because it doesn't skip the whole
>> THP when doing pfn walk.
>>
>> Unless I am missing something here this looks like a huge mess to me.
>
> +Kirill
>
> First, I agree with you that splitting a THP and only migrating its head page
> is a mess. But what you describe is also the behavior of migrate_page()
> _before_ THP migration support is added. I thought that was intended.
>
> Look at http://elixir.free-electrons.com/linux/v4.13.15/source/mm/migrate.c#L1091,
> unmap_and_move() splits THPs and only migrates the head page in v4.13 before THP
> migration is added. I think the behavior was introduced since v4.5 (I just skimmed
> v4.0 to v4.13 code and did not have time to use git blame), before that THPs are
> not migrated but shown as successfully migrated (at least from v4.4’s code).

Sorry, I misread v4.4’s code, it also does ‘splitting a THP and migrating its head page’.
This behavior was there for a long time, at least since v3.0.

The code in unmap_and_move() is:

if (unlikely(PageTransHuge(page)))
		if (unlikely(split_huge_page(page)))
			goto out;

Hope I did not miss anything else.


—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 557 bytes --]

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Michal Hocko]

On Wed 22-11-17 07:29:38, Zi Yan wrote:
> On 22 Nov 2017, at 7:13, Zi Yan wrote:
> 
> > On 22 Nov 2017, at 5:14, Michal Hocko wrote:
> >
> >> On Wed 22-11-17 10:35:10, Michal Hocko wrote:
> >> [...]
> >>> Moreover I am not really sure this is really working properly. Just look
> >>> at the split_huge_page. It moves all the tail pages to the LRU list
> >>> while migrate_pages has a list of pages to migrate. So we will migrate
> >>> the head page and all the rest will get back to the LRU list. What
> >>> guarantees that they will get migrated as well.
> >>
> >> OK, so this is as I've expected. It doesn't work! Some pfn walker based
> >> migration will just skip tail pages see madvise_inject_error.
> >> __alloc_contig_migrate_range will simply fail on THP page see
> >> isolate_migratepages_block so we even do not try to migrate it.
> >> do_move_page_to_node_array will simply migrate head and do not care
> >> about tail pages. do_mbind splits the page and then fall back to pte
> >> walk when thp migration is not supported but it doesn't handle tail
> >> pages if the THP migration path is not able to allocate a fresh THP
> >> AFAICS. Memory hotplug should be safe because it doesn't skip the whole
> >> THP when doing pfn walk.
> >>
> >> Unless I am missing something here this looks like a huge mess to me.
> >
> > +Kirill
> >
> > First, I agree with you that splitting a THP and only migrating its head page
> > is a mess. But what you describe is also the behavior of migrate_page()
> > _before_ THP migration support is added. I thought that was intended.
> >
> > Look at http://elixir.free-electrons.com/linux/v4.13.15/source/mm/migrate.c#L1091,
> > unmap_and_move() splits THPs and only migrates the head page in v4.13 before THP
> > migration is added. I think the behavior was introduced since v4.5 (I just skimmed
> > v4.0 to v4.13 code and did not have time to use git blame), before that THPs are
> > not migrated but shown as successfully migrated (at least from v4.4a??s code).
> 
> Sorry, I misread v4.4a??s code, it also does a??splitting a THP and migrating its head pagea??.
> This behavior was there for a long time, at least since v3.0.
> 
> The code in unmap_and_move() is:
> 
> if (unlikely(PageTransHuge(page)))
> 		if (unlikely(split_huge_page(page)))
> 			goto out;

I _think_ that this all should be handled at migrate_pages layer. Try to
migrate THP and fallback to split_huge_page into to the list when it
fails. I haven't checked whether there is something which would prevent
that though. THP tricks in specific paths then should be removed.


-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Michal Hocko]

On Wed 22-11-17 09:54:16, Michal Hocko wrote:
> On Mon 20-11-17 21:18:55, Zi Yan wrote:
[...]
> > diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> > index 895ec0c4942e..a2246cf670ba 100644
> > --- a/include/linux/migrate.h
> > +++ b/include/linux/migrate.h
> > @@ -54,7 +54,7 @@ static inline struct page *new_page_nodemask(struct page *page,
> >  	new_page = __alloc_pages_nodemask(gfp_mask, order,
> >  				preferred_nid, nodemask);
> >  
> > -	if (new_page && PageTransHuge(page))
> > +	if (new_page && PageTransHuge(new_page))
> >  		prep_transhuge_page(new_page);
> 
> I would keep the two checks consistent. But that leads to a more
> interesting question. new_page_nodemask does
> 
> 	if (thp_migration_supported() && PageTransHuge(page)) {
> 		order = HPAGE_PMD_ORDER;
> 		gfp_mask |= GFP_TRANSHUGE;
> 	}

And one more question/note. Why do we need thp_migration_supported
in the first place? 9c670ea37947 ("mm: thp: introduce
CONFIG_ARCH_ENABLE_THP_MIGRATION") says
: Introduce CONFIG_ARCH_ENABLE_THP_MIGRATION to limit thp migration
: functionality to x86_64, which should be safer at the first step.

but why is unsafe to enable the feature on other arches which support
THP? Is there any plan to do the next step and remove this config
option?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]



Michal Hocko wrote:
> On Wed 22-11-17 09:54:16, Michal Hocko wrote:
>> On Mon 20-11-17 21:18:55, Zi Yan wrote:
> [...]
>>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
>>> index 895ec0c4942e..a2246cf670ba 100644
>>> --- a/include/linux/migrate.h
>>> +++ b/include/linux/migrate.h
>>> @@ -54,7 +54,7 @@ static inline struct page *new_page_nodemask(struct page *page,
>>>  	new_page = __alloc_pages_nodemask(gfp_mask, order,
>>>  				preferred_nid, nodemask);
>>>  
>>> -	if (new_page && PageTransHuge(page))
>>> +	if (new_page && PageTransHuge(new_page))
>>>  		prep_transhuge_page(new_page);
>> I would keep the two checks consistent. But that leads to a more
>> interesting question. new_page_nodemask does
>>
>> 	if (thp_migration_supported() && PageTransHuge(page)) {
>> 		order = HPAGE_PMD_ORDER;
>> 		gfp_mask |= GFP_TRANSHUGE;
>> 	}
> 
> And one more question/note. Why do we need thp_migration_supported
> in the first place? 9c670ea37947 ("mm: thp: introduce
> CONFIG_ARCH_ENABLE_THP_MIGRATION") says
> : Introduce CONFIG_ARCH_ENABLE_THP_MIGRATION to limit thp migration
> : functionality to x86_64, which should be safer at the first step.
> 
> but why is unsafe to enable the feature on other arches which support
> THP? Is there any plan to do the next step and remove this config
> option?

Because different architectures have their own way of specifying a swap
entry. This means, to support THP migration, each architecture needs to
add its own __pmd_to_swp_entry() and __swp_entry_to_pmd(), which are
used for arch-independent pmd_to_swp_entry() and swp_entry_to_pmd().

We need this CONFIG_ARCH_ENABLE_THP_MIGRATION before archs that support
THP add code for pmd swap entry.

-- 
Best Regards,
Yan Zi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 537 bytes --]

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Michal Hocko]

On Wed 22-11-17 09:43:46, Zi Yan wrote:
> 
> 
> Michal Hocko wrote:
> > On Wed 22-11-17 09:54:16, Michal Hocko wrote:
> >> On Mon 20-11-17 21:18:55, Zi Yan wrote:
> > [...]
> >>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> >>> index 895ec0c4942e..a2246cf670ba 100644
> >>> --- a/include/linux/migrate.h
> >>> +++ b/include/linux/migrate.h
> >>> @@ -54,7 +54,7 @@ static inline struct page *new_page_nodemask(struct page *page,
> >>>  	new_page = __alloc_pages_nodemask(gfp_mask, order,
> >>>  				preferred_nid, nodemask);
> >>>  
> >>> -	if (new_page && PageTransHuge(page))
> >>> +	if (new_page && PageTransHuge(new_page))
> >>>  		prep_transhuge_page(new_page);
> >> I would keep the two checks consistent. But that leads to a more
> >> interesting question. new_page_nodemask does
> >>
> >> 	if (thp_migration_supported() && PageTransHuge(page)) {
> >> 		order = HPAGE_PMD_ORDER;
> >> 		gfp_mask |= GFP_TRANSHUGE;
> >> 	}
> > 
> > And one more question/note. Why do we need thp_migration_supported
> > in the first place? 9c670ea37947 ("mm: thp: introduce
> > CONFIG_ARCH_ENABLE_THP_MIGRATION") says
> > : Introduce CONFIG_ARCH_ENABLE_THP_MIGRATION to limit thp migration
> > : functionality to x86_64, which should be safer at the first step.
> > 
> > but why is unsafe to enable the feature on other arches which support
> > THP? Is there any plan to do the next step and remove this config
> > option?
> 
> Because different architectures have their own way of specifying a swap
> entry. This means, to support THP migration, each architecture needs to
> add its own __pmd_to_swp_entry() and __swp_entry_to_pmd(), which are
> used for arch-independent pmd_to_swp_entry() and swp_entry_to_pmd().

I understand that part. But this smells like a matter of coding, no?
I was suprised to see the note about safety which didn't make much sense
to me.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]



Michal Hocko wrote:
> On Wed 22-11-17 09:43:46, Zi Yan wrote:
>>
>> Michal Hocko wrote:
>>> On Wed 22-11-17 09:54:16, Michal Hocko wrote:
>>>> On Mon 20-11-17 21:18:55, Zi Yan wrote:
>>> [...]
>>>>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
>>>>> index 895ec0c4942e..a2246cf670ba 100644
>>>>> --- a/include/linux/migrate.h
>>>>> +++ b/include/linux/migrate.h
>>>>> @@ -54,7 +54,7 @@ static inline struct page *new_page_nodemask(struct page *page,
>>>>>  	new_page = __alloc_pages_nodemask(gfp_mask, order,
>>>>>  				preferred_nid, nodemask);
>>>>>  
>>>>> -	if (new_page && PageTransHuge(page))
>>>>> +	if (new_page && PageTransHuge(new_page))
>>>>>  		prep_transhuge_page(new_page);
>>>> I would keep the two checks consistent. But that leads to a more
>>>> interesting question. new_page_nodemask does
>>>>
>>>> 	if (thp_migration_supported() && PageTransHuge(page)) {
>>>> 		order = HPAGE_PMD_ORDER;
>>>> 		gfp_mask |= GFP_TRANSHUGE;
>>>> 	}
>>> And one more question/note. Why do we need thp_migration_supported
>>> in the first place? 9c670ea37947 ("mm: thp: introduce
>>> CONFIG_ARCH_ENABLE_THP_MIGRATION") says
>>> : Introduce CONFIG_ARCH_ENABLE_THP_MIGRATION to limit thp migration
>>> : functionality to x86_64, which should be safer at the first step.
>>>
>>> but why is unsafe to enable the feature on other arches which support
>>> THP? Is there any plan to do the next step and remove this config
>>> option?
>> Because different architectures have their own way of specifying a swap
>> entry. This means, to support THP migration, each architecture needs to
>> add its own __pmd_to_swp_entry() and __swp_entry_to_pmd(), which are
>> used for arch-independent pmd_to_swp_entry() and swp_entry_to_pmd().
> 
> I understand that part. But this smells like a matter of coding, no?
> I was suprised to see the note about safety which didn't make much sense
> to me.

And testing as well. I had powerpc book3s support in my initial patch
submission, but removed it because I do not have access to the powerpc
machine any more. I also tried ARM64, which seems working by adding the
code, but I have no hardware to test it now.

Any suggestions?

-- 
Best Regards,
Yan Zi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 537 bytes --]

Re: [PATCH] mm: migrate: fix an incorrect call of prep_transhuge_page()

[Michal Hocko]

On Wed 22-11-17 10:09:13, Zi Yan wrote:
> 
> 
> Michal Hocko wrote:
> > On Wed 22-11-17 09:43:46, Zi Yan wrote:
> >>
> >> Michal Hocko wrote:
[...]
> >>> but why is unsafe to enable the feature on other arches which support
> >>> THP? Is there any plan to do the next step and remove this config
> >>> option?
> >> Because different architectures have their own way of specifying a swap
> >> entry. This means, to support THP migration, each architecture needs to
> >> add its own __pmd_to_swp_entry() and __swp_entry_to_pmd(), which are
> >> used for arch-independent pmd_to_swp_entry() and swp_entry_to_pmd().
> > 
> > I understand that part. But this smells like a matter of coding, no?
> > I was suprised to see the note about safety which didn't make much sense
> > to me.
> 
> And testing as well. I had powerpc book3s support in my initial patch
> submission, but removed it because I do not have access to the powerpc
> machine any more. I also tried ARM64, which seems working by adding the
> code, but I have no hardware to test it now.
> 
> Any suggestions?

Cc arch maintainers and mailing lists?

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>