* [PATCH v2 1/3] use get_page_migratetype instead of page_private
2012-09-06 5:16 [PATCH v2 0/3] memory-hotplug: handle page race between allocation and isolation Minchan Kim
@ 2012-09-06 5:16 ` Minchan Kim
2012-09-06 6:06 ` Kamezawa Hiroyuki
2012-09-06 5:16 ` [PATCH v2 2/3] mm: remain migratetype in freed page Minchan Kim
2012-09-06 5:16 ` [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation Minchan Kim
2 siblings, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2012-09-06 5:16 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Mel Gorman, Kamezawa Hiroyuki,
Yasuaki Ishimatsu, Xishi Qiu, Wen Congyang, Minchan Kim
page allocator uses set_page_private and page_private for handling
migratetype when it frees page. Let's replace them with [set|get]
_freepage_migratetype to make it more clear.
* from v1
* Change set_page_migratetype with set_freepage_migratetype
* Add comment on set_freepage_migratetype
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/mm.h | 12 ++++++++++++
mm/page_alloc.c | 10 ++++++----
mm/page_isolation.c | 2 +-
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0514fe9..84d1663f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -237,6 +237,18 @@ struct inode;
#define page_private(page) ((page)->private)
#define set_page_private(page, v) ((page)->private = (v))
+/* It's valid only if the page is free path or free_list */
+static inline void set_freepage_migratetype(struct page *page, int migratetype)
+{
+ set_page_private(page, migratetype);
+}
+
+/* It's valid only if the page is free path or free_list */
+static inline int get_freepage_migratetype(struct page *page)
+{
+ return page_private(page);
+}
+
/*
* FIXME: take this include out, include page-flags.h in
* files which need it (119 of them)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ba3100a..f5ba236 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -671,8 +671,10 @@ static void free_pcppages_bulk(struct zone *zone, int count,
/* must delete as __free_one_page list manipulates */
list_del(&page->lru);
/* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
- __free_one_page(page, zone, 0, page_private(page));
- trace_mm_page_pcpu_drain(page, 0, page_private(page));
+ __free_one_page(page, zone, 0,
+ get_freepage_migratetype(page));
+ trace_mm_page_pcpu_drain(page, 0,
+ get_freepage_migratetype(page));
} while (--to_free && --batch_free && !list_empty(list));
}
__mod_zone_page_state(zone, NR_FREE_PAGES, count);
@@ -1134,7 +1136,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
if (!is_migrate_cma(mt) && mt != MIGRATE_ISOLATE)
mt = migratetype;
}
- set_page_private(page, mt);
+ set_freepage_migratetype(page, mt);
list = &page->lru;
}
__mod_zone_page_state(zone, NR_FREE_PAGES, -(i << order));
@@ -1301,7 +1303,7 @@ void free_hot_cold_page(struct page *page, int cold)
return;
migratetype = get_pageblock_migratetype(page);
- set_page_private(page, migratetype);
+ set_freepage_migratetype(page, migratetype);
local_irq_save(flags);
if (unlikely(wasMlocked))
free_page_mlock(page);
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 247d1f1..87a7929 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -196,7 +196,7 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
if (PageBuddy(page))
pfn += 1 << page_order(page);
else if (page_count(page) == 0 &&
- page_private(page) == MIGRATE_ISOLATE)
+ get_freepage_migratetype(page) == MIGRATE_ISOLATE)
pfn += 1;
else
break;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 1/3] use get_page_migratetype instead of page_private
2012-09-06 5:16 ` [PATCH v2 1/3] use get_page_migratetype instead of page_private Minchan Kim
@ 2012-09-06 6:06 ` Kamezawa Hiroyuki
0 siblings, 0 replies; 13+ messages in thread
From: Kamezawa Hiroyuki @ 2012-09-06 6:06 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Yasuaki Ishimatsu, Xishi Qiu, Wen Congyang
(2012/09/06 14:16), Minchan Kim wrote:
> page allocator uses set_page_private and page_private for handling
> migratetype when it frees page. Let's replace them with [set|get]
> _freepage_migratetype to make it more clear.
>
> * from v1
> * Change set_page_migratetype with set_freepage_migratetype
> * Add comment on set_freepage_migratetype
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
seems good to me.
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] mm: remain migratetype in freed page
2012-09-06 5:16 [PATCH v2 0/3] memory-hotplug: handle page race between allocation and isolation Minchan Kim
2012-09-06 5:16 ` [PATCH v2 1/3] use get_page_migratetype instead of page_private Minchan Kim
@ 2012-09-06 5:16 ` Minchan Kim
2012-09-06 6:13 ` Kamezawa Hiroyuki
2012-09-06 5:16 ` [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation Minchan Kim
2 siblings, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2012-09-06 5:16 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Mel Gorman, Kamezawa Hiroyuki,
Yasuaki Ishimatsu, Xishi Qiu, Wen Congyang, Minchan Kim
The page allocator caches the pageblock information in page->private while
it is in the PCP freelists but this is overwritten with the order of the
page when freed to the buddy allocator. This patch stores the migratetype
of the page in the page->index field so that it is available at all times
when the page remain in free_list.
This patch adds a new call site in __free_pages_ok so it might be
overhead a bit but it's for high order allocation.
So I believe damage isn't hurt.
* from v1
* Fix move_freepages's migratetype - Mel
* Add more kind explanation in description - Mel
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/mm.h | 4 ++--
mm/page_alloc.c | 7 +++++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 84d1663f..68f9e8d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -240,13 +240,13 @@ struct inode;
/* It's valid only if the page is free path or free_list */
static inline void set_freepage_migratetype(struct page *page, int migratetype)
{
- set_page_private(page, migratetype);
+ page->index = migratetype;
}
/* It's valid only if the page is free path or free_list */
static inline int get_freepage_migratetype(struct page *page)
{
- return page_private(page);
+ return page->index;
}
/*
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f5ba236..8531fa3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -723,6 +723,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
{
unsigned long flags;
int wasMlocked = __TestClearPageMlocked(page);
+ int migratetype;
if (!free_pages_prepare(page, order))
return;
@@ -731,8 +732,9 @@ static void __free_pages_ok(struct page *page, unsigned int order)
if (unlikely(wasMlocked))
free_page_mlock(page);
__count_vm_events(PGFREE, 1 << order);
- free_one_page(page_zone(page), page, order,
- get_pageblock_migratetype(page));
+ migratetype = get_pageblock_migratetype(page);
+ set_freepage_migratetype(page, migratetype);
+ free_one_page(page_zone(page), page, order, migratetype);
local_irq_restore(flags);
}
@@ -952,6 +954,7 @@ static int move_freepages(struct zone *zone,
order = page_order(page);
list_move(&page->lru,
&zone->free_area[order].free_list[migratetype]);
+ set_freepage_migratetype(page, migratetype);
page += 1 << order;
pages_moved += 1 << order;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] mm: remain migratetype in freed page
2012-09-06 5:16 ` [PATCH v2 2/3] mm: remain migratetype in freed page Minchan Kim
@ 2012-09-06 6:13 ` Kamezawa Hiroyuki
2012-09-07 0:08 ` Minchan Kim
0 siblings, 1 reply; 13+ messages in thread
From: Kamezawa Hiroyuki @ 2012-09-06 6:13 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Yasuaki Ishimatsu, Xishi Qiu, Wen Congyang
(2012/09/06 14:16), Minchan Kim wrote:
> The page allocator caches the pageblock information in page->private while
> it is in the PCP freelists but this is overwritten with the order of the
> page when freed to the buddy allocator. This patch stores the migratetype
> of the page in the page->index field so that it is available at all times
> when the page remain in free_list.
>
sounds reasonable.
> This patch adds a new call site in __free_pages_ok so it might be
> overhead a bit but it's for high order allocation.
> So I believe damage isn't hurt.
>
> * from v1
> * Fix move_freepages's migratetype - Mel
> * Add more kind explanation in description - Mel
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Hmm, page->index is valid only when the page is the head of buddy chunk ?
Anyway,
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
> include/linux/mm.h | 4 ++--
> mm/page_alloc.c | 7 +++++--
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 84d1663f..68f9e8d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -240,13 +240,13 @@ struct inode;
> /* It's valid only if the page is free path or free_list */
> static inline void set_freepage_migratetype(struct page *page, int migratetype)
> {
> - set_page_private(page, migratetype);
> + page->index = migratetype;
> }
>
> /* It's valid only if the page is free path or free_list */
> static inline int get_freepage_migratetype(struct page *page)
> {
> - return page_private(page);
> + return page->index;
> }
>
> /*
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f5ba236..8531fa3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -723,6 +723,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
> {
> unsigned long flags;
> int wasMlocked = __TestClearPageMlocked(page);
> + int migratetype;
>
> if (!free_pages_prepare(page, order))
> return;
> @@ -731,8 +732,9 @@ static void __free_pages_ok(struct page *page, unsigned int order)
> if (unlikely(wasMlocked))
> free_page_mlock(page);
> __count_vm_events(PGFREE, 1 << order);
> - free_one_page(page_zone(page), page, order,
> - get_pageblock_migratetype(page));
> + migratetype = get_pageblock_migratetype(page);
> + set_freepage_migratetype(page, migratetype);
> + free_one_page(page_zone(page), page, order, migratetype);
> local_irq_restore(flags);
> }
>
> @@ -952,6 +954,7 @@ static int move_freepages(struct zone *zone,
> order = page_order(page);
> list_move(&page->lru,
> &zone->free_area[order].free_list[migratetype]);
> + set_freepage_migratetype(page, migratetype);
> page += 1 << order;
> pages_moved += 1 << order;
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] mm: remain migratetype in freed page
2012-09-06 6:13 ` Kamezawa Hiroyuki
@ 2012-09-07 0:08 ` Minchan Kim
0 siblings, 0 replies; 13+ messages in thread
From: Minchan Kim @ 2012-09-07 0:08 UTC (permalink / raw)
To: Kamezawa Hiroyuki
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Yasuaki Ishimatsu, Xishi Qiu, Wen Congyang
On Thu, Sep 06, 2012 at 03:13:08PM +0900, Kamezawa Hiroyuki wrote:
> (2012/09/06 14:16), Minchan Kim wrote:
> > The page allocator caches the pageblock information in page->private while
> > it is in the PCP freelists but this is overwritten with the order of the
> > page when freed to the buddy allocator. This patch stores the migratetype
> > of the page in the page->index field so that it is available at all times
> > when the page remain in free_list.
> >
> sounds reasonable.
>
> > This patch adds a new call site in __free_pages_ok so it might be
> > overhead a bit but it's for high order allocation.
> > So I believe damage isn't hurt.
> >
> > * from v1
> > * Fix move_freepages's migratetype - Mel
> > * Add more kind explanation in description - Mel
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>
> Hmm, page->index is valid only when the page is the head of buddy chunk ?
Yes.
>
> Anyway,
>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Thanks, Kame!
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-09-06 5:16 [PATCH v2 0/3] memory-hotplug: handle page race between allocation and isolation Minchan Kim
2012-09-06 5:16 ` [PATCH v2 1/3] use get_page_migratetype instead of page_private Minchan Kim
2012-09-06 5:16 ` [PATCH v2 2/3] mm: remain migratetype in freed page Minchan Kim
@ 2012-09-06 5:16 ` Minchan Kim
2012-09-06 6:18 ` Kamezawa Hiroyuki
2012-09-06 7:17 ` Yasuaki Ishimatsu
2 siblings, 2 replies; 13+ messages in thread
From: Minchan Kim @ 2012-09-06 5:16 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Mel Gorman, Kamezawa Hiroyuki,
Yasuaki Ishimatsu, Xishi Qiu, Wen Congyang, Minchan Kim
Like below, memory-hotplug makes race between page-isolation
and page-allocation so it can hit BUG_ON in __offline_isolated_pages.
CPU A CPU B
start_isolate_page_range
set_migratetype_isolate
spin_lock_irqsave(zone->lock)
free_hot_cold_page(Page A)
/* without zone->lock */
migratetype = get_pageblock_migratetype(Page A);
/*
* Page could be moved into MIGRATE_MOVABLE
* of per_cpu_pages
*/
list_add_tail(&page->lru, &pcp->lists[migratetype]);
set_pageblock_isolate
move_freepages_block
drain_all_pages
/* Page A could be in MIGRATE_MOVABLE of free_list. */
check_pages_isolated
__test_page_isolated_in_pageblock
/*
* We can't catch freed page which
* is free_list[MIGRATE_MOVABLE]
*/
if (PageBuddy(page A))
pfn += 1 << page_order(page A);
/* So, Page A could be allocated */
__offline_isolated_pages
/*
* BUG_ON hit or offline page
* which is used by someone
*/
BUG_ON(!PageBuddy(page A));
This patch checks page's migratetype in freelist in __test_page_isolated_in_pageblock.
So now __test_page_isolated_in_pageblock can check the page caused by above race and
can fail of memory offlining.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
mm/page_isolation.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index 87a7929..7ba7405 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -193,8 +193,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
continue;
}
page = pfn_to_page(pfn);
- if (PageBuddy(page))
+ if (PageBuddy(page)) {
+ if (get_freepage_migratetype(page) != MIGRATE_ISOLATE)
+ break;
pfn += 1 << page_order(page);
+ }
else if (page_count(page) == 0 &&
get_freepage_migratetype(page) == MIGRATE_ISOLATE)
pfn += 1;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-09-06 5:16 ` [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation Minchan Kim
@ 2012-09-06 6:18 ` Kamezawa Hiroyuki
2012-09-06 7:17 ` Yasuaki Ishimatsu
1 sibling, 0 replies; 13+ messages in thread
From: Kamezawa Hiroyuki @ 2012-09-06 6:18 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Yasuaki Ishimatsu, Xishi Qiu, Wen Congyang
(2012/09/06 14:16), Minchan Kim wrote:
> Like below, memory-hotplug makes race between page-isolation
> and page-allocation so it can hit BUG_ON in __offline_isolated_pages.
>
> CPU A CPU B
>
> start_isolate_page_range
> set_migratetype_isolate
> spin_lock_irqsave(zone->lock)
>
> free_hot_cold_page(Page A)
> /* without zone->lock */
> migratetype = get_pageblock_migratetype(Page A);
> /*
> * Page could be moved into MIGRATE_MOVABLE
> * of per_cpu_pages
> */
> list_add_tail(&page->lru, &pcp->lists[migratetype]);
>
> set_pageblock_isolate
> move_freepages_block
> drain_all_pages
>
> /* Page A could be in MIGRATE_MOVABLE of free_list. */
>
> check_pages_isolated
> __test_page_isolated_in_pageblock
> /*
> * We can't catch freed page which
> * is free_list[MIGRATE_MOVABLE]
> */
> if (PageBuddy(page A))
> pfn += 1 << page_order(page A);
>
> /* So, Page A could be allocated */
>
> __offline_isolated_pages
> /*
> * BUG_ON hit or offline page
> * which is used by someone
> */
> BUG_ON(!PageBuddy(page A));
>
> This patch checks page's migratetype in freelist in __test_page_isolated_in_pageblock.
> So now __test_page_isolated_in_pageblock can check the page caused by above race and
> can fail of memory offlining.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
I agree this fix.
Maybe we (fujitsu) should revisit these logics around and make it cleaner....
Anyway, thank you!
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
> mm/page_isolation.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 87a7929..7ba7405 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -193,8 +193,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
> continue;
> }
> page = pfn_to_page(pfn);
> - if (PageBuddy(page))
> + if (PageBuddy(page)) {
> + if (get_freepage_migratetype(page) != MIGRATE_ISOLATE)
> + break;
> pfn += 1 << page_order(page);
> + }
> else if (page_count(page) == 0 &&
> get_freepage_migratetype(page) == MIGRATE_ISOLATE)
> pfn += 1;
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-09-06 5:16 ` [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation Minchan Kim
2012-09-06 6:18 ` Kamezawa Hiroyuki
@ 2012-09-06 7:17 ` Yasuaki Ishimatsu
2012-09-06 7:30 ` Minchan Kim
1 sibling, 1 reply; 13+ messages in thread
From: Yasuaki Ishimatsu @ 2012-09-06 7:17 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Kamezawa Hiroyuki, Xishi Qiu, Wen Congyang
Hi Minchan,
2012/09/06 14:16, Minchan Kim wrote:
> Like below, memory-hotplug makes race between page-isolation
> and page-allocation so it can hit BUG_ON in __offline_isolated_pages.
>
> CPU A CPU B
>
> start_isolate_page_range
> set_migratetype_isolate
> spin_lock_irqsave(zone->lock)
>
> free_hot_cold_page(Page A)
> /* without zone->lock */
> migratetype = get_pageblock_migratetype(Page A);
> /*
> * Page could be moved into MIGRATE_MOVABLE
> * of per_cpu_pages
> */
> list_add_tail(&page->lru, &pcp->lists[migratetype]);
>
> set_pageblock_isolate
> move_freepages_block
> drain_all_pages
>
> /* Page A could be in MIGRATE_MOVABLE of free_list. */
>
> check_pages_isolated
> __test_page_isolated_in_pageblock
> /*
> * We can't catch freed page which
> * is free_list[MIGRATE_MOVABLE]
> */
> if (PageBuddy(page A))
> pfn += 1 << page_order(page A);
>
> /* So, Page A could be allocated */
>
> __offline_isolated_pages
> /*
> * BUG_ON hit or offline page
> * which is used by someone
> */
> BUG_ON(!PageBuddy(page A));
>
> This patch checks page's migratetype in freelist in __test_page_isolated_in_pageblock.
> So now __test_page_isolated_in_pageblock can check the page caused by above race and
> can fail of memory offlining.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> mm/page_isolation.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 87a7929..7ba7405 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -193,8 +193,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
> continue;
> }
> page = pfn_to_page(pfn);
> - if (PageBuddy(page))
> + if (PageBuddy(page)) {
> + if (get_freepage_migratetype(page) != MIGRATE_ISOLATE)
> + break;
> pfn += 1 << page_order(page);
> + }
> else if (page_count(page) == 0 &&
> get_freepage_migratetype(page) == MIGRATE_ISOLATE)
When do the if statement, the page may be used by someone.
In this case, page->index may have some number. If the number is same as
MIGRATE_ISOLATE, the code goes worng.
Thanks,
Yasuaki Ishimatsu
> pfn += 1;
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-09-06 7:17 ` Yasuaki Ishimatsu
@ 2012-09-06 7:30 ` Minchan Kim
2012-09-06 9:14 ` Yasuaki Ishimatsu
0 siblings, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2012-09-06 7:30 UTC (permalink / raw)
To: Yasuaki Ishimatsu
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Kamezawa Hiroyuki, Xishi Qiu, Wen Congyang
Hello Yasuaki,
On Thu, Sep 06, 2012 at 04:17:54PM +0900, Yasuaki Ishimatsu wrote:
> Hi Minchan,
>
> 2012/09/06 14:16, Minchan Kim wrote:
> > Like below, memory-hotplug makes race between page-isolation
> > and page-allocation so it can hit BUG_ON in __offline_isolated_pages.
> >
> > CPU A CPU B
> >
> > start_isolate_page_range
> > set_migratetype_isolate
> > spin_lock_irqsave(zone->lock)
> >
> > free_hot_cold_page(Page A)
> > /* without zone->lock */
> > migratetype = get_pageblock_migratetype(Page A);
> > /*
> > * Page could be moved into MIGRATE_MOVABLE
> > * of per_cpu_pages
> > */
> > list_add_tail(&page->lru, &pcp->lists[migratetype]);
> >
> > set_pageblock_isolate
> > move_freepages_block
> > drain_all_pages
> >
> > /* Page A could be in MIGRATE_MOVABLE of free_list. */
> >
> > check_pages_isolated
> > __test_page_isolated_in_pageblock
> > /*
> > * We can't catch freed page which
> > * is free_list[MIGRATE_MOVABLE]
> > */
> > if (PageBuddy(page A))
> > pfn += 1 << page_order(page A);
> >
> > /* So, Page A could be allocated */
> >
> > __offline_isolated_pages
> > /*
> > * BUG_ON hit or offline page
> > * which is used by someone
> > */
> > BUG_ON(!PageBuddy(page A));
> >
> > This patch checks page's migratetype in freelist in __test_page_isolated_in_pageblock.
> > So now __test_page_isolated_in_pageblock can check the page caused by above race and
> > can fail of memory offlining.
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> > mm/page_isolation.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> > index 87a7929..7ba7405 100644
> > --- a/mm/page_isolation.c
> > +++ b/mm/page_isolation.c
> > @@ -193,8 +193,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
> > continue;
> > }
> > page = pfn_to_page(pfn);
> > - if (PageBuddy(page))
> > + if (PageBuddy(page)) {
> > + if (get_freepage_migratetype(page) != MIGRATE_ISOLATE)
> > + break;
> > pfn += 1 << page_order(page);
> > + }
>
> > else if (page_count(page) == 0 &&
> > get_freepage_migratetype(page) == MIGRATE_ISOLATE)
>
> When do the if statement, the page may be used by someone.
I can't understand your point.
We already hold zone->lock so that allocator and this function should be atomic
when the page is in free_list.
If I miss something, could you elaborate it more?
> In this case, page->index may have some number. If the number is same as
> MIGRATE_ISOLATE, the code goes worng.
>
> Thanks,
> Yasuaki Ishimatsu
>
> > pfn += 1;
> >
>
>
> --
> 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>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-09-06 7:30 ` Minchan Kim
@ 2012-09-06 9:14 ` Yasuaki Ishimatsu
2012-09-06 23:37 ` Minchan Kim
0 siblings, 1 reply; 13+ messages in thread
From: Yasuaki Ishimatsu @ 2012-09-06 9:14 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Kamezawa Hiroyuki, Xishi Qiu, Wen Congyang
Hi, Minchan,
2012/09/06 16:30, Minchan Kim wrote:
> Hello Yasuaki,
>
> On Thu, Sep 06, 2012 at 04:17:54PM +0900, Yasuaki Ishimatsu wrote:
>> Hi Minchan,
>>
>> 2012/09/06 14:16, Minchan Kim wrote:
>>> Like below, memory-hotplug makes race between page-isolation
>>> and page-allocation so it can hit BUG_ON in __offline_isolated_pages.
>>>
>>> CPU A CPU B
>>>
>>> start_isolate_page_range
>>> set_migratetype_isolate
>>> spin_lock_irqsave(zone->lock)
>>>
>>> free_hot_cold_page(Page A)
>>> /* without zone->lock */
>>> migratetype = get_pageblock_migratetype(Page A);
>>> /*
>>> * Page could be moved into MIGRATE_MOVABLE
>>> * of per_cpu_pages
>>> */
>>> list_add_tail(&page->lru, &pcp->lists[migratetype]);
>>>
>>> set_pageblock_isolate
>>> move_freepages_block
>>> drain_all_pages
>>>
>>> /* Page A could be in MIGRATE_MOVABLE of free_list. */
>>>
>>> check_pages_isolated
>>> __test_page_isolated_in_pageblock
>>> /*
>>> * We can't catch freed page which
>>> * is free_list[MIGRATE_MOVABLE]
>>> */
>>> if (PageBuddy(page A))
>>> pfn += 1 << page_order(page A);
>>>
>>> /* So, Page A could be allocated */
>>>
>>> __offline_isolated_pages
>>> /*
>>> * BUG_ON hit or offline page
>>> * which is used by someone
>>> */
>>> BUG_ON(!PageBuddy(page A));
>>>
>>> This patch checks page's migratetype in freelist in __test_page_isolated_in_pageblock.
>>> So now __test_page_isolated_in_pageblock can check the page caused by above race and
>>> can fail of memory offlining.
>>>
>>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>>> ---
>>> mm/page_isolation.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>> index 87a7929..7ba7405 100644
>>> --- a/mm/page_isolation.c
>>> +++ b/mm/page_isolation.c
>>> @@ -193,8 +193,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
>>> continue;
>>> }
>>> page = pfn_to_page(pfn);
>>> - if (PageBuddy(page))
>>> + if (PageBuddy(page)) {
>>> + if (get_freepage_migratetype(page) != MIGRATE_ISOLATE)
>>> + break;
>>> pfn += 1 << page_order(page);
>>> + }
>>
>>> else if (page_count(page) == 0 &&
>>> get_freepage_migratetype(page) == MIGRATE_ISOLATE)
>>
>> When do the if statement, the page may be used by someone.
>
> I can't understand your point.
> We already hold zone->lock so that allocator and this function should be atomic
> when the page is in free_list.
> If I miss something, could you elaborate it more?
According to your description, the page might be allocated by someone
at this point. So some value might be set the page->index by the
intended purpose. Thus page->index has the potential to become
MIGRATE_ISOLATE value.
Thanks,
Yasuaki Ishimatsu
>
>> In this case, page->index may have some number. If the number is same as
>> MIGRATE_ISOLATE, the code goes worng.
>>
>> Thanks,
>> Yasuaki Ishimatsu
>>
>>> pfn += 1;
>>>
>>
>>
>> --
>> 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>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-09-06 9:14 ` Yasuaki Ishimatsu
@ 2012-09-06 23:37 ` Minchan Kim
2012-09-07 3:47 ` Yasuaki Ishimatsu
0 siblings, 1 reply; 13+ messages in thread
From: Minchan Kim @ 2012-09-06 23:37 UTC (permalink / raw)
To: Yasuaki Ishimatsu
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Kamezawa Hiroyuki, Xishi Qiu, Wen Congyang
Hello Yasuaki,
On Thu, Sep 06, 2012 at 06:14:39PM +0900, Yasuaki Ishimatsu wrote:
> Hi, Minchan,
>
> 2012/09/06 16:30, Minchan Kim wrote:
> >Hello Yasuaki,
> >
> >On Thu, Sep 06, 2012 at 04:17:54PM +0900, Yasuaki Ishimatsu wrote:
> >>Hi Minchan,
> >>
> >>2012/09/06 14:16, Minchan Kim wrote:
> >>>Like below, memory-hotplug makes race between page-isolation
> >>>and page-allocation so it can hit BUG_ON in __offline_isolated_pages.
> >>>
> >>> CPU A CPU B
> >>>
> >>>start_isolate_page_range
> >>>set_migratetype_isolate
> >>>spin_lock_irqsave(zone->lock)
> >>>
> >>> free_hot_cold_page(Page A)
> >>> /* without zone->lock */
> >>> migratetype = get_pageblock_migratetype(Page A);
> >>> /*
> >>> * Page could be moved into MIGRATE_MOVABLE
> >>> * of per_cpu_pages
> >>> */
> >>> list_add_tail(&page->lru, &pcp->lists[migratetype]);
> >>>
> >>>set_pageblock_isolate
> >>>move_freepages_block
> >>>drain_all_pages
> >>>
> >>> /* Page A could be in MIGRATE_MOVABLE of free_list. */
> >>>
> >>>check_pages_isolated
> >>>__test_page_isolated_in_pageblock
> >>>/*
> >>> * We can't catch freed page which
> >>> * is free_list[MIGRATE_MOVABLE]
> >>> */
> >>>if (PageBuddy(page A))
> >>> pfn += 1 << page_order(page A);
> >>>
> >>> /* So, Page A could be allocated */
> >>>
> >>>__offline_isolated_pages
> >>>/*
> >>> * BUG_ON hit or offline page
> >>> * which is used by someone
> >>> */
> >>>BUG_ON(!PageBuddy(page A));
> >>>
> >>>This patch checks page's migratetype in freelist in __test_page_isolated_in_pageblock.
> >>>So now __test_page_isolated_in_pageblock can check the page caused by above race and
> >>>can fail of memory offlining.
> >>>
> >>>Signed-off-by: Minchan Kim <minchan@kernel.org>
> >>>---
> >>> mm/page_isolation.c | 5 ++++-
> >>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> >>>index 87a7929..7ba7405 100644
> >>>--- a/mm/page_isolation.c
> >>>+++ b/mm/page_isolation.c
> >>>@@ -193,8 +193,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
> >>> continue;
> >>> }
> >>> page = pfn_to_page(pfn);
> >>>- if (PageBuddy(page))
> >>>+ if (PageBuddy(page)) {
> >>>+ if (get_freepage_migratetype(page) != MIGRATE_ISOLATE)
> >>>+ break;
> >>> pfn += 1 << page_order(page);
> >>>+ }
> >>
> >>> else if (page_count(page) == 0 &&
> >>> get_freepage_migratetype(page) == MIGRATE_ISOLATE)
> >>
> >>When do the if statement, the page may be used by someone.
> >
> >I can't understand your point.
> >We already hold zone->lock so that allocator and this function should be atomic
> >when the page is in free_list.
> >If I miss something, could you elaborate it more?
>
> According to your description, the page might be allocated by someone
> at this point. So some value might be set the page->index by the
It seems you are misunderstanding my point.
Before my patch, Yes. It could be allocated by someone but
after my patch, it couldn't be allocated and inconsistency between
pageblock's migratetype and page's migratetype is detected so that
memory offline would be just failed simply.
> intended purpose. Thus page->index has the potential to become
> MIGRATE_ISOLATE value.
>
> Thanks,
> Yasuaki Ishimatsu
>
> >
> >>In this case, page->index may have some number. If the number is same as
> >>MIGRATE_ISOLATE, the code goes worng.
> >>
> >>Thanks,
> >>Yasuaki Ishimatsu
> >>
> >>> pfn += 1;
> >>>
> >>
> >>
> >>--
> >>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>
> >
>
>
> --
> 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>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-09-06 23:37 ` Minchan Kim
@ 2012-09-07 3:47 ` Yasuaki Ishimatsu
0 siblings, 0 replies; 13+ messages in thread
From: Yasuaki Ishimatsu @ 2012-09-07 3:47 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Mel Gorman,
Kamezawa Hiroyuki, Xishi Qiu, Wen Congyang
Hi Minchan,
2012/09/07 8:37, Minchan Kim wrote:
> Hello Yasuaki,
>
> On Thu, Sep 06, 2012 at 06:14:39PM +0900, Yasuaki Ishimatsu wrote:
>> Hi, Minchan,
>>
>> 2012/09/06 16:30, Minchan Kim wrote:
>>> Hello Yasuaki,
>>>
>>> On Thu, Sep 06, 2012 at 04:17:54PM +0900, Yasuaki Ishimatsu wrote:
>>>> Hi Minchan,
>>>>
>>>> 2012/09/06 14:16, Minchan Kim wrote:
>>>>> Like below, memory-hotplug makes race between page-isolation
>>>>> and page-allocation so it can hit BUG_ON in __offline_isolated_pages.
>>>>>
>>>>> CPU A CPU B
>>>>>
>>>>> start_isolate_page_range
>>>>> set_migratetype_isolate
>>>>> spin_lock_irqsave(zone->lock)
>>>>>
>>>>> free_hot_cold_page(Page A)
>>>>> /* without zone->lock */
>>>>> migratetype = get_pageblock_migratetype(Page A);
>>>>> /*
>>>>> * Page could be moved into MIGRATE_MOVABLE
>>>>> * of per_cpu_pages
>>>>> */
>>>>> list_add_tail(&page->lru, &pcp->lists[migratetype]);
>>>>>
>>>>> set_pageblock_isolate
>>>>> move_freepages_block
>>>>> drain_all_pages
>>>>>
>>>>> /* Page A could be in MIGRATE_MOVABLE of free_list. */
>>>>>
>>>>> check_pages_isolated
>>>>> __test_page_isolated_in_pageblock
>>>>> /*
>>>>> * We can't catch freed page which
>>>>> * is free_list[MIGRATE_MOVABLE]
>>>>> */
>>>>> if (PageBuddy(page A))
>>>>> pfn += 1 << page_order(page A);
>>>>>
>>>>> /* So, Page A could be allocated */
>>>>>
>>>>> __offline_isolated_pages
>>>>> /*
>>>>> * BUG_ON hit or offline page
>>>>> * which is used by someone
>>>>> */
>>>>> BUG_ON(!PageBuddy(page A));
>>>>>
>>>>> This patch checks page's migratetype in freelist in __test_page_isolated_in_pageblock.
>>>>> So now __test_page_isolated_in_pageblock can check the page caused by above race and
>>>>> can fail of memory offlining.
>>>>>
>>>>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>>>>> ---
>>>>> mm/page_isolation.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
>>>>> index 87a7929..7ba7405 100644
>>>>> --- a/mm/page_isolation.c
>>>>> +++ b/mm/page_isolation.c
>>>>> @@ -193,8 +193,11 @@ __test_page_isolated_in_pageblock(unsigned long pfn, unsigned long end_pfn)
>>>>> continue;
>>>>> }
>>>>> page = pfn_to_page(pfn);
>>>>> - if (PageBuddy(page))
>>>>> + if (PageBuddy(page)) {
>>>>> + if (get_freepage_migratetype(page) != MIGRATE_ISOLATE)
>>>>> + break;
>>>>> pfn += 1 << page_order(page);
>>>>> + }
>>>>
>>>>> else if (page_count(page) == 0 &&
>>>>> get_freepage_migratetype(page) == MIGRATE_ISOLATE)
>>>>
>>>> When do the if statement, the page may be used by someone.
>>>
>>> I can't understand your point.
>>> We already hold zone->lock so that allocator and this function should be atomic
>>> when the page is in free_list.
>>> If I miss something, could you elaborate it more?
>>
>> According to your description, the page might be allocated by someone
>> at this point. So some value might be set the page->index by the
>
> It seems you are misunderstanding my point.
> Before my patch, Yes. It could be allocated by someone but
> after my patch, it couldn't be allocated and inconsistency between
> pageblock's migratetype and page's migratetype is detected so that
> memory offline would be just failed simply.
Thank you for your explanation.
My concern disappeared.
Thanks,
Yasuaki Ishimatsu
>
>> intended purpose. Thus page->index has the potential to become
>> MIGRATE_ISOLATE value.
>>
>> Thanks,
>> Yasuaki Ishimatsu
>>
>>>
>>>> In this case, page->index may have some number. If the number is same as
>>>> MIGRATE_ISOLATE, the code goes worng.
>>>>
>>>> Thanks,
>>>> Yasuaki Ishimatsu
>>>>
>>>>> pfn += 1;
>>>>>
>>>>
>>>>
>>>> --
>>>> 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>
>>>
>>
>>
>> --
>> 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>
>
^ permalink raw reply [flat|nested] 13+ messages in thread