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

["Kirill A. Shutemov" <kirill@shutemov.name>]

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