[v3] mm/debug: __dump_page() prints an extra line
diff mbox series

Message ID 20191112012608.16926-1-rcampbell@nvidia.com
State New
Headers show
Series
  • [v3] mm/debug: __dump_page() prints an extra line
Related show

Commit Message

Ralph Campbell Nov. 12, 2019, 1:26 a.m. UTC
When dumping struct page information, __dump_page() prints the page type
with a trailing blank followed by the page flags on a separate line:

anon
flags: 0x100000000090034(uptodate|lru|active|head|swapbacked)

It looks like the intent was to use pr_cont() for printing "flags:"
but pr_cont() usage is discouraged so fix this by extending the format
to include the flags into a single line:

anon flags: 0x100000000090034(uptodate|lru|active|head|swapbacked)

If the page is file backed, the name might be long so use two lines:

shmem_aops name:"dev/zero"
flags: 0x10000000008000c(uptodate|dirty|swapbacked)

Eliminate pr_conf() usage as well for appending compound_mapcount.

Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
---
 mm/debug.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comments

Jerome Glisse Nov. 12, 2019, 7:55 p.m. UTC | #1
On Mon, Nov 11, 2019 at 05:26:08PM -0800, Ralph Campbell wrote:
> When dumping struct page information, __dump_page() prints the page type
> with a trailing blank followed by the page flags on a separate line:
> 
> anon
> flags: 0x100000000090034(uptodate|lru|active|head|swapbacked)
> 
> It looks like the intent was to use pr_cont() for printing "flags:"
> but pr_cont() usage is discouraged so fix this by extending the format
> to include the flags into a single line:
> 
> anon flags: 0x100000000090034(uptodate|lru|active|head|swapbacked)
> 
> If the page is file backed, the name might be long so use two lines:
> 
> shmem_aops name:"dev/zero"
> flags: 0x10000000008000c(uptodate|dirty|swapbacked)
> 
> Eliminate pr_conf() usage as well for appending compound_mapcount.
> 
> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>

Would be nice to have a changed since v1 v2 ... i was reading my
inbox in order and i saw Andrew reply after seeing the v2 ... so
where we at here ?

> ---
>  mm/debug.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 8345bb6e4769..772d4cf0691f 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -67,28 +67,31 @@ void __dump_page(struct page *page, const char *reason)
>  	 */
>  	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
>  
> -	pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx",
> -		  page, page_ref_count(page), mapcount,
> -		  page->mapping, page_to_pgoff(page));
>  	if (PageCompound(page))
> -		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
> -	pr_cont("\n");
> +		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
> +			"index:%#lx compound_mapcount: %d\n",
> +			page, page_ref_count(page), mapcount,
> +			page->mapping, page_to_pgoff(page),
> +			compound_mapcount(page));
> +	else
> +		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
> +			page, page_ref_count(page), mapcount,
> +			page->mapping, page_to_pgoff(page));
>  	if (PageAnon(page))
> -		pr_warn("anon ");
> +		pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
>  	else if (PageKsm(page))
> -		pr_warn("ksm ");
> +		pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
>  	else if (mapping) {
> -		pr_warn("%ps ", mapping->a_ops);
>  		if (mapping->host && mapping->host->i_dentry.first) {
>  			struct dentry *dentry;
>  			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
> -			pr_warn("name:\"%pd\" ", dentry);
> -		}
> +			pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
> +		} else
> +			pr_warn("%ps\n", mapping->a_ops);
> +		pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
>  	}
>  	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>  
> -	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
> -
>  hex_only:
>  	print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>  			sizeof(unsigned long), page,
> -- 
> 2.20.1
> 
>
Ralph Campbell Nov. 12, 2019, 9:35 p.m. UTC | #2
On 11/12/19 11:55 AM, Jerome Glisse wrote:
> On Mon, Nov 11, 2019 at 05:26:08PM -0800, Ralph Campbell wrote:
>> When dumping struct page information, __dump_page() prints the page type
>> with a trailing blank followed by the page flags on a separate line:
>>
>> anon
>> flags: 0x100000000090034(uptodate|lru|active|head|swapbacked)
>>
>> It looks like the intent was to use pr_cont() for printing "flags:"
>> but pr_cont() usage is discouraged so fix this by extending the format
>> to include the flags into a single line:
>>
>> anon flags: 0x100000000090034(uptodate|lru|active|head|swapbacked)
>>
>> If the page is file backed, the name might be long so use two lines:
>>
>> shmem_aops name:"dev/zero"
>> flags: 0x10000000008000c(uptodate|dirty|swapbacked)
>>
>> Eliminate pr_conf() usage as well for appending compound_mapcount.
>>
>> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
> 
> Would be nice to have a changed since v1 v2 ... i was reading my
> inbox in order and i saw Andrew reply after seeing the v2 ... so
> where we at here ?

