[PATCH v2 0/6] Improvements for dump_page()

[PATCH v2 0/6] Improvements for dump_page()

[Matthew Wilcox (Oracle)]

Thanks to everyone for your feedback on v1.  I haven't added any acks
from v1 because most of the patches have changed substantially from v1.

Here's a sample dump of a pagecache tail page with all of the patches applied:

page:000000006d1c49ca refcount:6 mapcount:0 mapping:00000000136b8d90 index:0x109 pfn:0x6c645
head:000000008bd38076 order:2 compound_mapcount:0 compound_pincount:0
aops:xfs_address_space_operations ino:800042 dentry name:"fd"
flags: 0x4000000000012014(uptodate|lru|private|head)
raw: 4000000000000000 ffffd46ac1b19101 ffffffff00000202 dead000000000004
raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
head: 4000000000012014 ffffd46ac1b1bbc8 ffffd46ac1b1bc08 ffff91976f659560
head: 0000000000000108 ffff919773220680 00000006ffffffff 0000000000000000
page dumped because: testing

Matthew Wilcox (Oracle) (6):
  mm: Handle page->mapping better in dump_page
  mm: Dump compound page information on a second line
  mm: Print head flags in dump_page
  mm: Switch dump_page to get_kernel_nofault
  mm: Print the inode number in dump_page
  mm: Print hashed address of struct page

 mm/debug.c | 75 +++++++++++++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 40 deletions(-)

-- 
2.27.0

[PATCH v2 1/6] mm: Handle page->mapping better in dump_page

[Matthew Wilcox (Oracle)]

If we can't call page_mapping() to get the page mapping, handle the
anon/ksm/movable bits correctly.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/debug.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/debug.c b/mm/debug.c
index 4f376514744d..e5de63406b59 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -70,7 +70,12 @@ void __dump_page(struct page *page, const char *reason)
 
 	if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
 		/* Corrupt page, cannot call page_mapping */
-		mapping = page->mapping;
+		unsigned long tmp = (unsigned long)page->mapping;
+
+		if (tmp & PAGE_MAPPING_ANON)
+			mapping = NULL;
+		else
+			mapping = (void *)(tmp & ~PAGE_MAPPING_FLAGS);
 		head = page;
 		compound = false;
 	} else {
-- 
2.27.0

[PATCH v2 2/6] mm: Dump compound page information on a second line

[Matthew Wilcox (Oracle)]

Simplify both the implementation and the output by splitting all the
compound page information onto a second line.

Reported-by: John Hubbard <jhubbard@nvidia.com>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/debug.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/mm/debug.c b/mm/debug.c
index e5de63406b59..f35c1ae1a7a5 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -89,27 +89,21 @@ void __dump_page(struct page *page, const char *reason)
 	 */
 	mapcount = PageSlab(head) ? 0 : page_mapcount(page);
 
-	if (compound)
+	pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
+			page, page_ref_count(head), mapcount, mapping,
+			page_to_pgoff(page));
+	if (compound) {
 		if (hpage_pincount_available(page)) {
-			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
-				"index:%#lx head:%px order:%u "
-				"compound_mapcount:%d compound_pincount:%d\n",
-				page, page_ref_count(head), mapcount,
-				mapping, page_to_pgoff(page), head,
-				compound_order(head), compound_mapcount(page),
-				compound_pincount(page));
+			pr_warn("head:%px order:%u compound_mapcount:%d compound_pincount:%d\n",
+					head, compound_order(head),
+					compound_mapcount(head),
+					compound_pincount(head));
 		} else {
-			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
-				"index:%#lx head:%px order:%u "
-				"compound_mapcount:%d\n",
-				page, page_ref_count(head), mapcount,
-				mapping, page_to_pgoff(page), head,
-				compound_order(head), compound_mapcount(page));
+			pr_warn("head:%px order:%u compound_mapcount:%d\n",
+					head, compound_order(head),
+					compound_mapcount(head));
 		}
-	else
-		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
-			page, page_ref_count(page), mapcount,
-			mapping, page_to_pgoff(page));
+	}
 	if (PageKsm(page))
 		type = "ksm ";
 	else if (PageAnon(page))
-- 
2.27.0

[PATCH v2 3/6] mm: Print head flags in dump_page

[Matthew Wilcox (Oracle)]

Tail page flags contain very little useful information.  Print the head
page's flags instead.  While the flags will contain "head" for tail pages,
this should not be too confusing as the previous line starts with the
word "head:" and so the flags should be interpreted as belonging to
the head page.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/debug.c b/mm/debug.c
index f35c1ae1a7a5..d2ffc926d8f5 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -162,7 +162,7 @@ void __dump_page(struct page *page, const char *reason)
 out_mapping:
 	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
 
-	pr_warn("%sflags: %#lx(%pGp)%s\n", type, page->flags, &page->flags,
+	pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
 		page_cma ? " CMA" : "");
 
 hex_only:
-- 
2.27.0

[PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault

[Matthew Wilcox (Oracle)]

This is simpler to use than copy_from_kernel_nofault().  Also
make some of the related error messages less verbose.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/debug.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/mm/debug.c b/mm/debug.c
index d2ffc926d8f5..fb64ff7454b6 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -109,54 +109,50 @@ void __dump_page(struct page *page, const char *reason)
 	else if (PageAnon(page))
 		type = "anon ";
 	else if (mapping) {
-		const struct inode *host;
+		struct inode *host;
 		const struct address_space_operations *a_ops;
-		const struct hlist_node *dentry_first;
-		const struct dentry *dentry_ptr;
+		struct hlist_node *dentry_first;
+		struct dentry *dentry_ptr;
 		struct dentry dentry;
 
 		/*
 		 * mapping can be invalid pointer and we don't want to crash
 		 * accessing it, so probe everything depending on it carefully
 		 */
-		if (copy_from_kernel_nofault(&host, &mapping->host,
-					sizeof(struct inode *)) ||
-		    copy_from_kernel_nofault(&a_ops, &mapping->a_ops,
-				sizeof(struct address_space_operations *))) {
-			pr_warn("failed to read mapping->host or a_ops, mapping not a valid kernel address?\n");
+		if (get_kernel_nofault(host, &mapping->host) ||
+		    get_kernel_nofault(a_ops, &mapping->a_ops)) {
+			pr_warn("failed to read mapping contents, not a valid kernel address?\n");
 			goto out_mapping;
 		}
 
 		if (!host) {
-			pr_warn("mapping->a_ops:%ps\n", a_ops);
+			pr_warn("aops:%ps\n", a_ops);
 			goto out_mapping;
 		}
 
-		if (copy_from_kernel_nofault(&dentry_first,
-			&host->i_dentry.first, sizeof(struct hlist_node *))) {
-			pr_warn("mapping->a_ops:%ps with invalid mapping->host inode address %px\n",
-				a_ops, host);
+		if (get_kernel_nofault(dentry_first, &host->i_dentry.first)) {
+			pr_warn("aops:%ps with invalid host inode %px\n",
+					a_ops, host);
 			goto out_mapping;
 		}
 
 		if (!dentry_first) {
-			pr_warn("mapping->a_ops:%ps\n", a_ops);
+			pr_warn("aops:%ps\n", a_ops);
 			goto out_mapping;
 		}
 
 		dentry_ptr = container_of(dentry_first, struct dentry, d_u.d_alias);
-		if (copy_from_kernel_nofault(&dentry, dentry_ptr,
-							sizeof(struct dentry))) {
-			pr_warn("mapping->aops:%ps with invalid mapping->host->i_dentry.first %px\n",
-				a_ops, dentry_ptr);
+		if (get_kernel_nofault(dentry, dentry_ptr)) {
+			pr_warn("aops:%ps with invalid dentry %px\n", a_ops,
+					dentry_ptr);
 		} else {
 			/*
 			 * if dentry is corrupted, the %pd handler may still
 			 * crash, but it's unlikely that we reach here with a
 			 * corrupted struct page
 			 */
-			pr_warn("mapping->aops:%ps dentry name:\"%pd\"\n",
-								a_ops, &dentry);
+			pr_warn("aops:%ps dentry name:\"%pd\"\n", a_ops,
+					&dentry);
 		}
 	}
 out_mapping:
-- 
2.27.0

[PATCH v2 5/6] mm: Print the inode number in dump_page

[Matthew Wilcox (Oracle)]

The inode number helps correlate this page with debug messages elsewhere
in the kernel.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/debug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/debug.c b/mm/debug.c
index fb64ff7454b6..60347d0d7609 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -137,7 +137,7 @@ void __dump_page(struct page *page, const char *reason)
 		}
 
 		if (!dentry_first) {
-			pr_warn("aops:%ps\n", a_ops);
+			pr_warn("aops:%ps ino:%lx\n", a_ops, host->i_ino);
 			goto out_mapping;
 		}
 
@@ -151,8 +151,8 @@ void __dump_page(struct page *page, const char *reason)
 			 * crash, but it's unlikely that we reach here with a
 			 * corrupted struct page
 			 */
-			pr_warn("aops:%ps dentry name:\"%pd\"\n", a_ops,
-					&dentry);
+			pr_warn("aops:%ps ino:%lx dentry name:\"%pd\"\n",
+					a_ops, host->i_ino, &dentry);
 		}
 	}
 out_mapping:
-- 
2.27.0

[PATCH v2 6/6] mm: Print hashed address of struct page

[Matthew Wilcox (Oracle)]

The actual address of the struct page isn't particularly helpful,
while the hashed address helps match with other messages elsewhere.
Add the PFN that the page refers to in order to help diagnose problems
where the page is improperly aligned for the purpose.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/debug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/debug.c b/mm/debug.c
index 60347d0d7609..033f04ffc398 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -89,17 +89,17 @@ void __dump_page(struct page *page, const char *reason)
 	 */
 	mapcount = PageSlab(head) ? 0 : page_mapcount(page);
 
-	pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
+	pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
 			page, page_ref_count(head), mapcount, mapping,
-			page_to_pgoff(page));
+			page_to_pgoff(page), page_to_pfn(page));
 	if (compound) {
 		if (hpage_pincount_available(page)) {
-			pr_warn("head:%px order:%u compound_mapcount:%d compound_pincount:%d\n",
+			pr_warn("head:%p order:%u compound_mapcount:%d compound_pincount:%d\n",
 					head, compound_order(head),
 					compound_mapcount(head),
 					compound_pincount(head));
 		} else {
-			pr_warn("head:%px order:%u compound_mapcount:%d\n",
+			pr_warn("head:%p order:%u compound_mapcount:%d\n",
 					head, compound_order(head),
 					compound_mapcount(head));
 		}
-- 
2.27.0

Re: [PATCH v2 0/6] Improvements for dump_page()

[Mike Rapoport]

On Thu, Jul 09, 2020 at 09:21:11PM +0100, Matthew Wilcox (Oracle) wrote:
> Thanks to everyone for your feedback on v1.  I haven't added any acks
> from v1 because most of the patches have changed substantially from v1.

Aside from my aesthetic preference to have printk continuation line
below the opening '(' rather then full tab indented the patches look
good.

Acked-by: Mike Rapoport <rppt@linux.ibm.com>

> Here's a sample dump of a pagecache tail page with all of the patches applied:
> 
> page:000000006d1c49ca refcount:6 mapcount:0 mapping:00000000136b8d90 index:0x109 pfn:0x6c645
> head:000000008bd38076 order:2 compound_mapcount:0 compound_pincount:0
> aops:xfs_address_space_operations ino:800042 dentry name:"fd"
> flags: 0x4000000000012014(uptodate|lru|private|head)
> raw: 4000000000000000 ffffd46ac1b19101 ffffffff00000202 dead000000000004
> raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> head: 4000000000012014 ffffd46ac1b1bbc8 ffffd46ac1b1bc08 ffff91976f659560
> head: 0000000000000108 ffff919773220680 00000006ffffffff 0000000000000000
> page dumped because: testing
> 
> Matthew Wilcox (Oracle) (6):
>   mm: Handle page->mapping better in dump_page
>   mm: Dump compound page information on a second line
>   mm: Print head flags in dump_page
>   mm: Switch dump_page to get_kernel_nofault
>   mm: Print the inode number in dump_page
>   mm: Print hashed address of struct page
> 
>  mm/debug.c | 75 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 35 insertions(+), 40 deletions(-)
> 
> -- 
> 2.27.0
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 0/6] Improvements for dump_page()

[William Kucharski]

Just a formatting suggestion:

> On Jul 9, 2020, at 2:21 PM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> 
> Thanks to everyone for your feedback on v1.  I haven't added any acks
> from v1 because most of the patches have changed substantially from v1.
> 
> Here's a sample dump of a pagecache tail page with all of the patches applied:
> 
> page:000000006d1c49ca refcount:6 mapcount:0 mapping:00000000136b8d90 index:0x109 pfn:0x6c645
> head:000000008bd38076 order:2 compound_mapcount:0 compound_pincount:0
> aops:xfs_address_space_operations ino:800042 dentry name:"fd"
> flags: 0x4000000000012014(uptodate|lru|private|head)
> raw: 4000000000000000 ffffd46ac1b19101 ffffffff00000202 dead000000000004
> raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> head: 4000000000012014 ffffd46ac1b1bbc8 ffffd46ac1b1bc08 ffff91976f659560
> head: 0000000000000108 ffff919773220680 00000006ffffffff 0000000000000000
> page dumped because: testing
> 
> Matthew Wilcox (Oracle) (6):
>  mm: Handle page->mapping better in dump_page
>  mm: Dump compound page information on a second line
>  mm: Print head flags in dump_page
>  mm: Switch dump_page to get_kernel_nofault
>  mm: Print the inode number in dump_page
>  mm: Print hashed address of struct page
> 
> mm/debug.c | 75 +++++++++++++++++++++++++-----------------------------
> 1 file changed, 35 insertions(+), 40 deletions(-)

As this output is meant to be taken as a block, it might be more legible
if each line after the first was indented by a space to make that instantly
clear by sight (as git does when generating the list of changed files above)
and perhaps add a prefix/suffix to make the "dumped because" line stand out
a bit more.

A space between the value and flags in the "flags:" would also be nice.

This would then look something like:

page:000000006d1c49ca refcount:6 mapcount:0 mapping:00000000136b8d90 index:0x109 pfn:0x6c645
 head:000000008bd38076 order:2 compound_mapcount:0 compound_pincount:0
 aops:xfs_address_space_operations ino:800042 dentry name:"fd"
 flags: 0x4000000000012014 (uptodate|lru|private|head)
 raw: 4000000000000000 ffffd46ac1b19101 ffffffff00000202 dead000000000004
 raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
 head: 4000000000012014 ffffd46ac1b1bbc8 ffffd46ac1b1bc08 ffff91976f659560
 head: 0000000000000108 ffff919773220680 00000006ffffffff 0000000000000000
 >> page dumped because: testing <<

However I realize this would mean making changes to things you haven't modified
as part of this patch, so feel free to put my suggestion on the back burner
for now if you like.

Re: [PATCH v2 1/6] mm: Handle page->mapping better in dump_page

[John Hubbard]

On 2020-07-09 13:21, Matthew Wilcox (Oracle) wrote:
> If we can't call page_mapping() to get the page mapping, handle the
> anon/ksm/movable bits correctly.

Hi Matthew,

I see why you did this, as otherwise the new printing won't work.

> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/debug.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 4f376514744d..e5de63406b59 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -70,7 +70,12 @@ void __dump_page(struct page *page, const char *reason)
>   
>   	if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
>   		/* Corrupt page, cannot call page_mapping */

It's tricky to step lightly through the debris of corrupted page
pointer here, but how about this updated comment, to replace the one-liner
above:

		/*
		 * Corrupt page, so we cannot call page_mapping. Instead, do a
		 * safe subset of the steps that page_mapping() does. Caution:
		 * this will be misleading for tail pages, PageSwapCache pages,
		 * and potentially other situations. (See the page_mapping()
		 * implementation for what's missing here.)
		 */


> -		mapping = page->mapping;
> +		unsigned long tmp = (unsigned long)page->mapping;
> +
> +		if (tmp & PAGE_MAPPING_ANON)
> +			mapping = NULL;
> +		else
> +			mapping = (void *)(tmp & ~PAGE_MAPPING_FLAGS);
>   		head = page;
>   		compound = false;
>   	} else {
> 

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2 1/6] mm: Handle page->mapping better in dump_page

[John Hubbard]

On 2020-07-10 18:30, John Hubbard wrote:
> On 2020-07-09 13:21, Matthew Wilcox (Oracle) wrote:
>> If we can't call page_mapping() to get the page mapping, handle the
>> anon/ksm/movable bits correctly.
> 
> Hi Matthew,
> 
> I see why you did this, as otherwise the new printing won't work.

Oh, and I forget to add: my only comment was just a documentation
suggestion, not critical at all, and so:

     Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA
> 
>>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>   mm/debug.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/debug.c b/mm/debug.c
>> index 4f376514744d..e5de63406b59 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -70,7 +70,12 @@ void __dump_page(struct page *page, const char *reason)
>>       if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
>>           /* Corrupt page, cannot call page_mapping */
> 
> It's tricky to step lightly through the debris of corrupted page
> pointer here, but how about this updated comment, to replace the one-liner
> above:
> 
>          /*
>           * Corrupt page, so we cannot call page_mapping. Instead, do a
>           * safe subset of the steps that page_mapping() does. Caution:
>           * this will be misleading for tail pages, PageSwapCache pages,
>           * and potentially other situations. (See the page_mapping()
>           * implementation for what's missing here.)
>           */
> 
> 
>> -        mapping = page->mapping;
>> +        unsigned long tmp = (unsigned long)page->mapping;
>> +
>> +        if (tmp & PAGE_MAPPING_ANON)
>> +            mapping = NULL;
>> +        else
>> +            mapping = (void *)(tmp & ~PAGE_MAPPING_FLAGS);
>>           head = page;
>>           compound = false;
>>       } else {
>>
> 

Re: [PATCH v2 2/6] mm: Dump compound page information on a second line

[John Hubbard]

On 2020-07-09 13:21, Matthew Wilcox (Oracle) wrote:
> Simplify both the implementation and the output by splitting all the
> compound page information onto a second line.
> 
> Reported-by: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/debug.c | 30 ++++++++++++------------------
>   1 file changed, 12 insertions(+), 18 deletions(-)
> 

Yes, looks good and is working nicely for me, too.

     Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA

> diff --git a/mm/debug.c b/mm/debug.c
> index e5de63406b59..f35c1ae1a7a5 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -89,27 +89,21 @@ void __dump_page(struct page *page, const char *reason)
>   	 */
>   	mapcount = PageSlab(head) ? 0 : page_mapcount(page);
>   
> -	if (compound)
> +	pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
> +			page, page_ref_count(head), mapcount, mapping,
> +			page_to_pgoff(page));
> +	if (compound) {
>   		if (hpage_pincount_available(page)) {
> -			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%px order:%u "
> -				"compound_mapcount:%d compound_pincount:%d\n",
> -				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page),
> -				compound_pincount(page));
> +			pr_warn("head:%px order:%u compound_mapcount:%d compound_pincount:%d\n",
> +					head, compound_order(head),
> +					compound_mapcount(head),
> +					compound_pincount(head));
>   		} else {
> -			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%px order:%u "
> -				"compound_mapcount:%d\n",
> -				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page));
> +			pr_warn("head:%px order:%u compound_mapcount:%d\n",
> +					head, compound_order(head),
> +					compound_mapcount(head));
>   		}
> -	else
> -		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
> -			page, page_ref_count(page), mapcount,
> -			mapping, page_to_pgoff(page));
> +	}
>   	if (PageKsm(page))
>   		type = "ksm ";
>   	else if (PageAnon(page))
> 

