From: Hugh Dickins <hughd@google.com>
To: Chris Metcalf <cmetcalf@tilera.com>, Mel Gorman <mel@csn.ul.ie>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Hillf Danton <dhillf@gmail.com>, Michal Hocko <mhocko@suse.cz>,
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow()
Date: Mon, 30 Apr 2012 13:19:27 -0700 (PDT) [thread overview]
Message-ID: <alpine.LSU.2.00.1204301308090.2829@eggly.anvils> (raw)
In-Reply-To: <201204291936.q3TJa4Mv008924@farm-0027.internal.tilera.com>
On Sun, 29 Apr 2012, Chris Metcalf wrote:
> Commit 66aebce747eaf added code to avoid a race condition by
> elevating the page refcount in hugetlb_fault() while calling
> hugetlb_cow(). However, one code path in hugetlb_cow() includes
> an assertion that the page count is 1, whereas it may now also
> have the value 2 in this path.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
> We discovered this while testing the original path; one particular
> application triggered this due to the specific number of huge pages
> it started with.
Well done finding that. But I think it would be better to remove the
BUG_ON() than complicate it, and then no need to add a comment there.
IIRC it's unsafe to make any assertions about what a page_count() may
be, beyond whether it's 0 or non-0: because of speculative accesses to
the page from elsewhere (perhaps it used to be visible in a radix_tree,
perhaps __isolate_lru_pages is having a go at it).
I'd say that BUG_ON() has outlived its usefulness, and should just be
eliminated now: but git "blames" Mel for it, so let's see if he agrees.
Hugh
>
> mm/hugetlb.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index cd65cb1..d5b0254 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2498,7 +2498,14 @@ retry_avoidcopy:
> if (outside_reserve) {
> BUG_ON(huge_pte_none(pte));
> if (unmap_ref_private(mm, vma, old_page, address)) {
> - BUG_ON(page_count(old_page) != 1);
> + /*
> + * Page refcount may be 1 in the common case,
> + * but since we may do an extra get_page()
> + * when called from hugetlb_fault(), we allow
> + * a page refcount of 2 as well.
> + */
> + BUG_ON(page_count(old_page) != 1 &&
> + page_count(old_page) != 2);
> BUG_ON(huge_pte_none(pte));
> spin_lock(&mm->page_table_lock);
> ptep = huge_pte_offset(mm, address & huge_page_mask(h));
> --
> 1.6.5.2
next prev parent reply other threads:[~2012-04-30 20:19 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-29 19:04 [PATCH] hugetlb: avoid gratuitous BUG_ON in hugetlb_fault() -> hugetlb_cow() Chris Metcalf
2012-04-30 20:19 ` Hugh Dickins [this message]
2012-05-01 13:14 ` Mel Gorman
2012-04-29 19:04 ` Chris Metcalf
2012-05-01 13:47 ` Mel Gorman
2012-05-01 13:53 ` Hillf Danton
2012-05-01 15:43 ` Hugh Dickins
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=alpine.LSU.2.00.1204301308090.2829@eggly.anvils \
--to=hughd@google.com \
--cc=akpm@linux-foundation.org \
--cc=cmetcalf@tilera.com \
--cc=dhillf@gmail.com \
--cc=kamezawa.hiroyu@jp.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mel@csn.ul.ie \
--cc=mhocko@suse.cz \
/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).