diff mbox series

[v5,01/12] mm: dump_page(): better diagnostics for compound pages

Message ID 20200207033735.308000-2-jhubbard@nvidia.com (mailing list archive)
State New
Headers show
Series mm/gup: track FOLL_PIN pages | expand

Commit Message

John Hubbard Feb. 7, 2020, 3:37 a.m. UTC 
A compound page collects the refcount in the head page, while leaving
the refcount of each tail page at zero. Therefore, when debugging a
problem that involves compound pages, it's best to have diagnostics that
reflect that situation. However, dump_page() is oblivious to these
points.

Change dump_page() as follows:

1) For tail pages, print relevant head page information: refcount, in
   particular. But only do this if the page is not corrupted so badly
   that the pointer to the head page is all wrong.

2) Do a separate check to catch any (rare) cases of the tail page's
   refcount being non-zero, and issue a separate, clear pr_warn() if
   that ever happens.

Suggested-by: Matthew Wilcox <willy@infradead.org>
Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/debug.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

Comments

Matthew Wilcox (Oracle) Feb. 7, 2020, 5:27 p.m. UTC | #1 
On Thu, Feb 06, 2020 at 07:37:24PM -0800, John Hubbard wrote:
> A compound page collects the refcount in the head page, while leaving
> the refcount of each tail page at zero. Therefore, when debugging a
> problem that involves compound pages, it's best to have diagnostics that
> reflect that situation. However, dump_page() is oblivious to these
> points.
> 
> Change dump_page() as follows:
> 
> 1) For tail pages, print relevant head page information: refcount, in
>    particular. But only do this if the page is not corrupted so badly
>    that the pointer to the head page is all wrong.
> 
> 2) Do a separate check to catch any (rare) cases of the tail page's
>    refcount being non-zero, and issue a separate, clear pr_warn() if
>    that ever happens.
> 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  mm/debug.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index ecccd9f17801..f074077eee11 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -42,6 +42,33 @@ const struct trace_print_flags vmaflag_names[] = {
>  	{0, NULL}
>  };
>  
> +static void __dump_tail_page(struct page *page, int mapcount)
> +{
> +	struct page *head = compound_head(page);
> +
> +	if ((page < head) || (page >= head + MAX_ORDER_NR_PAGES)) {
> +		/*
> +		 * Page is hopelessly corrupted, so limit any reporting to
> +		 * information about the page itself. Do not attempt to look at
> +		 * the head page.
> +		 */
> +		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
> +			"index:%#lx (corrupted tail page case)\n",
> +			page, page_ref_count(page), mapcount, page->mapping,
> +			page_to_pgoff(page));
> +	} else {
> +		pr_warn("page:%px compound refcount:%d mapcount:%d mapping:%px "
> +			"index:%#lx compound_mapcount:%d\n",
> +			page, page_ref_count(head), mapcount, head->mapping,
> +			page_to_pgoff(head), compound_mapcount(page));
> +	}
> +
> +	if (page_ref_count(page) != 0) {
> +		pr_warn("page:%px PROBLEM: non-zero refcount (==%d) on this "
> +			"tail page\n", page, page_ref_count(page));
> +	}
> +}
> +
>  void __dump_page(struct page *page, const char *reason)
>  {
>  	struct address_space *mapping;
> @@ -75,12 +102,8 @@ void __dump_page(struct page *page, const char *reason)
>  	 */
>  	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
>  
> -	if (PageCompound(page))
> -		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
> -			"index:%#lx compound_mapcount: %d\n",
> -			page, page_ref_count(page), mapcount,
> -			page->mapping, page_to_pgoff(page),
> -			compound_mapcount(page));
> +	if (PageTail(page))
> +		__dump_tail_page(page, mapcount);
>  	else
>  		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
>  			page, page_ref_count(page), mapcount,

A definite improvement, but I think we could do better.  For example,
you've changed PageCompound to PageTail here, whereas we really do want
to dump some more information for PageHead pages than the plain vanilla
order-0 page has.  Another thing is that page_mapping() calls compound_head(),
so if the page is corrupted, we're going to get a funky pointer dereference.

I spent a bit of time on this reimplementation ... what do you think?

 - Print the mapping pointer using %p insted of %px.  The actual value of
   the pointer can be read out of the raw page dump and using %p gives a
   chance to correlate it to earlier printk of the mapping pointer.
 - Add the order of the page for compound pages
 - Dump the raw head page as well as the raw page being dumped

