Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/6] Improvements for dump_page()
@ 2020-07-09 20:21 Matthew Wilcox (Oracle)
  2020-07-09 20:21 ` [PATCH v2 1/6] mm: Handle page->mapping better in dump_page Matthew Wilcox (Oracle)
                   ` (7 more replies)
  0 siblings, 8 replies; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-07-09 20:21 UTC (permalink / raw)
  To: linux-mm
  Cc: Matthew Wilcox (Oracle),
	Andrew Morton, Vlastimil Babka, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 1/6] mm: Handle page->mapping better in dump_page
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
@ 2020-07-09 20:21 ` Matthew Wilcox (Oracle)
  2020-07-11  1:30   ` John Hubbard
  2020-07-14 11:06   ` Vlastimil Babka
  2020-07-09 20:21 ` [PATCH v2 2/6] mm: Dump compound page information on a second line Matthew Wilcox (Oracle)
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-07-09 20:21 UTC (permalink / raw)
  To: linux-mm
  Cc: Matthew Wilcox (Oracle),
	Andrew Morton, Vlastimil Babka, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 2/6] mm: Dump compound page information on a second line
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
  2020-07-09 20:21 ` [PATCH v2 1/6] mm: Handle page->mapping better in dump_page Matthew Wilcox (Oracle)
@ 2020-07-09 20:21 ` Matthew Wilcox (Oracle)
  2020-07-11  1:35   ` John Hubbard
                     ` (2 more replies)
  2020-07-09 20:21 ` [PATCH v2 3/6] mm: Print head flags in dump_page Matthew Wilcox (Oracle)
                   ` (5 subsequent siblings)
  7 siblings, 3 replies; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-07-09 20:21 UTC (permalink / raw)
  To: linux-mm
  Cc: Matthew Wilcox (Oracle),
	Andrew Morton, Vlastimil Babka, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 3/6] mm: Print head flags in dump_page
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
  2020-07-09 20:21 ` [PATCH v2 1/6] mm: Handle page->mapping better in dump_page Matthew Wilcox (Oracle)
  2020-07-09 20:21 ` [PATCH v2 2/6] mm: Dump compound page information on a second line Matthew Wilcox (Oracle)