Re: [PATCH v2 3/6] mm: Print head flags in dump_page

[John Hubbard]

On 2020-07-09 13:21, Matthew Wilcox (Oracle) wrote:
> Tail page flags contain very little useful information.  Print the head
> page's flags instead.  While the flags will contain "head" for tail pages,
> this should not be too confusing as the previous line starts with the
> word "head:" and so the flags should be interpreted as belonging to
> the head page.

Yes, OK. I'm still *slightly* disappointed about the tail page showing "head"
in the flags, but that's a minor complaint and we've seen that it's actually
pretty hard to do better, given the situation. And overall, the output is
definitely way better than before this series. Thanks for all the cleanup!

     Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA

> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/debug.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index f35c1ae1a7a5..d2ffc926d8f5 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -162,7 +162,7 @@ void __dump_page(struct page *page, const char *reason)
>   out_mapping:
>   	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>   
> -	pr_warn("%sflags: %#lx(%pGp)%s\n", type, page->flags, &page->flags,
> +	pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
>   		page_cma ? " CMA" : "");
>   
>   hex_only:
> 

Re: [PATCH v2 6/6] mm: Print hashed address of struct page

[John Hubbard]

On 2020-07-09 13:21, Matthew Wilcox (Oracle) wrote:
> The actual address of the struct page isn't particularly helpful,
> while the hashed address helps match with other messages elsewhere.
> Add the PFN that the page refers to in order to help diagnose problems
> where the page is improperly aligned for the purpose.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/debug.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 60347d0d7609..033f04ffc398 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -89,17 +89,17 @@ void __dump_page(struct page *page, const char *reason)
>   	 */
>   	mapcount = PageSlab(head) ? 0 : page_mapcount(page);
>   
> -	pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
> +	pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
>   			page, page_ref_count(head), mapcount, mapping,
> -			page_to_pgoff(page));
> +			page_to_pgoff(page), page_to_pfn(page));
>   	if (compound) {
>   		if (hpage_pincount_available(page)) {
> -			pr_warn("head:%px order:%u compound_mapcount:%d compound_pincount:%d\n",
> +			pr_warn("head:%p order:%u compound_mapcount:%d compound_pincount:%d\n",
>   					head, compound_order(head),
>   					compound_mapcount(head),
>   					compound_pincount(head));
>   		} else {
> -			pr_warn("head:%px order:%u compound_mapcount:%d\n",
> +			pr_warn("head:%p order:%u compound_mapcount:%d\n",
>   					head, compound_order(head),
>   					compound_mapcount(head));
>   		}
> 

Seems reasonable. The first output line is getting long again, but it's still
OK-ish.

     Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2 5/6] mm: Print the inode number in dump_page

[John Hubbard]

On 2020-07-09 13:21, Matthew Wilcox (Oracle) wrote:
> The inode number helps correlate this page with debug messages elsewhere
> in the kernel.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/debug.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 

     Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA


> diff --git a/mm/debug.c b/mm/debug.c
> index fb64ff7454b6..60347d0d7609 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -137,7 +137,7 @@ void __dump_page(struct page *page, const char *reason)
>   		}
>   
>   		if (!dentry_first) {
> -			pr_warn("aops:%ps\n", a_ops);
> +			pr_warn("aops:%ps ino:%lx\n", a_ops, host->i_ino);
>   			goto out_mapping;
>   		}
>   
> @@ -151,8 +151,8 @@ void __dump_page(struct page *page, const char *reason)
>   			 * crash, but it's unlikely that we reach here with a
>   			 * corrupted struct page
>   			 */
> -			pr_warn("aops:%ps dentry name:\"%pd\"\n", a_ops,
> -					&dentry);
> +			pr_warn("aops:%ps ino:%lx dentry name:\"%pd\"\n",
> +					a_ops, host->i_ino, &dentry);
>   		}
>   	}
>   out_mapping:
> 

