From: Vlastimil Babka <vbabka@suse.cz>
To: Matthew Wilcox <willy@infradead.org>
Cc: John Hubbard <jhubbard@nvidia.com>,
Andrew Morton <akpm@linux-foundation.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-mm@kvack.org, cai@lca.pw, kirill@shutemov.name,
rppt@linux.ibm.com, william.kucharski@oracle.com,
"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH v2] mm, dump_page: do not crash with bad compound_mapcount()
Date: Thu, 6 Aug 2020 17:13:05 +0200 [thread overview]
Message-ID: <790ae9a4-6874-ac34-d2a2-28a2137335cb@suse.cz> (raw)
In-Reply-To: <20200806134851.GN23808@casper.infradead.org>
On 8/6/20 3:48 PM, Matthew Wilcox wrote:
> On Thu, Aug 06, 2020 at 01:45:11PM +0200, Vlastimil Babka wrote:
>> How about this additional patch now that we have head_mapcoun()? (I wouldn't
>> go for squashing as the goal and scope is too different).
>
> I like it. It bothers me that the compiler doesn't know that
> compound_head(compound_head(x)) == compound_head(x). I updated
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=32911 with a request to be
> able to tell the compiler that compound_head() is idempotent.
Yeah it would be nice to get the benefits everywhere automatically. But I guess
the compiler would have to discard the idempotence assumptions if there are
multiple consecutive (perhaps hidden behind page flag access)
compound_head(page) from a function, as soon as we modify the struct page somewhere.
>> The bloat-o-meter difference without DEBUG_VM is the following:
>>
>> add/remove: 0/0 grow/shrink: 1/4 up/down: 32/-56 (-24)
>> Function old new delta
>> __split_huge_pmd 2867 2899 +32
>> shrink_page_list 3860 3847 -13
>> reuse_swap_page 762 748 -14
>> page_trans_huge_mapcount 153 139 -14
>> total_mapcount 187 172 -15
>> Total: Before=8687306, After=8687282, chg -0.00%
>
> That's great. I'm expecting improvements from my thp_head() macro when
> that lands (currently in Andrew's tree). I have been reluctant to replace
> current callers of compound_head() with thp_head(), but I suspect PF_HEAD
> could use thp_head() and save a few bytes on a tinyconfig build.
>
>> +++ b/mm/huge_memory.c
>> @@ -2125,7 +2125,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>> * Set PG_double_map before dropping compound_mapcount to avoid
>> * false-negative page_mapped().
>> */
>> - if (compound_mapcount(page) > 1 && !TestSetPageDoubleMap(page)) {
>> + if (head_mapcount(page) > 1 && !TestSetPageDoubleMap(page)) {
>
> I'm a little nervous about this one. The page does actually come from
> pmd_page(), and today that's guaranteed to be a head page. But I'm
> not convinced that's going to still be true in twenty years. With the
> current THP patchset, I won't allocate pages larger than PMD order, but
> I can see there being interest in tracking pages in chunks larger than
> 2MB in the future. And then pmd_page() might well return a tail page.
> So it might be a good idea to not convert this one.
Hmm the function converts the compound mapcount of the whole page to a
HPAGE_PMD_NR of base pages. If suddenly the compound page was bigger than a pmd,
then I guess this wouldn't work properly anymore without changes anyway?
Maybe we could stick something like VM_BUG_ON(PageTransHuge(page)) there as
"enforced documentation" for now?
next prev parent reply other threads:[~2020-08-06 16:54 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-04 21:48 [PATCH v2] mm, dump_page: do not crash with bad compound_mapcount() John Hubbard
2020-08-06 11:45 ` Vlastimil Babka
2020-08-06 13:48 ` Matthew Wilcox
2020-08-06 15:13 ` Vlastimil Babka [this message]
2020-08-06 15:39 ` Matthew Wilcox
2020-08-06 15:53 ` Vlastimil Babka
2020-08-06 17:15 ` Matthew Wilcox
2020-08-07 14:53 ` Kirill A. Shutemov
2020-08-07 14:35 ` Kirill A. Shutemov
2020-08-07 15:10 ` Matthew Wilcox
2020-08-07 16:48 ` Kirill A. Shutemov
2020-08-07 18:33 ` [PATCH] mm, dump_page: rename head_mapcount() --> head_compound_mapcount() John Hubbard
2020-08-07 22:40 ` [PATCH v2] mm, dump_page: do not crash with bad compound_mapcount() John Hubbard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=790ae9a4-6874-ac34-d2a2-28a2137335cb@suse.cz \
--to=vbabka@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=cai@lca.pw \
--cc=jhubbard@nvidia.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=kirill@shutemov.name \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=rppt@linux.ibm.com \
--cc=william.kucharski@oracle.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.