linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Rik van Riel <riel@redhat.com>, Mel Gorman <mgorman@suse.de>,
	Michal Hocko <mhocko@suse.cz>,
	"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
	Hugh Dickins <hughd@google.com>,
	Davidlohr Bueso <davidlohr.bueso@hp.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Joonsoo Kim <js1304@gmail.com>,
	Wanpeng Li <liwanp@linux.vnet.ibm.com>,
	Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	Hillf Danton <dhillf@gmail.com>
Subject: Re: [PATCH v3 13/14] mm, hugetlb: retry if failed to allocate and there is concurrent user
Date: Thu, 19 Dec 2013 17:02:02 -0800	[thread overview]
Message-ID: <20131219170202.0df2d82a2adefa3ab616bdaa@linux-foundation.org> (raw)
In-Reply-To: <1387349640-8071-14-git-send-email-iamjoonsoo.kim@lge.com>

On Wed, 18 Dec 2013 15:53:59 +0900 Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:

> If parallel fault occur, we can fail to allocate a hugepage,
> because many threads dequeue a hugepage to handle a fault of same address.
> This makes reserved pool shortage just for a little while and this cause
> faulting thread who can get hugepages to get a SIGBUS signal.
> 
> To solve this problem, we already have a nice solution, that is,
> a hugetlb_instantiation_mutex. This blocks other threads to dive into
> a fault handler. This solve the problem clearly, but it introduce
> performance degradation, because it serialize all fault handling.
> 
> Now, I try to remove a hugetlb_instantiation_mutex to get rid of
> performance degradation.

So the whole point of the patch is to improve performance, but the
changelog doesn't include any performance measurements!

Please, run some quantitative tests and include a nice summary of the
results in the changelog.

This is terribly important, because if the performance benefit is
infinitesimally small or negative, the patch goes into the bit bucket ;)

> For achieving it, at first, we should ensure that
> no one get a SIGBUS if there are enough hugepages.
> 
> For this purpose, if we fail to allocate a new hugepage when there is
> concurrent user, we return just 0, instead of VM_FAULT_SIGBUS. With this,
> these threads defer to get a SIGBUS signal until there is no
> concurrent user, and so, we can ensure that no one get a SIGBUS if there
> are enough hugepages.

So if I'm understanding this correctly...  if N threads all generate a
fault against the same address, they will all dive in and allocate a
hugepage, will then do an enormous memcpy into that page and will then
attempt to instantiate the page in pagetables.  All threads except one
will lose the race and will free the page again!  This sounds terribly
inefficient; it would be useful to write a microbenchmark which
triggers this scenario so we can explore the impact.

I'm wondering if a better solution to all of this would be to make
hugetlb_instantiation_mutex an array of, say, 1024 mutexes and index it
with a hash of the faulting address.  That will 99.9% solve the
performance issue which you believe exists without introducing this new
performance issue?

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

  reply	other threads:[~2013-12-20  1:02 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-18  6:53 [PATCH v3 00/14] mm, hugetlb: remove a hugetlb_instantiation_mutex Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 01/14] mm, hugetlb: unify region structure handling Joonsoo Kim
2013-12-21  9:04   ` David Gibson
2014-01-07  2:37   ` Davidlohr Bueso
2013-12-18  6:53 ` [PATCH v3 02/14] mm, hugetlb: region manipulation functions take resv_map rather list_head Joonsoo Kim
2013-12-21 13:43   ` David Gibson
2014-01-07  2:39   ` Davidlohr Bueso
2013-12-18  6:53 ` [PATCH v3 03/14] mm, hugetlb: protect region tracking via newly introduced resv_map lock Joonsoo Kim
2013-12-21 13:58   ` David Gibson
2013-12-23  1:05     ` Joonsoo Kim
2013-12-24 12:00       ` David Gibson
2014-01-06  0:12         ` Joonsoo Kim
2014-01-07  2:39   ` Davidlohr Bueso
2013-12-18  6:53 ` [PATCH v3 04/14] mm, hugetlb: remove resv_map_put() Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 05/14] mm, hugetlb: make vma_resv_map() works for all mapping type Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 06/14] mm, hugetlb: remove vma_has_reserves() Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 07/14] mm, hugetlb: mm, hugetlb: unify chg and avoid_reserve to use_reserve Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 08/14] mm, hugetlb: call vma_needs_reservation before entering alloc_huge_page() Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 09/14] mm, hugetlb: remove a check for return value of alloc_huge_page() Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 10/14] mm, hugetlb: move down outside_reserve check Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 11/14] mm, hugetlb: move up anon_vma_prepare() Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 12/14] mm, hugetlb: clean-up error handling in hugetlb_cow() Joonsoo Kim
2013-12-18  6:53 ` [PATCH v3 13/14] mm, hugetlb: retry if failed to allocate and there is concurrent user Joonsoo Kim
2013-12-20  1:02   ` Andrew Morton [this message]
2013-12-20  1:58     ` Joonsoo Kim
2013-12-20  2:15       ` Andrew Morton
2013-12-20  5:00         ` Joonsoo Kim
2013-12-20  2:31     ` Davidlohr Bueso
2013-12-20  4:47       ` Joonsoo Kim
2013-12-20 14:01     ` Mel Gorman
2013-12-21  6:48       ` Davidlohr Bueso
2013-12-23  0:44         ` Joonsoo Kim
2013-12-23  2:11           ` Joonsoo Kim
2014-01-03 19:55             ` Davidlohr Bueso
2014-01-06  0:19               ` Joonsoo Kim
2014-01-06 12:19                 ` Davidlohr Bueso
2014-01-07  1:57                   ` Joonsoo Kim
2014-01-07  2:36                     ` Davidlohr Bueso
2014-01-15  3:08                       ` David Rientjes
2014-01-15  4:37                         ` Davidlohr Bueso
2014-01-15  4:56                           ` Andrew Morton
2014-01-15 20:47                             ` Davidlohr Bueso
2014-01-15 20:50                               ` Andrew Morton
2013-12-18  6:54 ` [PATCH v3 14/14] mm, hugetlb: remove a hugetlb_instantiation_mutex Joonsoo Kim
2014-03-31 16:27 ` [PATCH v3 00/14] " Dave Hansen
2014-03-31 17:26   ` Davidlohr Bueso
2014-03-31 18:41     ` Dave Hansen

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=20131219170202.0df2d82a2adefa3ab616bdaa@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=aneesh.kumar@linux.vnet.ibm.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=davidlohr.bueso@hp.com \
    --cc=dhillf@gmail.com \
    --cc=hughd@google.com \
    --cc=iamjoonsoo.kim@lge.com \
    --cc=js1304@gmail.com \
    --cc=kamezawa.hiroyu@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=liwanp@linux.vnet.ibm.com \
    --cc=mgorman@suse.de \
    --cc=mhocko@suse.cz \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=riel@redhat.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