Re: [PATCH v2 1/6] mm: Handle page->mapping better in dump_page

[Vlastimil Babka]

On 7/9/20 10:21 PM, Matthew Wilcox (Oracle) wrote:
> If we can't call page_mapping() to get the page mapping, handle the
> anon/ksm/movable bits correctly.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

John's updated comment looks fine too.

> ---
>  mm/debug.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 4f376514744d..e5de63406b59 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -70,7 +70,12 @@ void __dump_page(struct page *page, const char *reason)
>  
>  	if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
>  		/* Corrupt page, cannot call page_mapping */
> -		mapping = page->mapping;
> +		unsigned long tmp = (unsigned long)page->mapping;
> +
> +		if (tmp & PAGE_MAPPING_ANON)
> +			mapping = NULL;
> +		else
> +			mapping = (void *)(tmp & ~PAGE_MAPPING_FLAGS);
>  		head = page;
>  		compound = false;
>  	} else {
> 

Re: [PATCH v2 2/6] mm: Dump compound page information on a second line

[Vlastimil Babka]

On 7/9/20 10:21 PM, Matthew Wilcox (Oracle) wrote:
> Simplify both the implementation and the output by splitting all the
> compound page information onto a second line.
> 
> Reported-by: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/debug.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index e5de63406b59..f35c1ae1a7a5 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -89,27 +89,21 @@ void __dump_page(struct page *page, const char *reason)
>  	 */
>  	mapcount = PageSlab(head) ? 0 : page_mapcount(page);
>  
> -	if (compound)
> +	pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
> +			page, page_ref_count(head), mapcount, mapping,
> +			page_to_pgoff(page));
> +	if (compound) {
>  		if (hpage_pincount_available(page)) {
> -			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%px order:%u "
> -				"compound_mapcount:%d compound_pincount:%d\n",
> -				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page),
> -				compound_pincount(page));
> +			pr_warn("head:%px order:%u compound_mapcount:%d compound_pincount:%d\n",
> +					head, compound_order(head),
> +					compound_mapcount(head),
> +					compound_pincount(head));
>  		} else {
> -			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%px order:%u "
> -				"compound_mapcount:%d\n",
> -				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page));
> +			pr_warn("head:%px order:%u compound_mapcount:%d\n",
> +					head, compound_order(head),
> +					compound_mapcount(head));
>  		}
> -	else
> -		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
> -			page, page_ref_count(page), mapcount,
> -			mapping, page_to_pgoff(page));
> +	}
>  	if (PageKsm(page))
>  		type = "ksm ";
>  	else if (PageAnon(page))
> 

Re: [PATCH v2 3/6] mm: Print head flags in dump_page

[Vlastimil Babka]

On 7/9/20 10:21 PM, Matthew Wilcox (Oracle) wrote:
> Tail page flags contain very little useful information.  Print the head
> page's flags instead.  While the flags will contain "head" for tail pages,
> this should not be too confusing as the previous line starts with the
> word "head:" and so the flags should be interpreted as belonging to
> the head page.

Should be fine, hopefully.

> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/debug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index f35c1ae1a7a5..d2ffc926d8f5 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -162,7 +162,7 @@ void __dump_page(struct page *page, const char *reason)
>  out_mapping:
>  	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>  
> -	pr_warn("%sflags: %#lx(%pGp)%s\n", type, page->flags, &page->flags,
> +	pr_warn("%sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
>  		page_cma ? " CMA" : "");
>  
>  hex_only:
> 

Re: [PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault

[Vlastimil Babka]

On 7/9/20 10:21 PM, Matthew Wilcox (Oracle) wrote:
> This is simpler to use than copy_from_kernel_nofault().  Also
> make some of the related error messages less verbose.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Re: [PATCH v2 5/6] mm: Print the inode number in dump_page

[Vlastimil Babka]

On 7/9/20 10:21 PM, Matthew Wilcox (Oracle) wrote:
> The inode number helps correlate this page with debug messages elsewhere
> in the kernel.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/debug.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index fb64ff7454b6..60347d0d7609 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -137,7 +137,7 @@ void __dump_page(struct page *page, const char *reason)
>  		}
>  
>  		if (!dentry_first) {
> -			pr_warn("aops:%ps\n", a_ops);
> +			pr_warn("aops:%ps ino:%lx\n", a_ops, host->i_ino);

Hmm it's a bit subtle that we assume reading host->i_ino is safe because we
passed the get_kernel_nofault() for &host->i_dentry.first. Maybe worth a comment?

>  			goto out_mapping;
>  		}
>  
> @@ -151,8 +151,8 @@ void __dump_page(struct page *page, const char *reason)

Would it be worth printing the inode number also here in the 'pr_warn("aops:%ps
with invalid dentry %px\n"' case?

>  			 * crash, but it's unlikely that we reach here with a
>  			 * corrupted struct page
>  			 */
> -			pr_warn("aops:%ps dentry name:\"%pd\"\n", a_ops,
> -					&dentry);
> +			pr_warn("aops:%ps ino:%lx dentry name:\"%pd\"\n",
> +					a_ops, host->i_ino, &dentry);
>  		}
>  	}
>  out_mapping:
> 

Re: [PATCH v2 2/6] mm: Dump compound page information on a second line

[Qian Cai]

On Thu, Jul 09, 2020 at 09:21:13PM +0100, Matthew Wilcox (Oracle) wrote:
> Simplify both the implementation and the output by splitting all the
> compound page information onto a second line.
> 
> Reported-by: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

dump_page() starts to crash occasionally. The offset below points to this
patch. Any thought?

