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

[Vlastimil Babka <vbabka@suse.cz>]

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