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