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

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jun 29, 2020 at 02:55:21PM -0700, John Hubbard wrote:
> Cool! I've got a few minor feedback comments coming for the individual
> patches, but an easy question first:
> 
> For the FOLL_PIN pages, I left in a minor mess: the lines are basically
> 2x too long. Would you be interested in folding in something approximately
> like this, below, to one of your patches? It's just a line break, effectively.
> It generates output like this:
> 
> [  260.545212] page:0000000035202f6e refcount:513 mapcount:1 mapping:0000000000000000 index:0x0
> [  260.552834]      head:0000000035202f6e order:9 compound_mapcount:1 compound_pincount:512
> [  260.560087] anon flags: 0x17ffe000001000e(referenced|uptodate|dirty|head)
> [  260.565993] raw: 017ffe000001000e ffffffff83649ca0 ffffea0020758008 ffff888890f03839
> [  260.572888] raw: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
> [  260.579745] page dumped because: gup_benchmark: head page: dump_page test
> 
> (I'm not sure if the indentation is a good idea or not, actually.)
> 
> Or if it's too much fuss, I can send it separately with your series as a
> prerequisite:
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index fcf3a16902b2..d522bf24e3ff 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -87,19 +87,22 @@ void __dump_page(struct page *page, const char *reason)
>  	if (compound)
>  		if (hpage_pincount_available(page)) {
>  			pr_warn("page:%p refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%p order:%u "
> -				"compound_mapcount:%d compound_pincount:%d\n",
> +				"index:%#lx\n",
>  				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page),
> +				mapping, page_to_pgoff(page));
> +			pr_warn("     head:%p order:%u compound_mapcount:%d "
> +				"compound_pincount:%d\n",
> +				head, compound_order(head),
> +				compound_mapcount(page),
>  				compound_pincount(page));
>  		} else {
>  			pr_warn("page:%p refcount:%d mapcount:%d mapping:%p "
> -				"index:%#lx head:%p order:%u "
> -				"compound_mapcount:%d\n",
> -				page, page_ref_count(head), mapcount,
> -				mapping, page_to_pgoff(page), head,
> -				compound_order(head), compound_mapcount(page));
> +				"index:%#lx\n",
> +				page, page_ref_count(head), mapcount, mapping,
> +				page_to_pgoff(page));
> +			pr_warn("     head:%p order:%u compound_mapcount:%d\n",
> +				head, compound_order(head),
> +				compound_mapcount(page));
>  		}

Hmm ... If we're going to two lines, then I'd rather do something like this:

+++ b/mm/debug.c
@@ -84,27 +84,22 @@ void __dump_page(struct page *page, const char *reason)
         */
        mapcount = PageSlab(head) ? 0 : page_mapcount(page);
 
-       if (compound)
+       pr_warn("page:%p 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:%p refcount:%d mapcount:%d mapping:%p "
-                               "index:%#lx head:%p 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:%p order:%u compound_mapcount:%d compound_pincount:%d\n",
+                                       head, compound_order(head),
+                                       compound_mapcount(page),
+                                       compound_pincount(page));
                } else {
-                       pr_warn("page:%p refcount:%d mapcount:%d mapping:%p "
-                               "index:%#lx head:%p 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:%p order:%u compound_mapcount:%d\n",
+                                       head, compound_order(head),
+                                       compound_mapcount(page));
                }
-       else
-               pr_warn("page:%p 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))

I haven't dumped a page with this yet, so I may change my mind, but
this seems like a good reduction in the amount of redundant code in
this function.