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
