[PATCH 0/3] Improvements for dump_page()

[PATCH 0/3] Improvements for dump_page()

[Matthew Wilcox (Oracle)]

I've been looking at a lot of calls to dump_page() recently due to the
THP work, and these three patches make my life easier.  Please consider
applying.

Matthew Wilcox (Oracle) (3):
  mm: Print head flags in dump_page
  mm: Print the inode number in dump_page
  mm: Print hashed address of struct page

 mm/debug.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

-- 
2.27.0

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

[Matthew Wilcox (Oracle)]

Tail page flags contain very little useful information.  Print the head
page's flags instead (even though PageHead is a little misleading).

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 4f376514744d..44d843b3b07c 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -163,7 +163,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

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

[John Hubbard]

On 2020-06-29 08:19, Matthew Wilcox (Oracle) wrote:
> Tail page flags contain very little useful information.  Print the head
> page's flags instead (even though PageHead is a little misleading).

You are right about the tail page. And the raw output provides the tail
page flags, in case someone *really* needs to dig into tail page problems,
so that's all good.

However, I just gave this a spin, and seeing the "|head" in the list for
my "tail page: dump_page test" is also slightly misleading for me, too.

Sure, one can eventually work through it and conclude "my dump_page output
is misleading", but I think a small tweak would avoid that:

> 
> 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 4f376514744d..44d843b3b07c 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -163,7 +163,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" : "");


Perhaps just adding "head " to the above will clear it all up:

diff --git a/mm/debug.c b/mm/debug.c
index fcf3a16902b2..b7ac8af71940 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -165,7 +165,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, head->flags, &head->flags,
+	pr_warn("head %sflags: %#lx(%pGp)%s\n", type, head->flags, &head->flags,
  		page_cma ? " CMA" : "");

  hex_only:


Sample output:

[   62.064271] page:000000001c8f0c3e refcount:513 mapcount:1 mapping:0000000000000000 index:0x11 
head:00000000ba9ccef2 order:9 compound_mapcount:1 compound_pincount:512
[   62.078432] head anon flags: 0x17ffe000001000e(referenced|uptodate|dirty|head)
[   62.088454] raw: 017ffe0000000000 ffffea00209e8001 ffffea00209e8448 dead000000000400
[   62.095356] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[   62.102289] head: 017ffe000001000e ffffffff83649ca0 ffffea00209e0008 ffff88889663c001
[   62.109286] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
[   62.116301] page dumped because: gup_benchmark: tail page: dump_page test



thanks,
-- 
John Hubbard
NVIDIA

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

[Matthew Wilcox]

On Mon, Jun 29, 2020 at 03:38:13PM -0700, John Hubbard wrote:
> On 2020-06-29 08:19, Matthew Wilcox (Oracle) wrote:
> > Tail page flags contain very little useful information.  Print the head
> > page's flags instead (even though PageHead is a little misleading).
> 
> You are right about the tail page. And the raw output provides the tail
> page flags, in case someone *really* needs to dig into tail page problems,
> so that's all good.
> 
> However, I just gave this a spin, and seeing the "|head" in the list for
> my "tail page: dump_page test" is also slightly misleading for me, too.

We could also do ...

@@ -48,6 +48,8 @@ void __dump_page(struct page *page, const char *reason)
        struct address_space *mapping;
        bool page_poisoned = PagePoisoned(page);
        bool compound = PageCompound(page);
