Re: [PATCH] mm: Improve dump_page() for compound pages

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

On Mon, Feb 10, 2020 at 11:50:21AM -0800, John Hubbard wrote:
> On 2/10/20 4:42 AM, Kirill A. Shutemov wrote:
> ...
> >> @@ -66,25 +68,32 @@ void __dump_page(struct page *page, const char *reason)
> >>  		goto hex_only;
> >>  	}
> >>  
> >> -	mapping = page_mapping(page);
> >> +	if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
> >> +		/* Corrupt page, cannot call page_mapping */
> >> +		mapping = page->mapping;
> >> +		head = page;
> >> +		compound = false;
> >> +	} else {
> >> +		mapping = page_mapping(page);
> >> +	}
> >>  
> >>  	/*
> >>  	 * Avoid VM_BUG_ON() in page_mapcount().
> >>  	 * page->_mapcount space in struct page is used by sl[aou]b pages to
> >>  	 * encode own info.
> >>  	 */
> >> -	mapcount = PageSlab(page) ? 0 : page_mapcount(page);
> >> +	mapcount = PageSlab(head) ? 0 : page_mapcount(head);
> > 
> > This is wrong. We want to see mapcount for the tail page, not head.
> > 
> 
> I see what you mean: page_mapcount(page) sums up both the page's and the
> head page's mapcount in some cases. The function doesn't seem to work
> correctly unless it is fed the tail page.

What makes you think this? It has to be called on the page user provided.
Head or tail.

> Here, even though the "head" variable's meaning is overloaded (=="head page, 
> unless the tail page was corrupted, in which case, tail page"), it would still be
> accurate to change that line back to the original line, so that it once again
> reads:
> 
>         mapcount = PageSlab(page) ? 0 : page_mapcount(page);

PageSlab() can be called for the head page.

> 
> Matthew?
> 
> (Also, I see that __page_mapcount is EXPORT-ed, which is odd: nothing uses it
> other than page_mapcount.

... and page_mapcount() can be inlined anywhere.

-- 
 Kirill A. Shutemov