@ 2020-07-09 20:21 ` Matthew Wilcox (Oracle)
  2020-07-11  1:40   ` John Hubbard
  2020-07-14 12:06   ` Vlastimil Babka
  2020-07-09 20:21 ` [PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault Matthew Wilcox (Oracle)
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-07-09 20:21 UTC (permalink / raw)
  To: linux-mm
  Cc: Matthew Wilcox (Oracle),
	Andrew Morton, Vlastimil Babka, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
                   ` (2 preceding siblings ...)
  2020-07-09 20:21 ` [PATCH v2 3/6] mm: Print head flags in dump_page Matthew Wilcox (Oracle)
@ 2020-07-09 20:21 ` Matthew Wilcox (Oracle)
  2020-07-14 12:11   ` Vlastimil Babka
  2020-07-09 20:21 ` [PATCH v2 5/6] mm: Print the inode number in dump_page Matthew Wilcox (Oracle)
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-07-09 20:21 UTC (permalink / raw)
  To: linux-mm
  Cc: Matthew Wilcox (Oracle),
	Andrew Morton, Vlastimil Babka, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 5/6] mm: Print the inode number in dump_page
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
                   ` (3 preceding siblings ...)
  2020-07-09 20:21 ` [PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault Matthew Wilcox (Oracle)
@ 2020-07-09 20:21 ` Matthew Wilcox (Oracle)
  2020-07-11  2:04   ` John Hubbard
  2020-07-14 12:18   ` Vlastimil Babka
  2020-07-09 20:21 ` [PATCH v2 6/6] mm: Print hashed address of struct page Matthew Wilcox (Oracle)
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-07-09 20:21 UTC (permalink / raw)
  To: linux-mm
  Cc: Matthew Wilcox (Oracle),
	Andrew Morton, Vlastimil Babka, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 6/6] mm: Print hashed address of struct page
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
                   ` (4 preceding siblings ...)
  2020-07-09 20:21 ` [PATCH v2 5/6] mm: Print the inode number in dump_page Matthew Wilcox (Oracle)
@ 2020-07-09 20:21 ` Matthew Wilcox (Oracle)
  2020-07-11  1:48   ` John Hubbard
  2020-07-09 20:54 ` [PATCH v2 0/6] Improvements for dump_page() Mike Rapoport
  2020-07-09 20:54 ` William Kucharski
  7 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox (Oracle) @ 2020-07-09 20:21 UTC (permalink / raw)
  To: linux-mm
  Cc: Matthew Wilcox (Oracle),
	Andrew Morton, Vlastimil Babka, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 0/6] Improvements for dump_page()
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
                   ` (5 preceding siblings ...)
  2020-07-09 20:21 ` [PATCH v2 6/6] mm: Print hashed address of struct page Matthew Wilcox (Oracle)
@ 2020-07-09 20:54 ` Mike Rapoport
  2020-07-09 20:54 ` William Kucharski
  7 siblings, 0 replies; 26+ messages in thread
From: Mike Rapoport @ 2020-07-09 20:54 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle)
  Cc: linux-mm, Andrew Morton, Vlastimil Babka, John Hubbard,
	Kirill A. Shutemov, William Kucharski

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.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 0/6] Improvements for dump_page()
  2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
                   ` (6 preceding siblings ...)
  2020-07-09 20:54 ` [PATCH v2 0/6] Improvements for dump_page() Mike Rapoport
@ 2020-07-09 20:54 ` William Kucharski
  7 siblings, 0 replies; 26+ messages in thread
From: William Kucharski @ 2020-07-09 20:54 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle)
  Cc: linux-mm, Andrew Morton, Vlastimil Babka, John Hubbard,
	Kirill A. Shutemov, Mike Rapoport

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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/6] mm: Handle page->mapping better in dump_page
  2020-07-09 20:21 ` [PATCH v2 1/6] mm: Handle page->mapping better in dump_page Matthew Wilcox (Oracle)
@ 2020-07-11  1:30   ` John Hubbard
  2020-07-11  1:32     ` John Hubbard
  2020-07-14 11:06   ` Vlastimil Babka
  1 sibling, 1 reply; 26+ messages in thread
From: John Hubbard @ 2020-07-11  1:30 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, Vlastimil Babka, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/6] mm: Handle page->mapping better in dump_page
  2020-07-11  1:30   ` John Hubbard
@ 2020-07-11  1:32     ` John Hubbard
  0 siblings, 0 replies; 26+ messages in thread
From: John Hubbard @ 2020-07-11  1:32 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, Vlastimil Babka, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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 {
>>
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/6] mm: Dump compound page information on a second line
  2020-07-09 20:21 ` [PATCH v2 2/6] mm: Dump compound page information on a second line Matthew Wilcox (Oracle)
@ 2020-07-11  1:35   ` John Hubbard
  2020-07-14 12:03   ` Vlastimil Babka
  2020-08-04 15:37   ` Qian Cai
  2 siblings, 0 replies; 26+ messages in thread
From: John Hubbard @ 2020-07-11  1:35 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, Vlastimil Babka, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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))
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 3/6] mm: Print head flags in dump_page
  2020-07-09 20:21 ` [PATCH v2 3/6] mm: Print head flags in dump_page Matthew Wilcox (Oracle)
@ 2020-07-11  1:40   ` John Hubbard
  2020-07-14 12:06   ` Vlastimil Babka
  1 sibling, 0 replies; 26+ messages in thread
From: John Hubbard @ 2020-07-11  1:40 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, Vlastimil Babka, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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:
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 6/6] mm: Print hashed address of struct page
  2020-07-09 20:21 ` [PATCH v2 6/6] mm: Print hashed address of struct page Matthew Wilcox (Oracle)
@ 2020-07-11  1:48   ` John Hubbard
  0 siblings, 0 replies; 26+ messages in thread
From: John Hubbard @ 2020-07-11  1:48 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, Vlastimil Babka, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 5/6] mm: Print the inode number in dump_page
  2020-07-09 20:21 ` [PATCH v2 5/6] mm: Print the inode number in dump_page Matthew Wilcox (Oracle)