+       unsigned long flags = page->flags;
+
        /*
         * Accessing the pageblock without the zone lock. It could change to
         * "isolate" again in the meantime, but since we are just dumping the
@@ -165,7 +162,9 @@ 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, head->flags, &head->flags,
+       if (head != page)
+               flags = head->flags & ~PG_head;
+       pr_warn("%sflags: %#lx(%pGp)%s\n", type, flags, &flags,
                page_cma ? " CMA" : "");
 
 hex_only:

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

[John Hubbard]

On 2020-06-29 15:51, Matthew Wilcox wrote:
> On Mon, Jun 29, 2020 at 03:38:13PM -0700, John Hubbard wrote:
>> On 2020-06-29 08:19, Matthew Wilcox (Oracle) wrote:
>>> Tail page flags contain very little useful information.  Print the head
>>> page's flags instead (even though PageHead is a little misleading).
>>
>> You are right about the tail page. And the raw output provides the tail
>> page flags, in case someone *really* needs to dig into tail page problems,
>> so that's all good.
>>
>> However, I just gave this a spin, and seeing the "|head" in the list for
>> my "tail page: dump_page test" is also slightly misleading for me, too.
> 
> We could also do ...
> 
> @@ -48,6 +48,8 @@ void __dump_page(struct page *page, const char *reason)
>          struct address_space *mapping;
>          bool page_poisoned = PagePoisoned(page);
>          bool compound = PageCompound(page);
> +       unsigned long flags = page->flags;
> +
>          /*
>           * Accessing the pageblock without the zone lock. It could change to
>           * "isolate" again in the meantime, but since we are just dumping the
> @@ -165,7 +162,9 @@ 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, head->flags, &head->flags,
> +       if (head != page)
> +               flags = head->flags & ~PG_head;
> +       pr_warn("%sflags: %#lx(%pGp)%s\n", type, flags, &flags,
>                  page_cma ? " CMA" : "");
>   

OK, I'll give this a spin, along with your line break approach...

thanks,
-- 
John Hubbard
NVIDIA

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

[John Hubbard]

On 2020-06-29 15:51, Matthew Wilcox wrote:
> On Mon, Jun 29, 2020 at 03:38:13PM -0700, John Hubbard wrote:
>> On 2020-06-29 08:19, Matthew Wilcox (Oracle) wrote:
>>> Tail page flags contain very little useful information.  Print the head
>>> page's flags instead (even though PageHead is a little misleading).
>>
>> You are right about the tail page. And the raw output provides the tail
>> page flags, in case someone *really* needs to dig into tail page problems,
>> so that's all good.
>>
>> However, I just gave this a spin, and seeing the "|head" in the list for
>> my "tail page: dump_page test" is also slightly misleading for me, too.
> 
> We could also do ...
> 
> @@ -48,6 +48,8 @@ void __dump_page(struct page *page, const char *reason)
>          struct address_space *mapping;
>          bool page_poisoned = PagePoisoned(page);
>          bool compound = PageCompound(page);
> +       unsigned long flags = page->flags;
> +
>          /*
>           * Accessing the pageblock without the zone lock. It could change to
>           * "isolate" again in the meantime, but since we are just dumping the
> @@ -165,7 +162,9 @@ 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, head->flags, &head->flags,
> +       if (head != page)
> +               flags = head->flags & ~PG_head;

The above should be:

                    flags = head->flags & ~(1UL << PG_head);


> +       pr_warn("%sflags: %#lx(%pGp)%s\n", type, flags, &flags,
>                  page_cma ? " CMA" : "");
>   
>   hex_only:
> 

...so with that fix, along with your line break approach in the other thread,
a tail page dump of a FOLL_PIN page looks like this:

[   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
[   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
[   38.049155] anon flags: 0x17ffe000000000e(referenced|uptodate|dirty)
[   38.055465] raw: 017ffe0000000000 ffffea0020dd0001 ffffea0020dd0448 dead000000000400
[   38.062319] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[   38.069183] head: 017ffe000001000e ffffffff83649ca0 ffffea0020dd8008 ffff88888e0b6641
[   38.076141] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
[   38.083102] page dumped because: gup_benchmark: tail page: dump_page test

So, good. However, I feel that the "head " prefix approach is slightly
preferable, because it's doing less processing (the more code one
adds to little-exercised debug paths, the more likely the debugging has
bugs) and is instead just printing out what it sees directly. And it seems a little
odd to remove the PG_head bit from the output.

The "head " prefix approach looks like this:


[   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
[   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
[   38.049155] head anon flags: 0x17ffe000000000e(referenced|uptodate|dirty|head)
[   38.055465] raw: 017ffe0000000000 ffffea0020dd0001 ffffea0020dd0448 dead000000000400
[   38.062319] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[   38.069183] head: 017ffe000001000e ffffffff83649ca0 ffffea0020dd8008 ffff88888e0b6641
[   38.076141] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
[   38.083102] page dumped because: gup_benchmark: tail page: dump_page test




thanks,
-- 
John Hubbard
NVIDIA

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

[Vlastimil Babka]

On 6/30/20 1:35 AM, John Hubbard wrote:
>> +       pr_warn("%sflags: %#lx(%pGp)%s\n", type, flags, &flags,
>>                  page_cma ? " CMA" : "");
>>   
>>   hex_only:
>> 
> 
> ...so with that fix, along with your line break approach in the other thread,
> a tail page dump of a FOLL_PIN page looks like this:
> 
> [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
> [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
> [   38.049155] anon flags: 0x17ffe000000000e(referenced|uptodate|dirty)
> [   38.055465] raw: 017ffe0000000000 ffffea0020dd0001 ffffea0020dd0448 dead000000000400
> [   38.062319] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
> [   38.069183] head: 017ffe000001000e ffffffff83649ca0 ffffea0020dd8008 ffff88888e0b6641
> [   38.076141] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
> [   38.083102] page dumped because: gup_benchmark: tail page: dump_page test
> 
> So, good. However, I feel that the "head " prefix approach is slightly
> preferable, because it's doing less processing (the more code one
> adds to little-exercised debug paths, the more likely the debugging has
> bugs) and is instead just printing out what it sees directly. And it seems a little
> odd to remove the PG_head bit from the output.
> 
> The "head " prefix approach looks like this:

I would also prefer this approach.It would be also nice to know the tail index.
As long as page pointer wasn't hashed, it was possible to figure this out, but
now it's not. Maybe print pfn of both page and head?

> [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
> [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
> [   38.049155] head anon flags: 0x17ffe000000000e(referenced|uptodate|dirty|head)
> [   38.055465] raw: 017ffe0000000000 ffffea0020dd0001 ffffea0020dd0448 dead000000000400
> [   38.062319] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
> [   38.069183] head: 017ffe000001000e ffffffff83649ca0 ffffea0020dd8008 ffff88888e0b6641
> [   38.076141] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
> [   38.083102] page dumped because: gup_benchmark: tail page: dump_page test
> 
> 
> 
> 
> thanks,
> 

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

[Matthew Wilcox]

On Tue, Jun 30, 2020 at 10:02:50AM +0200, Vlastimil Babka wrote:
> I would also prefer this approach.It would be also nice to know the tail index.
> As long as page pointer wasn't hashed, it was possible to figure this out, but
> now it's not. Maybe print pfn of both page and head?

> On 6/30/20 1:35 AM, John Hubbard wrote:
> > ...so with that fix, along with your line break approach in the other thread,
> > a tail page dump of a FOLL_PIN page looks like this:
> > 
> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11

index is the last thing printed on this line.  If it's something like
index:0x12345678, you can reduce it mod 1<<order, as printed on the next
line.

> > [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512

> > [   38.049155] anon flags: 0x17ffe000000000e(referenced|uptodate|dirty)
> > [   38.055465] raw: 017ffe0000000000 ffffea0020dd0001 ffffea0020dd0448 dead000000000400
> > [   38.062319] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
> > [   38.069183] head: 017ffe000001000e ffffffff83649ca0 ffffea0020dd8008 ffff88888e0b6641
> > [   38.076141] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
> > [   38.083102] page dumped because: gup_benchmark: tail page: dump_page test
> > 
> > So, good. However, I feel that the "head " prefix approach is slightly
> > preferable, because it's doing less processing (the more code one
> > adds to little-exercised debug paths, the more likely the debugging has
> > bugs) and is instead just printing out what it sees directly. And it seems a little
> > odd to remove the PG_head bit from the output.
> > 
> > The "head " prefix approach looks like this:
> 
> I would also prefer this approach.It would be also nice to know the tail index.
> As long as page pointer wasn't hashed, it was possible to figure this out, but
> now it's not. Maybe print pfn of both page and head?
> 
> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
> > [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
> > [   38.049155] head anon flags: 0x17ffe000000000e(referenced|uptodate|dirty|head)

How about ...
[   38.049155] flags: 0x17ffe000000000e(anon|referenced|uptodate|dirty|compound)

That is, change pageflag_names[] to print 'head' as 'compound' and move the
'anon' or 'ksm' to look like a pageflag.  Also CMA.  Like this:

+++ b/include/trace/events/mmflags.h
@@ -96,7 +96,7 @@
        {1UL << PG_private,             "private"       },              \
        {1UL << PG_private_2,           "private_2"     },              \
        {1UL << PG_writeback,           "writeback"     },              \
-       {1UL << PG_head,                "head"          },              \
+       {1UL << PG_head,                "compound"      },              \
        {1UL << PG_mappedtodisk,        "mappedtodisk"  },              \
        {1UL << PG_reclaim,             "reclaim"       },              \
        {1UL << PG_swapbacked,          "swapbacked"    },              \
+++ b/mm/debug.c
@@ -54,7 +54,7 @@ void __dump_page(struct page *page, const char *reason)
         * state for debugging, it should be fine to accept a bit of
         * inaccuracy here due to racing.
         */
