Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1341630 > unrolled thread

[PATCH] mm, memory hotplug: print more failure information for online_pages

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm, memory hotplug: print more failure information for online_pages Chen Yucong <slaoub@gmail.com> - 2016-02-24 09:10 +0100
    Re: [PATCH] mm, memory hotplug: print more failure information for  online_pages David Rientjes <rientjes@google.com> - 2016-02-24 22:40 +0100
      Re: [PATCH] mm, memory hotplug: print more failure information for  online_pages Chen Yucong <slaoub@gmail.com> - 2016-02-25 02:10 +0100
        Re: [PATCH] mm, memory hotplug: print more failure information for  online_pages David Rientjes <rientjes@google.com> - 2016-02-25 02:50 +0100

#1341630 — [PATCH] mm, memory hotplug: print more failure information for online_pages

online_pages() simply returns an error value if
memory_notify(MEM_GOING_ONLINE, &arg) return a value that is not
what we want for successfully onlining target pages. This patch
arms to print more failure information like offline_pages() in
online_pages. And this patch also converts printk(KERN_<LEVEL>)
to pr_<level>().

Signed-off-by: Chen Yucong <slaoub@gmail.com>
---
 mm/memory_hotplug.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c832ef3..e4b6dec3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1059,10 +1059,9 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 
 	ret = memory_notify(MEM_GOING_ONLINE, &arg);
 	ret = notifier_to_errno(ret);
-	if (ret) {
-		memory_notify(MEM_CANCEL_ONLINE, &arg);
-		return ret;
-	}
+	if (ret)
+		goto failed_addition;
+
 	/*
 	 * If this zone is not populated, then it is not in zonelist.
 	 * This means the page allocator ignores this zone.
@@ -1080,12 +1079,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 		if (need_zonelists_rebuild)
 			zone_pcp_reset(zone);
 		mutex_unlock(&zonelists_mutex);
-		printk(KERN_DEBUG "online_pages [mem %#010llx-%#010llx] failed\n",
-		       (unsigned long long) pfn << PAGE_SHIFT,
-		       (((unsigned long long) pfn + nr_pages)
-			    << PAGE_SHIFT) - 1);
-		memory_notify(MEM_CANCEL_ONLINE, &arg);
-		return ret;
+		goto failed_addition;
 	}
 
 	zone->present_pages += onlined_pages;
@@ -1118,6 +1112,13 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	if (onlined_pages)
 		memory_notify(MEM_ONLINE, &arg);
 	return 0;
+
+failed_addition:
+	pr_info("online_pages [mem %#010llx-%#010llx] failed\n",
+		(unsigned long long) pfn << PAGE_SHIFT,
+		(((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
+	memory_notify(MEM_CANCEL_ONLINE, &arg);
+	return ret;
 }
 #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
 
@@ -1529,8 +1530,7 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 
 		} else {
 #ifdef CONFIG_DEBUG_VM
-			printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
-			       pfn);
+			pr_alert("removing pfn %lx from LRU failed\n", pfn);
 			dump_page(page, "failed to remove from LRU");
 #endif
 			put_page(page);
@@ -1858,7 +1858,7 @@ repeat:
 		ret = -EBUSY;
 		goto failed_removal;
 	}
-	printk(KERN_INFO "Offlined Pages %ld\n", offlined_pages);
+	pr_info("Offlined Pages %ld\n", offlined_pages);
 	/* Ok, all of our target is isolated.
 	   We cannot do rollback at this point. */
 	offline_isolated_pages(start_pfn, end_pfn);
@@ -1895,9 +1895,9 @@ repeat:
 	return 0;
 
 failed_removal:
-	printk(KERN_INFO "memory offlining [mem %#010llx-%#010llx] failed\n",
-	       (unsigned long long) start_pfn << PAGE_SHIFT,
-	       ((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
+	pr_info("memory offlining [mem %#010llx-%#010llx] failed\n",
+		(unsigned long long) start_pfn << PAGE_SHIFT,
+		((unsigned long long) end_pfn << PAGE_SHIFT) - 1);
 	memory_notify(MEM_CANCEL_OFFLINE, &arg);
 	/* pushback to free area */
 	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1342454 — Re: [PATCH] mm, memory hotplug: print more failure information for online_pages

On Wed, 24 Feb 2016, Chen Yucong wrote:

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c832ef3..e4b6dec3 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1059,10 +1059,9 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  
>  	ret = memory_notify(MEM_GOING_ONLINE, &arg);
>  	ret = notifier_to_errno(ret);
> -	if (ret) {
> -		memory_notify(MEM_CANCEL_ONLINE, &arg);
> -		return ret;
> -	}
> +	if (ret)
> +		goto failed_addition;
> +
>  	/*
>  	 * If this zone is not populated, then it is not in zonelist.
>  	 * This means the page allocator ignores this zone.
> @@ -1080,12 +1079,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  		if (need_zonelists_rebuild)
>  			zone_pcp_reset(zone);
>  		mutex_unlock(&zonelists_mutex);
> -		printk(KERN_DEBUG "online_pages [mem %#010llx-%#010llx] failed\n",
> -		       (unsigned long long) pfn << PAGE_SHIFT,
> -		       (((unsigned long long) pfn + nr_pages)
> -			    << PAGE_SHIFT) - 1);
> -		memory_notify(MEM_CANCEL_ONLINE, &arg);
> -		return ret;
> +		goto failed_addition;
>  	}
>  
>  	zone->present_pages += onlined_pages;
> @@ -1118,6 +1112,13 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  	if (onlined_pages)
>  		memory_notify(MEM_ONLINE, &arg);
>  	return 0;
> +
> +failed_addition:
> +	pr_info("online_pages [mem %#010llx-%#010llx] failed\n",
> +		(unsigned long long) pfn << PAGE_SHIFT,
> +		(((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
> +	memory_notify(MEM_CANCEL_ONLINE, &arg);
> +	return ret;
>  }
>  #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
>  

Please explain how the conversion from KERN_DEBUG to KERN_INFO level is 
better?

If the onlining returns an error value, which it will, why do we need to 
leave an artifact behind in the kernel log that it failed?

[toc] | [prev] | [next] | [standalone]

#1342557 — Re: [PATCH] mm, memory hotplug: print more failure information for online_pages

On Wed, 2016-02-24 at 13:33 -0800, David Rientjes wrote:
> On Wed, 24 Feb 2016, Chen Yucong wrote:
> 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index c832ef3..e4b6dec3 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1059,10 +1059,9 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
> >  
> >  	ret = memory_notify(MEM_GOING_ONLINE, &arg);
> >  	ret = notifier_to_errno(ret);
> > -	if (ret) {
> > -		memory_notify(MEM_CANCEL_ONLINE, &arg);
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		goto failed_addition;
> > +
> >  	/*
> >  	 * If this zone is not populated, then it is not in zonelist.
> >  	 * This means the page allocator ignores this zone.
> > @@ -1080,12 +1079,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
> >  		if (need_zonelists_rebuild)
> >  			zone_pcp_reset(zone);
> >  		mutex_unlock(&zonelists_mutex);
> > -		printk(KERN_DEBUG "online_pages [mem %#010llx-%#010llx] failed\n",
> > -		       (unsigned long long) pfn << PAGE_SHIFT,
> > -		       (((unsigned long long) pfn + nr_pages)
> > -			    << PAGE_SHIFT) - 1);
> > -		memory_notify(MEM_CANCEL_ONLINE, &arg);
> > -		return ret;
> > +		goto failed_addition;
> >  	}
> >  
> >  	zone->present_pages += onlined_pages;
> > @@ -1118,6 +1112,13 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
> >  	if (onlined_pages)
> >  		memory_notify(MEM_ONLINE, &arg);
> >  	return 0;
> > +
> > +failed_addition:
> > +	pr_info("online_pages [mem %#010llx-%#010llx] failed\n",
> > +		(unsigned long long) pfn << PAGE_SHIFT,
> > +		(((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
> > +	memory_notify(MEM_CANCEL_ONLINE, &arg);
> > +	return ret;
> >  }
> >  #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
> >  
> 
> Please explain how the conversion from KERN_DEBUG to KERN_INFO level is 
> better?

Like __offline_pages(), printk() in online_pages() is used for reporting
an failed addition rather than debug information.
Another reason is that pr_debug() is not an exact equivalent of 
printk(KERN_DEBUG ...)

/* If you are writing a driver, please use dev_dbg instead */
#if defined(CONFIG_DYNAMIC_DEBUG)
/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here
*/
#define pr_debug(fmt, ...) \
        dynamic_pr_debug(fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define pr_debug(fmt, ...) \
        printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#else
#define pr_debug(fmt, ...) \
        no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#endif
 

> If the onlining returns an error value, which it will, why do we need to 
> leave an artifact behind in the kernel log that it failed?

In __offline_pages(), we can find the following snippet:

...
        ret = memory_notify(MEM_GOING_OFFLINE, &arg);
        ret = notifier_to_errno(ret);
        if (ret)
                goto failed_removal;
...
        offlined_pages = check_pages_isolated(start_pfn, end_pfn);
        if (offlined_pages < 0) {
                ret = -EBUSY;
                goto failed_removal;
        }
...
failed_removal:
        printk(KERN_INFO "memory offlining [mem %#010llx-%#010llx] 
...

Similarly, there's no single cause for failed online_pages operation.
So if memory_notify(MEM_GOING_ONLINE, &arg) returns an error
value, the result of online_pages is also ""online_pages [mem %#010llx-%
#010llx] failed\n".

thx!
    cyc

[toc] | [prev] | [next] | [standalone]

#1342574 — Re: [PATCH] mm, memory hotplug: print more failure information for online_pages

On Thu, 25 Feb 2016, Chen Yucong wrote:

> > Please explain how the conversion from KERN_DEBUG to KERN_INFO level is 
> > better?
> 
> Like __offline_pages(), printk() in online_pages() is used for reporting
> an failed addition rather than debug information.
> Another reason is that pr_debug() is not an exact equivalent of 
> printk(KERN_DEBUG ...)
> 
> /* If you are writing a driver, please use dev_dbg instead */
> #if defined(CONFIG_DYNAMIC_DEBUG)
> /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here
> */
> #define pr_debug(fmt, ...) \
>         dynamic_pr_debug(fmt, ##__VA_ARGS__)
> #elif defined(DEBUG)
> #define pr_debug(fmt, ...) \
>         printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #else
> #define pr_debug(fmt, ...) \
>         no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #endif
> 

My question is why in either __offline_pages() (today's code) or 
__online_pages() (your patch) we would want to leave behind a message in 
the kernel log to indicate failure?  I don't think it's helpful to spam 
the kernel log with unnecessary information when onlining or offlining 
failed.  Userspace already knows the range that it attempted to online or 
offline, it already has the correct error value, why spam it?

A patch to move __offline_pages() to not print this with KERN_INFO would 
make more sense.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web