diff --git a/mm/debug.c b/mm/debug.c
index ecccd9f17801..0564d4cb8233 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -44,8 +44,10 @@ const struct trace_print_flags vmaflag_names[] = {
 
 void __dump_page(struct page *page, const char *reason)
 {
+	struct page *head = compound_head(page);
 	struct address_space *mapping;
 	bool page_poisoned = PagePoisoned(page);
+	bool compound = PageCompound(page);
 	/*
 	 * Accessing the pageblock without the zone lock. It could change to
 	 * "isolate" again in the meantime, but since we are just dumping the
@@ -66,25 +68,32 @@ void __dump_page(struct page *page, const char *reason)
 		goto hex_only;
 	}
 
-	mapping = page_mapping(page);
+	if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
+		/* Corrupt page, cannot call page_mapping */
+		mapping = page->mapping;
+		head = page;
+		compound = false;
+	} else {
+		mapping = page_mapping(page);
+	}
 
 	/*
 	 * Avoid VM_BUG_ON() in page_mapcount().
 	 * page->_mapcount space in struct page is used by sl[aou]b pages to
 	 * encode own info.
 	 */
-	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
+	mapcount = PageSlab(head) ? 0 : page_mapcount(head);
 
-	if (PageCompound(page))
-		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
-			"index:%#lx compound_mapcount: %d\n",
-			page, page_ref_count(page), mapcount,
+	if (compound)
+		pr_warn("page:%px head:%px refcount:%d mapcount:%d mapping:%p "
+			"index:%#lx order:%u compound_mapcount: %d\n",
+			page, head, page_ref_count(page), mapcount,
 			page->mapping, page_to_pgoff(page),
-			compound_mapcount(page));
+			compound_order(head), compound_mapcount(page));
 	else
-		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
+		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
 			page, page_ref_count(page), mapcount,
-			page->mapping, page_to_pgoff(page));
+			mapping, page_to_pgoff(page));
 	if (PageKsm(page))
 		type = "ksm ";
 	else if (PageAnon(page))
@@ -106,6 +115,10 @@ void __dump_page(struct page *page, const char *reason)
 	print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
 			sizeof(unsigned long), page,
 			sizeof(struct page), false);
+	if (!page_poisoned && compound)
+		print_hex_dump(KERN_WARNING, "head: ", DUMP_PREFIX_NONE, 32,
+			sizeof(unsigned long), head,
+			sizeof(struct page), false);
 
 	if (reason)
 		pr_warn("page dumped because: %s\n", reason);
John Hubbard Feb. 7, 2020, 9:05 p.m. UTC | #2 
On 2/7/20 9:27 AM, Matthew Wilcox wrote:
...
> 
> A definite improvement, but I think we could do better.  For example,
> you've changed PageCompound to PageTail here, whereas we really do want
> to dump some more information for PageHead pages than the plain vanilla
> order-0 page has.  Another thing is that page_mapping() calls compound_head(),
> so if the page is corrupted, we're going to get a funky pointer dereference.
> 
> I spent a bit of time on this reimplementation ... what do you think?
> 

It looks fine to me. I gave it a quick spin, here's the output for a normal
and a huge page, and it has everything we want to see:

page:ffffea0010f0d640 refcount:1025 mapcount:1 mapping:0000000021857089 index:0xed
anon flags: 0x17ffe0000080036(referenced|uptodate|lru|active|swapbacked)
raw: 017ffe0000080036 ffffea0011731f08 ffffea0011730008 ffff8884777272c1
raw: 00000000000000ed 0000000000000000 0000040100000000 0000000000000000
page dumped because: testing dump_page()

page:ffffea0010ef1b80 head:ffffea0010ef0000 refcount:0 mapcount:1 mapping:00000000a8e1c7fa index:0xed order:9 compound_mapcount: 1
anon flags: 0x17ffe0000000000()
raw: 017ffe0000000000 ffffea0010ef0001 ffffea0010ef1b88 dead000000000400
raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
head: 017ffe0000090036 ffffea0011734548 ffffea0010ef8008 ffff8884777271b9
head: 000000000000007f 0000000000000000 00000201ffffffff 0000000000000000
page dumped because: testing dump_page()