[ 2999.340055][ T5516] page:0000000084997a23 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888bf243bf40 pfn:0xbf2438
[ 2999.398824][ T5516] page:0000000084997a23 refcount:0 mapcount:0 mapping:0000000000000000 index:0xffff888bf243ad40 pfn:0xbf2438
[ 2999.455037][ T5516] flags: 0x1bfffc000000000()
[ 2999.476870][ T5516] raw: 01bfffc000000000 dead000000000100 dead000000000122 0000000000000000
[ 2999.518459][ T5516] raw: ffff888bf243ad40 00000000002a0000 00000000ffffffff 0000000000000000
[ 2999.560177][ T5516] page dumped because: VM_BUG_ON_PAGE(!PageCompound(page))
[ 2999.595087][ T5516] ------------[ cut here ]------------
[ 2999.625075][ T5516] kernel BUG at include/linux/mm.h:786!
[ 2999.652433][ T5516] invalid opcode: 0000 [#1] SMP KASAN PTI
[ 2999.679523][ T5516] CPU: 35 PID: 5516 Comm: test.sh Not tainted 5.8.0-next-20200804 #2
[ 2999.718652][ T5516] Hardware name: HP ProLiant ML350 Gen9, BIOS P92 07/11/2014
[ 2999.755698][ T5516] RIP: 0010:__dump_page+0xe8b/0x12f0
compound_mapcount at include/linux/mm.h:786
(inlined by) compound_mapcount at include/linux/mm.h:784
(inlined by) __dump_page at mm/debug.c:108
[ 2999.784590][ T5516] Code: 06 49 01 c1 e9 72 f4 ff ff 48 89 df e8 5e 12 16 00 e9 09 fe ff ff 48 c7 c6 80 41 cc a8 48 89 df e8 7a f1 ff ff 0f 1f 44 00 00 <0f> 0b 48 c7 c7 80 25 cb ad e8 33 f5 71 00 48 c7 c6 80 41 cc a8 48
[ 2999.890974][ T5516] RSP: 0018:ffffc9000a3c78d0 EFLAGS: 00010282
[ 2999.920196][ T5516] RAX: 0000000000000000 RBX: ffffea002fc90e00 RCX: ffffffffa8095282
[ 2999.958834][ T5516] RDX: 1ffffd4005f921c7 RSI: 0000000000000000 RDI: ffffea002fc90e38
[ 2999.997374][ T5516] RBP: 1ffff92001478f22 R08: ffffed118cf0de7a R09: ffffed118cf0de7a
[ 3000.035972][ T5516] R10: ffff888c6786f3cf R11: ffffed118cf0de79 R12: ffffea002fc90e08
[ 3000.074836][ T5516] R13: ffffea002fc90e00 R14: ffffea002fc90e08 R15: ffffea002fc90e00
[ 3000.113842][ T5516] FS:  00007fae1ff5e740(0000) GS:ffff888c67840000(0000) knlGS:0000000000000000
[ 3000.156525][ T5516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3000.189099][ T5516] CR2: 00005578d493a1b8 CR3: 0000000610836005 CR4: 00000000001706e0
[ 3000.227643][ T5516] Call Trace:
[ 3000.243118][ T5516]  ? page_init_poison+0x40/0x40
[ 3000.266242][ T5516]  ? lockdep_hardirqs_on_prepare+0x33e/0x4e0
[ 3000.294621][ T5516]  ? _raw_spin_unlock_irq+0x1f/0x30
[ 3000.322775][ T5516]  ? trace_hardirqs_on+0x20/0x1b5
[ 3000.350565][ T5516]  ? lockdep_hardirqs_on_prepare+0x4e0/0x4e0
[ 3000.383465][ T5516]  ? start_isolate_page_range+0x301/0x690
[ 3000.416514][ T5516]  ? drain_all_pages+0x4a/0x7a0
[ 3000.441585][ T5516]  ? mark_lock+0x147/0x1800
[ 3000.463184][ T5516]  ? init_pwq+0x340/0x340
[ 3000.483738][ T5516]  ? find_held_lock+0x33/0x1c0
[ 3000.506497][ T5516]  ? print_irqtrace_events+0x270/0x270
[ 3000.532505][ T5516]  ? start_isolate_page_range+0x344/0x690
[ 3000.560632][ T5516]  ? mark_held_locks+0xb0/0x110
[ 3000.583751][ T5516]  ? rwlock_bug.part.1+0x90/0x90
[ 3000.610492][ T5516]  ? lockdep_hardirqs_on_prepare+0x33e/0x4e0
[ 3000.640146][ T5516]  ? _raw_spin_unlock_irqrestore+0x39/0x40
[ 3000.671680][ T5516]  dump_page+0x9/0x20
[ 3000.690807][ T5516]  start_isolate_page_range+0x4ac/0x690
[ 3000.717529][ T5516]  ? state_show+0xc0/0xc0
[ 3000.738137][ T5516]  __offline_pages+0x199/0xe30
[ 3000.761471][ T5516]  ? __mutex_lock+0x4aa/0x1390
[ 3000.784203][ T5516]  ? rwlock_bug.part.1+0x90/0x90
[ 3000.807941][ T5516]  ? device_offline+0x75/0x1c0
[ 3000.833233][ T5516]  ? __add_memory+0x60/0x60
[ 3000.857530][ T5516]  ? mutex_lock_io_nested+0x1270/0x1270
[ 3000.886608][ T5516]  ? klist_next+0x253/0x430
[ 3000.911512][ T5516]  ? device_is_dependent+0x140/0x140
[ 3000.937751][ T5516]  ? device_for_each_child+0xe1/0x140
[ 3000.963576][ T5516]  ? device_remove_class_symlinks+0x1a0/0x1a0
[ 3000.992753][ T5516]  ? state_show+0xc0/0xc0
[ 3001.013490][ T5516]  memory_subsys_offline+0xcb/0x160
[ 3001.038346][ T5516]  device_offline+0x131/0x1c0
[ 3001.060614][ T5516]  ? sysfs_file_ops+0x160/0x160
[ 3001.083866][ T5516]  state_store+0x78/0xa0
[ 3001.103882][ T5516]  kernfs_fop_write+0x208/0x410
[ 3001.127602][ T5516]  ? __sb_start_write+0x1f8/0x2e0
[ 3001.151596][ T5516]  vfs_write+0x1a3/0x5d0
[ 3001.171713][ T5516]  ksys_write+0xec/0x1c0
[ 3001.192625][ T5516]  ? __x64_sys_read+0xa0/0xa0
[ 3001.215692][ T5516]  ? syscall_enter_from_user_mode+0x20/0x210
[ 3001.245499][ T5516]  ? trace_hardirqs_on+0x20/0x1b5
[ 3001.269466][ T5516]  do_syscall_64+0x33/0x40
[ 3001.290710][ T5516]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 3001.319164][ T5516] RIP: 0033:0x7fae1f63fb28
[ 3001.340214][ T5516] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 35 4b 2d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
[ 3001.448642][ T5516] RSP: 002b:00007ffdad11d068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[ 3001.492000][ T5516] RAX: ffffffffffffffda RBX: 0000000000000008 RCX: 00007fae1f63fb28
[ 3001.531309][ T5516] RDX: 0000000000000008 RSI: 00005649a73b6240 RDI: 0000000000000001
[ 3001.571310][ T5516] RBP: 00005649a73b6240 R08: 000000000000000a R09: 00007fae1f6d0c80
[ 3001.610126][ T5516] R10: 000000000000000a R11: 0000000000000246 R12: 00007fae1f9106c0
[ 3001.648869][ T5516] R13: 0000000000000008 R14: 00007fae1f90b880 R15: 0000000000000008
[ 3001.687284][ T5516] Modules linked in: loop kvm_intel kvm irqbypass ip_tables x_tables sd_mod ahci libahci tg3 firmware_class libata libphy hpsa scsi_transport_sas dm_mirror dm_region_hash dm_log dm_mod
[ 3001.776222][ T5516] ---[ end trace 263095a22deddb65 ]---
[ 3001.802318][ T5516] RIP: 0010:__dump_page+0xe8b/0x12f0
[ 3001.828306][ T5516] Code: 06 49 01 c1 e9 72 f4 ff ff 48 89 df e8 5e 12 16 00 e9 09 fe ff ff 48 c7 c6 80 41 cc a8 48 89 df e8 7a f1 ff ff 0f 1f 44 00 00 <0f> 0b 48 c7 c7 80 25 cb ad e8 33 f5 71 00 48 c7 c6 80 41 cc a8 48
[ 3001.931924][ T5516] RSP: 0018:ffffc9000a3c78d0 EFLAGS: 00010282
[ 3001.965478][ T5516] RAX: 0000000000000000 RBX: ffffea002fc90e00 RCX: ffffffffa8095282
[ 3002.005570][ T5516] RDX: 1ffffd4005f921c7 RSI: 0000000000000000 RDI: ffffea002fc90e38
[ 3002.044022][ T5516] RBP: 1ffff92001478f22 R08: ffffed118cf0de7a R09: ffffed118cf0de7a
[ 3002.082857][ T5516] R10: ffff888c6786f3cf R11: ffffed118cf0de79 R12: ffffea002fc90e08
[ 3002.121273][ T5516] R13: ffffea002fc90e00 R14: ffffea002fc90e08 R15: ffffea002fc90e00
[ 3002.160008][ T5516] FS:  00007fae1ff5e740(0000) GS:ffff888c67840000(0000) knlGS:0000000000000000
[ 3002.204033][ T5516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3002.235743][ T5516] CR2: 00005578d493a1b8 CR3: 0000000610836005 CR4: 00000000001706e0
[ 3002.274246][ T5516] Kernel panic - not syncing: Fatal exception
[ 3002.303579][ T5516] Kernel Offset: 0x26400000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[ 3002.360369][ T5516] ---[ end Kernel panic - not syncing: Fatal exception ]---