-       bool page_cma = is_migrate_cma_page(page);
+       char *cma = is_migrate_cma_page(page) ? "|cma" : "";
        int mapcount;
        char *type = "";
 
@@ -71,6 +71,10 @@ 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;
+               if ((unsigned long)mapping & PAGE_MAPPING_ANON)
+                       mapping = NULL;
+               mapping = (struct address_space *)
+                               ((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
                head = page;
                compound = false;
        } else {
[...]
        if (PageKsm(page))
-               type = "ksm ";
+               type = "ksm|";
        else if (PageAnon(page))
-               type = "anon ";
-       else if (mapping) {
+               type = "anon|";
+
+       BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
+       pr_warn("flags: %#lx(%s%pGp%s)\n", page->flags, type, &page->flags,
+                       cma);
+
+       if (mapping) {
                const struct inode *host;
                const struct address_space_operations *a_ops;
@@ -163,11 +167,6 @@ 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, head->flags, &head->flags,
-               page_cma ? " CMA" : "");
-
 hex_only:
        print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
                        sizeof(unsigned long), page,

Can also delete the 'out_mapping' label this way.

(I really like it that we're debating this ... it feels like we've been
in a bit of a push-pull with the debug patches over the years)

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

[John Hubbard]

On 2020-06-30 04:59, Matthew Wilcox wrote:
...
> How about ...
> [   38.049155] flags: 0x17ffe000000000e(anon|referenced|uptodate|dirty|compound)
> 
> That is, change pageflag_names[] to print 'head' as 'compound' and move the
> 'anon' or 'ksm' to look like a pageflag.  Also CMA.  Like this:
> 
> +++ b/include/trace/events/mmflags.h
> @@ -96,7 +96,7 @@
>          {1UL << PG_private,             "private"       },              \
>          {1UL << PG_private_2,           "private_2"     },              \
>          {1UL << PG_writeback,           "writeback"     },              \
> -       {1UL << PG_head,                "head"          },              \
> +       {1UL << PG_head,                "compound"      },              \
>          {1UL << PG_mappedtodisk,        "mappedtodisk"  },              \
>          {1UL << PG_reclaim,             "reclaim"       },              \
>          {1UL << PG_swapbacked,          "swapbacked"    },              \
> +++ b/mm/debug.c
> @@ -54,7 +54,7 @@ void __dump_page(struct page *page, const char *reason)
>           * state for debugging, it should be fine to accept a bit of
>           * inaccuracy here due to racing.
>           */
> -       bool page_cma = is_migrate_cma_page(page);
> +       char *cma = is_migrate_cma_page(page) ? "|cma" : "";
>          int mapcount;
>          char *type = "";
>   
> @@ -71,6 +71,10 @@ 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;
> +               if ((unsigned long)mapping & PAGE_MAPPING_ANON)
> +                       mapping = NULL;
> +               mapping = (struct address_space *)
> +                               ((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
>                  head = page;
>                  compound = false;
>          } else {
> [...]
>          if (PageKsm(page))
> -               type = "ksm ";
> +               type = "ksm|";
>          else if (PageAnon(page))
> -               type = "anon ";
> -       else if (mapping) {
> +               type = "anon|";
> +
> +       BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
> +       pr_warn("flags: %#lx(%s%pGp%s)\n", page->flags, type, &page->flags,
> +                       cma);
> +
> +       if (mapping) {
>                  const struct inode *host;
>                  const struct address_space_operations *a_ops;
> @@ -163,11 +167,6 @@ 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, head->flags, &head->flags,
> -               page_cma ? " CMA" : "");
> -
>   hex_only:
>          print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>                          sizeof(unsigned long), page,
> 
> Can also delete the 'out_mapping' label this way.

OK, so after applying that on top of your original series, and your
line-break approach, here's what the output looks like for regular
and THP, head and tail pages (all involving FOLL_PIN):

THP FOLL_PIN page: head page:

[   42.360473] page:0000000025f35fdb refcount:513 mapcount:1 mapping:0000000000000000 index:0x0
[   42.368012] head:0000000025f35fdb order:9 compound_mapcount:1 compound_pincount:512
[   42.374761] flags: 0x17ffe000001000e(anon|referenced|uptodate|dirty|compound)
[   42.380994] raw: 017ffe000001000e ffffffff83649ca0 ffffea0020c80008 ffff888898091901
[   42.387822] raw: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
[   42.394680] page dumped because: gup_benchmark: head page: dump_page test

THP FOLL_PIN page: tail page:

[   42.408222] page:00000000803d233b refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
[   42.415850] head:0000000025f35fdb order:9 compound_mapcount:1 compound_pincount:512
[   42.422607] flags: 0x17ffe0000000000(anon|)
[   42.425772] raw: 017ffe0000000000 ffffea0020c98001 ffffea0020c98448 dead000000000400
[   42.432636] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[   42.439490] head: 017ffe000001000e ffffffff83649ca0 ffffea0020c80008 ffff888898091901
[   42.446431] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
[   42.453355] page dumped because: gup_benchmark: tail page: dump_page test

Non-THP FOLL_PIN page: head page:

[   41.513677] page:00000000190e28ba refcount:1025 mapcount:1 mapping:0000000000000000 index:0x0
[   41.521331] flags: 0x17ffe0000080034(anon|uptodate|lru|active|swapbacked)
[   41.527189] raw: 017ffe0000080034 ffffea002228db08 ffffea0022215108 ffff888898090191
[   41.534020] raw: 0000000000000000 0000000000000000 0000040100000000 0000000000000000
[   41.540863] page dumped because: gup_benchmark: head page: dump_page test

Non-THP FOLL_PIN page: tail page:

[   41.554472] page:00000000696a8210 refcount:1025 mapcount:1 mapping:0000000000000000 index:0x11
[   41.562230] flags: 0x17ffe0000080034(anon|uptodate|lru|active|swapbacked)
[   41.568073] raw: 017ffe0000080034 ffffea0022195688 ffffea0021a4e608 ffff888898090191
[   41.574940] raw: 0000000000000011 0000000000000000 0000040100000000 0000000000000000
[   41.581768] page dumped because: gup_benchmark: tail page: dump_page test

> 
> (I really like it that we're debating this ... it feels like we've been
> in a bit of a push-pull with the debug patches over the years)
> 

Yes, it's good that we're pausing a moment to polish this up. Lots of goodness
here and I like the above output.

thanks,
-- 
John Hubbard
NVIDIA

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

[Vlastimil Babka]

On 6/30/20 1:59 PM, Matthew Wilcox wrote:
> On Tue, Jun 30, 2020 at 10:02:50AM +0200, Vlastimil Babka wrote:
>> I would also prefer this approach.It would be also nice to know the tail index.
>> As long as page pointer wasn't hashed, it was possible to figure this out, but
>> now it's not. Maybe print pfn of both page and head?
> 
>> On 6/30/20 1:35 AM, John Hubbard wrote:
>> > ...so with that fix, along with your line break approach in the other thread,
>> > a tail page dump of a FOLL_PIN page looks like this:
>> > 
>> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
> 
> index is the last thing printed on this line.  If it's something like
> index:0x12345678, you can reduce it mod 1<<order, as printed on the next
> line.

Hmm, I guess that works as long as head pages really have index aligned to 1 <<
order ... they should, right?

But does it fail for HugeTLB? CC Kirill (git blamed) and Mike.
page_to_pgoff() has for PageHeadHuge this:
	return page->index << compound_order(page)

but page_to_index() does simply
        pgoff = compound_head(page)->index;
        pgoff += page - compound_head(page);

Shouldn't it also do a <<compound_order(head) for HugeTLB?

>> > [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
> 
>> > [   38.049155] anon flags: 0x17ffe000000000e(referenced|uptodate|dirty)
>> > [   38.055465] raw: 017ffe0000000000 ffffea0020dd0001 ffffea0020dd0448 dead000000000400
>> > [   38.062319] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
>> > [   38.069183] head: 017ffe000001000e ffffffff83649ca0 ffffea0020dd8008 ffff88888e0b6641
>> > [   38.076141] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
>> > [   38.083102] page dumped because: gup_benchmark: tail page: dump_page test
>> > 
>> > So, good. However, I feel that the "head " prefix approach is slightly
>> > preferable, because it's doing less processing (the more code one
>> > adds to little-exercised debug paths, the more likely the debugging has
>> > bugs) and is instead just printing out what it sees directly. And it seems a little
>> > odd to remove the PG_head bit from the output.
>> > 
>> > The "head " prefix approach looks like this:
>> 
>> I would also prefer this approach.It would be also nice to know the tail index.
>> As long as page pointer wasn't hashed, it was possible to figure this out, but
>> now it's not. Maybe print pfn of both page and head?
>> 
>> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
>> > [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
>> > [   38.049155] head anon flags: 0x17ffe000000000e(referenced|uptodate|dirty|head)
> 
> How about ...
> [   38.049155] flags: 0x17ffe000000000e(anon|referenced|uptodate|dirty|compound)
> 
> That is, change pageflag_names[] to print 'head' as 'compound' and move the
> 'anon' or 'ksm' to look like a pageflag.  Also CMA.  Like this:
> 
> +++ b/include/trace/events/mmflags.h
> @@ -96,7 +96,7 @@
>         {1UL << PG_private,             "private"       },              \
>         {1UL << PG_private_2,           "private_2"     },              \
>         {1UL << PG_writeback,           "writeback"     },              \
> -       {1UL << PG_head,                "head"          },              \
> +       {1UL << PG_head,                "compound"      },              \

Dunno about this, "compound" used to always mean "head or tail" AFAIK.

>         {1UL << PG_mappedtodisk,        "mappedtodisk"  },              \
>         {1UL << PG_reclaim,             "reclaim"       },              \
>         {1UL << PG_swapbacked,          "swapbacked"    },              \
> +++ b/mm/debug.c
> @@ -54,7 +54,7 @@ void __dump_page(struct page *page, const char *reason)
>          * state for debugging, it should be fine to accept a bit of
>          * inaccuracy here due to racing.
>          */
> -       bool page_cma = is_migrate_cma_page(page);
> +       char *cma = is_migrate_cma_page(page) ? "|cma" : "";
>         int mapcount;
>         char *type = "";
>  
> @@ -71,6 +71,10 @@ 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;
> +               if ((unsigned long)mapping & PAGE_MAPPING_ANON)
> +                       mapping = NULL;
> +               mapping = (struct address_space *)
> +                               ((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
>                 head = page;
>                 compound = false;
>         } else {
> [...]
>         if (PageKsm(page))
> -               type = "ksm ";
> +               type = "ksm|";
>         else if (PageAnon(page))
> -               type = "anon ";
> -       else if (mapping) {
> +               type = "anon|";
> +
> +       BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
> +       pr_warn("flags: %#lx(%s%pGp%s)\n", page->flags, type, &page->flags,
> +                       cma);
> +
> +       if (mapping) {
>                 const struct inode *host;
>                 const struct address_space_operations *a_ops;
> @@ -163,11 +167,6 @@ 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, head->flags, &head->flags,
> -               page_cma ? " CMA" : "");
> -
>  hex_only:
>         print_hex_dump(KERN_WARNING, "raw: ", DUMP_PREFIX_NONE, 32,
>                         sizeof(unsigned long), page,
> 
> Can also delete the 'out_mapping' label this way.
> 
> (I really like it that we're debating this ... it feels like we've been
> in a bit of a push-pull with the debug patches over the years)
> 

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

[Kirill A. Shutemov]

On Thu, Jul 02, 2020 at 04:59:14PM +0200, Vlastimil Babka wrote:
> On 6/30/20 1:59 PM, Matthew Wilcox wrote:
> > On Tue, Jun 30, 2020 at 10:02:50AM +0200, Vlastimil Babka wrote:
> >> I would also prefer this approach.It would be also nice to know the tail index.
> >> As long as page pointer wasn't hashed, it was possible to figure this out, but
> >> now it's not. Maybe print pfn of both page and head?
> > 
> >> On 6/30/20 1:35 AM, John Hubbard wrote:
> >> > ...so with that fix, along with your line break approach in the other thread,
> >> > a tail page dump of a FOLL_PIN page looks like this:
> >> > 
> >> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
> > 
> > index is the last thing printed on this line.  If it's something like
> > index:0x12345678, you can reduce it mod 1<<order, as printed on the next
> > line.
> 
> Hmm, I guess that works as long as head pages really have index aligned to 1 <<
> order ... they should, right?
> 
> But does it fail for HugeTLB? CC Kirill (git blamed) and Mike.
> page_to_pgoff() has for PageHeadHuge this:
> 	return page->index << compound_order(page)
> 
> but page_to_index() does simply
>         pgoff = compound_head(page)->index;
>         pgoff += page - compound_head(page);
> 
> Shouldn't it also do a <<compound_order(head) for HugeTLB?

PageHeadHuge() is only true for head pages, so we are fine here.

> 
> >> > [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
> > 
> >> > [   38.049155] anon flags: 0x17ffe000000000e(referenced|uptodate|dirty)
> >> > [   38.055465] raw: 017ffe0000000000 ffffea0020dd0001 ffffea0020dd0448 dead000000000400
> >> > [   38.062319] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
> >> > [   38.069183] head: 017ffe000001000e ffffffff83649ca0 ffffea0020dd8008 ffff88888e0b6641
> >> > [   38.076141] head: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
> >> > [   38.083102] page dumped because: gup_benchmark: tail page: dump_page test
> >> > 
> >> > So, good. However, I feel that the "head " prefix approach is slightly
> >> > preferable, because it's doing less processing (the more code one
> >> > adds to little-exercised debug paths, the more likely the debugging has
> >> > bugs) and is instead just printing out what it sees directly. And it seems a little
> >> > odd to remove the PG_head bit from the output.
> >> > 
> >> > The "head " prefix approach looks like this:
> >> 
> >> I would also prefer this approach.It would be also nice to know the tail index.
> >> As long as page pointer wasn't hashed, it was possible to figure this out, but
> >> now it's not. Maybe print pfn of both page and head?
> >> 
> >> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
> >> > [   38.035633] head:00000000675be53c order:9 compound_mapcount:1 compound_pincount:512
> >> > [   38.049155] head anon flags: 0x17ffe000000000e(referenced|uptodate|dirty|head)
> > 
> > How about ...
> > [   38.049155] flags: 0x17ffe000000000e(anon|referenced|uptodate|dirty|compound)
> > 
> > That is, change pageflag_names[] to print 'head' as 'compound' and move the
> > 'anon' or 'ksm' to look like a pageflag.  Also CMA.  Like this:
> > 
> > +++ b/include/trace/events/mmflags.h
> > @@ -96,7 +96,7 @@
> >         {1UL << PG_private,             "private"       },              \
> >         {1UL << PG_private_2,           "private_2"     },              \
> >         {1UL << PG_writeback,           "writeback"     },              \
> > -       {1UL << PG_head,                "head"          },              \
> > +       {1UL << PG_head,                "compound"      },              \
> 
> Dunno about this, "compound" used to always mean "head or tail" AFAIK.

Yeah. I'm not convinced by the change either. show_page_flags() also uses
this difinitions and I think it can get confusing in the trace.

-- 
 Kirill A. Shutemov

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

[Vlastimil Babka]

On 7/2/20 5:53 PM, Kirill A. Shutemov wrote:
> On Thu, Jul 02, 2020 at 04:59:14PM +0200, Vlastimil Babka wrote:
>> On 6/30/20 1:59 PM, Matthew Wilcox wrote:
>> > On Tue, Jun 30, 2020 at 10:02:50AM +0200, Vlastimil Babka wrote:
>> >> I would also prefer this approach.It would be also nice to know the tail index.
>> >> As long as page pointer wasn't hashed, it was possible to figure this out, but
>> >> now it's not. Maybe print pfn of both page and head?
>> > 
>> >> On 6/30/20 1:35 AM, John Hubbard wrote:
>> >> > ...so with that fix, along with your line break approach in the other thread,
>> >> > a tail page dump of a FOLL_PIN page looks like this:
>> >> > 
>> >> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
>> > 
>> > index is the last thing printed on this line.  If it's something like
>> > index:0x12345678, you can reduce it mod 1<<order, as printed on the next
>> > line.
>> 
>> Hmm, I guess that works as long as head pages really have index aligned to 1 <<
>> order ... they should, right?
>> 
>> But does it fail for HugeTLB? CC Kirill (git blamed) and Mike.
>> page_to_pgoff() has for PageHeadHuge this:
>> 	return page->index << compound_order(page)
>> 
>> but page_to_index() does simply
>>         pgoff = compound_head(page)->index;
>>         pgoff += page - compound_head(page);
>> 
>> Shouldn't it also do a <<compound_order(head) for HugeTLB?
> 
> PageHeadHuge() is only true for head pages, so we are fine here.

Really? We are calculatting index (pgoff) of tail page, which should be index of
head page plus n for n'th tail page; the unit is base pages.
But ff HugeTLB head pages use the unit of huge page in page->index, and
page_to_pgoff() translates it to unit of base pages, then we should do the same
when calculating the index of tail page, no? Otherwise we are adding up units of
huge pages (from head->index) with units of base page (n'th tail) and get
garbage as a result, AFAICS?

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

[Kirill A. Shutemov]

On Thu, Jul 02, 2020 at 06:19:08PM +0200, Vlastimil Babka wrote:
> On 7/2/20 5:53 PM, Kirill A. Shutemov wrote:
> > On Thu, Jul 02, 2020 at 04:59:14PM +0200, Vlastimil Babka wrote:
> >> On 6/30/20 1:59 PM, Matthew Wilcox wrote:
> >> > On Tue, Jun 30, 2020 at 10:02:50AM +0200, Vlastimil Babka wrote:
> >> >> I would also prefer this approach.It would be also nice to know the tail index.
> >> >> As long as page pointer wasn't hashed, it was possible to figure this out, but
> >> >> now it's not. Maybe print pfn of both page and head?
> >> > 
> >> >> On 6/30/20 1:35 AM, John Hubbard wrote:
> >> >> > ...so with that fix, along with your line break approach in the other thread,
> >> >> > a tail page dump of a FOLL_PIN page looks like this:
> >> >> > 
> >> >> > [   38.027987] page:00000000abaef9ae refcount:513 mapcount:1 mapping:0000000000000000 index:0x11
> >> > 
> >> > index is the last thing printed on this line.  If it's something like
> >> > index:0x12345678, you can reduce it mod 1<<order, as printed on the next
> >> > line.
> >> 
> >> Hmm, I guess that works as long as head pages really have index aligned to 1 <<
> >> order ... they should, right?
> >> 
> >> But does it fail for HugeTLB? CC Kirill (git blamed) and Mike.
> >> page_to_pgoff() has for PageHeadHuge this:
> >> 	return page->index << compound_order(page)
> >> 
> >> but page_to_index() does simply
> >>         pgoff = compound_head(page)->index;
> >>         pgoff += page - compound_head(page);
> >> 
> >> Shouldn't it also do a <<compound_order(head) for HugeTLB?
> > 
> > PageHeadHuge() is only true for head pages, so we are fine here.
> 
> Really? We are calculatting index (pgoff) of tail page, which should be index of
> head page plus n for n'th tail page; the unit is base pages.
> But ff HugeTLB head pages use the unit of huge page in page->index, and
> page_to_pgoff() translates it to unit of base pages, then we should do the same
> when calculating the index of tail page, no? Otherwise we are adding up units of
> huge pages (from head->index) with units of base page (n'th tail) and get
> garbage as a result, AFAICS?

You are right. I guess we can get away with this because nobody calls
page_to_pgoff() on tail pages of hugetlb page. Except when something goes
wrong and dump_page() has to deal with it.

I'm not sure if it's worth fixing and whether the fix should be inside
page_to_pgoff().

The best fix would be to remove hugetlb pages special-casing: it has to
have ->index in base pagesize, not huge page.

-- 
 Kirill A. Shutemov

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

[Matthew Wilcox]

On Thu, Jul 02, 2020 at 11:39:07PM +0300, Kirill A. Shutemov wrote:
> On Thu, Jul 02, 2020 at 06:19:08PM +0200, Vlastimil Babka wrote:
> > Really? We are calculatting index (pgoff) of tail page, which should be index of
> > head page plus n for n'th tail page; the unit is base pages.
> > But ff HugeTLB head pages use the unit of huge page in page->index, and
> > page_to_pgoff() translates it to unit of base pages, then we should do the same
> > when calculating the index of tail page, no? Otherwise we are adding up units of
> > huge pages (from head->index) with units of base page (n'th tail) and get
> > garbage as a result, AFAICS?
> 
> You are right. I guess we can get away with this because nobody calls
> page_to_pgoff() on tail pages of hugetlb page. Except when something goes
> wrong and dump_page() has to deal with it.
> 
> I'm not sure if it's worth fixing and whether the fix should be inside
> page_to_pgoff().
> 
> The best fix would be to remove hugetlb pages special-casing: it has to
> have ->index in base pagesize, not huge page.

https://lore.kernel.org/linux-mm/20200629152033.16175-1-willy@infradead.org/
would be a good place to start.  We'll use 8 entries for an order-9 page
instead of the 1 entry that we currently do, but that's a lot better
than using 512 entries.

[PATCH 2/3] mm: Print the inode number in dump_page

[Matthew Wilcox (Oracle)]

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

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

diff --git a/mm/debug.c b/mm/debug.c
index 44d843b3b07c..5c58d4509dc9 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -134,14 +134,16 @@ void __dump_page(struct page *page, const char *reason)
 		}
 
 		if (copy_from_kernel_nofault(&dentry_first,
-			&host->i_dentry.first, sizeof(struct hlist_node *))) {
+				&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);
 			goto out_mapping;
 		}
 
 		if (!dentry_first) {
-			pr_warn("mapping->a_ops:%ps\n", a_ops);
+			pr_warn("mapping->a_ops:%ps ino %lx\n", a_ops,
+					host->i_ino);
 			goto out_mapping;
 		}
 
@@ -156,8 +158,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("mapping->aops:%ps dentry name:\"%pd\"\n",
-								a_ops, &dentry);
+			pr_warn("mapping->aops:%ps ino %lx dentry name:\"%pd\"\n",
+					a_ops, host->i_ino, &dentry);
 		}
 	}
 out_mapping:
-- 
2.27.0

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

[Matthew Wilcox (Oracle)]

The actual address of the struct page isn't particularly helpful,
while the hashed address helps match with other messages elsewhere.

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

diff --git a/mm/debug.c b/mm/debug.c
index 5c58d4509dc9..fcf3a16902b2 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -86,23 +86,23 @@ void __dump_page(struct page *page, const char *reason)
 
 	if (compound)
 		if (hpage_pincount_available(page)) {
-			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
-				"index:%#lx head:%px order:%u "
+			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));
 		} else {
-			pr_warn("page:%px refcount:%d mapcount:%d mapping:%p "
-				"index:%#lx head:%px order:%u "
+			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));
 		}
 	else
-		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\n",
 			page, page_ref_count(page), mapcount,
 			mapping, page_to_pgoff(page));
 	if (PageKsm(page))
-- 
2.27.0

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

[Kirill A. Shutemov]

On Mon, Jun 29, 2020 at 04:19:18PM +0100, 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.

Hashed addresses would lose useful alignment information. I found it
useful more than once to check if the page is THP-aligned or not.

-- 
 Kirill A. Shutemov

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

[William Kucharski]

I REALLY like these; anything that gives more information in debug messages
is greatly appreciated.

Reviewed-by: William Kucharski <william.kucharski@oracle.com>

> On Jun 29, 2020, at 9:19 AM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> 
> I've been looking at a lot of calls to dump_page() recently due to the
> THP work, and these three patches make my life easier.  Please consider
> applying.
> 
> Matthew Wilcox (Oracle) (3):
>  mm: Print head flags in dump_page
>  mm: Print the inode number in dump_page
>  mm: Print hashed address of struct page
> 
> mm/debug.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
> 
> -- 
> 2.27.0

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

[Mike Rapoport]

On Mon, Jun 29, 2020 at 04:19:15PM +0100, Matthew Wilcox (Oracle) wrote:
> I've been looking at a lot of calls to dump_page() recently due to the
> THP work, and these three patches make my life easier.  Please consider
> applying.
> 
> Matthew Wilcox (Oracle) (3):
>   mm: Print head flags in dump_page
>   mm: Print the inode number in dump_page
>   mm: Print hashed address of struct page

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

>  mm/debug.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> -- 
> 2.27.0
> 
> 

-- 
Sincerely yours,
Mike.

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

[John Hubbard]

On 2020-06-29 08:19, Matthew Wilcox (Oracle) wrote:
> I've been looking at a lot of calls to dump_page() recently due to the
> THP work, and these three patches make my life easier.  Please consider
> applying.
> 
> Matthew Wilcox (Oracle) (3):
>    mm: Print head flags in dump_page
>    mm: Print the inode number in dump_page
>    mm: Print hashed address of struct page
> 
>   mm/debug.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)


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));
  		}
  	else
  		pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx\n",



thanks,
-- 
John Hubbard
NVIDIA

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

[Matthew Wilcox]

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.

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

[John Hubbard]

On 2020-06-29 15:35, Matthew Wilcox wrote:
...
> 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.
> 

That is a much better fix. I hated myself for the code duplication there. :)
I've pasted in a sample run in the other thread (patch 1/3).


thanks,
-- 
John Hubbard
NVIDIA