[v6,12/12] mm: dump_page(): additional diagnostics for huge pinned pages
diff mbox series

Message ID 20200211001536.1027652-13-jhubbard@nvidia.com
State New
Headers show
Series
  • mm/gup: track FOLL_PIN pages
Related show

Commit Message

John Hubbard Feb. 11, 2020, 12:15 a.m. UTC
As part of pin_user_pages() and related API calls, pages are
"dma-pinned". For the case of compound pages of order > 1, the per-page
accounting of dma pins is accomplished via the 3rd struct page in the
compound page. In order to support debugging of any pin_user_pages()-
related problems, enhance dump_page() so as to report the pin count
in that case.

Documentation/core-api/pin_user_pages.rst is also updated accordingly.

Cc: Jan Kara <jack@suse.cz>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 Documentation/core-api/pin_user_pages.rst |  7 +++++++
 mm/debug.c                                | 21 ++++++++++++++++-----
 2 files changed, 23 insertions(+), 5 deletions(-)

Comments

Kirill A. Shutemov Feb. 11, 2020, 1:21 p.m. UTC | #1
On Mon, Feb 10, 2020 at 04:15:36PM -0800, John Hubbard wrote:
> As part of pin_user_pages() and related API calls, pages are
> "dma-pinned". For the case of compound pages of order > 1, the per-page
> accounting of dma pins is accomplished via the 3rd struct page in the
> compound page. In order to support debugging of any pin_user_pages()-
> related problems, enhance dump_page() so as to report the pin count
> in that case.
> 
> Documentation/core-api/pin_user_pages.rst is also updated accordingly.
> 
> Cc: Jan Kara <jack@suse.cz>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  Documentation/core-api/pin_user_pages.rst |  7 +++++++
>  mm/debug.c                                | 21 ++++++++++++++++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/core-api/pin_user_pages.rst b/Documentation/core-api/pin_user_pages.rst
> index 5c8a5f89756b..2e939ff10b86 100644
> --- a/Documentation/core-api/pin_user_pages.rst
> +++ b/Documentation/core-api/pin_user_pages.rst
> @@ -238,6 +238,13 @@ long-term [R]DMA pins in place, or during pin/unpin transitions.
>  (...unless it was already out of balance due to a long-term RDMA pin being in
>  place.)
>  
> +Other diagnostics
> +=================
> +
> +dump_page() has been enhanced slightly, to handle these new counting fields, and
> +to better report on compound pages in general. Specifically, for compound pages
> +with order > 1, the exact (hpage_pinned_refcount) pincount is reported.
> +
>  References
>  ==========
>  
> diff --git a/mm/debug.c b/mm/debug.c
> index f5ffb0784559..2189357f0987 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -85,11 +85,22 @@ 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 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));
> +		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));
> +		} 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));
> +		}

Have you considered using pr_cont() here. I guess it would be easier to
read.

You can use my Ack anyway.


>  	else
>  		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
>  			page, page_ref_count(page), mapcount,
> -- 
> 2.25.0
>
John Hubbard Feb. 12, 2020, 2:10 a.m. UTC | #2
On 2/11/20 5:21 AM, Kirill A. Shutemov wrote:
...
>> diff --git a/mm/debug.c b/mm/debug.c
>> index f5ffb0784559..2189357f0987 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -85,11 +85,22 @@ 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 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));
>> +		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));
>> +		} 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));
>> +		}
> 
> Have you considered using pr_cont() here. I guess it would be easier to
> read.

Yes, and it does have the advantage of removing some of the code duplication above. 
On the other hand, though, it leaves the end result (the long lines being printed) 
the same, and introduces a window in which the output can get garbled by another 
thread that is printk'-ing. And actually, what I'd really like is to shorten the
printed output lines, as I mentioned in [1].

So overall, given that this series has been fairly difficult to get finalized, 
and it's now in Andrew's tree at last, I'd *really* like to leave it as-is right 
now, and build on top of it. So I will submit a follow-on patch to formally propose
shortening the printed lines, and that can live or die independently of this series,
which is hopefully over now.

> 
> You can use my Ack anyway.


Thanks, and I appreciate all of your reviews and bug spotting and ideas for improvements 
on this series, it's been really helpful.


> 
> 
>>  	else
>>  		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
>>  			page, page_ref_count(page), mapcount,
>> -- 
>> 2.25.0
>>
> 


[1] https://lore.kernel.org/r/96e1f693-0e7b-2817-f13d-1946ff7654a1@nvidia.com

thanks,

Patch
diff mbox series

diff --git a/Documentation/core-api/pin_user_pages.rst b/Documentation/core-api/pin_user_pages.rst
index 5c8a5f89756b..2e939ff10b86 100644
--- a/Documentation/core-api/pin_user_pages.rst
+++ b/Documentation/core-api/pin_user_pages.rst
@@ -238,6 +238,13 @@  long-term [R]DMA pins in place, or during pin/unpin transitions.
 (...unless it was already out of balance due to a long-term RDMA pin being in
 place.)
 
+Other diagnostics
+=================
+
+dump_page() has been enhanced slightly, to handle these new counting fields, and
+to better report on compound pages in general. Specifically, for compound pages
+with order > 1, the exact (hpage_pinned_refcount) pincount is reported.
+
 References
 ==========
 
diff --git a/mm/debug.c b/mm/debug.c
index f5ffb0784559..2189357f0987 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -85,11 +85,22 @@  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 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));
+		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));
+		} 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));
+		}
 	else
 		pr_warn("page:%px refcount:%d mapcount:%d mapping:%p index:%#lx\n",
 			page, page_ref_count(page), mapcount,