> ---
>  mm/debug.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index e5de63406b59..f35c1ae1a7a5 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -89,27 +89,21 @@ void __dump_page(struct page *page, const char *reason)
>  	 */
>  	mapcount = PageSlab(head) ? 0 : page_mapcount(page);
>  
> -	if (compound)
> +	pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
> +			page, page_ref_count(head), mapcount, mapping,
> +			page_to_pgoff(page));
> +	if (compound) {
>  		if (hpage_pincount_available(page)) {
> -			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%px order:%u "
> -				"compound_mapcount:%d compound_pincount:%d\n",
> -				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page),
> -				compound_pincount(page));
> +			pr_warn("head:%px order:%u compound_mapcount:%d compound_pincount:%d\n",
> +					head, compound_order(head),
> +					compound_mapcount(head),
> +					compound_pincount(head));
>  		} else {
> -			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%px order:%u "
> -				"compound_mapcount:%d\n",
> -				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page));
> +			pr_warn("head:%px order:%u compound_mapcount:%d\n",
> +					head, compound_order(head),
> +					compound_mapcount(head));
>  		}
> -	else
> -		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
> -			page, page_ref_count(page), mapcount,
> -			mapping, page_to_pgoff(page));
> +	}
>  	if (PageKsm(page))
>  		type = "ksm ";
>  	else if (PageAnon(page))
> -- 
> 2.27.0
> 
> 

Re: [PATCH v2 2/6] mm: Dump compound page information on a second line

[Matthew Wilcox]

On Tue, Aug 04, 2020 at 11:37:43AM -0400, Qian Cai wrote:
> On Thu, Jul 09, 2020 at 09:21:13PM +0100, Matthew Wilcox (Oracle) wrote:
> > Simplify both the implementation and the output by splitting all the
> > compound page information onto a second line.
> > 
> > Reported-by: John Hubbard <jhubbard@nvidia.com>
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> dump_page() starts to crash occasionally. The offset below points to this
> patch. Any thought?

I don't think this patch is the cause.  It just moves around code
that is implicated by the chain below.  You're actually getting _more_
useful output with this patch than you were before, so there's no call
to abandon this patch.  But maybe we can make dump_page() more robust.

> [ 2999.340055][ T5516] page:0000000084997a23 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888bf243bf40 pfn:0xbf2438
> [ 2999.398824][ T5516] page:0000000084997a23 refcount:0 mapcount:0 mapping:0000000000000000 index:0xffff888bf243ad40 pfn:0xbf2438

This is fun.  We've got a recursive call to dump_page(), so we dump the
same page twice.  First, we find its head:

        struct page *head = compound_head(page);

(and I believe it is a tail page at this point)

... then the page is split, perhaps?  Anyway, by the time we call this
line, the subpage has a refcount of 1:

        pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
                        page, page_ref_count(head), mapcount, mapping,
                        page_to_pgoff(page), page_to_pfn(page));

by this point, head no longer has PageHead set, and we end up calling
compound_mapcount() here:

                        pr_warn("head:%p order:%u compound_mapcount:%d\n",
                                        head, compound_order(head),
                                        compound_mapcount(head));

