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

[John Hubbard <jhubbard@nvidia.com>]

On 2/10/20 1:21 PM, Kirill A. Shutemov wrote:
> 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.


Actually, we are in total agreement there, but I meant "if it is a real
tail page"--in other words, I should have written:

"the function doesn't seem to work correctly unless it is fed the original 
page. If you feed it the head page unconditionally, it breaks 
page_mapcounts()'s assumptions". Sorry for the confusing response.


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


It's not clear to me whether it's safer to use the...user-provided page, or the
head page, for checking PageSlab()--it's probably set on both head and non-head
pages, right? I guess you're saying it should be like this:

	mapcount = PageSlab(head) ? 0 : page_mapcount(page);

...yes?

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

sigh, please ignore anything I say about EXPORTS--I've been reminded of this point
before, I'm afraid. :)


thanks,
-- 
John Hubbard
NVIDIA