[RFC PATCH 0/5] mm, memory_hotplug: improve memory offlining failures debugging

[RFC PATCH 0/5] mm, memory_hotplug: improve memory offlining failures debugging

[Michal Hocko]

Hi,
I have been promissing to improve memory offlining failures debugging
for quite some time. As things stand now we get only very limited
information in the kernel log when the offlining fails. It is usually
only
[ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed
without no further details. We do not know what exactly fails and for
what reason. Whenever I was forced to debug such a failure I've always
had to do a debugging patch to tell me more. We can enable some
tracepoints but it would be much better to get a better picture without
using them.

This patch series does 2 things. The first one is to make dump_page
more usable by printing more information about the mapping patch 1.
Then it reduces the log level from emerg to warning so that this
function is usable from less critical context patch 2. Then I have
added more detailed information about the offlining failure patch 4
and finally add dump_page to isolation and offlining migration paths.
Patch 3 is a trivial cleanup.

Does this look go to you?

Shortlog
Michal Hocko (5):
      mm: print more information about mapping in __dump_page
      mm: lower the printk loglevel for __dump_page messages
      mm, memory_hotplug: drop pointless block alignment checks from __offline_pages
      mm, memory_hotplug: print reason for the offlining failure
      mm, memory_hotplug: be more verbose for memory offline failures

Diffstat:
 mm/debug.c          | 23 ++++++++++++++++++-----
 mm/memory_hotplug.c | 52 +++++++++++++++++++++++++++++++---------------------
 mm/page_alloc.c     |  1 +
 3 files changed, 50 insertions(+), 26 deletions(-)

[RFC PATCH 0/5] mm, memory_hotplug: improve memory offlining failures debugging

[Michal Hocko]

Hi,
I have been promissing to improve memory offlining failures debugging
for quite some time. As things stand now we get only very limited
information in the kernel log when the offlining fails. It is usually
only
[ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed
without no further details. We do not know what exactly fails and for
what reason. Whenever I was forced to debug such a failure I've always
had to do a debugging patch to tell me more. We can enable some
tracepoints but it would be much better to get a better picture without
using them.

This patch series does 2 things. The first one is to make dump_page
more usable by printing more information about the mapping patch 1.
Then it reduces the log level from emerg to warning so that this
function is usable from less critical context patch 2. Then I have
added more detailed information about the offlining failure patch 4
and finally add dump_page to isolation and offlining migration paths.
Patch 3 is a trivial cleanup.

Does this look go to you?

Shortlog
Michal Hocko (5):
      mm: print more information about mapping in __dump_page
      mm: lower the printk loglevel for __dump_page messages
      mm, memory_hotplug: drop pointless block alignment checks from __offline_pages
      mm, memory_hotplug: print reason for the offlining failure
      mm, memory_hotplug: be more verbose for memory offline failures

Diffstat:
 mm/debug.c          | 23 ++++++++++++++++++-----
 mm/memory_hotplug.c | 52 +++++++++++++++++++++++++++++++---------------------
 mm/page_alloc.c     |  1 +
 3 files changed, 50 insertions(+), 26 deletions(-)

[RFC PATCH 1/5] mm: print more information about mapping in __dump_page

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

__dump_page prints the mapping pointer but that is quite unhelpful
for many reports because the pointer itself only helps to distinguish
anon/ksm mappings from other ones (because of lowest bits
set). Sometimes it would be much more helpful to know what kind of
mapping that is actually and if we know this is a file mapping then also
try to resolve the dentry name.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/debug.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/mm/debug.c b/mm/debug.c
index cdacba12e09a..a33177bfc856 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -44,6 +44,7 @@ const struct trace_print_flags vmaflag_names[] = {
 
 void __dump_page(struct page *page, const char *reason)
 {
+	struct address_space *mapping = page_mapping(page);
 	bool page_poisoned = PagePoisoned(page);
 	int mapcount;
 
@@ -70,6 +71,18 @@ void __dump_page(struct page *page, const char *reason)
 	if (PageCompound(page))
 		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
 	pr_cont("\n");
+	if (PageAnon(page))
+		pr_emerg("anon ");
+	else if (PageKsm(page))
+		pr_emerg("ksm ");
+	else if (mapping) {
+		pr_emerg("%ps ", mapping->a_ops);
+		if (mapping->host->i_dentry.first) {
+			struct dentry *dentry;
+			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
+			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
+		}
+	}
 	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
 
 	pr_emerg("flags: %#lx(%pGp)\n", page->flags, &page->flags);
-- 
2.19.1

[RFC PATCH 1/5] mm: print more information about mapping in __dump_page

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

__dump_page prints the mapping pointer but that is quite unhelpful
for many reports because the pointer itself only helps to distinguish
anon/ksm mappings from other ones (because of lowest bits
set). Sometimes it would be much more helpful to know what kind of
mapping that is actually and if we know this is a file mapping then also
try to resolve the dentry name.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/debug.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/mm/debug.c b/mm/debug.c
index cdacba12e09a..a33177bfc856 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -44,6 +44,7 @@ const struct trace_print_flags vmaflag_names[] = {
 
 void __dump_page(struct page *page, const char *reason)
 {
+	struct address_space *mapping = page_mapping(page);
 	bool page_poisoned = PagePoisoned(page);
 	int mapcount;
 
@@ -70,6 +71,18 @@ void __dump_page(struct page *page, const char *reason)
 	if (PageCompound(page))
 		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
 	pr_cont("\n");
+	if (PageAnon(page))
+		pr_emerg("anon ");
+	else if (PageKsm(page))
+		pr_emerg("ksm ");
+	else if (mapping) {
+		pr_emerg("%ps ", mapping->a_ops);
+		if (mapping->host->i_dentry.first) {
+			struct dentry *dentry;
+			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
+			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
+		}
+	}
 	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
 
 	pr_emerg("flags: %#lx(%pGp)\n", page->flags, &page->flags);
-- 
2.19.1

Re: [RFC PATCH 1/5] mm: print more information about mapping in __dump_page

[Andrew Morton]

On Wed,  7 Nov 2018 11:18:26 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> __dump_page prints the mapping pointer but that is quite unhelpful
> for many reports because the pointer itself only helps to distinguish
> anon/ksm mappings from other ones (because of lowest bits
> set). Sometimes it would be much more helpful to know what kind of
> mapping that is actually and if we know this is a file mapping then also
> try to resolve the dentry name.
> 
> ...
>
> --- a/mm/debug.c
> +++ b/mm/debug.c
>
> ...
>
> @@ -70,6 +71,18 @@ void __dump_page(struct page *page, const char *reason)
>  	if (PageCompound(page))
>  		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
>  	pr_cont("\n");
> +	if (PageAnon(page))
> +		pr_emerg("anon ");
> +	else if (PageKsm(page))
> +		pr_emerg("ksm ");
> +	else if (mapping) {
> +		pr_emerg("%ps ", mapping->a_ops);
> +		if (mapping->host->i_dentry.first) {
> +			struct dentry *dentry;
> +			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
> +			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
> +		}
> +	}

There has to be a better way of printing the filename.  It is so often
needed.

The (poorly named and gleefully undocumented)
take_dentry_name_snapshot() looks promising.  However it's unclear that
__dump_page() is always called from contexts where
take_dentry_name_snapshot() and release_dentry_name_snapshot() can be
safely called.  Probably it's OK, but how to guarantee it?

Re: [RFC PATCH 1/5] mm: print more information about mapping in __dump_page

[Andrew Morton]

On Wed,  7 Nov 2018 11:18:26 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> __dump_page prints the mapping pointer but that is quite unhelpful
> for many reports because the pointer itself only helps to distinguish
> anon/ksm mappings from other ones (because of lowest bits
> set). Sometimes it would be much more helpful to know what kind of
> mapping that is actually and if we know this is a file mapping then also
> try to resolve the dentry name.
> 
> ...
>
> --- a/mm/debug.c
> +++ b/mm/debug.c
>
> ...
>
> @@ -70,6 +71,18 @@ void __dump_page(struct page *page, const char *reason)
>  	if (PageCompound(page))
>  		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
>  	pr_cont("\n");
> +	if (PageAnon(page))
> +		pr_emerg("anon ");
> +	else if (PageKsm(page))
> +		pr_emerg("ksm ");
> +	else if (mapping) {
> +		pr_emerg("%ps ", mapping->a_ops);
> +		if (mapping->host->i_dentry.first) {
> +			struct dentry *dentry;
> +			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
> +			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
> +		}
> +	}

There has to be a better way of printing the filename.  It is so often
needed.

The (poorly named and gleefully undocumented)
take_dentry_name_snapshot() looks promising.  However it's unclear that
__dump_page() is always called from contexts where
take_dentry_name_snapshot() and release_dentry_name_snapshot() can be
safely called.  Probably it's OK, but how to guarantee it?

Re: [RFC PATCH 1/5] mm: print more information about mapping in __dump_page

[Michal Hocko]

On Fri 23-11-18 16:04:04, Andrew Morton wrote:
> On Wed,  7 Nov 2018 11:18:26 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > __dump_page prints the mapping pointer but that is quite unhelpful
> > for many reports because the pointer itself only helps to distinguish
> > anon/ksm mappings from other ones (because of lowest bits
> > set). Sometimes it would be much more helpful to know what kind of
> > mapping that is actually and if we know this is a file mapping then also
> > try to resolve the dentry name.
> > 
> > ...
> >
> > --- a/mm/debug.c
> > +++ b/mm/debug.c
> >
> > ...
> >
> > @@ -70,6 +71,18 @@ void __dump_page(struct page *page, const char *reason)
> >  	if (PageCompound(page))
> >  		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
> >  	pr_cont("\n");
> > +	if (PageAnon(page))
> > +		pr_emerg("anon ");
> > +	else if (PageKsm(page))
> > +		pr_emerg("ksm ");
> > +	else if (mapping) {
> > +		pr_emerg("%ps ", mapping->a_ops);
> > +		if (mapping->host->i_dentry.first) {
> > +			struct dentry *dentry;
> > +			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
> > +			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
> > +		}
> > +	}
> 
> There has to be a better way of printing the filename.  It is so often
> needed.
> 
> The (poorly named and gleefully undocumented)
> take_dentry_name_snapshot() looks promising.  However it's unclear that
> __dump_page() is always called from contexts where
> take_dentry_name_snapshot() and release_dentry_name_snapshot() can be
> safely called.  Probably it's OK, but how to guarantee it?

http://lkml.kernel.org/r/20181125080834.GB12455@dhcp22.suse.cz as
suggested by Tetsuo?
-- 
Michal Hocko
SUSE Labs

[RFC PATCH 2/5] mm: lower the printk loglevel for __dump_page messages

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

__dump_page messages use KERN_EMERG resp. KERN_ALERT loglevel (this is
the case since 2004). Most callers of this function are really detecting
a critical page state and BUG right after. On the other hand the
function is called also from contexts which just want to inform about
the page state and those would rather not disrupt logs that much (e.g.
some systems route these messages to the normal console).

Reduce the loglevel to KERN_WARNING to make dump_page easier to reuse
for other contexts while those messages will still make it to the kernel
log in most setups. Even if the loglevel setup filters warnings away
those paths that are really critical already print the more targeted
error or panic and that should make it to the kernel log.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/debug.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/debug.c b/mm/debug.c
index a33177bfc856..d18c5cea3320 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -54,7 +54,7 @@ void __dump_page(struct page *page, const char *reason)
 	 * dump_page() when detected.
 	 */
 	if (page_poisoned) {
-		pr_emerg("page:%px is uninitialized and poisoned", page);
+		pr_warn("page:%px is uninitialized and poisoned", page);
 		goto hex_only;
 	}
 
@@ -65,27 +65,27 @@ void __dump_page(struct page *page, const char *reason)
 	 */
 	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
 
-	pr_emerg("page:%px count:%d mapcount:%d mapping:%px index:%#lx",
+	pr_warn("page:%px count:%d mapcount:%d mapping:%px index:%#lx",
 		  page, page_ref_count(page), mapcount,
 		  page->mapping, page_to_pgoff(page));
 	if (PageCompound(page))
 		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
 	pr_cont("\n");
 	if (PageAnon(page))
-		pr_emerg("anon ");
+		pr_warn("anon ");
 	else if (PageKsm(page))
-		pr_emerg("ksm ");
+		pr_warn("ksm ");
 	else if (mapping) {
-		pr_emerg("%ps ", mapping->a_ops);
+		pr_warn("%ps ", mapping->a_ops);
 		if (mapping->host->i_dentry.first) {
 			struct dentry *dentry;
 			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
-			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
+			pr_warn("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
 		}
 	}
 	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
 
-	pr_emerg("flags: %#lx(%pGp)\n", page->flags, &page->flags);
+	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
 
 hex_only:
 	print_hex_dump(KERN_ALERT, "raw: ", DUMP_PREFIX_NONE, 32,
@@ -93,11 +93,11 @@ void __dump_page(struct page *page, const char *reason)
 			sizeof(struct page), false);
 
 	if (reason)
-		pr_alert("page dumped because: %s\n", reason);
+		pr_warn("page dumped because: %s\n", reason);
 
 #ifdef CONFIG_MEMCG
 	if (!page_poisoned && page->mem_cgroup)
-		pr_alert("page->mem_cgroup:%px\n", page->mem_cgroup);
+		pr_warn("page->mem_cgroup:%px\n", page->mem_cgroup);
 #endif
 }
 
-- 
2.19.1

[RFC PATCH 2/5] mm: lower the printk loglevel for __dump_page messages

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

__dump_page messages use KERN_EMERG resp. KERN_ALERT loglevel (this is
the case since 2004). Most callers of this function are really detecting
a critical page state and BUG right after. On the other hand the
function is called also from contexts which just want to inform about
the page state and those would rather not disrupt logs that much (e.g.
some systems route these messages to the normal console).

Reduce the loglevel to KERN_WARNING to make dump_page easier to reuse
for other contexts while those messages will still make it to the kernel
log in most setups. Even if the loglevel setup filters warnings away
those paths that are really critical already print the more targeted
error or panic and that should make it to the kernel log.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/debug.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/debug.c b/mm/debug.c
index a33177bfc856..d18c5cea3320 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -54,7 +54,7 @@ void __dump_page(struct page *page, const char *reason)
 	 * dump_page() when detected.
 	 */
 	if (page_poisoned) {
-		pr_emerg("page:%px is uninitialized and poisoned", page);
+		pr_warn("page:%px is uninitialized and poisoned", page);
 		goto hex_only;
 	}
 
@@ -65,27 +65,27 @@ void __dump_page(struct page *page, const char *reason)
 	 */
 	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
 
-	pr_emerg("page:%px count:%d mapcount:%d mapping:%px index:%#lx",
+	pr_warn("page:%px count:%d mapcount:%d mapping:%px index:%#lx",
 		  page, page_ref_count(page), mapcount,
 		  page->mapping, page_to_pgoff(page));
 	if (PageCompound(page))
 		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
 	pr_cont("\n");
 	if (PageAnon(page))
-		pr_emerg("anon ");
+		pr_warn("anon ");
 	else if (PageKsm(page))
-		pr_emerg("ksm ");
+		pr_warn("ksm ");
 	else if (mapping) {
-		pr_emerg("%ps ", mapping->a_ops);
+		pr_warn("%ps ", mapping->a_ops);
 		if (mapping->host->i_dentry.first) {
 			struct dentry *dentry;
 			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
-			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
+			pr_warn("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
 		}
 	}
 	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
 
-	pr_emerg("flags: %#lx(%pGp)\n", page->flags, &page->flags);
+	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
 
 hex_only:
 	print_hex_dump(KERN_ALERT, "raw: ", DUMP_PREFIX_NONE, 32,
@@ -93,11 +93,11 @@ void __dump_page(struct page *page, const char *reason)
 			sizeof(struct page), false);
 
 	if (reason)
-		pr_alert("page dumped because: %s\n", reason);
+		pr_warn("page dumped because: %s\n", reason);
 
 #ifdef CONFIG_MEMCG
 	if (!page_poisoned && page->mem_cgroup)
-		pr_alert("page->mem_cgroup:%px\n", page->mem_cgroup);
+		pr_warn("page->mem_cgroup:%px\n", page->mem_cgroup);
 #endif
 }
 
-- 
2.19.1

Re: [RFC PATCH 2/5] mm: lower the printk loglevel for __dump_page messages

[Baoquan He]

On 11/07/18 at 11:18am, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> __dump_page messages use KERN_EMERG resp. KERN_ALERT loglevel (this is
> the case since 2004). Most callers of this function are really detecting
> a critical page state and BUG right after. On the other hand the
> function is called also from contexts which just want to inform about
> the page state and those would rather not disrupt logs that much (e.g.
> some systems route these messages to the normal console).
> 
> Reduce the loglevel to KERN_WARNING to make dump_page easier to reuse
> for other contexts while those messages will still make it to the kernel
> log in most setups. Even if the loglevel setup filters warnings away
> those paths that are really critical already print the more targeted
> error or panic and that should make it to the kernel log.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/debug.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index a33177bfc856..d18c5cea3320 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -54,7 +54,7 @@ void __dump_page(struct page *page, const char *reason)
>  	 * dump_page() when detected.
>  	 */
>  	if (page_poisoned) {
> -		pr_emerg("page:%px is uninitialized and poisoned", page);
> +		pr_warn("page:%px is uninitialized and poisoned", page);
>  		goto hex_only;
>  	}
>  
> @@ -65,27 +65,27 @@ void __dump_page(struct page *page, const char *reason)
>  	 */
>  	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
>  
> -	pr_emerg("page:%px count:%d mapcount:%d mapping:%px index:%#lx",
> +	pr_warn("page:%px count:%d mapcount:%d mapping:%px index:%#lx",
	pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx",

Better print it as refcount since we have renamed it. 

>  		  page, page_ref_count(page), mapcount,
>  		  page->mapping, page_to_pgoff(page));
>  	if (PageCompound(page))
>  		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
>  	pr_cont("\n");
>  	if (PageAnon(page))
> -		pr_emerg("anon ");
> +		pr_warn("anon ");
>  	else if (PageKsm(page))
> -		pr_emerg("ksm ");
> +		pr_warn("ksm ");
>  	else if (mapping) {
> -		pr_emerg("%ps ", mapping->a_ops);
> +		pr_warn("%ps ", mapping->a_ops);
>  		if (mapping->host->i_dentry.first) {
>  			struct dentry *dentry;
>  			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
> -			pr_emerg("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
> +			pr_warn("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name);
>  		}
>  	}
>  	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>  
> -	pr_emerg("flags: %#lx(%pGp)\n", page->flags, &page->flags);
> +	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
>  
>  hex_only:
>  	print_hex_dump(KERN_ALERT, "raw: ", DUMP_PREFIX_NONE, 32,
> @@ -93,11 +93,11 @@ void __dump_page(struct page *page, const char *reason)
>  			sizeof(struct page), false);
>  
>  	if (reason)
> -		pr_alert("page dumped because: %s\n", reason);
> +		pr_warn("page dumped because: %s\n", reason);
>  
>  #ifdef CONFIG_MEMCG
>  	if (!page_poisoned && page->mem_cgroup)
> -		pr_alert("page->mem_cgroup:%px\n", page->mem_cgroup);
> +		pr_warn("page->mem_cgroup:%px\n", page->mem_cgroup);
>  #endif
>  }
>  
> -- 
> 2.19.1
> 

Re: [RFC PATCH 2/5] mm: lower the printk loglevel for __dump_page messages

[Michal Hocko]

It seems one follow up fix got lost on my side. Andrew, could you fold
this in please?

diff --git a/mm/debug.c b/mm/debug.c
index 68e9a9f2df16..4c916e1abedc 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -88,7 +88,7 @@ void __dump_page(struct page *page, const char *reason)
 	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
 
 hex_only:
-	print_hex_dump(KERN_ALERT, "raw: ", DUMP_PREFIX_NONE, 32,
+	print_hex_dump(KERN_WARN, "raw: ", DUMP_PREFIX_NONE, 32,
 			sizeof(unsigned long), page,
 			sizeof(struct page), false);
 
-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH 2/5] mm: lower the printk loglevel for __dump_page messages

[Michal Hocko]

On Wed 12-12-18 15:25:40, Michal Hocko wrote:
> It seems one follow up fix got lost on my side. Andrew, could you fold
> this in please?
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 68e9a9f2df16..4c916e1abedc 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -88,7 +88,7 @@ void __dump_page(struct page *page, const char *reason)
>  	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
>  
>  hex_only:
> -	print_hex_dump(KERN_ALERT, "raw: ", DUMP_PREFIX_NONE, 32,
> +	print_hex_dump(KERN_WARN, "raw: ", DUMP_PREFIX_NONE, 32,
>  			sizeof(unsigned long), page,
>  			sizeof(struct page), false);

s@KERN_WARN@KERN_WARNING@ of course. Sigh. Sorry
-- 
Michal Hocko
SUSE Labs

[RFC PATCH 3/5] mm, memory_hotplug: drop pointless block alignment checks from __offline_pages

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

This function is never called from a context which would provide
misaligned pfn range so drop the pointless check.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory_hotplug.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2b2b3ccbbfb5..a92b1b8f6218 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1554,12 +1554,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	struct zone *zone;
 	struct memory_notify arg;
 
-	/* at least, alignment against pageblock is necessary */
-	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
-		return -EINVAL;
-	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
-		return -EINVAL;
-
 	mem_hotplug_begin();
 
 	/* This makes hotplug much easier...and readable.
-- 
2.19.1

[RFC PATCH 3/5] mm, memory_hotplug: drop pointless block alignment checks from __offline_pages

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

This function is never called from a context which would provide
misaligned pfn range so drop the pointless check.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory_hotplug.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2b2b3ccbbfb5..a92b1b8f6218 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1554,12 +1554,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	struct zone *zone;
 	struct memory_notify arg;
 
-	/* at least, alignment against pageblock is necessary */
-	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
-		return -EINVAL;
-	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
-		return -EINVAL;
-
 	mem_hotplug_begin();
 
 	/* This makes hotplug much easier...and readable.
-- 
2.19.1

[RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

The memory offlining failure reporting is inconsistent and insufficient.
Some error paths simply do not report the failure to the log at all.
When we do report there are no details about the reason of the failure
and there are several of them which makes memory offlining failures
hard to debug.

Make sure that the
	memory offlining [mem %#010llx-%#010llx] failed
message is printed for all failures and also provide a short textual
reason for the failure e.g.

[ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff

this tells us that the offlining has failed because of a signal pending
aka user intervention.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory_hotplug.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a92b1b8f6218..1badac89c58e 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1553,6 +1553,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	unsigned long valid_start, valid_end;
 	struct zone *zone;
 	struct memory_notify arg;
+	char *reason;
 
 	mem_hotplug_begin();
 
@@ -1561,7 +1562,9 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	if (!test_pages_in_a_zone(start_pfn, end_pfn, &valid_start,
 				  &valid_end)) {
 		mem_hotplug_done();
-		return -EINVAL;
+		ret = -EINVAL;
+		reason = "multizone range";
+		goto failed_removal;
 	}
 
 	zone = page_zone(pfn_to_page(valid_start));
@@ -1573,7 +1576,8 @@ static int __ref __offline_pages(unsigned long start_pfn,
 				       MIGRATE_MOVABLE, true);
 	if (ret) {
 		mem_hotplug_done();
-		return ret;
+		reason = "failed to isolate range";
+		goto failed_removal
 	}
 
 	arg.start_pfn = start_pfn;
@@ -1582,15 +1586,19 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
 	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
 	ret = notifier_to_errno(ret);
-	if (ret)
-		goto failed_removal;
+	if (ret) {
+		reason = "notifiers failure";
+		goto failed_removal_isolated;
+	}
 
 	pfn = start_pfn;
 repeat:
 	/* start memory hot removal */
 	ret = -EINTR;
-	if (signal_pending(current))
-		goto failed_removal;
+	if (signal_pending(current)) {
+		reason = "signal backoff";
+		goto failed_removal_isolated;
+	}
 
 	cond_resched();
 	lru_add_drain_all();
@@ -1607,8 +1615,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	 * actually in order to make hugetlbfs's object counting consistent.
 	 */
 	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
-	if (ret)
-		goto failed_removal;
+	if (ret) {
+		reason = "fails to disolve hugetlb pages";
+		goto failed_removal_isolated;
+	}
 	/* check again */
 	offlined_pages = check_pages_isolated(start_pfn, end_pfn);
 	if (offlined_pages < 0)
@@ -1648,13 +1658,15 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	mem_hotplug_done();
 	return 0;
 
+failed_removal_isolated:
+	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 failed_removal:
-	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
+	pr_debug("memory offlining [mem %#010llx-%#010llx] failed due to %s\n",
 		 (unsigned long long) start_pfn << PAGE_SHIFT,
-		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
+		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1,
+		 reason);
 	memory_notify(MEM_CANCEL_OFFLINE, &arg);
 	/* pushback to free area */
-	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 	mem_hotplug_done();
 	return ret;
 }
-- 
2.19.1

[RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

The memory offlining failure reporting is inconsistent and insufficient.
Some error paths simply do not report the failure to the log at all.
When we do report there are no details about the reason of the failure
and there are several of them which makes memory offlining failures
hard to debug.

Make sure that the
	memory offlining [mem %#010llx-%#010llx] failed
message is printed for all failures and also provide a short textual
reason for the failure e.g.

[ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff

this tells us that the offlining has failed because of a signal pending
aka user intervention.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory_hotplug.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a92b1b8f6218..1badac89c58e 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1553,6 +1553,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	unsigned long valid_start, valid_end;
 	struct zone *zone;
 	struct memory_notify arg;
+	char *reason;
 
 	mem_hotplug_begin();
 
@@ -1561,7 +1562,9 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	if (!test_pages_in_a_zone(start_pfn, end_pfn, &valid_start,
 				  &valid_end)) {
 		mem_hotplug_done();
-		return -EINVAL;
+		ret = -EINVAL;
+		reason = "multizone range";
+		goto failed_removal;
 	}
 
 	zone = page_zone(pfn_to_page(valid_start));
@@ -1573,7 +1576,8 @@ static int __ref __offline_pages(unsigned long start_pfn,
 				       MIGRATE_MOVABLE, true);
 	if (ret) {
 		mem_hotplug_done();
-		return ret;
+		reason = "failed to isolate range";
+		goto failed_removal
 	}
 
 	arg.start_pfn = start_pfn;
@@ -1582,15 +1586,19 @@ static int __ref __offline_pages(unsigned long start_pfn,
 
 	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
 	ret = notifier_to_errno(ret);
-	if (ret)
-		goto failed_removal;
+	if (ret) {
+		reason = "notifiers failure";
+		goto failed_removal_isolated;
+	}
 
 	pfn = start_pfn;
 repeat:
 	/* start memory hot removal */
 	ret = -EINTR;
-	if (signal_pending(current))
-		goto failed_removal;
+	if (signal_pending(current)) {
+		reason = "signal backoff";
+		goto failed_removal_isolated;
+	}
 
 	cond_resched();
 	lru_add_drain_all();
@@ -1607,8 +1615,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	 * actually in order to make hugetlbfs's object counting consistent.
 	 */
 	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
-	if (ret)
-		goto failed_removal;
+	if (ret) {
+		reason = "fails to disolve hugetlb pages";
+		goto failed_removal_isolated;
+	}
 	/* check again */
 	offlined_pages = check_pages_isolated(start_pfn, end_pfn);
 	if (offlined_pages < 0)
@@ -1648,13 +1658,15 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	mem_hotplug_done();
 	return 0;
 
+failed_removal_isolated:
+	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 failed_removal:
-	pr_debug("memory offlining [mem %#010llx-%#010llx] failed\n",
+	pr_debug("memory offlining [mem %#010llx-%#010llx] failed due to %s\n",
 		 (unsigned long long) start_pfn << PAGE_SHIFT,
-		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
+		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1,
+		 reason);
 	memory_notify(MEM_CANCEL_OFFLINE, &arg);
 	/* pushback to free area */
-	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 	mem_hotplug_done();
 	return ret;
 }
-- 
2.19.1

Re: [RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Andrew Morton]

On Wed,  7 Nov 2018 11:18:29 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> The memory offlining failure reporting is inconsistent and insufficient.
> Some error paths simply do not report the failure to the log at all.
> When we do report there are no details about the reason of the failure
> and there are several of them which makes memory offlining failures
> hard to debug.
> 
> Make sure that the
> 	memory offlining [mem %#010llx-%#010llx] failed
> message is printed for all failures and also provide a short textual
> reason for the failure e.g.
> 
> [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> 
> this tells us that the offlining has failed because of a signal pending
> aka user intervention.
> 
> ...

Some of these messages will come out looking a bit odd.

> @@ -1573,7 +1576,8 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  				       MIGRATE_MOVABLE, true);
>  	if (ret) {
>  		mem_hotplug_done();
> -		return ret;
> +		reason = "failed to isolate range";

"memory offlining [mem ...] failed due to failed to isolate range"

> +		goto failed_removal
>  	}
>  
>  	arg.start_pfn = start_pfn;
> @@ -1582,15 +1586,19 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  
>  	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
>  	ret = notifier_to_errno(ret);
> -	if (ret)
> -		goto failed_removal;
> +	if (ret) {
> +		reason = "notifiers failure";

"memory offlining [mem ...] failed due to notifiers failure"

> @@ -1607,8 +1615,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  	 * actually in order to make hugetlbfs's object counting consistent.
>  	 */
>  	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
> -	if (ret)
> -		goto failed_removal;
> +	if (ret) {
> +		reason = "fails to disolve hugetlb pages";

"memory offlining [mem ...] failed due to fails to disolve hugetlb pages"


Fix:

--- a/mm/memory_hotplug.c~mm-memory_hotplug-print-reason-for-the-offlining-failure-fix
+++ a/mm/memory_hotplug.c
@@ -1576,7 +1576,7 @@ static int __ref __offline_pages(unsigne
 				       MIGRATE_MOVABLE, true);
 	if (ret) {
 		mem_hotplug_done();
-		reason = "failed to isolate range";
+		reason = "failure to isolate range";
 		goto failed_removal
 	}
 
@@ -1587,7 +1587,7 @@ static int __ref __offline_pages(unsigne
 	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
 	ret = notifier_to_errno(ret);
 	if (ret) {
-		reason = "notifiers failure";
+		reason = "notifier failure";
 		goto failed_removal_isolated;
 	}
 
@@ -1616,7 +1616,7 @@ repeat:
 	 */
 	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
 	if (ret) {
-		reason = "fails to disolve hugetlb pages";
+		reason = "failure to dissolve huge pages";
 		goto failed_removal_isolated;
 	}
 	/* check again */
_

Re: [RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Andrew Morton]

On Wed,  7 Nov 2018 11:18:29 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> The memory offlining failure reporting is inconsistent and insufficient.
> Some error paths simply do not report the failure to the log at all.
> When we do report there are no details about the reason of the failure
> and there are several of them which makes memory offlining failures
> hard to debug.
> 
> Make sure that the
> 	memory offlining [mem %#010llx-%#010llx] failed
> message is printed for all failures and also provide a short textual
> reason for the failure e.g.
> 
> [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> 
> this tells us that the offlining has failed because of a signal pending
> aka user intervention.
> 
> ...

Some of these messages will come out looking a bit odd.

> @@ -1573,7 +1576,8 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  				       MIGRATE_MOVABLE, true);
>  	if (ret) {
>  		mem_hotplug_done();
> -		return ret;
> +		reason = "failed to isolate range";

"memory offlining [mem ...] failed due to failed to isolate range"

> +		goto failed_removal
>  	}
>  
>  	arg.start_pfn = start_pfn;
> @@ -1582,15 +1586,19 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  
>  	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
>  	ret = notifier_to_errno(ret);
> -	if (ret)
> -		goto failed_removal;
> +	if (ret) {
> +		reason = "notifiers failure";

"memory offlining [mem ...] failed due to notifiers failure"

> @@ -1607,8 +1615,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  	 * actually in order to make hugetlbfs's object counting consistent.
>  	 */
>  	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
> -	if (ret)
> -		goto failed_removal;
> +	if (ret) {
> +		reason = "fails to disolve hugetlb pages";

"memory offlining [mem ...] failed due to fails to disolve hugetlb pages"


Fix:

--- a/mm/memory_hotplug.c~mm-memory_hotplug-print-reason-for-the-offlining-failure-fix
+++ a/mm/memory_hotplug.c
@@ -1576,7 +1576,7 @@ static int __ref __offline_pages(unsigne
 				       MIGRATE_MOVABLE, true);
 	if (ret) {
 		mem_hotplug_done();
-		reason = "failed to isolate range";
+		reason = "failure to isolate range";
 		goto failed_removal
 	}
 
@@ -1587,7 +1587,7 @@ static int __ref __offline_pages(unsigne
 	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
 	ret = notifier_to_errno(ret);
 	if (ret) {
-		reason = "notifiers failure";
+		reason = "notifier failure";
 		goto failed_removal_isolated;
 	}
 
@@ -1616,7 +1616,7 @@ repeat:
 	 */
 	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
 	if (ret) {
-		reason = "fails to disolve hugetlb pages";
+		reason = "failure to dissolve huge pages";
 		goto failed_removal_isolated;
 	}
 	/* check again */
_

Re: [RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Michal Hocko]

On Wed 07-11-18 14:04:13, Andrew Morton wrote:
[...]
> Fix:
> 
> --- a/mm/memory_hotplug.c~mm-memory_hotplug-print-reason-for-the-offlining-failure-fix
> +++ a/mm/memory_hotplug.c
> @@ -1576,7 +1576,7 @@ static int __ref __offline_pages(unsigne
>  				       MIGRATE_MOVABLE, true);
>  	if (ret) {
>  		mem_hotplug_done();
> -		reason = "failed to isolate range";
> +		reason = "failure to isolate range";
>  		goto failed_removal
>  	}
>  
> @@ -1587,7 +1587,7 @@ static int __ref __offline_pages(unsigne
>  	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
>  	ret = notifier_to_errno(ret);
>  	if (ret) {
> -		reason = "notifiers failure";
> +		reason = "notifier failure";
>  		goto failed_removal_isolated;
>  	}
>  
> @@ -1616,7 +1616,7 @@ repeat:
>  	 */
>  	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
>  	if (ret) {
> -		reason = "fails to disolve hugetlb pages";
> +		reason = "failure to dissolve huge pages";
>  		goto failed_removal_isolated;
>  	}
>  	/* check again */
> _
> 

LGTM, thanks!

-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Michal Hocko]

On Wed 07-11-18 14:04:13, Andrew Morton wrote:
> On Wed,  7 Nov 2018 11:18:29 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > The memory offlining failure reporting is inconsistent and insufficient.
> > Some error paths simply do not report the failure to the log at all.
> > When we do report there are no details about the reason of the failure
> > and there are several of them which makes memory offlining failures
> > hard to debug.
> > 
> > Make sure that the
> > 	memory offlining [mem %#010llx-%#010llx] failed
> > message is printed for all failures and also provide a short textual
> > reason for the failure e.g.
> > 
> > [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> > 
> > this tells us that the offlining has failed because of a signal pending
> > aka user intervention.
> > 
> > ...
> 
> Some of these messages will come out looking a bit odd.
> 
> > @@ -1573,7 +1576,8 @@ static int __ref __offline_pages(unsigned long start_pfn,
> >  				       MIGRATE_MOVABLE, true);
> >  	if (ret) {
> >  		mem_hotplug_done();
> > -		return ret;
> > +		reason = "failed to isolate range";
> 
> "memory offlining [mem ...] failed due to failed to isolate range"
> 
> > +		goto failed_removal
> >  	}
> >  
> >  	arg.start_pfn = start_pfn;
> > @@ -1582,15 +1586,19 @@ static int __ref __offline_pages(unsigned long start_pfn,
> >  
> >  	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
> >  	ret = notifier_to_errno(ret);
> > -	if (ret)
> > -		goto failed_removal;
> > +	if (ret) {
> > +		reason = "notifiers failure";
> 
> "memory offlining [mem ...] failed due to notifiers failure"
> 
> > @@ -1607,8 +1615,10 @@ static int __ref __offline_pages(unsigned long start_pfn,
> >  	 * actually in order to make hugetlbfs's object counting consistent.
> >  	 */
> >  	ret = dissolve_free_huge_pages(start_pfn, end_pfn);
> > -	if (ret)
> > -		goto failed_removal;
> > +	if (ret) {
> > +		reason = "fails to disolve hugetlb pages";
> 
> "memory offlining [mem ...] failed due to fails to disolve hugetlb pages"
> 
> 
> Fix:
> 
> --- a/mm/memory_hotplug.c~mm-memory_hotplug-print-reason-for-the-offlining-failure-fix
> +++ a/mm/memory_hotplug.c
> @@ -1576,7 +1576,7 @@ static int __ref __offline_pages(unsigne
>  				       MIGRATE_MOVABLE, true);
>  	if (ret) {
>  		mem_hotplug_done();
> -		reason = "failed to isolate range";
> +		reason = "failure to isolate range";
>  		goto failed_removal
>  	}

0day has noticed the missing ; here.

Andrew, could you pick up the follow up fix please?


commit 614212af5c20126aea1edaceb78aa586e19802cf
Author: Michal Hocko <mhocko@suse.com>
Date:   Tue Nov 13 09:01:50 2018 +0100

    fold me "mm, memory_hotplug: print reason for the offlining failure"

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f5f1b2a27cb3..c82193db4be6 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1581,7 +1581,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
 	if (ret) {
 		mem_hotplug_done();
 		reason = "failure to isolate range";
-		goto failed_removal
+		goto failed_removal;
 	}
 
 	arg.start_pfn = start_pfn;
-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Anshuman Khandual]

On 11/07/2018 03:48 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> The memory offlining failure reporting is inconsistent and insufficient.
> Some error paths simply do not report the failure to the log at all.
> When we do report there are no details about the reason of the failure
> and there are several of them which makes memory offlining failures
> hard to debug.
> 
> Make sure that the
> 	memory offlining [mem %#010llx-%#010llx] failed
> message is printed for all failures and also provide a short textual
> reason for the failure e.g.
> 
> [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> 
> this tells us that the offlining has failed because of a signal pending
> aka user intervention.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

It might help to enumerate these failure reason strings and use macros.

Re: [RFC PATCH 4/5] mm, memory_hotplug: print reason for the offlining failure

[Michal Hocko]

On Thu 08-11-18 11:53:21, Anshuman Khandual wrote:
> 
> 
> On 11/07/2018 03:48 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > The memory offlining failure reporting is inconsistent and insufficient.
> > Some error paths simply do not report the failure to the log at all.
> > When we do report there are no details about the reason of the failure
> > and there are several of them which makes memory offlining failures
> > hard to debug.
> > 
> > Make sure that the
> > 	memory offlining [mem %#010llx-%#010llx] failed
> > message is printed for all failures and also provide a short textual
> > reason for the failure e.g.
> > 
> > [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> > 
> > this tells us that the offlining has failed because of a signal pending
> > aka user intervention.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> It might help to enumerate these failure reason strings and use macros.

Does it really make sense when all of them are on-off things? I would
agree if they were reused somewhere.

-- 
Michal Hocko
SUSE Labs

[RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

There is only very limited information printed when the memory offlining
fails:
[ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff

This tells us that the failure is triggered by the userspace
intervention but it doesn't tell us much more about the underlying
reason. It might be that the page migration failes repeatedly and the
userspace timeout expires and send a signal or it might be some of the
earlier steps (isolation, memory notifier) takes too long.

If the migration failes then it would be really helpful to see which
page that and its state. The same applies to the isolation phase. If we
fail to isolate a page from the allocator then knowing the state of the
page would be helpful as well.

Dump the page state that fails to get isolated or migrated. This will
tell us more about the failure and what to focus on during debugging.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory_hotplug.c | 12 ++++++++----
 mm/page_alloc.c     |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 1badac89c58e..bf214beccda3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1388,10 +1388,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 						    page_is_file_cache(page));
 
 		} else {
-#ifdef CONFIG_DEBUG_VM
-			pr_alert("failed to isolate pfn %lx\n", pfn);
+			pr_warn("failed to isolate pfn %lx\n", pfn);
 			dump_page(page, "isolation failed");
-#endif
 			put_page(page);
 			/* Because we don't have big zone->lock. we should
 			   check this again here. */
@@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		/* Allocate a new page from the nearest neighbor node */
 		ret = migrate_pages(&source, new_node_page, NULL, 0,
 					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
-		if (ret)
+		if (ret) {
+			list_for_each_entry(page, &source, lru) {
+				pr_warn("migrating pfn %lx failed ",
+				       page_to_pfn(page), ret);
+				dump_page(page, NULL);
+			}
 			putback_movable_pages(&source);
+		}
 	}
 out:
 	return ret;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a919ba5cb3c8..23267767bf98 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7845,6 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 	return false;
 unmovable:
 	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
+	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");
 	return true;
 }
 
-- 
2.19.1

[RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

There is only very limited information printed when the memory offlining
fails:
[ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff

This tells us that the failure is triggered by the userspace
intervention but it doesn't tell us much more about the underlying
reason. It might be that the page migration failes repeatedly and the
userspace timeout expires and send a signal or it might be some of the
earlier steps (isolation, memory notifier) takes too long.

If the migration failes then it would be really helpful to see which
page that and its state. The same applies to the isolation phase. If we
fail to isolate a page from the allocator then knowing the state of the
page would be helpful as well.

Dump the page state that fails to get isolated or migrated. This will
tell us more about the failure and what to focus on during debugging.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memory_hotplug.c | 12 ++++++++----
 mm/page_alloc.c     |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 1badac89c58e..bf214beccda3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1388,10 +1388,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 						    page_is_file_cache(page));
 
 		} else {
-#ifdef CONFIG_DEBUG_VM
-			pr_alert("failed to isolate pfn %lx\n", pfn);
+			pr_warn("failed to isolate pfn %lx\n", pfn);
 			dump_page(page, "isolation failed");
-#endif
 			put_page(page);
 			/* Because we don't have big zone->lock. we should
 			   check this again here. */
@@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		/* Allocate a new page from the nearest neighbor node */
 		ret = migrate_pages(&source, new_node_page, NULL, 0,
 					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
-		if (ret)
+		if (ret) {
+			list_for_each_entry(page, &source, lru) {
+				pr_warn("migrating pfn %lx failed ",
+				       page_to_pfn(page), ret);
+				dump_page(page, NULL);
+			}
 			putback_movable_pages(&source);
+		}
 	}
 out:
 	return ret;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a919ba5cb3c8..23267767bf98 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7845,6 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 	return false;
 unmovable:
 	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
+	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");
 	return true;
 }
 
-- 
2.19.1

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Anshuman Khandual]

On 11/07/2018 03:48 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> There is only very limited information printed when the memory offlining
> fails:
> [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> 
> This tells us that the failure is triggered by the userspace
> intervention but it doesn't tell us much more about the underlying
> reason. It might be that the page migration failes repeatedly and the
> userspace timeout expires and send a signal or it might be some of the
> earlier steps (isolation, memory notifier) takes too long.
> 
> If the migration failes then it would be really helpful to see which
> page that and its state. The same applies to the isolation phase. If we
> fail to isolate a page from the allocator then knowing the state of the
> page would be helpful as well.
> 
> Dump the page state that fails to get isolated or migrated. This will
> tell us more about the failure and what to focus on during debugging.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/memory_hotplug.c | 12 ++++++++----
>  mm/page_alloc.c     |  1 +
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 1badac89c58e..bf214beccda3 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1388,10 +1388,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  						    page_is_file_cache(page));
>  
>  		} else {
> -#ifdef CONFIG_DEBUG_VM
> -			pr_alert("failed to isolate pfn %lx\n", pfn);
> +			pr_warn("failed to isolate pfn %lx\n", pfn)>  			dump_page(page, "isolation failed");
> -#endif
>  			put_page(page);
>  			/* Because we don't have big zone->lock. we should
>  			   check this again here. */
> @@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		/* Allocate a new page from the nearest neighbor node */
>  		ret = migrate_pages(&source, new_node_page, NULL, 0,
>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
> -		if (ret)
> +		if (ret) {
> +			list_for_each_entry(page, &source, lru) {
> +				pr_warn("migrating pfn %lx failed ",
> +				       page_to_pfn(page), ret);

Seems like pr_warn() needs to have %d in here to print 'ret'. Though
dumping return code from migrate_pages() makes sense, wondering if
it is required for each and every page which failed to migrate here
or just one instance is enough.

> +				dump_page(page, NULL);
> +			}

s/NULL/failed to migrate/ for dump_page().

>  			putback_movable_pages(&source);
> +		}
>  	}
>  out:
>  	return ret;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a919ba5cb3c8..23267767bf98 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7845,6 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>  	return false;
>  unmovable:
>  	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
> +	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");

s/has_unmovable_pages/is unmovable/

If we eally care about the function name, then dump_page() should be
followed by dump_stack() like the case in some other instances.

>  	return true;

This will be dumped from HugeTLB and CMA allocation paths as well through
alloc_contig_range(). But it should be okay as those occurrences should be
rare and dumping page state then will also help.

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Michal Hocko]

On Thu 08-11-18 12:46:47, Anshuman Khandual wrote:
> 
> 
> On 11/07/2018 03:48 PM, Michal Hocko wrote:
[...]
> > @@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  		/* Allocate a new page from the nearest neighbor node */
> >  		ret = migrate_pages(&source, new_node_page, NULL, 0,
> >  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
> > -		if (ret)
> > +		if (ret) {
> > +			list_for_each_entry(page, &source, lru) {
> > +				pr_warn("migrating pfn %lx failed ",
> > +				       page_to_pfn(page), ret);
> 
> Seems like pr_warn() needs to have %d in here to print 'ret'.

Dohh. Rebase hickup. You are right ret:%d got lost on the way.

> Though
> dumping return code from migrate_pages() makes sense, wondering if
> it is required for each and every page which failed to migrate here
> or just one instance is enough.

Does it matter enough to special case one printk?

> > +				dump_page(page, NULL);
> > +			}
> 
> s/NULL/failed to migrate/ for dump_page().

Yes, makes sense.

> 
> >  			putback_movable_pages(&source);
> > +		}
> >  	}
> >  out:
> >  	return ret;
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index a919ba5cb3c8..23267767bf98 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -7845,6 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> >  	return false;
> >  unmovable:
> >  	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
> > +	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");
> 
> s/has_unmovable_pages/is unmovable/

OK

> If we eally care about the function name, then dump_page() should be
> followed by dump_stack() like the case in some other instances.
>
> >  	return true;
> 
> This will be dumped from HugeTLB and CMA allocation paths as well through
> alloc_contig_range(). But it should be okay as those occurrences should be
> rare and dumping page state then will also help.

yes

Thanks and here is the incremental fix:

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index bf214beccda3..820397e18e59 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1411,9 +1411,9 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
 		if (ret) {
 			list_for_each_entry(page, &source, lru) {
-				pr_warn("migrating pfn %lx failed ",
+				pr_warn("migrating pfn %lx failed ret:%d ",
 				       page_to_pfn(page), ret);
-				dump_page(page, NULL);
+				dump_page(page, "migration failure");
 			}
 			putback_movable_pages(&source);
 		}
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 23267767bf98..ec2c7916dc2d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7845,7 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
 	return false;
 unmovable:
 	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
-	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");
+	dump_page(pfn_to_page(pfn+iter), "unmovable page");
 	return true;
 }
 
-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Anshuman Khandual]

On 11/08/2018 01:42 PM, Michal Hocko wrote:
> On Thu 08-11-18 12:46:47, Anshuman Khandual wrote:
>>
>>
>> On 11/07/2018 03:48 PM, Michal Hocko wrote:
> [...]
>>> @@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>>  		/* Allocate a new page from the nearest neighbor node */
>>>  		ret = migrate_pages(&source, new_node_page, NULL, 0,
>>>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
>>> -		if (ret)
>>> +		if (ret) {
>>> +			list_for_each_entry(page, &source, lru) {
>>> +				pr_warn("migrating pfn %lx failed ",
>>> +				       page_to_pfn(page), ret);
>>
>> Seems like pr_warn() needs to have %d in here to print 'ret'.
> 
> Dohh. Rebase hickup. You are right ret:%d got lost on the way.
> 
>> Though
>> dumping return code from migrate_pages() makes sense, wondering if
>> it is required for each and every page which failed to migrate here
>> or just one instance is enough.
> 
> Does it matter enough to special case one printk?
I just imagined how a bunch of prints will look like when multiple pages
failed to migrate probably for the same reason. But I guess its okay.

> 
>>> +				dump_page(page, NULL);
>>> +			}
>>
>> s/NULL/failed to migrate/ for dump_page().
> 
> Yes, makes sense.
> 
>>
>>>  			putback_movable_pages(&source);
>>> +		}
>>>  	}
>>>  out:
>>>  	return ret;
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index a919ba5cb3c8..23267767bf98 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -7845,6 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>>>  	return false;
>>>  unmovable:
>>>  	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
>>> +	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");
>>
>> s/has_unmovable_pages/is unmovable/
> 
> OK
> 
>> If we eally care about the function name, then dump_page() should be
>> followed by dump_stack() like the case in some other instances.
>>
>>>  	return true;
>>
>> This will be dumped from HugeTLB and CMA allocation paths as well through
>> alloc_contig_range(). But it should be okay as those occurrences should be
>> rare and dumping page state then will also help.
> 
> yes
> 
> Thanks and here is the incremental fix:
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index bf214beccda3..820397e18e59 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1411,9 +1411,9 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
>  		if (ret) {
>  			list_for_each_entry(page, &source, lru) {
> -				pr_warn("migrating pfn %lx failed ",
> +				pr_warn("migrating pfn %lx failed ret:%d ",
>  				       page_to_pfn(page), ret);
> -				dump_page(page, NULL);
> +				dump_page(page, "migration failure");
>  			}
>  			putback_movable_pages(&source);
>  		}
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 23267767bf98..ec2c7916dc2d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7845,7 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>  	return false;
>  unmovable:
>  	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
> -	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");
> +	dump_page(pfn_to_page(pfn+iter), "unmovable page");
>  	return true;
>  }

It looks good.

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Michal Hocko]

Andrew, could you pick up this one as well please? Let me know if you
prefer me to send the whole pile with all the fixes again.

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index bf214beccda3..820397e18e59 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1411,9 +1411,9 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
>  		if (ret) {
>  			list_for_each_entry(page, &source, lru) {
> -				pr_warn("migrating pfn %lx failed ",
> +				pr_warn("migrating pfn %lx failed ret:%d ",
>  				       page_to_pfn(page), ret);
> -				dump_page(page, NULL);
> +				dump_page(page, "migration failure");
>  			}
>  			putback_movable_pages(&source);
>  		}
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 23267767bf98..ec2c7916dc2d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7845,7 +7845,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>  	return false;
>  unmovable:
>  	WARN_ON_ONCE(zone_idx(zone) == ZONE_MOVABLE);
> -	dump_page(pfn_to_page(pfn+iter), "has_unmovable_pages");
> +	dump_page(pfn_to_page(pfn+iter), "unmovable page");
>  	return true;
>  }
>  
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Andrew Morton]

On Wed,  7 Nov 2018 11:18:30 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> There is only very limited information printed when the memory offlining
> fails:
> [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> 
> This tells us that the failure is triggered by the userspace
> intervention but it doesn't tell us much more about the underlying
> reason. It might be that the page migration failes repeatedly and the
> userspace timeout expires and send a signal or it might be some of the
> earlier steps (isolation, memory notifier) takes too long.
> 
> If the migration failes then it would be really helpful to see which
> page that and its state. The same applies to the isolation phase. If we
> fail to isolate a page from the allocator then knowing the state of the
> page would be helpful as well.
> 
> Dump the page state that fails to get isolated or migrated. This will
> tell us more about the failure and what to focus on during debugging.
> 
> ...
>
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1388,10 +1388,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  						    page_is_file_cache(page));
>  
>  		} else {
> -#ifdef CONFIG_DEBUG_VM
> -			pr_alert("failed to isolate pfn %lx\n", pfn);
> +			pr_warn("failed to isolate pfn %lx\n", pfn);
>  			dump_page(page, "isolation failed");
> -#endif
>  			put_page(page);
>  			/* Because we don't have big zone->lock. we should
>  			   check this again here. */
> @@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		/* Allocate a new page from the nearest neighbor node */
>  		ret = migrate_pages(&source, new_node_page, NULL, 0,
>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
> -		if (ret)
> +		if (ret) {
> +			list_for_each_entry(page, &source, lru) {
> +				pr_warn("migrating pfn %lx failed ",
> +				       page_to_pfn(page), ret);
> +				dump_page(page, NULL);
> +			}

./include/linux/kern_levels.h:5:18: warning: too many arguments for format [-Wformat-extra-args]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^
./include/linux/kern_levels.h:12:22: note: in expansion of macro ‘KERN_SOH’
 #define KERN_WARNING KERN_SOH "4" /* warning conditions */
                      ^~~~~~~~
./include/linux/printk.h:310:9: note: in expansion of macro ‘KERN_WARNING’
  printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
         ^~~~~~~~~~~~
./include/linux/printk.h:311:17: note: in expansion of macro ‘pr_warning’
 #define pr_warn pr_warning
                 ^~~~~~~~~~
mm/memory_hotplug.c:1414:5: note: in expansion of macro ‘pr_warn’
     pr_warn("migrating pfn %lx failed ",
     ^~~~~~~

--- a/mm/memory_hotplug.c~mm-memory_hotplug-be-more-verbose-for-memory-offline-failures-fix
+++ a/mm/memory_hotplug.c
@@ -1411,7 +1411,7 @@ do_migrate_range(unsigned long start_pfn
 					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
 		if (ret) {
 			list_for_each_entry(page, &source, lru) {
-				pr_warn("migrating pfn %lx failed ",
+				pr_warn("migrating pfn %lx failed: %d",
 				       page_to_pfn(page), ret);
 				dump_page(page, NULL);
 			}

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Andrew Morton]

On Wed,  7 Nov 2018 11:18:30 +0100 Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> There is only very limited information printed when the memory offlining
> fails:
> [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> 
> This tells us that the failure is triggered by the userspace
> intervention but it doesn't tell us much more about the underlying
> reason. It might be that the page migration failes repeatedly and the
> userspace timeout expires and send a signal or it might be some of the
> earlier steps (isolation, memory notifier) takes too long.
> 
> If the migration failes then it would be really helpful to see which
> page that and its state. The same applies to the isolation phase. If we
> fail to isolate a page from the allocator then knowing the state of the
> page would be helpful as well.
> 
> Dump the page state that fails to get isolated or migrated. This will
> tell us more about the failure and what to focus on during debugging.
> 
> ...
>
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1388,10 +1388,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  						    page_is_file_cache(page));
>  
>  		} else {
> -#ifdef CONFIG_DEBUG_VM
> -			pr_alert("failed to isolate pfn %lx\n", pfn);
> +			pr_warn("failed to isolate pfn %lx\n", pfn);
>  			dump_page(page, "isolation failed");
> -#endif
>  			put_page(page);
>  			/* Because we don't have big zone->lock. we should
>  			   check this again here. */
> @@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  		/* Allocate a new page from the nearest neighbor node */
>  		ret = migrate_pages(&source, new_node_page, NULL, 0,
>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
> -		if (ret)
> +		if (ret) {
> +			list_for_each_entry(page, &source, lru) {
> +				pr_warn("migrating pfn %lx failed ",
> +				       page_to_pfn(page), ret);
> +				dump_page(page, NULL);
> +			}

./include/linux/kern_levels.h:5:18: warning: too many arguments for format [-Wformat-extra-args]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
                  ^
./include/linux/kern_levels.h:12:22: note: in expansion of macro a??KERN_SOHa??
 #define KERN_WARNING KERN_SOH "4" /* warning conditions */
                      ^~~~~~~~
./include/linux/printk.h:310:9: note: in expansion of macro a??KERN_WARNINGa??
  printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
         ^~~~~~~~~~~~
./include/linux/printk.h:311:17: note: in expansion of macro a??pr_warninga??
 #define pr_warn pr_warning
                 ^~~~~~~~~~
mm/memory_hotplug.c:1414:5: note: in expansion of macro a??pr_warna??
     pr_warn("migrating pfn %lx failed ",
     ^~~~~~~

--- a/mm/memory_hotplug.c~mm-memory_hotplug-be-more-verbose-for-memory-offline-failures-fix
+++ a/mm/memory_hotplug.c
@@ -1411,7 +1411,7 @@ do_migrate_range(unsigned long start_pfn
 					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
 		if (ret) {
 			list_for_each_entry(page, &source, lru) {
-				pr_warn("migrating pfn %lx failed ",
+				pr_warn("migrating pfn %lx failed: %d",
 				       page_to_pfn(page), ret);
 				dump_page(page, NULL);
 			}

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Michal Hocko]

On Thu 15-11-18 16:07:16, Andrew Morton wrote:
> On Wed,  7 Nov 2018 11:18:30 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > There is only very limited information printed when the memory offlining
> > fails:
> > [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> > 
> > This tells us that the failure is triggered by the userspace
> > intervention but it doesn't tell us much more about the underlying
> > reason. It might be that the page migration failes repeatedly and the
> > userspace timeout expires and send a signal or it might be some of the
> > earlier steps (isolation, memory notifier) takes too long.
> > 
> > If the migration failes then it would be really helpful to see which
> > page that and its state. The same applies to the isolation phase. If we
> > fail to isolate a page from the allocator then knowing the state of the
> > page would be helpful as well.
> > 
> > Dump the page state that fails to get isolated or migrated. This will
> > tell us more about the failure and what to focus on during debugging.
> > 
> > ...
> >
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1388,10 +1388,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  						    page_is_file_cache(page));
> >  
> >  		} else {
> > -#ifdef CONFIG_DEBUG_VM
> > -			pr_alert("failed to isolate pfn %lx\n", pfn);
> > +			pr_warn("failed to isolate pfn %lx\n", pfn);
> >  			dump_page(page, "isolation failed");
> > -#endif
> >  			put_page(page);
> >  			/* Because we don't have big zone->lock. we should
> >  			   check this again here. */
> > @@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  		/* Allocate a new page from the nearest neighbor node */
> >  		ret = migrate_pages(&source, new_node_page, NULL, 0,
> >  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
> > -		if (ret)
> > +		if (ret) {
> > +			list_for_each_entry(page, &source, lru) {
> > +				pr_warn("migrating pfn %lx failed ",
> > +				       page_to_pfn(page), ret);
> > +				dump_page(page, NULL);
> > +			}
> 
> ./include/linux/kern_levels.h:5:18: warning: too many arguments for format [-Wformat-extra-args]
>  #define KERN_SOH "\001"  /* ASCII Start Of Header */
>                   ^
> ./include/linux/kern_levels.h:12:22: note: in expansion of macro ‘KERN_SOH’
>  #define KERN_WARNING KERN_SOH "4" /* warning conditions */
>                       ^~~~~~~~
> ./include/linux/printk.h:310:9: note: in expansion of macro ‘KERN_WARNING’
>   printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>          ^~~~~~~~~~~~
> ./include/linux/printk.h:311:17: note: in expansion of macro ‘pr_warning’
>  #define pr_warn pr_warning
>                  ^~~~~~~~~~
> mm/memory_hotplug.c:1414:5: note: in expansion of macro ‘pr_warn’
>      pr_warn("migrating pfn %lx failed ",
>      ^~~~~~~

yeah, 0day already complained and I've posted a follow up fix
http://lkml.kernel.org/r/20181108081231.GN27423@dhcp22.suse.cz

Let me post a version 2 with all the fixups.
 
Thanks!

> --- a/mm/memory_hotplug.c~mm-memory_hotplug-be-more-verbose-for-memory-offline-failures-fix
> +++ a/mm/memory_hotplug.c
> @@ -1411,7 +1411,7 @@ do_migrate_range(unsigned long start_pfn
>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
>  		if (ret) {
>  			list_for_each_entry(page, &source, lru) {
> -				pr_warn("migrating pfn %lx failed ",
> +				pr_warn("migrating pfn %lx failed: %d",
>  				       page_to_pfn(page), ret);
>  				dump_page(page, NULL);
>  			}
> 

-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH 5/5] mm, memory_hotplug: be more verbose for memory offline failures

[Michal Hocko]

On Thu 15-11-18 16:07:16, Andrew Morton wrote:
> On Wed,  7 Nov 2018 11:18:30 +0100 Michal Hocko <mhocko@kernel.org> wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > There is only very limited information printed when the memory offlining
> > fails:
> > [ 1984.506184] rac1 kernel: memory offlining [mem 0x82600000000-0x8267fffffff] failed due to signal backoff
> > 
> > This tells us that the failure is triggered by the userspace
> > intervention but it doesn't tell us much more about the underlying
> > reason. It might be that the page migration failes repeatedly and the
> > userspace timeout expires and send a signal or it might be some of the
> > earlier steps (isolation, memory notifier) takes too long.
> > 
> > If the migration failes then it would be really helpful to see which
> > page that and its state. The same applies to the isolation phase. If we
> > fail to isolate a page from the allocator then knowing the state of the
> > page would be helpful as well.
> > 
> > Dump the page state that fails to get isolated or migrated. This will
> > tell us more about the failure and what to focus on during debugging.
> > 
> > ...
> >
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1388,10 +1388,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  						    page_is_file_cache(page));
> >  
> >  		} else {
> > -#ifdef CONFIG_DEBUG_VM
> > -			pr_alert("failed to isolate pfn %lx\n", pfn);
> > +			pr_warn("failed to isolate pfn %lx\n", pfn);
> >  			dump_page(page, "isolation failed");
> > -#endif
> >  			put_page(page);
> >  			/* Because we don't have big zone->lock. we should
> >  			   check this again here. */
> > @@ -1411,8 +1409,14 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >  		/* Allocate a new page from the nearest neighbor node */
> >  		ret = migrate_pages(&source, new_node_page, NULL, 0,
> >  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
> > -		if (ret)
> > +		if (ret) {
> > +			list_for_each_entry(page, &source, lru) {
> > +				pr_warn("migrating pfn %lx failed ",
> > +				       page_to_pfn(page), ret);
> > +				dump_page(page, NULL);
> > +			}
> 
> ./include/linux/kern_levels.h:5:18: warning: too many arguments for format [-Wformat-extra-args]
>  #define KERN_SOH "\001"  /* ASCII Start Of Header */
>                   ^
> ./include/linux/kern_levels.h:12:22: note: in expansion of macro a??KERN_SOHa??
>  #define KERN_WARNING KERN_SOH "4" /* warning conditions */
>                       ^~~~~~~~
> ./include/linux/printk.h:310:9: note: in expansion of macro a??KERN_WARNINGa??
>   printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>          ^~~~~~~~~~~~
> ./include/linux/printk.h:311:17: note: in expansion of macro a??pr_warninga??
>  #define pr_warn pr_warning
>                  ^~~~~~~~~~
> mm/memory_hotplug.c:1414:5: note: in expansion of macro a??pr_warna??
>      pr_warn("migrating pfn %lx failed ",
>      ^~~~~~~

yeah, 0day already complained and I've posted a follow up fix
http://lkml.kernel.org/r/20181108081231.GN27423@dhcp22.suse.cz

Let me post a version 2 with all the fixups.
 
Thanks!

> --- a/mm/memory_hotplug.c~mm-memory_hotplug-be-more-verbose-for-memory-offline-failures-fix
> +++ a/mm/memory_hotplug.c
> @@ -1411,7 +1411,7 @@ do_migrate_range(unsigned long start_pfn
>  					MIGRATE_SYNC, MR_MEMORY_HOTPLUG);
>  		if (ret) {
>  			list_for_each_entry(page, &source, lru) {
> -				pr_warn("migrating pfn %lx failed ",
> +				pr_warn("migrating pfn %lx failed: %d",
>  				       page_to_pfn(page), ret);
>  				dump_page(page, NULL);
>  			}
> 

-- 
Michal Hocko
SUSE Labs