From: Oleg Nesterov <oleg@redhat.com>
To: Alex Thorlton <athorlton@sgi.com>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Rik van Riel <riel@redhat.com>,
Wanpeng Li <liwanp@linux.vnet.ibm.com>,
Mel Gorman <mgorman@suse.de>,
Michel Lespinasse <walken@google.com>,
Benjamin LaHaise <bcrl@kvack.org>,
"Eric W. Biederman" <ebiederm@xmission.com>,
Andy Lutomirski <luto@amacapital.net>,
Al Viro <viro@zeniv.linux.org.uk>,
David Rientjes <rientjes@google.com>,
Zhang Yanfei <zhangyanfei@cn.fujitsu.com>,
Peter Zijlstra <peterz@infradead.org>,
Johannes Weiner <hannes@cmpxchg.org>,
Michal Hocko <mhocko@suse.cz>, Jiang Liu <jiang.liu@huawei.com>,
Cody P Schafer <cody@linux.vnet.ibm.com>,
Glauber Costa <glommer@parallels.com>,
Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 3/3] Change THP behavior
Date: Fri, 13 Dec 2013 19:12:05 +0100 [thread overview]
Message-ID: <20131213181205.GA16534@redhat.com> (raw)
In-Reply-To: <20131212180057.GD134240@sgi.com>
I know almost nothing about thp, unlikely I understand this patch
correctly...
But, afaics, the main idea is that until we have mm->thp_threshold
faults we install the tail pages of temp_hugepage->page as a normal
anonymous page, then we actually add the Head/Tail metadata and add
the necessary huge_pmd/etc.
I simply can't understand how this can work until make_compound_page()
is called. Just for example, what happens after sys_munmap() ? If
nothing else, who will find and free temp_hugepage connected to this
area? Or, what if sys_mremap() moves this vma? find_pmd_mm_freelist()
won't find the right temp_thp after that? Or split_vma, etc.
And make_compound_page() itself looks suspicious,
On 12/12, Alex Thorlton wrote:
>
> +void make_compound_page(struct page *page, unsigned long order)
> +{
> + int i, max_count = 0, max_mapcount = 0;
> + int nr_pages = 1 << order;
> +
> + set_compound_page_dtor(page, free_compound_page);
> + set_compound_order(page, order);
> +
> + __SetPageHead(page);
> +
> + /*
> + * we clear all the mappings here, so we have to remember to set
> + * them back up!
> + */
> + page->mapping = NULL;
> +
> + max_count = (int) atomic_read(&page->_count);
> + max_mapcount = (int) atomic_read(&page->_mapcount);
> +
> + for (i = 1; i < nr_pages; i++) {
> + int cur_count, cur_mapcount;
> + struct page *p = page + i;
> + p->flags = 0; /* this seems dumb */
> + __SetPageTail(p);
Just for example, what if put_page(p) or get_page(p) is called after
__SetPageTail() ?
Afaics, this page was already visible to, say, get_user_pages() and
it can have external references.
In fact everything else looks suspicious too but let me repeat that
I do not really understand this code. So please do not count this as
review, but perhaps the changelog should tell more to explain what
this patch actually does and how this all should work?
Oleg.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
prev parent reply other threads:[~2013-12-13 18:12 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1386790423.git.athorlton@sgi.com>
2013-12-12 18:00 ` [RFC PATCH 1/3] Add flags for temporary compound pages Alex Thorlton
2013-12-12 18:00 ` [RFC PATCH 2/3] Add tunable to control THP behavior Alex Thorlton
2013-12-12 20:11 ` Andy Lutomirski
2013-12-12 20:49 ` Alex Thorlton
2013-12-12 20:52 ` Andy Lutomirski
2013-12-12 21:04 ` Alex Thorlton
2013-12-12 21:37 ` Rik van Riel
2013-12-12 23:17 ` Alex Thorlton
2013-12-12 18:00 ` [RFC PATCH 3/3] Change " Alex Thorlton
2013-12-13 13:13 ` Kirill A. Shutemov
2013-12-16 17:37 ` Alex Thorlton
2013-12-13 18:12 ` Oleg Nesterov [this message]
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=20131213181205.GA16534@redhat.com \
--to=oleg@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=athorlton@sgi.com \
--cc=bcrl@kvack.org \
--cc=benh@kernel.crashing.org \
--cc=cody@linux.vnet.ibm.com \
--cc=ebiederm@xmission.com \
--cc=glommer@parallels.com \
--cc=hannes@cmpxchg.org \
--cc=jiang.liu@huawei.com \
--cc=kamezawa.hiroyu@jp.fujitsu.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=liwanp@linux.vnet.ibm.com \
--cc=luto@amacapital.net \
--cc=mgorman@suse.de \
--cc=mhocko@suse.cz \
--cc=n-horiguchi@ah.jp.nec.com \
--cc=peterz@infradead.org \
--cc=riel@redhat.com \
--cc=rientjes@google.com \
--cc=viro@zeniv.linux.org.uk \
--cc=walken@google.com \
--cc=zhangyanfei@cn.fujitsu.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).