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

[Matthew Wilcox <willy@infradead.org>]

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)