@ 2020-07-11  2:04   ` John Hubbard
  2020-07-14 12:18   ` Vlastimil Babka
  1 sibling, 0 replies; 26+ messages in thread
From: John Hubbard @ 2020-07-11  2:04 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, Vlastimil Babka, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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:
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 1/6] mm: Handle page->mapping better in dump_page
  2020-07-09 20:21 ` [PATCH v2 1/6] mm: Handle page->mapping better in dump_page Matthew Wilcox (Oracle)
  2020-07-11  1:30   ` John Hubbard
@ 2020-07-14 11:06   ` Vlastimil Babka
  1 sibling, 0 replies; 26+ messages in thread
From: Vlastimil Babka @ 2020-07-14 11:06 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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 {
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/6] mm: Dump compound page information on a second line
  2020-07-09 20:21 ` [PATCH v2 2/6] mm: Dump compound page information on a second line Matthew Wilcox (Oracle)
  2020-07-11  1:35   ` John Hubbard
@ 2020-07-14 12:03   ` Vlastimil Babka
  2020-08-04 15:37   ` Qian Cai
  2 siblings, 0 replies; 26+ messages in thread
From: Vlastimil Babka @ 2020-07-14 12:03 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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))
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 3/6] mm: Print head flags in dump_page
  2020-07-09 20:21 ` [PATCH v2 3/6] mm: Print head flags in dump_page Matthew Wilcox (Oracle)
  2020-07-11  1:40   ` John Hubbard
@ 2020-07-14 12:06   ` Vlastimil Babka
  1 sibling, 0 replies; 26+ messages in thread
From: Vlastimil Babka @ 2020-07-14 12:06 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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:
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault
  2020-07-09 20:21 ` [PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault Matthew Wilcox (Oracle)
@ 2020-07-14 12:11   ` Vlastimil Babka
  0 siblings, 0 replies; 26+ messages in thread
From: Vlastimil Babka @ 2020-07-14 12:11 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 5/6] mm: Print the inode number in dump_page
  2020-07-09 20:21 ` [PATCH v2 5/6] mm: Print the inode number in dump_page Matthew Wilcox (Oracle)
  2020-07-11  2:04   ` John Hubbard
@ 2020-07-14 12:18   ` Vlastimil Babka
  1 sibling, 0 replies; 26+ messages in thread
From: Vlastimil Babka @ 2020-07-14 12:18 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle), linux-mm
  Cc: Andrew Morton, John Hubbard, Kirill A. Shutemov,
	William Kucharski, Mike Rapoport

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:
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/6] mm: Dump compound page information on a second line
  2020-07-09 20:21 ` [PATCH v2 2/6] mm: Dump compound page information on a second line Matthew Wilcox (Oracle)
  2020-07-11  1:35   ` John Hubbard
  2020-07-14 12:03   ` Vlastimil Babka
@ 2020-08-04 15:37   ` Qian Cai
  2020-08-04 16:14     ` Matthew Wilcox
  2 siblings, 1 reply; 26+ messages in thread
From: Qian Cai @ 2020-08-04 15:37 UTC (permalink / raw)
  To: Matthew Wilcox (Oracle)
  Cc: linux-mm, Andrew Morton, Vlastimil Babka, John Hubbard,
	Kirill A. Shutemov, William Kucharski, Mike Rapoport

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
> 
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 2/6] mm: Dump compound page information on a second line
  2020-08-04 15:37   ` Qian Cai
@ 2020-08-04 16:14     ` Matthew Wilcox
  2020-08-04 18:39       ` [PATCH] mm, dump_page: do not crash with bad compound_page() John Hubbard
  0 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox @ 2020-08-04 16:14 UTC (permalink / raw)
  To: Qian Cai
  Cc: linux-mm, Andrew Morton, Vlastimil Babka, John Hubbard,
	Kirill A. Shutemov, William Kucharski, Mike Rapoport

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 ]---


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] mm, dump_page: do not crash with bad compound_page()
  2020-08-04 16:14     ` Matthew Wilcox
