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

[John Hubbard <jhubbard@nvidia.com>]

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