>  - Print the mapping pointer using %p insted of %px.  The actual value of
>    the pointer can be read out of the raw page dump and using %p gives a
>    chance to correlate it to earlier printk of the mapping pointer.
>  - Add the order of the page for compound pages
>  - Dump the raw head page as well as the raw page being dumped
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index ecccd9f17801..0564d4cb8233 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -44,8 +44,10 @@ const struct trace_print_flags vmaflag_names[] = {
>  
>  void __dump_page(struct page *page, const char *reason)
>  {
> +	struct page *head = compound_head(page);
>  	struct address_space *mapping;
>  	bool page_poisoned = PagePoisoned(page);
> +	bool compound = PageCompound(page);
>  	/*
>  	 * Accessing the pageblock without the zone lock. It could change to
>  	 * "isolate" again in the meantime, but since we are just dumping the
> @@ -66,25 +68,32 @@ void __dump_page(struct page *page, const char *reason)
>  		goto hex_only;
>  	}
>  
> -	mapping = page_mapping(page);
> +	if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
> +		/* Corrupt page, cannot call page_mapping */
> +		mapping = page->mapping;
> +		head = page;
> +		compound = false;
> +	} else {
> +		mapping = page_mapping(page);
> +	}
>  
>  	/*
>  	 * Avoid VM_BUG_ON() in page_mapcount().
>  	 * page->_mapcount space in struct page is used by sl[aou]b pages to
>  	 * encode own info.
>  	 */
> -	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
> +	mapcount = PageSlab(head) ? 0 : page_mapcount(head);
>  
> -	if (PageCompound(page))
> -		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
> -			"index:%#lx compound_mapcount: %d\n",
> -			page, page_ref_count(page), mapcount,
> +	if (compound)
> +		pr_warn("page:%px head:%px refcount:%d mapcount:%d mapping:%p "
> +			"index:%#lx order:%u compound_mapcount: %d\n",
> +			page, head, page_ref_count(page), mapcount,
>  			page->mapping, page_to_pgoff(page),
> -			compound_mapcount(page));
> +			compound_order(head), compound_mapcount(page));
>  	else
> -		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
> +		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
>  			page, page_ref_count(page), mapcount,
> -			page->mapping, page_to_pgoff(page));
> +			mapping, page_to_pgoff(page));
>  	if (PageKsm(page))
>  		type = "ksm ";
>  	else if (PageAnon(page))
> @@ -106,6 +115,10 @@ void __dump_page(struct page *page, const char *reason)
>  	print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>  			sizeof(unsigned long), page,
>  			sizeof(struct page), false);
> +	if (!page_poisoned && compound)
> +		print_hex_dump(KERN_WARNING, "head: ", DUMP_PREFIX_NONE, 32,
> +			sizeof(unsigned long), head,
> +			sizeof(struct page), false);


Good thought to get the hex dump of the head page in this case, yes.


>  
>  	if (reason)
>  		pr_warn("page dumped because: %s\n", reason);
> 


Seeing as how I want to further enhance dump_page() slightly for this series (to 
include the 3rd struct page's hpage_pincount), would you care to send this as a 
formal patch that I could insert into this series, to replace patch 5?


thanks,
John Hubbard Feb. 7, 2020, 9:14 p.m. UTC | #3 
On 2/7/20 1:05 PM, John Hubbard wrote:
...

> Seeing as how I want to further enhance dump_page() slightly for this series (to 
> include the 3rd struct page's hpage_pincount), would you care to send this as a 
> formal patch that I could insert into this series, to replace patch 5?
> 

ahem, make that "to replace patch 1", please. Too many 5's in the subject lines for 
me, I guess. :)

thanks,
diff mbox series

Patch

diff --git a/mm/debug.c b/mm/debug.c
index ecccd9f17801..f074077eee11 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -42,6 +42,33 @@  const struct trace_print_flags vmaflag_names[] = {
 	{0, NULL}
 };
 
+static void __dump_tail_page(struct page *page, int mapcount)
+{
+	struct page *head = compound_head(page);
+
+	if ((page < head) || (page >= head + MAX_ORDER_NR_PAGES)) {
+		/*
+		 * Page is hopelessly corrupted, so limit any reporting to
+		 * information about the page itself. Do not attempt to look at
+		 * the head page.
+		 */
+		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
+			"index:%#lx (corrupted tail page case)\n",
+			page, page_ref_count(page), mapcount, page->mapping,
+			page_to_pgoff(page));
+	} else {
+		pr_warn("page:%px compound refcount:%d mapcount:%d mapping:%px "
+			"index:%#lx compound_mapcount:%d\n",
+			page, page_ref_count(head), mapcount, head->mapping,
+			page_to_pgoff(head), compound_mapcount(page));
+	}
+
+	if (page_ref_count(page) != 0) {
+		pr_warn("page:%px PROBLEM: non-zero refcount (==%d) on this "
+			"tail page\n", page, page_ref_count(page));
+	}
+}
+
 void __dump_page(struct page *page, const char *reason)
 {
 	struct address_space *mapping;
@@ -75,12 +102,8 @@  void __dump_page(struct page *page, const char *reason)
 	 */
 	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
 
-	if (PageCompound(page))
-		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
-			"index:%#lx compound_mapcount: %d\n",
-			page, page_ref_count(page), mapcount,
-			page->mapping, page_to_pgoff(page),
-			compound_mapcount(page));
+	if (PageTail(page))
+		__dump_tail_page(page, mapcount);
 	else
 		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
 			page, page_ref_count(page), mapcount,