@ 2020-08-04 18:39       ` John Hubbard
  2020-08-04 18:48         ` Matthew Wilcox
  0 siblings, 1 reply; 26+ messages in thread
From: John Hubbard @ 2020-08-04 18:39 UTC (permalink / raw)
  To: willy
  Cc: akpm, cai, jhubbard, kirill, linux-mm, rppt, vbabka,
	william.kucharski, Kirill A . Shutemov

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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] mm, dump_page: do not crash with bad compound_page()
  2020-08-04 18:39       ` [PATCH] mm, dump_page: do not crash with bad compound_page() John Hubbard
@ 2020-08-04 18:48         ` Matthew Wilcox
  2020-08-04 19:17           ` John Hubbard
  0 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox @ 2020-08-04 18:48 UTC (permalink / raw)
  To: John Hubbard
  Cc: akpm, cai, kirill, linux-mm, rppt, vbabka, william.kucharski,
	Kirill A . Shutemov

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'.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] mm, dump_page: do not crash with bad compound_page()
  2020-08-04 18:48         ` Matthew Wilcox
@ 2020-08-04 19:17           ` John Hubbard
  2020-08-04 19:42             ` Matthew Wilcox
  0 siblings, 1 reply; 26+ messages in thread
From: John Hubbard @ 2020-08-04 19:17 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: akpm, cai, kirill, linux-mm, rppt, vbabka, william.kucharski,
	Kirill A . Shutemov

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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] mm, dump_page: do not crash with bad compound_page()
  2020-08-04 19:17           ` John Hubbard
@ 2020-08-04 19:42             ` Matthew Wilcox
  0 siblings, 0 replies; 26+ messages in thread
From: Matthew Wilcox @ 2020-08-04 19:42 UTC (permalink / raw)
  To: John Hubbard
  Cc: akpm, cai, kirill, linux-mm, rppt, vbabka, william.kucharski,
	Kirill A . Shutemov

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


^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 20:21 [PATCH v2 0/6] Improvements for dump_page() Matthew Wilcox (Oracle)
2020-07-09 20:21 ` [PATCH v2 1/6] mm: Handle page->mapping better in dump_page Matthew Wilcox (Oracle)
2020-07-11  1:30   ` John Hubbard
2020-07-11  1:32     ` John Hubbard
2020-07-14 11:06   ` Vlastimil Babka
2020-07-09 20:21 ` [PATCH v2 2/6] mm: Dump compound page information on a second line Matthew Wilcox (Oracle)
2020-07-11  1:35   ` John Hubbard
2020-07-14 12:03   ` Vlastimil Babka
2020-08-04 15:37   ` Qian Cai
2020-08-04 16:14     ` Matthew Wilcox
2020-08-04 18:39       ` [PATCH] mm, dump_page: do not crash with bad compound_page() John Hubbard
2020-08-04 18:48         ` Matthew Wilcox
2020-08-04 19:17           ` John Hubbard
2020-08-04 19:42             ` Matthew Wilcox
2020-07-09 20:21 ` [PATCH v2 3/6] mm: Print head flags in dump_page Matthew Wilcox (Oracle)
2020-07-11  1:40   ` John Hubbard
2020-07-14 12:06   ` Vlastimil Babka
2020-07-09 20:21 ` [PATCH v2 4/6] mm: Switch dump_page to get_kernel_nofault Matthew Wilcox (Oracle)
2020-07-14 12:11   ` Vlastimil Babka
2020-07-09 20:21 ` [PATCH v2 5/6] mm: Print the inode number in dump_page Matthew Wilcox (Oracle)
2020-07-11  2:04   ` John Hubbard
2020-07-14 12:18   ` Vlastimil Babka
2020-07-09 20:21 ` [PATCH v2 6/6] mm: Print hashed address of struct page Matthew Wilcox (Oracle)
2020-07-11  1:48   ` John Hubbard
2020-07-09 20:54 ` [PATCH v2 0/6] Improvements for dump_page() Mike Rapoport
2020-07-09 20:54 ` William Kucharski

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git