* [RFC 0/3] memory-hotplug: handle page race between allocation and isolation
@ 2012-07-17 7:01 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 7:01 UTC (permalink / raw)
To: Kamezawa Hiroyuki, Mel Gorman
Cc: linux-mm, linux-kernel, Andrew Morton, KOSAKI Motohiro,
Hugh Dickins, Minchan Kim
Memory hotplug has a subtle race problem so this patchset fixes the problem
(Look at [3/3] for detail and please confirm the problem before review
other patches in this series.)
[1/3] is just clean up and help for [2/3].
[2/3] keeps the migratetype information to freed page's index field
and [3/3] uses the information.
[3/3] fixes the race problem with [2/3]'s information.
After applying [2/3], migratetype argument in __free_one_page
and free_one_page is redundant so we can remove it but I decide
to not touch them because it increases code size about 50 byte.
Minchan Kim (3):
mm: use get_page_migratetype instead of page_private
mm: remain migratetype in freed page
memory-hotplug: bug fix race between isolation and allocation
include/linux/mm.h | 12 ++++++++++++
mm/page_alloc.c | 16 ++++++++++------
mm/page_isolation.c | 7 +++++--
3 files changed, 27 insertions(+), 8 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 24+ messages in thread* [RFC 1/3] mm: use get_page_migratetype instead of page_private
2012-07-17 7:01 ` Minchan Kim
@ 2012-07-17 7:01 ` Minchan Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 7:01 UTC (permalink / raw)
To: Kamezawa Hiroyuki, Mel Gorman
Cc: linux-mm, linux-kernel, Andrew Morton, KOSAKI Motohiro,
Hugh Dickins, 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]
_page_migratetype to make it more clear.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/mm.h | 10 ++++++++++
mm/page_alloc.c | 11 +++++++----
mm/page_isolation.c | 2 +-
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5c76634..86d61d6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -249,6 +249,16 @@ struct inode;
#define page_private(page) ((page)->private)
#define set_page_private(page, v) ((page)->private = (v))
+static inline void set_page_migratetype(struct page *page, int migratetype)
+{
+ set_page_private(page, migratetype);
+}
+
+static inline int get_page_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 710d91c..103ba66 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_page_migratetype(page));
+ trace_mm_page_pcpu_drain(page, 0,
+ get_page_migratetype(page));
} while (--to_free && --batch_free && !list_empty(list));
}
__mod_zone_page_state(zone, NR_FREE_PAGES, count);
@@ -731,6 +733,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
__count_vm_events(PGFREE, 1 << order);
free_one_page(page_zone(page), page, order,
get_pageblock_migratetype(page));
+
local_irq_restore(flags);
}
@@ -1134,7 +1137,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_page_migratetype(page, mt);
list = &page->lru;
}
__mod_zone_page_state(zone, NR_FREE_PAGES, -(i << order));
@@ -1301,7 +1304,7 @@ void free_hot_cold_page(struct page *page, int cold)
return;
migratetype = get_pageblock_migratetype(page);
- set_page_private(page, migratetype);
+ set_page_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 64abb33..acf65a7 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -199,7 +199,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_page_migratetype(page) == MIGRATE_ISOLATE)
pfn += 1;
else
break;
--
1.7.9.5
--
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 related [flat|nested] 24+ messages in thread* [RFC 1/3] mm: use get_page_migratetype instead of page_private
@ 2012-07-17 7:01 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 7:01 UTC (permalink / raw)
To: Kamezawa Hiroyuki, Mel Gorman
Cc: linux-mm, linux-kernel, Andrew Morton, KOSAKI Motohiro,
Hugh Dickins, 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]
_page_migratetype to make it more clear.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/mm.h | 10 ++++++++++
mm/page_alloc.c | 11 +++++++----
mm/page_isolation.c | 2 +-
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5c76634..86d61d6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -249,6 +249,16 @@ struct inode;
#define page_private(page) ((page)->private)
#define set_page_private(page, v) ((page)->private = (v))
+static inline void set_page_migratetype(struct page *page, int migratetype)
+{
+ set_page_private(page, migratetype);
+}
+
+static inline int get_page_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 710d91c..103ba66 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_page_migratetype(page));
+ trace_mm_page_pcpu_drain(page, 0,
+ get_page_migratetype(page));
} while (--to_free && --batch_free && !list_empty(list));
}
__mod_zone_page_state(zone, NR_FREE_PAGES, count);
@@ -731,6 +733,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
__count_vm_events(PGFREE, 1 << order);
free_one_page(page_zone(page), page, order,
get_pageblock_migratetype(page));
+
local_irq_restore(flags);
}
@@ -1134,7 +1137,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_page_migratetype(page, mt);
list = &page->lru;
}
__mod_zone_page_state(zone, NR_FREE_PAGES, -(i << order));
@@ -1301,7 +1304,7 @@ void free_hot_cold_page(struct page *page, int cold)
return;
migratetype = get_pageblock_migratetype(page);
- set_page_private(page, migratetype);
+ set_page_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 64abb33..acf65a7 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -199,7 +199,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_page_migratetype(page) == MIGRATE_ISOLATE)
pfn += 1;
else
break;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC 2/3] mm: remain migratetype in freed page
2012-07-17 7:01 ` Minchan Kim
@ 2012-07-17 7:01 ` Minchan Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 7:01 UTC (permalink / raw)
To: Kamezawa Hiroyuki, Mel Gorman
Cc: linux-mm, linux-kernel, Andrew Morton, KOSAKI Motohiro,
Hugh Dickins, Minchan Kim
Page allocator doesn't keep migratetype information to page
when the page is freed. This patch remains the information
to freed page's index field which isn't used by free/alloc
preparing so it shouldn't change any behavir except below one.
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.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/mm.h | 6 ++++--
mm/page_alloc.c | 7 ++++---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 86d61d6..8fd32da 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -251,12 +251,14 @@ struct inode;
static inline void set_page_migratetype(struct page *page, int migratetype)
{
- set_page_private(page, migratetype);
+ VM_BUG_ON((unsigned int)migratetype >= MIGRATE_TYPES);
+ page->index = migratetype;
}
static inline int get_page_migratetype(struct page *page)
{
- return page_private(page);
+ VM_BUG_ON((unsigned int)page->index >= MIGRATE_TYPES);
+ return page->index;
}
/*
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 103ba66..32985dd 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,9 +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_page_migratetype(page, migratetype);
+ free_one_page(page_zone(page), page, order, migratetype);
local_irq_restore(flags);
}
--
1.7.9.5
--
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 related [flat|nested] 24+ messages in thread* [RFC 2/3] mm: remain migratetype in freed page
@ 2012-07-17 7:01 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 7:01 UTC (permalink / raw)
To: Kamezawa Hiroyuki, Mel Gorman
Cc: linux-mm, linux-kernel, Andrew Morton, KOSAKI Motohiro,
Hugh Dickins, Minchan Kim
Page allocator doesn't keep migratetype information to page
when the page is freed. This patch remains the information
to freed page's index field which isn't used by free/alloc
preparing so it shouldn't change any behavir except below one.
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.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/mm.h | 6 ++++--
mm/page_alloc.c | 7 ++++---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 86d61d6..8fd32da 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -251,12 +251,14 @@ struct inode;
static inline void set_page_migratetype(struct page *page, int migratetype)
{
- set_page_private(page, migratetype);
+ VM_BUG_ON((unsigned int)migratetype >= MIGRATE_TYPES);
+ page->index = migratetype;
}
static inline int get_page_migratetype(struct page *page)
{
- return page_private(page);
+ VM_BUG_ON((unsigned int)page->index >= MIGRATE_TYPES);
+ return page->index;
}
/*
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 103ba66..32985dd 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,9 +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_page_migratetype(page, migratetype);
+ free_one_page(page_zone(page), page, order, migratetype);
local_irq_restore(flags);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-17 7:01 ` Minchan Kim
@ 2012-07-17 7:01 ` Minchan Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 7:01 UTC (permalink / raw)
To: Kamezawa Hiroyuki, Mel Gorman
Cc: linux-mm, linux-kernel, Andrew Morton, KOSAKI Motohiro,
Hugh Dickins, 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));
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
I found this problem during code review so please confirm it.
Kame?
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 acf65a7..4699d1f 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -196,8 +196,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)) {
pfn += 1 << page_order(page);
+ if (get_page_migratetype(page) != MIGRATE_ISOLATE)
+ break;
+ }
else if (page_count(page) == 0 &&
get_page_migratetype(page) == MIGRATE_ISOLATE)
pfn += 1;
--
1.7.9.5
--
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 related [flat|nested] 24+ messages in thread* [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-17 7:01 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 7:01 UTC (permalink / raw)
To: Kamezawa Hiroyuki, Mel Gorman
Cc: linux-mm, linux-kernel, Andrew Morton, KOSAKI Motohiro,
Hugh Dickins, 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));
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
I found this problem during code review so please confirm it.
Kame?
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 acf65a7..4699d1f 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -196,8 +196,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)) {
pfn += 1 << page_order(page);
+ if (get_page_migratetype(page) != MIGRATE_ISOLATE)
+ break;
+ }
else if (page_count(page) == 0 &&
get_page_migratetype(page) == MIGRATE_ISOLATE)
pfn += 1;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-17 7:01 ` Minchan Kim
@ 2012-07-17 10:13 ` Bob Liu
-1 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-17 10:13 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
Hi Minchan,
On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> I found this problem during code review so please confirm it.
> Kame?
>
> 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 acf65a7..4699d1f 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -196,8 +196,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)) {
> pfn += 1 << page_order(page);
> + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> + break;
> + }
test_page_isolated() already have check
get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> else if (page_count(page) == 0 &&
> get_page_migratetype(page) == MIGRATE_ISOLATE)
> pfn += 1;
> --
> 1.7.9.5
>
--
Regards,
--Bob
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-17 10:13 ` Bob Liu
0 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-17 10:13 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
Hi Minchan,
On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> I found this problem during code review so please confirm it.
> Kame?
>
> 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 acf65a7..4699d1f 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -196,8 +196,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)) {
> pfn += 1 << page_order(page);
> + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> + break;
> + }
test_page_isolated() already have check
get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> else if (page_count(page) == 0 &&
> get_page_migratetype(page) == MIGRATE_ISOLATE)
> pfn += 1;
> --
> 1.7.9.5
>
--
Regards,
--Bob
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-17 10:13 ` Bob Liu
@ 2012-07-17 23:40 ` Minchan Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 23:40 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
Hi Bob,
On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> Hi Minchan,
>
> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> > I found this problem during code review so please confirm it.
> > Kame?
> >
> > 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 acf65a7..4699d1f 100644
> > --- a/mm/page_isolation.c
> > +++ b/mm/page_isolation.c
> > @@ -196,8 +196,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)) {
> > pfn += 1 << page_order(page);
> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> > + break;
> > + }
>
> test_page_isolated() already have check
> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>
That's why I send a patch.
As I describe in description, pageblock migration type of get_page_migratetype(page)
is inconsistent with free_list[migrationtype].
I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
--
Kind regards,
Minchan Kim
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-17 23:40 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-17 23:40 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
Hi Bob,
On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> Hi Minchan,
>
> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> > I found this problem during code review so please confirm it.
> > Kame?
> >
> > 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 acf65a7..4699d1f 100644
> > --- a/mm/page_isolation.c
> > +++ b/mm/page_isolation.c
> > @@ -196,8 +196,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)) {
> > pfn += 1 << page_order(page);
> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> > + break;
> > + }
>
> test_page_isolated() already have check
> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>
That's why I send a patch.
As I describe in description, pageblock migration type of get_page_migratetype(page)
is inconsistent with free_list[migrationtype].
I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-17 23:40 ` Minchan Kim
@ 2012-07-18 2:12 ` Bob Liu
-1 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-18 2:12 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> Hi Bob,
>
> On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> Hi Minchan,
>>
>> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >
>> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> > ---
>> > I found this problem during code review so please confirm it.
>> > Kame?
>> >
>> > 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 acf65a7..4699d1f 100644
>> > --- a/mm/page_isolation.c
>> > +++ b/mm/page_isolation.c
>> > @@ -196,8 +196,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)) {
>> > pfn += 1 << page_order(page);
>> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> > + break;
>> > + }
>>
>> test_page_isolated() already have check
>> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>>
>
> That's why I send a patch.
> As I describe in description, pageblock migration type of get_page_migratetype(page)
> is inconsistent with free_list[migrationtype].
> I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>
Sorry, I'm still not get the situation how this race happens.
set_pageblock_isolate
move_freepages_block
drain_all_pages
/* Page A could be in MIGRATE_MOVABLE
of free_list. */
I think move_freepages_block() will call list_move() to move Page A to
free_list[MIGRATE_ISOLATE], so this case can't happen?
--
Thanks,
--Bob
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-18 2:12 ` Bob Liu
0 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-18 2:12 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> Hi Bob,
>
> On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> Hi Minchan,
>>
>> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >
>> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> > ---
>> > I found this problem during code review so please confirm it.
>> > Kame?
>> >
>> > 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 acf65a7..4699d1f 100644
>> > --- a/mm/page_isolation.c
>> > +++ b/mm/page_isolation.c
>> > @@ -196,8 +196,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)) {
>> > pfn += 1 << page_order(page);
>> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> > + break;
>> > + }
>>
>> test_page_isolated() already have check
>> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>>
>
> That's why I send a patch.
> As I describe in description, pageblock migration type of get_page_migratetype(page)
> is inconsistent with free_list[migrationtype].
> I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>
Sorry, I'm still not get the situation how this race happens.
set_pageblock_isolate
move_freepages_block
drain_all_pages
/* Page A could be in MIGRATE_MOVABLE
of free_list. */
I think move_freepages_block() will call list_move() to move Page A to
free_list[MIGRATE_ISOLATE], so this case can't happen?
--
Thanks,
--Bob
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-18 2:12 ` Bob Liu
@ 2012-07-18 2:41 ` Minchan Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-18 2:41 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> > Hi Bob,
> >
> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> Hi Minchan,
> >>
> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >
> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> > ---
> >> > I found this problem during code review so please confirm it.
> >> > Kame?
> >> >
> >> > 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 acf65a7..4699d1f 100644
> >> > --- a/mm/page_isolation.c
> >> > +++ b/mm/page_isolation.c
> >> > @@ -196,8 +196,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)) {
> >> > pfn += 1 << page_order(page);
> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> > + break;
> >> > + }
> >>
> >> test_page_isolated() already have check
> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >>
> >
> > That's why I send a patch.
> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> > is inconsistent with free_list[migrationtype].
> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >
>
> Sorry, I'm still not get the situation how this race happens.
>
> set_pageblock_isolate
> move_freepages_block
> drain_all_pages
>
> /* Page A could be in MIGRATE_MOVABLE
> of free_list. */
>
> I think move_freepages_block() will call list_move() to move Page A to
> free_list[MIGRATE_ISOLATE], so this case can't happen?
move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>
> --
> Thanks,
> --Bob
>
> --
> 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
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-18 2:41 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-18 2:41 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> > Hi Bob,
> >
> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> Hi Minchan,
> >>
> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >
> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> > ---
> >> > I found this problem during code review so please confirm it.
> >> > Kame?
> >> >
> >> > 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 acf65a7..4699d1f 100644
> >> > --- a/mm/page_isolation.c
> >> > +++ b/mm/page_isolation.c
> >> > @@ -196,8 +196,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)) {
> >> > pfn += 1 << page_order(page);
> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> > + break;
> >> > + }
> >>
> >> test_page_isolated() already have check
> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >>
> >
> > That's why I send a patch.
> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> > is inconsistent with free_list[migrationtype].
> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >
>
> Sorry, I'm still not get the situation how this race happens.
>
> set_pageblock_isolate
> move_freepages_block
> drain_all_pages
>
> /* Page A could be in MIGRATE_MOVABLE
> of free_list. */
>
> I think move_freepages_block() will call list_move() to move Page A to
> free_list[MIGRATE_ISOLATE], so this case can't happen?
move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>
> --
> Thanks,
> --Bob
>
> --
> 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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-18 2:41 ` Minchan Kim
@ 2012-07-18 3:12 ` Bob Liu
-1 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-18 3:12 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
>> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
>> > Hi Bob,
>> >
>> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> >> Hi Minchan,
>> >>
>> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >> >
>> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> >> > ---
>> >> > I found this problem during code review so please confirm it.
>> >> > Kame?
>> >> >
>> >> > 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 acf65a7..4699d1f 100644
>> >> > --- a/mm/page_isolation.c
>> >> > +++ b/mm/page_isolation.c
>> >> > @@ -196,8 +196,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)) {
>> >> > pfn += 1 << page_order(page);
>> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> >> > + break;
>> >> > + }
>> >>
>> >> test_page_isolated() already have check
>> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>> >>
>> >
>> > That's why I send a patch.
>> > As I describe in description, pageblock migration type of get_page_migratetype(page)
>> > is inconsistent with free_list[migrationtype].
>> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
>> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>> >
>>
>> Sorry, I'm still not get the situation how this race happens.
>>
>> set_pageblock_isolate
>> move_freepages_block
>> drain_all_pages
>>
>> /* Page A could be in MIGRATE_MOVABLE
>> of free_list. */
>>
>> I think move_freepages_block() will call list_move() to move Page A to
>> free_list[MIGRATE_ISOLATE], so this case can't happen?
>
> move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>
Got it, then why not just drain pcp pages before move_freepages_block() ?
And I didn't see the effect by adding the check if
(get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
Since set_pageblock_isolate() have been called by CPU A, this check
will always false which cause CPU A still consider Page A isolated,
then PAGE A still can be allocated by CPU B from pcp.
--
Regards,
--Bob
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-18 3:12 ` Bob Liu
0 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-18 3:12 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
>> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
>> > Hi Bob,
>> >
>> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> >> Hi Minchan,
>> >>
>> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >> >
>> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> >> > ---
>> >> > I found this problem during code review so please confirm it.
>> >> > Kame?
>> >> >
>> >> > 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 acf65a7..4699d1f 100644
>> >> > --- a/mm/page_isolation.c
>> >> > +++ b/mm/page_isolation.c
>> >> > @@ -196,8 +196,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)) {
>> >> > pfn += 1 << page_order(page);
>> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> >> > + break;
>> >> > + }
>> >>
>> >> test_page_isolated() already have check
>> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>> >>
>> >
>> > That's why I send a patch.
>> > As I describe in description, pageblock migration type of get_page_migratetype(page)
>> > is inconsistent with free_list[migrationtype].
>> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
>> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>> >
>>
>> Sorry, I'm still not get the situation how this race happens.
>>
>> set_pageblock_isolate
>> move_freepages_block
>> drain_all_pages
>>
>> /* Page A could be in MIGRATE_MOVABLE
>> of free_list. */
>>
>> I think move_freepages_block() will call list_move() to move Page A to
>> free_list[MIGRATE_ISOLATE], so this case can't happen?
>
> move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>
Got it, then why not just drain pcp pages before move_freepages_block() ?
And I didn't see the effect by adding the check if
(get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
Since set_pageblock_isolate() have been called by CPU A, this check
will always false which cause CPU A still consider Page A isolated,
then PAGE A still can be allocated by CPU B from pcp.
--
Regards,
--Bob
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-18 3:12 ` Bob Liu
@ 2012-07-18 3:56 ` Minchan Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-18 3:56 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> > Hi Bob,
> >> >
> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> >> Hi Minchan,
> >> >>
> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >> >
> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> >> > ---
> >> >> > I found this problem during code review so please confirm it.
> >> >> > Kame?
> >> >> >
> >> >> > 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 acf65a7..4699d1f 100644
> >> >> > --- a/mm/page_isolation.c
> >> >> > +++ b/mm/page_isolation.c
> >> >> > @@ -196,8 +196,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)) {
> >> >> > pfn += 1 << page_order(page);
> >> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> >> > + break;
> >> >> > + }
> >> >>
> >> >> test_page_isolated() already have check
> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >> >>
> >> >
> >> > That's why I send a patch.
> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> >> > is inconsistent with free_list[migrationtype].
> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >> >
> >>
> >> Sorry, I'm still not get the situation how this race happens.
> >>
> >> set_pageblock_isolate
> >> move_freepages_block
> >> drain_all_pages
> >>
> >> /* Page A could be in MIGRATE_MOVABLE
> >> of free_list. */
> >>
> >> I think move_freepages_block() will call list_move() to move Page A to
> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
> >
> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
> >
>
> Got it, then why not just drain pcp pages before move_freepages_block() ?
CPU A CPU B
drain_all_pages
lock(zone->lock);
free_hot_cold_page
MIGRATE_MOVABLE = get_pageblock_migratetype(page);
list_add(&page->lru, &pcp->lists[migratetype])
set_pageblock_isolate
move_free_pages_block
unlock(zone->lock);
We can't make it atomic.
>
> And I didn't see the effect by adding the check if
> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
> Since set_pageblock_isolate() have been called by CPU A, this check
> will always false which cause CPU A still consider Page A isolated,
> then PAGE A still can be allocated by CPU B from pcp.
Please don't confuse get_page_migratetype and get_pageblock_migratetype.
get_page_migratetype returns migratetype inforamtion of *page* which is
in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
>
> --
> Regards,
> --Bob
>
> --
> 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
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-18 3:56 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-18 3:56 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> > Hi Bob,
> >> >
> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> >> Hi Minchan,
> >> >>
> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >> >
> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> >> > ---
> >> >> > I found this problem during code review so please confirm it.
> >> >> > Kame?
> >> >> >
> >> >> > 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 acf65a7..4699d1f 100644
> >> >> > --- a/mm/page_isolation.c
> >> >> > +++ b/mm/page_isolation.c
> >> >> > @@ -196,8 +196,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)) {
> >> >> > pfn += 1 << page_order(page);
> >> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> >> > + break;
> >> >> > + }
> >> >>
> >> >> test_page_isolated() already have check
> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >> >>
> >> >
> >> > That's why I send a patch.
> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> >> > is inconsistent with free_list[migrationtype].
> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >> >
> >>
> >> Sorry, I'm still not get the situation how this race happens.
> >>
> >> set_pageblock_isolate
> >> move_freepages_block
> >> drain_all_pages
> >>
> >> /* Page A could be in MIGRATE_MOVABLE
> >> of free_list. */
> >>
> >> I think move_freepages_block() will call list_move() to move Page A to
> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
> >
> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
> >
>
> Got it, then why not just drain pcp pages before move_freepages_block() ?
CPU A CPU B
drain_all_pages
lock(zone->lock);
free_hot_cold_page
MIGRATE_MOVABLE = get_pageblock_migratetype(page);
list_add(&page->lru, &pcp->lists[migratetype])
set_pageblock_isolate
move_free_pages_block
unlock(zone->lock);
We can't make it atomic.
>
> And I didn't see the effect by adding the check if
> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
> Since set_pageblock_isolate() have been called by CPU A, this check
> will always false which cause CPU A still consider Page A isolated,
> then PAGE A still can be allocated by CPU B from pcp.
Please don't confuse get_page_migratetype and get_pageblock_migratetype.
get_page_migratetype returns migratetype inforamtion of *page* which is
in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
>
> --
> Regards,
> --Bob
>
> --
> 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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-18 3:56 ` Minchan Kim
@ 2012-07-18 5:29 ` Bob Liu
-1 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-18 5:29 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 11:56 AM, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
>> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
>> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
>> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
>> >> > Hi Bob,
>> >> >
>> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> >> >> Hi Minchan,
>> >> >>
>> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >> >> >
>> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> >> >> > ---
>> >> >> > I found this problem during code review so please confirm it.
>> >> >> > Kame?
>> >> >> >
>> >> >> > 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 acf65a7..4699d1f 100644
>> >> >> > --- a/mm/page_isolation.c
>> >> >> > +++ b/mm/page_isolation.c
>> >> >> > @@ -196,8 +196,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)) {
>> >> >> > pfn += 1 << page_order(page);
>> >> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> >> >> > + break;
>> >> >> > + }
>> >> >>
>> >> >> test_page_isolated() already have check
>> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>> >> >>
>> >> >
>> >> > That's why I send a patch.
>> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
>> >> > is inconsistent with free_list[migrationtype].
>> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
>> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>> >> >
>> >>
>> >> Sorry, I'm still not get the situation how this race happens.
>> >>
>> >> set_pageblock_isolate
>> >> move_freepages_block
>> >> drain_all_pages
>> >>
>> >> /* Page A could be in MIGRATE_MOVABLE
>> >> of free_list. */
>> >>
>> >> I think move_freepages_block() will call list_move() to move Page A to
>> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
>> >
>> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>> >
>>
>> Got it, then why not just drain pcp pages before move_freepages_block() ?
>
> CPU A CPU B
>
> drain_all_pages
> lock(zone->lock);
> free_hot_cold_page
> MIGRATE_MOVABLE = get_pageblock_migratetype(page);
> list_add(&page->lru, &pcp->lists[migratetype])
> set_pageblock_isolate
> move_free_pages_block
> unlock(zone->lock);
>
> We can't make it atomic.
>
>>
>> And I didn't see the effect by adding the check if
>> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
>> Since set_pageblock_isolate() have been called by CPU A, this check
>> will always false which cause CPU A still consider Page A isolated,
>> then PAGE A still can be allocated by CPU B from pcp.
>
> Please don't confuse get_page_migratetype and get_pageblock_migratetype.
> get_page_migratetype returns migratetype inforamtion of *page* which is
> in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
>
Oh, yes i mixed them up.
Last question, it's better to break only before add pfn.
like:
- if (PageBuddy(page))
+ if (PageBuddy(page)) {
+ if (get_page_migratetype(page) != MIGRATE_ISOLATE)
+ break;
pfn += 1 << page_order(page);
+ }
--
Regards,
--Bob
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-18 5:29 ` Bob Liu
0 siblings, 0 replies; 24+ messages in thread
From: Bob Liu @ 2012-07-18 5:29 UTC (permalink / raw)
To: Minchan Kim
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 11:56 AM, Minchan Kim <minchan@kernel.org> wrote:
> On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
>> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
>> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
>> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
>> >> > Hi Bob,
>> >> >
>> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
>> >> >> Hi Minchan,
>> >> >>
>> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
>> >> >> >
>> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> >> >> > ---
>> >> >> > I found this problem during code review so please confirm it.
>> >> >> > Kame?
>> >> >> >
>> >> >> > 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 acf65a7..4699d1f 100644
>> >> >> > --- a/mm/page_isolation.c
>> >> >> > +++ b/mm/page_isolation.c
>> >> >> > @@ -196,8 +196,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)) {
>> >> >> > pfn += 1 << page_order(page);
>> >> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
>> >> >> > + break;
>> >> >> > + }
>> >> >>
>> >> >> test_page_isolated() already have check
>> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
>> >> >>
>> >> >
>> >> > That's why I send a patch.
>> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
>> >> > is inconsistent with free_list[migrationtype].
>> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
>> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
>> >> >
>> >>
>> >> Sorry, I'm still not get the situation how this race happens.
>> >>
>> >> set_pageblock_isolate
>> >> move_freepages_block
>> >> drain_all_pages
>> >>
>> >> /* Page A could be in MIGRATE_MOVABLE
>> >> of free_list. */
>> >>
>> >> I think move_freepages_block() will call list_move() to move Page A to
>> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
>> >
>> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
>> >
>>
>> Got it, then why not just drain pcp pages before move_freepages_block() ?
>
> CPU A CPU B
>
> drain_all_pages
> lock(zone->lock);
> free_hot_cold_page
> MIGRATE_MOVABLE = get_pageblock_migratetype(page);
> list_add(&page->lru, &pcp->lists[migratetype])
> set_pageblock_isolate
> move_free_pages_block
> unlock(zone->lock);
>
> We can't make it atomic.
>
>>
>> And I didn't see the effect by adding the check if
>> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
>> Since set_pageblock_isolate() have been called by CPU A, this check
>> will always false which cause CPU A still consider Page A isolated,
>> then PAGE A still can be allocated by CPU B from pcp.
>
> Please don't confuse get_page_migratetype and get_pageblock_migratetype.
> get_page_migratetype returns migratetype inforamtion of *page* which is
> in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
>
Oh, yes i mixed them up.
Last question, it's better to break only before add pfn.
like:
- if (PageBuddy(page))
+ if (PageBuddy(page)) {
+ if (get_page_migratetype(page) != MIGRATE_ISOLATE)
+ break;
pfn += 1 << page_order(page);
+ }
--
Regards,
--Bob
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
2012-07-18 5:29 ` Bob Liu
@ 2012-07-18 5:58 ` Minchan Kim
-1 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-18 5:58 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 01:29:47PM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 11:56 AM, Minchan Kim <minchan@kernel.org> wrote:
> > On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
> >> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> >> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> >> > Hi Bob,
> >> >> >
> >> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> >> >> Hi Minchan,
> >> >> >>
> >> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >> >> >
> >> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> >> >> > ---
> >> >> >> > I found this problem during code review so please confirm it.
> >> >> >> > Kame?
> >> >> >> >
> >> >> >> > 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 acf65a7..4699d1f 100644
> >> >> >> > --- a/mm/page_isolation.c
> >> >> >> > +++ b/mm/page_isolation.c
> >> >> >> > @@ -196,8 +196,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)) {
> >> >> >> > pfn += 1 << page_order(page);
> >> >> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> >> >> > + break;
> >> >> >> > + }
> >> >> >>
> >> >> >> test_page_isolated() already have check
> >> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >> >> >>
> >> >> >
> >> >> > That's why I send a patch.
> >> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> >> >> > is inconsistent with free_list[migrationtype].
> >> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> >> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >> >> >
> >> >>
> >> >> Sorry, I'm still not get the situation how this race happens.
> >> >>
> >> >> set_pageblock_isolate
> >> >> move_freepages_block
> >> >> drain_all_pages
> >> >>
> >> >> /* Page A could be in MIGRATE_MOVABLE
> >> >> of free_list. */
> >> >>
> >> >> I think move_freepages_block() will call list_move() to move Page A to
> >> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
> >> >
> >> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
> >> >
> >>
> >> Got it, then why not just drain pcp pages before move_freepages_block() ?
> >
> > CPU A CPU B
> >
> > drain_all_pages
> > lock(zone->lock);
> > free_hot_cold_page
> > MIGRATE_MOVABLE = get_pageblock_migratetype(page);
> > list_add(&page->lru, &pcp->lists[migratetype])
> > set_pageblock_isolate
> > move_free_pages_block
> > unlock(zone->lock);
> >
> > We can't make it atomic.
> >
> >>
> >> And I didn't see the effect by adding the check if
> >> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
> >> Since set_pageblock_isolate() have been called by CPU A, this check
> >> will always false which cause CPU A still consider Page A isolated,
> >> then PAGE A still can be allocated by CPU B from pcp.
> >
> > Please don't confuse get_page_migratetype and get_pageblock_migratetype.
> > get_page_migratetype returns migratetype inforamtion of *page* which is
> > in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
> >
>
> Oh, yes i mixed them up.
>
> Last question, it's better to break only before add pfn.
> like:
>
> - if (PageBuddy(page))
> + if (PageBuddy(page)) {
> + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> + break;
> pfn += 1 << page_order(page);
> + }
Sure thing!
--
Kind regards,
Minchan Kim
--
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] 24+ messages in thread* Re: [RFC 3/3] memory-hotplug: bug fix race between isolation and allocation
@ 2012-07-18 5:58 ` Minchan Kim
0 siblings, 0 replies; 24+ messages in thread
From: Minchan Kim @ 2012-07-18 5:58 UTC (permalink / raw)
To: Bob Liu
Cc: Kamezawa Hiroyuki, Mel Gorman, linux-mm, linux-kernel,
Andrew Morton, KOSAKI Motohiro, Hugh Dickins
On Wed, Jul 18, 2012 at 01:29:47PM +0800, Bob Liu wrote:
> On Wed, Jul 18, 2012 at 11:56 AM, Minchan Kim <minchan@kernel.org> wrote:
> > On Wed, Jul 18, 2012 at 11:12:27AM +0800, Bob Liu wrote:
> >> On Wed, Jul 18, 2012 at 10:41 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> > On Wed, Jul 18, 2012 at 10:12:57AM +0800, Bob Liu wrote:
> >> >> On Wed, Jul 18, 2012 at 7:40 AM, Minchan Kim <minchan@kernel.org> wrote:
> >> >> > Hi Bob,
> >> >> >
> >> >> > On Tue, Jul 17, 2012 at 06:13:17PM +0800, Bob Liu wrote:
> >> >> >> Hi Minchan,
> >> >> >>
> >> >> >> On Tue, Jul 17, 2012 at 3:01 PM, Minchan Kim <minchan@kernel.org> 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));
> >> >> >> >
> >> >> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> >> >> > ---
> >> >> >> > I found this problem during code review so please confirm it.
> >> >> >> > Kame?
> >> >> >> >
> >> >> >> > 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 acf65a7..4699d1f 100644
> >> >> >> > --- a/mm/page_isolation.c
> >> >> >> > +++ b/mm/page_isolation.c
> >> >> >> > @@ -196,8 +196,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)) {
> >> >> >> > pfn += 1 << page_order(page);
> >> >> >> > + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> >> >> >> > + break;
> >> >> >> > + }
> >> >> >>
> >> >> >> test_page_isolated() already have check
> >> >> >> get_pageblock_migratetype(page) != MIGRATE_ISOLATE.
> >> >> >>
> >> >> >
> >> >> > That's why I send a patch.
> >> >> > As I describe in description, pageblock migration type of get_page_migratetype(page)
> >> >> > is inconsistent with free_list[migrationtype].
> >> >> > I mean get_pageblock_migratetype(page) will return MIGRATE_ISOLATE but the page would be
> >> >> > in free_list[MIGRATE_MOVABLE] so it could be allocated for someone if that race happens.
> >> >> >
> >> >>
> >> >> Sorry, I'm still not get the situation how this race happens.
> >> >>
> >> >> set_pageblock_isolate
> >> >> move_freepages_block
> >> >> drain_all_pages
> >> >>
> >> >> /* Page A could be in MIGRATE_MOVABLE
> >> >> of free_list. */
> >> >>
> >> >> I think move_freepages_block() will call list_move() to move Page A to
> >> >> free_list[MIGRATE_ISOLATE], so this case can't happen?
> >> >
> >> > move_freepages_block handles only pages in free_list but Page A is on pcp, not free_list.
> >> >
> >>
> >> Got it, then why not just drain pcp pages before move_freepages_block() ?
> >
> > CPU A CPU B
> >
> > drain_all_pages
> > lock(zone->lock);
> > free_hot_cold_page
> > MIGRATE_MOVABLE = get_pageblock_migratetype(page);
> > list_add(&page->lru, &pcp->lists[migratetype])
> > set_pageblock_isolate
> > move_free_pages_block
> > unlock(zone->lock);
> >
> > We can't make it atomic.
> >
> >>
> >> And I didn't see the effect by adding the check if
> >> (get_page_migratetype(page) != MIGRATE_ISOLATE) for this race.
> >> Since set_pageblock_isolate() have been called by CPU A, this check
> >> will always false which cause CPU A still consider Page A isolated,
> >> then PAGE A still can be allocated by CPU B from pcp.
> >
> > Please don't confuse get_page_migratetype and get_pageblock_migratetype.
> > get_page_migratetype returns migratetype inforamtion of *page* which is
> > in free_list while get_pageblock_migratetype returns *pageblock*'s migratetype.
> >
>
> Oh, yes i mixed them up.
>
> Last question, it's better to break only before add pfn.
> like:
>
> - if (PageBuddy(page))
> + if (PageBuddy(page)) {
> + if (get_page_migratetype(page) != MIGRATE_ISOLATE)
> + break;
> pfn += 1 << page_order(page);
> + }
Sure thing!
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 24+ messages in thread