but compound_mapcount() checks that the page is compound and it's not,
so we call __dump_page() again, and print the page _again_, this time
with a refcount of 0 (because it's been truncated?).

I don't really know what to do about this.  Should dump_page() try to
take a reference on the page to prevent this from happening?  Or open-
code compound_mapcount() to prevent a crash, even though it will
print incorrect data due to this race?

> [ 2999.455037][ T5516] flags: 0x1bfffc000000000()
> [ 2999.476870][ T5516] raw: 01bfffc000000000 dead000000000100 dead000000000122 0000000000000000
> [ 2999.518459][ T5516] raw: ffff888bf243ad40 00000000002a0000 00000000ffffffff 0000000000000000
> [ 2999.560177][ T5516] page dumped because: VM_BUG_ON_PAGE(!PageCompound(page))
> [ 2999.595087][ T5516] ------------[ cut here ]------------
> [ 2999.625075][ T5516] kernel BUG at include/linux/mm.h:786!
> [ 2999.652433][ T5516] invalid opcode: 0000 [#1] SMP KASAN PTI
> [ 2999.679523][ T5516] CPU: 35 PID: 5516 Comm: test.sh Not tainted 5.8.0-next-20200804 #2
> [ 2999.718652][ T5516] Hardware name: HP ProLiant ML350 Gen9, BIOS P92 07/11/2014
> [ 2999.755698][ T5516] RIP: 0010:__dump_page+0xe8b/0x12f0
> compound_mapcount at include/linux/mm.h:786
> (inlined by) compound_mapcount at include/linux/mm.h:784
> (inlined by) __dump_page at mm/debug.c:108
> [ 2999.784590][ T5516] Code: 06 49 01 c1 e9 72 f4 ff ff 48 89 df e8 5e 12 16 00 e9 09 fe ff ff 48 c7 c6 80 41 cc a8 48 89 df e8 7a f1 ff ff 0f 1f 44 00 00 <0f> 0b 48 c7 c7 80 25 cb ad e8 33 f5 71 00 48 c7 c6 80 41 cc a8 48
> [ 2999.890974][ T5516] RSP: 0018:ffffc9000a3c78d0 EFLAGS: 00010282
> [ 2999.920196][ T5516] RAX: 0000000000000000 RBX: ffffea002fc90e00 RCX: ffffffffa8095282
> [ 2999.958834][ T5516] RDX: 1ffffd4005f921c7 RSI: 0000000000000000 RDI: ffffea002fc90e38
> [ 2999.997374][ T5516] RBP: 1ffff92001478f22 R08: ffffed118cf0de7a R09: ffffed118cf0de7a
> [ 3000.035972][ T5516] R10: ffff888c6786f3cf R11: ffffed118cf0de79 R12: ffffea002fc90e08
> [ 3000.074836][ T5516] R13: ffffea002fc90e00 R14: ffffea002fc90e08 R15: ffffea002fc90e00
> [ 3000.113842][ T5516] FS:  00007fae1ff5e740(0000) GS:ffff888c67840000(0000) knlGS:0000000000000000
> [ 3000.156525][ T5516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3000.189099][ T5516] CR2: 00005578d493a1b8 CR3: 0000000610836005 CR4: 00000000001706e0
> [ 3000.227643][ T5516] Call Trace:
> [ 3000.243118][ T5516]  ? page_init_poison+0x40/0x40
> [ 3000.266242][ T5516]  ? lockdep_hardirqs_on_prepare+0x33e/0x4e0
> [ 3000.294621][ T5516]  ? _raw_spin_unlock_irq+0x1f/0x30
> [ 3000.322775][ T5516]  ? trace_hardirqs_on+0x20/0x1b5
> [ 3000.350565][ T5516]  ? lockdep_hardirqs_on_prepare+0x4e0/0x4e0
> [ 3000.383465][ T5516]  ? start_isolate_page_range+0x301/0x690
> [ 3000.416514][ T5516]  ? drain_all_pages+0x4a/0x7a0
> [ 3000.441585][ T5516]  ? mark_lock+0x147/0x1800
> [ 3000.463184][ T5516]  ? init_pwq+0x340/0x340
> [ 3000.483738][ T5516]  ? find_held_lock+0x33/0x1c0
> [ 3000.506497][ T5516]  ? print_irqtrace_events+0x270/0x270
> [ 3000.532505][ T5516]  ? start_isolate_page_range+0x344/0x690
> [ 3000.560632][ T5516]  ? mark_held_locks+0xb0/0x110
> [ 3000.583751][ T5516]  ? rwlock_bug.part.1+0x90/0x90
> [ 3000.610492][ T5516]  ? lockdep_hardirqs_on_prepare+0x33e/0x4e0
> [ 3000.640146][ T5516]  ? _raw_spin_unlock_irqrestore+0x39/0x40
> [ 3000.671680][ T5516]  dump_page+0x9/0x20
> [ 3000.690807][ T5516]  start_isolate_page_range+0x4ac/0x690
> [ 3000.717529][ T5516]  ? state_show+0xc0/0xc0
> [ 3000.738137][ T5516]  __offline_pages+0x199/0xe30
> [ 3000.761471][ T5516]  ? __mutex_lock+0x4aa/0x1390
> [ 3000.784203][ T5516]  ? rwlock_bug.part.1+0x90/0x90
> [ 3000.807941][ T5516]  ? device_offline+0x75/0x1c0
> [ 3000.833233][ T5516]  ? __add_memory+0x60/0x60
> [ 3000.857530][ T5516]  ? mutex_lock_io_nested+0x1270/0x1270
> [ 3000.886608][ T5516]  ? klist_next+0x253/0x430
> [ 3000.911512][ T5516]  ? device_is_dependent+0x140/0x140
> [ 3000.937751][ T5516]  ? device_for_each_child+0xe1/0x140
> [ 3000.963576][ T5516]  ? device_remove_class_symlinks+0x1a0/0x1a0
> [ 3000.992753][ T5516]  ? state_show+0xc0/0xc0
> [ 3001.013490][ T5516]  memory_subsys_offline+0xcb/0x160
> [ 3001.038346][ T5516]  device_offline+0x131/0x1c0
> [ 3001.060614][ T5516]  ? sysfs_file_ops+0x160/0x160
> [ 3001.083866][ T5516]  state_store+0x78/0xa0
> [ 3001.103882][ T5516]  kernfs_fop_write+0x208/0x410
> [ 3001.127602][ T5516]  ? __sb_start_write+0x1f8/0x2e0
> [ 3001.151596][ T5516]  vfs_write+0x1a3/0x5d0
> [ 3001.171713][ T5516]  ksys_write+0xec/0x1c0
> [ 3001.192625][ T5516]  ? __x64_sys_read+0xa0/0xa0
> [ 3001.215692][ T5516]  ? syscall_enter_from_user_mode+0x20/0x210
> [ 3001.245499][ T5516]  ? trace_hardirqs_on+0x20/0x1b5
> [ 3001.269466][ T5516]  do_syscall_64+0x33/0x40
> [ 3001.290710][ T5516]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 3001.319164][ T5516] RIP: 0033:0x7fae1f63fb28
> [ 3001.340214][ T5516] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 35 4b 2d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
> [ 3001.448642][ T5516] RSP: 002b:00007ffdad11d068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [ 3001.492000][ T5516] RAX: ffffffffffffffda RBX: 0000000000000008 RCX: 00007fae1f63fb28
> [ 3001.531309][ T5516] RDX: 0000000000000008 RSI: 00005649a73b6240 RDI: 0000000000000001
> [ 3001.571310][ T5516] RBP: 00005649a73b6240 R08: 000000000000000a R09: 00007fae1f6d0c80
> [ 3001.610126][ T5516] R10: 000000000000000a R11: 0000000000000246 R12: 00007fae1f9106c0
> [ 3001.648869][ T5516] R13: 0000000000000008 R14: 00007fae1f90b880 R15: 0000000000000008
> [ 3001.687284][ T5516] Modules linked in: loop kvm_intel kvm irqbypass ip_tables x_tables sd_mod ahci libahci tg3 firmware_class libata libphy hpsa scsi_transport_sas dm_mirror dm_region_hash dm_log dm_mod
> [ 3001.776222][ T5516] ---[ end trace 263095a22deddb65 ]---
> [ 3001.802318][ T5516] RIP: 0010:__dump_page+0xe8b/0x12f0
> [ 3001.828306][ T5516] Code: 06 49 01 c1 e9 72 f4 ff ff 48 89 df e8 5e 12 16 00 e9 09 fe ff ff 48 c7 c6 80 41 cc a8 48 89 df e8 7a f1 ff ff 0f 1f 44 00 00 <0f> 0b 48 c7 c7 80 25 cb ad e8 33 f5 71 00 48 c7 c6 80 41 cc a8 48
> [ 3001.931924][ T5516] RSP: 0018:ffffc9000a3c78d0 EFLAGS: 00010282
> [ 3001.965478][ T5516] RAX: 0000000000000000 RBX: ffffea002fc90e00 RCX: ffffffffa8095282
> [ 3002.005570][ T5516] RDX: 1ffffd4005f921c7 RSI: 0000000000000000 RDI: ffffea002fc90e38
> [ 3002.044022][ T5516] RBP: 1ffff92001478f22 R08: ffffed118cf0de7a R09: ffffed118cf0de7a
> [ 3002.082857][ T5516] R10: ffff888c6786f3cf R11: ffffed118cf0de79 R12: ffffea002fc90e08
> [ 3002.121273][ T5516] R13: ffffea002fc90e00 R14: ffffea002fc90e08 R15: ffffea002fc90e00
> [ 3002.160008][ T5516] FS:  00007fae1ff5e740(0000) GS:ffff888c67840000(0000) knlGS:0000000000000000
> [ 3002.204033][ T5516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3002.235743][ T5516] CR2: 00005578d493a1b8 CR3: 0000000610836005 CR4: 00000000001706e0
> [ 3002.274246][ T5516] Kernel panic - not syncing: Fatal exception
> [ 3002.303579][ T5516] Kernel Offset: 0x26400000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
> [ 3002.360369][ T5516] ---[ end Kernel panic - not syncing: Fatal exception ]---

[PATCH] mm, dump_page: do not crash with bad compound_page()

[John Hubbard]

If a compound page is being split while dump_page() is being run on that
page, we can end up calling compound_mapcount() on a page that is no
longer compound. This leads to a crash (already seen at least once in
the field), due to the VM_BUG_ON_PAGE() assertion inside
compound_mapcount().

(The above is from Matthew Wilcox's analysis of Qian Cai's bug report.)

In order to avoid this kind of crash, make dump_page() slightly more
robust, by providing a version of compound_page() that doesn't assert,
but just warns the first time that such a thing happens. And the first
time is usually enough.

For debug tools, we don't want to go *too* far in this direction, but
this is a simple small fix, and the crash has already been seen, so it's
a good trade-off.

Reported-by: Qian Cai <cai@lca.pw>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 include/linux/mm.h | 17 +++++++++++++++--
 mm/debug.c         |  4 ++--
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index dc7b87310c10..e3991fbb42c0 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -779,6 +779,13 @@ static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
 extern void kvfree(const void *addr);
 extern void kvfree_sensitive(const void *addr, size_t len);
 
+
+static inline int __compound_mapcount(struct page *page)
+{
+	page = compound_head(page);
+	return atomic_read(compound_mapcount_ptr(page)) + 1;
+}
+
 /*
  * Mapcount of compound page as a whole, does not include mapped sub-pages.
  *
@@ -787,8 +794,14 @@ extern void kvfree_sensitive(const void *addr, size_t len);
 static inline int compound_mapcount(struct page *page)
 {
 	VM_BUG_ON_PAGE(!PageCompound(page), page);
-	page = compound_head(page);
-	return atomic_read(compound_mapcount_ptr(page)) + 1;
+	return __compound_mapcount(page);
+}
+
+static inline int dump_page_compound_mapcount(struct page *page)
+{
+	if (WARN_ON_ONCE(!PageCompound(page)))
+		return 0;
+	return __compound_mapcount(page);
 }
 
 /*
diff --git a/mm/debug.c b/mm/debug.c
index 4f376514744d..eab4244aabd8 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -91,7 +91,7 @@ void __dump_page(struct page *page, const char *reason)
 				"compound_mapcount:%d compound_pincount:%d\n",
 				page, page_ref_count(head), mapcount,
 				mapping, page_to_pgoff(page), head,
-				compound_order(head), compound_mapcount(page),
+				compound_order(head), dump_page_compound_mapcount(page),
 				compound_pincount(page));
 		} else {
 			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
@@ -99,7 +99,7 @@ void __dump_page(struct page *page, const char *reason)
 				"compound_mapcount:%d\n",
 				page, page_ref_count(head), mapcount,
 				mapping, page_to_pgoff(page), head,
-				compound_order(head), compound_mapcount(page));
+				compound_order(head), dump_page_compound_mapcount(page));
 		}
 	else
 		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
-- 
2.28.0

Re: [PATCH] mm, dump_page: do not crash with bad compound_page()

[Matthew Wilcox]

On Tue, Aug 04, 2020 at 11:39:43AM -0700, John Hubbard wrote:
> +static inline int __compound_mapcount(struct page *page)
> +{
> +	page = compound_head(page);
> +	return atomic_read(compound_mapcount_ptr(page)) + 1;
> +}

I'd suggest instead:

static inline int head_mapcount(struct page *head)
{
	return atomic_read(compound_mapcount_ptr(head)) + 1;
}

> +static inline int dump_page_compound_mapcount(struct page *page)
> +{
> +	if (WARN_ON_ONCE(!PageCompound(page)))
> +		return 0;
> +	return __compound_mapcount(page);
>  }

And just dropping this ... it shouldn't be used outside mm/debug.c anyway.

Thinking about it, we'll get the hint later that this is not to be trusted
when the flags from the page do not include 'head'.

Re: [PATCH] mm, dump_page: do not crash with bad compound_page()

[John Hubbard]

On 8/4/20 11:48 AM, Matthew Wilcox wrote:
> On Tue, Aug 04, 2020 at 11:39:43AM -0700, John Hubbard wrote:
>> +static inline int __compound_mapcount(struct page *page)
>> +{
>> +	page = compound_head(page);
>> +	return atomic_read(compound_mapcount_ptr(page)) + 1;
>> +}
> 
> I'd suggest instead:
> 
> static inline int head_mapcount(struct page *head)
> {
> 	return atomic_read(compound_mapcount_ptr(head)) + 1;
> }
> 
>> +static inline int dump_page_compound_mapcount(struct page *page)
>> +{
>> +	if (WARN_ON_ONCE(!PageCompound(page)))
>> +		return 0;
>> +	return __compound_mapcount(page);
>>   }
> 
> And just dropping this ... it shouldn't be used outside mm/debug.c anyway.

Yes, that's cleaner. I'll send a v2 that is not a reply, so that Andrew can
spot it more easily. OK to add your "Suggested-by:" to that?

> 
> Thinking about it, we'll get the hint later that this is not to be trusted
> when the flags from the page do not include 'head'.
> 

Good point. I think this will work out pretty well, then.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] mm, dump_page: do not crash with bad compound_page()

[Matthew Wilcox]

On Tue, Aug 04, 2020 at 12:17:34PM -0700, John Hubbard wrote:
> On 8/4/20 11:48 AM, Matthew Wilcox wrote:
> > On Tue, Aug 04, 2020 at 11:39:43AM -0700, John Hubbard wrote:
> > > +static inline int __compound_mapcount(struct page *page)
> > > +{
> > > +	page = compound_head(page);
> > > +	return atomic_read(compound_mapcount_ptr(page)) + 1;
> > > +}
> > 
> > I'd suggest instead:
> > 
> > static inline int head_mapcount(struct page *head)
> > {
> > 	return atomic_read(compound_mapcount_ptr(head)) + 1;
> > }
> > 
> > > +static inline int dump_page_compound_mapcount(struct page *page)
> > > +{
> > > +	if (WARN_ON_ONCE(!PageCompound(page)))
> > > +		return 0;
> > > +	return __compound_mapcount(page);
> > >   }
> > 
> > And just dropping this ... it shouldn't be used outside mm/debug.c anyway.
> 
> Yes, that's cleaner. I'll send a v2 that is not a reply, so that Andrew can
> spot it more easily. OK to add your "Suggested-by:" to that?

Of course!  Thanks!

> > Thinking about it, we'll get the hint later that this is not to be trusted
> > when the flags from the page do not include 'head'.
> > 
> 
> Good point. I think this will work out pretty well, then.
> 
> thanks,
> -- 
> John Hubbard
> NVIDIA