All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mel Gorman <mgorman@suse.de>
To: Dave Hansen <dave@sr71.net>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	dave.jiang@intel.com, akpm@linux-foundation.org,
	dhillf@gmail.com, Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Subject: Re: [v3][PATCH 2/2] mm: thp: give transparent hugepage code a separate copy_page
Date: Mon, 18 Nov 2013 10:32:47 +0000	[thread overview]
Message-ID: <20131118103247.GF26002@suse.de> (raw)
In-Reply-To: <20131115225553.B0E9DFFB@viggo.jf.intel.com>

On Fri, Nov 15, 2013 at 02:55:53PM -0800, Dave Hansen wrote:
> 
> Changes from v2:
>  * 
> Changes from v1:
>  * removed explicit might_sleep() in favor of the one that we
>    get from the cond_resched();
> 
> --
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Right now, the migration code in migrate_page_copy() uses
> copy_huge_page() for hugetlbfs and thp pages:
> 
>        if (PageHuge(page) || PageTransHuge(page))
>                 copy_huge_page(newpage, page);
> 
> So, yay for code reuse.  But:
> 
> void copy_huge_page(struct page *dst, struct page *src)
> {
>         struct hstate *h = page_hstate(src);
> 
> and a non-hugetlbfs page has no page_hstate().  This works 99% of
> the time because page_hstate() determines the hstate from the
> page order alone.  Since the page order of a THP page matches the
> default hugetlbfs page order, it works.
> 
> But, if you change the default huge page size on the boot
> command-line (say default_hugepagesz=1G), then we might not even
> *have* a 2MB hstate so page_hstate() returns null and
> copy_huge_page() oopses pretty fast since copy_huge_page()
> dereferences the hstate:
> 
> void copy_huge_page(struct page *dst, struct page *src)
> {
>         struct hstate *h = page_hstate(src);
>         if (unlikely(pages_per_huge_page(h) > MAX_ORDER_NR_PAGES)) {
> ...
> 
> Mel noticed that the migration code is really the only user of
> these functions.  This moves all the copy code over to migrate.c
> and makes copy_huge_page() work for THP by checking for it
> explicitly.
> 
> I believe the bug was introduced in b32967ff101:
> Author: Mel Gorman <mgorman@suse.de>
> Date:   Mon Nov 19 12:35:47 2012 +0000
> mm: numa: Add THP migration for the NUMA working set scanning fault case.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

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

WARNING: multiple messages have this Message-ID (diff)
From: Mel Gorman <mgorman@suse.de>
To: Dave Hansen <dave@sr71.net>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	dave.jiang@intel.com, akpm@linux-foundation.org,
	dhillf@gmail.com, Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Subject: Re: [v3][PATCH 2/2] mm: thp: give transparent hugepage code a separate copy_page
Date: Mon, 18 Nov 2013 10:32:47 +0000	[thread overview]
Message-ID: <20131118103247.GF26002@suse.de> (raw)
In-Reply-To: <20131115225553.B0E9DFFB@viggo.jf.intel.com>

On Fri, Nov 15, 2013 at 02:55:53PM -0800, Dave Hansen wrote:
> 
> Changes from v2:
>  * 
> Changes from v1:
>  * removed explicit might_sleep() in favor of the one that we
>    get from the cond_resched();
> 
> --
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Right now, the migration code in migrate_page_copy() uses
> copy_huge_page() for hugetlbfs and thp pages:
> 
>        if (PageHuge(page) || PageTransHuge(page))
>                 copy_huge_page(newpage, page);
> 
> So, yay for code reuse.  But:
> 
> void copy_huge_page(struct page *dst, struct page *src)
> {
>         struct hstate *h = page_hstate(src);
> 
> and a non-hugetlbfs page has no page_hstate().  This works 99% of
> the time because page_hstate() determines the hstate from the
> page order alone.  Since the page order of a THP page matches the
> default hugetlbfs page order, it works.
> 
> But, if you change the default huge page size on the boot
> command-line (say default_hugepagesz=1G), then we might not even
> *have* a 2MB hstate so page_hstate() returns null and
> copy_huge_page() oopses pretty fast since copy_huge_page()
> dereferences the hstate:
> 
> void copy_huge_page(struct page *dst, struct page *src)
> {
>         struct hstate *h = page_hstate(src);
>         if (unlikely(pages_per_huge_page(h) > MAX_ORDER_NR_PAGES)) {
> ...
> 
> Mel noticed that the migration code is really the only user of
> these functions.  This moves all the copy code over to migrate.c
> and makes copy_huge_page() work for THP by checking for it
> explicitly.
> 
> I believe the bug was introduced in b32967ff101:
> Author: Mel Gorman <mgorman@suse.de>
> Date:   Mon Nov 19 12:35:47 2012 +0000
> mm: numa: Add THP migration for the NUMA working set scanning fault case.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

  reply	other threads:[~2013-11-18 10:33 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-15 22:55 [v3][PATCH 0/2] v3: fix hugetlb vs. anon-thp copy page Dave Hansen
2013-11-15 22:55 ` Dave Hansen
2013-11-15 22:55 ` [v3][PATCH 1/2] mm: hugetlbfs: Add VM_BUG_ON()s to catch non-hugetlbfs pages Dave Hansen
2013-11-15 22:55   ` Dave Hansen
2013-11-15 22:55 ` [v3][PATCH 2/2] mm: thp: give transparent hugepage code a separate copy_page Dave Hansen
2013-11-15 22:55   ` Dave Hansen
2013-11-18 10:32   ` Mel Gorman [this message]
2013-11-18 10:32     ` Mel Gorman
2013-11-18 18:51   ` [v3][PATCH 2/2] mm: thp: give transparent hugepage code aseparate copy_page Naoya Horiguchi
2013-11-18 18:51     ` Naoya Horiguchi
2013-11-18 18:54     ` [PATCH] mm: call cond_resched() per MAX_ORDER_NR_PAGES pages copy Naoya Horiguchi
2013-11-18 18:54       ` Naoya Horiguchi
2013-11-18 19:02       ` Dave Hansen
2013-11-18 19:02         ` Dave Hansen
2013-11-18 20:20         ` Naoya Horiguchi
2013-11-18 20:20           ` Naoya Horiguchi
2013-11-18 20:48           ` Dave Hansen
2013-11-18 20:48             ` Dave Hansen
2013-11-18 21:56             ` Naoya Horiguchi
2013-11-18 21:56               ` Naoya Horiguchi
2013-11-18 22:29               ` Dave Hansen
2013-11-18 22:29                 ` Dave Hansen
2013-11-19  0:34                 ` Naoya Horiguchi
2013-11-19  0:34                   ` Naoya Horiguchi
2013-11-18 19:23 ` [v3][PATCH 0/2] v3: fix hugetlb vs. anon-thp copy page Jiang, Dave
2013-11-18 19:23   ` Jiang, Dave

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=20131118103247.GF26002@suse.de \
    --to=mgorman@suse.de \
    --cc=akpm@linux-foundation.org \
    --cc=dave.jiang@intel.com \
    --cc=dave@sr71.net \
    --cc=dhillf@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=n-horiguchi@ah.jp.nec.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 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.