V3 is the latest and looks like Andrew has queued it for mm.
My bad, I should have had the version changes listed in the cover
letter portion of the email.
Andrew commented on v1, v2 simply fixed the subject line which
was truncated in v1, and v3 is as you see.

Actually, I now see another bug which I'll post separately:
The "if (PageAnon(page))" will always trigger so that
"else if (PageKsm(page))" will never be executed.

>> ---
>>   mm/debug.c | 27 +++++++++++++++------------
>>   1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/debug.c b/mm/debug.c
>> index 8345bb6e4769..772d4cf0691f 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -67,28 +67,31 @@ void __dump_page(struct page *page, const char *reason)
>>   	 */
>>   	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
>>   
>> -	pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx",
>> -		  page, page_ref_count(page), mapcount,
>> -		  page->mapping, page_to_pgoff(page));
>>   	if (PageCompound(page))
>> -		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
>> -	pr_cont("\n");
>> +		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
>> +			"index:%#lx compound_mapcount: %d\n",
>> +			page, page_ref_count(page), mapcount,
>> +			page->mapping, page_to_pgoff(page),
>> +			compound_mapcount(page));
>> +	else
>> +		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
>> +			page, page_ref_count(page), mapcount,
>> +			page->mapping, page_to_pgoff(page));
>>   	if (PageAnon(page))
>> -		pr_warn("anon ");
>> +		pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
>>   	else if (PageKsm(page))
>> -		pr_warn("ksm ");
>> +		pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
>>   	else if (mapping) {
>> -		pr_warn("%ps ", mapping->a_ops);
>>   		if (mapping->host && mapping->host->i_dentry.first) {
>>   			struct dentry *dentry;
>>   			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
>> -			pr_warn("name:\"%pd\" ", dentry);
>> -		}
>> +			pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
>> +		} else
>> +			pr_warn("%ps\n", mapping->a_ops);
>> +		pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
>>   	}
>>   	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
>>   
>> -	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
>> -
>>   hex_only:
>>   	print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>>   			sizeof(unsigned long), page,
>> -- 
>> 2.20.1
>>
>>
>

Patch
diff mbox series

diff --git a/mm/debug.c b/mm/debug.c
index 8345bb6e4769..772d4cf0691f 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -67,28 +67,31 @@  void __dump_page(struct page *page, const char *reason)
 	 */
 	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
 
-	pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx",
-		  page, page_ref_count(page), mapcount,
-		  page->mapping, page_to_pgoff(page));
 	if (PageCompound(page))
-		pr_cont(" compound_mapcount: %d", compound_mapcount(page));
-	pr_cont("\n");
+		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px "
+			"index:%#lx compound_mapcount: %d\n",
+			page, page_ref_count(page), mapcount,
+			page->mapping, page_to_pgoff(page),
+			compound_mapcount(page));
+	else
+		pr_warn("page:%px refcount:%d mapcount:%d mapping:%px index:%#lx\n",
+			page, page_ref_count(page), mapcount,
+			page->mapping, page_to_pgoff(page));
 	if (PageAnon(page))
-		pr_warn("anon ");
+		pr_warn("anon flags: %#lx(%pGp)\n", page->flags, &page->flags);
 	else if (PageKsm(page))
-		pr_warn("ksm ");
+		pr_warn("ksm flags: %#lx(%pGp)\n", page->flags, &page->flags);
 	else if (mapping) {
-		pr_warn("%ps ", mapping->a_ops);
 		if (mapping->host && mapping->host->i_dentry.first) {
 			struct dentry *dentry;
 			dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias);
-			pr_warn("name:\"%pd\" ", dentry);
-		}
+			pr_warn("%ps name:\"%pd\"\n", mapping->a_ops, dentry);
+		} else
+			pr_warn("%ps\n", mapping->a_ops);
+		pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
 	}
 	BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
 
-	pr_warn("flags: %#lx(%pGp)\n", page->flags, &page->flags);
-
 hex_only:
 	print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
 			sizeof(unsigned long), page,