* [PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone @ 2010-05-17 8:20 Haicheng Li 2010-05-17 16:09 ` Christoph Lameter 0 siblings, 1 reply; 7+ messages in thread From: Haicheng Li @ 2010-05-17 8:20 UTC (permalink / raw) To: linux-mm@kvack.org, akpm@linux-foundation.org Cc: linux-kernel@vger.kernel.org, Wu, Fengguang, Andi Kleen, Mel Gorman, Christoph Lameter, Tejun Heo Make "zone->present_pages gets increased" and "building zonelist" an atomic operation to prevent possible race. It is merely a theoretical race: after new zone gets populated, its pages might be allocated by others before itself building zonelist. Besides, atomic operation ensures alloc_percpu() will never fail since there is a new fresh memory block added. Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> Reviewed-by: Andi Kleen <andi.kleen@intel.com> --- include/linux/memory_hotplug.h | 8 ++++++++ mm/memory_hotplug.c | 15 +++++++++------ mm/page_alloc.c | 20 ++++++++++++++++---- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h index 35b07b7..42b1416 100644 --- a/include/linux/memory_hotplug.h +++ b/include/linux/memory_hotplug.h @@ -157,6 +157,14 @@ extern void register_page_bootmem_info_node(struct pglist_data *pgdat); extern void put_page_bootmem(struct page *page); #endif +/* online_pages() will pass such zone info to build_all_zonelists() + * when it needs to initialize a new zone. + */ +struct zone_online_info { + struct zone *zone; + unsigned long onlined_pages; +}; + #else /* ! CONFIG_MEMORY_HOTPLUG */ /* * Stub functions for when hotplug is off diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index b564b6a..06738b2 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -389,6 +389,7 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) int nid; int ret; struct memory_notify arg; + struct zone_online_info zone_online_info; /* * mutex to protect zone->pageset when it's still shared * in onlined_pages() @@ -434,13 +435,15 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) return ret; } - zone->present_pages += onlined_pages; - zone->zone_pgdat->node_present_pages += onlined_pages; - if (need_zonelists_rebuild) - build_all_zonelists(zone); - else + if (need_zonelists_rebuild) { + zone_online_info.zone = zone; + zone_online_info.onlined_pages = onlined_pages; + build_all_zonelists(&zone_online_info); + } else { + zone->present_pages += onlined_pages; + zone->zone_pgdat->node_present_pages += onlined_pages; zone_pcp_update(zone); - + } mutex_unlock(&zone_pageset_mutex); setup_per_zone_wmarks(); calculate_zone_inactive_ratio(zone); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 72c1211..0729a82 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2783,6 +2783,20 @@ static __init_refok int __build_all_zonelists(void *data) { int nid; int cpu; +#ifdef CONFIG_MEMORY_HOTPLUG + struct zone_online_info *new = (struct zone_online_info *)data; + + /* + * Populate the new zone before build zonelists, which could + * happen only when onlining a new node after system is booted. + */ + if (new) { + /* We are expecting a new memory block here. */ + WARN_ON(!new->onlined_pages); + new->zone->present_pages += new->onlined_pages; + new->zone->zone_pgdat->node_present_pages += new->onlined_pages; + } +#endif #ifdef CONFIG_NUMA memset(node_load, 0, sizeof(node_load)); @@ -2796,10 +2810,8 @@ static __init_refok int __build_all_zonelists(void *data) #ifdef CONFIG_MEMORY_HOTPLUG /* Setup real pagesets for the new zone */ - if (data) { - struct zone *zone = data; - setup_zone_pageset(zone); - } + if (new) + setup_zone_pageset(new->zone); #endif /* -- 1.6.0.rc1 -- 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] 7+ messages in thread
* Re: [PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone 2010-05-17 8:20 [PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone Haicheng Li @ 2010-05-17 16:09 ` Christoph Lameter 2010-05-18 2:19 ` Wu Fengguang 0 siblings, 1 reply; 7+ messages in thread From: Christoph Lameter @ 2010-05-17 16:09 UTC (permalink / raw) To: Haicheng Li Cc: linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Wu, Fengguang, Andi Kleen, Mel Gorman, Tejun Heo On Mon, 17 May 2010, Haicheng Li wrote: > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 72c1211..0729a82 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2783,6 +2783,20 @@ static __init_refok int __build_all_zonelists(void > *data) > { > int nid; > int cpu; > +#ifdef CONFIG_MEMORY_HOTPLUG > + struct zone_online_info *new = (struct zone_online_info *)data; > + > + /* > + * Populate the new zone before build zonelists, which could > + * happen only when onlining a new node after system is booted. > + */ > + if (new) { > + /* We are expecting a new memory block here. */ > + WARN_ON(!new->onlined_pages); > + new->zone->present_pages += new->onlined_pages; > + new->zone->zone_pgdat->node_present_pages += > new->onlined_pages; > + } > +#endif Building a zonelist now has the potential side effect of changes to the size of the zone? Can we have a global mutex that protects against size modification of zonelists instead? And it could also serialize the pageset setup? -- 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] 7+ messages in thread
* Re: [PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone 2010-05-17 16:09 ` Christoph Lameter @ 2010-05-18 2:19 ` Wu Fengguang 2010-05-18 9:02 ` [RESEND][PATCH " Haicheng Li 0 siblings, 1 reply; 7+ messages in thread From: Wu Fengguang @ 2010-05-18 2:19 UTC (permalink / raw) To: Christoph Lameter Cc: Haicheng Li, linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Andi Kleen, Mel Gorman, Tejun Heo On Tue, May 18, 2010 at 12:09:31AM +0800, Christoph Lameter wrote: > On Mon, 17 May 2010, Haicheng Li wrote: > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 72c1211..0729a82 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -2783,6 +2783,20 @@ static __init_refok int __build_all_zonelists(void > > *data) > > { > > int nid; > > int cpu; > > +#ifdef CONFIG_MEMORY_HOTPLUG > > + struct zone_online_info *new = (struct zone_online_info *)data; > > + > > + /* > > + * Populate the new zone before build zonelists, which could > > + * happen only when onlining a new node after system is booted. > > + */ > > + if (new) { > > + /* We are expecting a new memory block here. */ > > + WARN_ON(!new->onlined_pages); > > + new->zone->present_pages += new->onlined_pages; > > + new->zone->zone_pgdat->node_present_pages += > > new->onlined_pages; > > + } > > +#endif > > > Building a zonelist now has the potential side effect of changes to the > size of the zone? Yeah, this sounds a bit hacky. > Can we have a global mutex that protects against size modification of > zonelists instead? And it could also serialize the pageset setup? Good suggestion. We could make zone_pageset_mutex a global mutex and take it in all the functions that call build_all_zonelists() -- currently only online_pages() and numa_zonelist_order_handler(). This can equally fix the possible race: CPU0 CPU1 CPU2 (1) zone->present_pages += online_pages; (2) build_all_zonelists(); (3) alloc_page(); (4) free_page(); (5) build_all_zonelists(); (6) __build_all_zonelists(); (7) zone->pageset = alloc_percpu(); In step (3,4), zone->pageset still points to boot_pageset, so bad things may happen if 2+ nodes are in this state. Even if only 1 node is accessing the boot_pageset, (3) may still consume too much memory to fail the memory allocations in step (7). Thanks, Fengguang -- 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] 7+ messages in thread
* [RESEND][PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone 2010-05-18 2:19 ` Wu Fengguang @ 2010-05-18 9:02 ` Haicheng Li 2010-05-18 13:55 ` Christoph Lameter 0 siblings, 1 reply; 7+ messages in thread From: Haicheng Li @ 2010-05-18 9:02 UTC (permalink / raw) To: Wu Fengguang Cc: Christoph Lameter, linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Andi Kleen, Mel Gorman, Tejun Heo Wu Fengguang wrote: > On Tue, May 18, 2010 at 12:09:31AM +0800, Christoph Lameter wrote: >> Building a zonelist now has the potential side effect of changes to the >> size of the zone? > > Yeah, this sounds a bit hacky. > >> Can we have a global mutex that protects against size modification of >> zonelists instead? And it could also serialize the pageset setup? > > Good suggestion. We could make zone_pageset_mutex a global mutex and > take it in all the functions that call build_all_zonelists() -- > currently only online_pages() and numa_zonelist_order_handler(). Yes, if we don't mind adding a global mutex, it's another way to fix the race. > This can equally fix the possible race: > > CPU0 CPU1 CPU2 > (1) zone->present_pages += online_pages; > (2) build_all_zonelists(); > (3) alloc_page(); > (4) free_page(); > (5) build_all_zonelists(); > (6) __build_all_zonelists(); > (7) zone->pageset = alloc_percpu(); > > In step (3,4), zone->pageset still points to boot_pageset, so bad > things may happen if 2+ nodes are in this state. Even if only 1 node > is accessing the boot_pageset, (3) may still consume too much memory > to fail the memory allocations in step (7). Fengguang, this is a nice description of the possible race, I'd include it into the changelog of this patch. So how about the revised patch below? --- From 0b1e776dc6c135b1d0c9a0bfb35daa9e9cfb046d Mon Sep 17 00:00:00 2001 From: Haicheng Li <haicheng.li@linux.intel.com> Date: Tue, 18 May 2010 15:53:28 +0800 Subject: [PATCH] mem-hotplug: fix potential setup_zone_pageset race Add global mutex zonelists_pageset_mutex to fix the possible race: CPU0 CPU1 CPU2 (1) zone->present_pages += online_pages; (2) build_all_zonelists(); (3) alloc_page(); (4) free_page(); (5) build_all_zonelists(); (6) __build_all_zonelists(); (7) zone->pageset = alloc_percpu(); In step (3,4), zone->pageset still points to boot_pageset, so bad things may happen if 2+ nodes are in this state. Even if only 1 node is accessing the boot_pageset, (3) may still consume too much memory to fail the memory allocations in step (7). Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> Reviewed-by: Andi Kleen <andi.kleen@intel.com> --- include/linux/mmzone.h | 1 + mm/memory_hotplug.c | 11 +++-------- mm/page_alloc.c | 15 ++++++++++++++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index dbbcd50..5103747 100644 --- a/include/linux/mmzone.h +++ b/include/linux/mmzone.h @@ -641,6 +641,7 @@ typedef struct pglist_data { #include <linux/memory_hotplug.h> +extern struct mutex zonelists_pageset_mutex; void get_zone_counts(unsigned long *active, unsigned long *inactive, unsigned long *free); void build_all_zonelists(void *data); diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index b564b6a..b32f3eb 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -389,11 +389,6 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) int nid; int ret; struct memory_notify arg; - /* - * mutex to protect zone->pageset when it's still shared - * in onlined_pages() - */ - static DEFINE_MUTEX(zone_pageset_mutex); arg.start_pfn = pfn; arg.nr_pages = nr_pages; @@ -420,14 +415,14 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) * This means the page allocator ignores this zone. * So, zonelist must be updated after online. */ - mutex_lock(&zone_pageset_mutex); + mutex_lock(&zonelists_pageset_mutex); if (!populated_zone(zone)) need_zonelists_rebuild = 1; ret = walk_system_ram_range(pfn, nr_pages, &onlined_pages, online_pages_range); if (ret) { - mutex_unlock(&zone_pageset_mutex); + mutex_unlock(&zonelists_pageset_mutex); printk(KERN_DEBUG "online_pages %lx at %lx failed\n", nr_pages, pfn); memory_notify(MEM_CANCEL_ONLINE, &arg); @@ -441,7 +436,7 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) else zone_pcp_update(zone); - mutex_unlock(&zone_pageset_mutex); + mutex_unlock(&zonelists_pageset_mutex); setup_per_zone_wmarks(); calculate_zone_inactive_ratio(zone); if (onlined_pages) { diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 72c1211..602c4b7 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2434,8 +2434,11 @@ int numa_zonelist_order_handler(ctl_table *table, int write, strncpy((char*)table->data, saved_string, NUMA_ZONELIST_ORDER_LEN); user_zonelist_order = oldval; - } else if (oldval != user_zonelist_order) + } else if (oldval != user_zonelist_order) { + mutex_lock(&zonelists_pageset_mutex); build_all_zonelists(NULL); + mutex_unlock(&zonelists_pageset_mutex); + } } out: mutex_unlock(&zl_order_mutex); @@ -2778,6 +2781,12 @@ static void setup_pageset(struct per_cpu_pageset *p, unsigned long batch); static DEFINE_PER_CPU(struct per_cpu_pageset, boot_pageset); static void setup_zone_pageset(struct zone *zone); +/* + * Global mutex to protect size modification of zonelists + * as well as to serialize pageset setup for a new populated zone. + */ +DEFINE_MUTEX(zonelists_pageset_mutex); + /* return values int ....just for stop_machine() */ static __init_refok int __build_all_zonelists(void *data) { @@ -2821,6 +2830,10 @@ static __init_refok int __build_all_zonelists(void *data) return 0; } +/* + * Called with zonelists_pageset_mutex held always + * unless system_state == SYSTEM_BOOTING. + */ void build_all_zonelists(void *data) { set_zonelist_order(); -- 1.5.6.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 related [flat|nested] 7+ messages in thread
* Re: [RESEND][PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone 2010-05-18 9:02 ` [RESEND][PATCH " Haicheng Li @ 2010-05-18 13:55 ` Christoph Lameter 2010-05-19 3:48 ` [RESEND -v2][PATCH " Haicheng Li 0 siblings, 1 reply; 7+ messages in thread From: Christoph Lameter @ 2010-05-18 13:55 UTC (permalink / raw) To: Haicheng Li Cc: Wu Fengguang, linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Andi Kleen, Mel Gorman, Tejun Heo On Tue, 18 May 2010, Haicheng Li wrote: > +extern struct mutex zonelists_pageset_mutex; The mutext is used for multiple serializations having to do with zones. "pageset" suggests its only for pagesets. So zones_mutex? or zonelists_mutex? -- 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] 7+ messages in thread
* [RESEND -v2][PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone 2010-05-18 13:55 ` Christoph Lameter @ 2010-05-19 3:48 ` Haicheng Li 2010-05-19 15:21 ` Christoph Lameter 0 siblings, 1 reply; 7+ messages in thread From: Haicheng Li @ 2010-05-19 3:48 UTC (permalink / raw) To: Christoph Lameter Cc: Wu Fengguang, linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Andi Kleen, Mel Gorman, Tejun Heo Christoph Lameter wrote: > On Tue, 18 May 2010, Haicheng Li wrote: > >> +extern struct mutex zonelists_pageset_mutex; > > The mutext is used for multiple serializations having to do with zones. > "pageset" suggests its only for pagesets. hmm yes, "pageset" sounds a little bit confusing. > So > > zones_mutex? > > or > > zonelists_mutex? I prefer zonelists_mutex. Christoph, how about below patch? Thanks. --- From 3ccfd04d6ae9127bf0f5472db0b266e7b3f158bd Mon Sep 17 00:00:00 2001 From: Haicheng Li <haicheng.li@linux.intel.com> Date: Wed, 19 May 2010 11:07:21 +0800 Subject: [PATCH] mem-hotplug: fix potential race while building zonelist for new populated zone Add global mutex zonelists_mutex to fix the possible race: CPU0 CPU1 CPU2 (1) zone->present_pages += online_pages; (2) build_all_zonelists(); (3) alloc_page(); (4) free_page(); (5) build_all_zonelists(); (6) __build_all_zonelists(); (7) zone->pageset = alloc_percpu(); In step (3,4), zone->pageset still points to boot_pageset, so bad things may happen if 2+ nodes are in this state. Even if only 1 node is accessing the boot_pageset, (3) may still consume too much memory to fail the memory allocations in step (7). Besides, atomic operation ensures alloc_percpu() in step (7) will never fail since there is a new fresh memory block added in step(6). Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> Reviewed-by: Andi Kleen <andi.kleen@intel.com> --- include/linux/mmzone.h | 1 + mm/memory_hotplug.c | 11 +++-------- mm/page_alloc.c | 15 ++++++++++++++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index dbbcd50..4d87baa 100644 --- a/include/linux/mmzone.h +++ b/include/linux/mmzone.h @@ -641,6 +641,7 @@ typedef struct pglist_data { #include <linux/memory_hotplug.h> +extern struct mutex zonelists_mutex; void get_zone_counts(unsigned long *active, unsigned long *inactive, unsigned long *free); void build_all_zonelists(void *data); diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index b564b6a..bc4a942 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -389,11 +389,6 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) int nid; int ret; struct memory_notify arg; - /* - * mutex to protect zone->pageset when it's still shared - * in onlined_pages() - */ - static DEFINE_MUTEX(zone_pageset_mutex); arg.start_pfn = pfn; arg.nr_pages = nr_pages; @@ -420,14 +415,14 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) * This means the page allocator ignores this zone. * So, zonelist must be updated after online. */ - mutex_lock(&zone_pageset_mutex); + mutex_lock(&zonelists_mutex); if (!populated_zone(zone)) need_zonelists_rebuild = 1; ret = walk_system_ram_range(pfn, nr_pages, &onlined_pages, online_pages_range); if (ret) { - mutex_unlock(&zone_pageset_mutex); + mutex_unlock(&zonelists_mutex); printk(KERN_DEBUG "online_pages %lx at %lx failed\n", nr_pages, pfn); memory_notify(MEM_CANCEL_ONLINE, &arg); @@ -441,7 +436,7 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) else zone_pcp_update(zone); - mutex_unlock(&zone_pageset_mutex); + mutex_unlock(&zonelists_mutex); setup_per_zone_wmarks(); calculate_zone_inactive_ratio(zone); if (onlined_pages) { diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 72c1211..c8e6146 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2434,8 +2434,11 @@ int numa_zonelist_order_handler(ctl_table *table, int write, strncpy((char*)table->data, saved_string, NUMA_ZONELIST_ORDER_LEN); user_zonelist_order = oldval; - } else if (oldval != user_zonelist_order) + } else if (oldval != user_zonelist_order) { + mutex_lock(&zonelists_mutex); build_all_zonelists(NULL); + mutex_unlock(&zonelists_mutex); + } } out: mutex_unlock(&zl_order_mutex); @@ -2778,6 +2781,12 @@ static void setup_pageset(struct per_cpu_pageset *p, unsigned long batch); static DEFINE_PER_CPU(struct per_cpu_pageset, boot_pageset); static void setup_zone_pageset(struct zone *zone); +/* + * Global mutex to protect against size modification of zonelists + * as well as to serialize pageset setup for the new populated zone. + */ +DEFINE_MUTEX(zonelists_mutex); + /* return values int ....just for stop_machine() */ static __init_refok int __build_all_zonelists(void *data) { @@ -2821,6 +2830,10 @@ static __init_refok int __build_all_zonelists(void *data) return 0; } +/* + * Called with zonelists_mutex held always + * unless system_state == SYSTEM_BOOTING. + */ void build_all_zonelists(void *data) { set_zonelist_order(); -- 1.5.6.1 -haicheng -- 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] 7+ messages in thread
* Re: [RESEND -v2][PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone 2010-05-19 3:48 ` [RESEND -v2][PATCH " Haicheng Li @ 2010-05-19 15:21 ` Christoph Lameter 0 siblings, 0 replies; 7+ messages in thread From: Christoph Lameter @ 2010-05-19 15:21 UTC (permalink / raw) To: Haicheng Li Cc: Wu Fengguang, linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Andi Kleen, Mel Gorman, Tejun Heo Looks good on its own.... On Wed, 19 May 2010, Haicheng Li wrote: > Christoph Lameter wrote: > > On Tue, 18 May 2010, Haicheng Li wrote: > > > > > +extern struct mutex zonelists_pageset_mutex; > > > > The mutext is used for multiple serializations having to do with zones. > > "pageset" suggests its only for pagesets. > > hmm yes, "pageset" sounds a little bit confusing. > > > So > > > > zones_mutex? > > > > or > > > > zonelists_mutex? > > I prefer zonelists_mutex. > > Christoph, how about below patch? Thanks. > > --- > From 3ccfd04d6ae9127bf0f5472db0b266e7b3f158bd Mon Sep 17 00:00:00 2001 > From: Haicheng Li <haicheng.li@linux.intel.com> > Date: Wed, 19 May 2010 11:07:21 +0800 > Subject: [PATCH] mem-hotplug: fix potential race while building zonelist for > new populated zone > > Add global mutex zonelists_mutex to fix the possible race: > > CPU0 CPU1 CPU2 > (1) zone->present_pages += online_pages; > (2) build_all_zonelists(); > (3) > alloc_page(); > (4) free_page(); > (5) build_all_zonelists(); > (6) __build_all_zonelists(); > (7) zone->pageset = alloc_percpu(); > > In step (3,4), zone->pageset still points to boot_pageset, so bad > things may happen if 2+ nodes are in this state. Even if only 1 node > is accessing the boot_pageset, (3) may still consume too much memory > to fail the memory allocations in step (7). > > Besides, atomic operation ensures alloc_percpu() in step (7) will never fail > since there is a new fresh memory block added in step(6). > > Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > Reviewed-by: Andi Kleen <andi.kleen@intel.com> > --- > include/linux/mmzone.h | 1 + > mm/memory_hotplug.c | 11 +++-------- > mm/page_alloc.c | 15 ++++++++++++++- > 3 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h > index dbbcd50..4d87baa 100644 > --- a/include/linux/mmzone.h > +++ b/include/linux/mmzone.h > @@ -641,6 +641,7 @@ typedef struct pglist_data { > > #include <linux/memory_hotplug.h> > > +extern struct mutex zonelists_mutex; > void get_zone_counts(unsigned long *active, unsigned long *inactive, > unsigned long *free); > void build_all_zonelists(void *data); > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index b564b6a..bc4a942 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -389,11 +389,6 @@ int online_pages(unsigned long pfn, unsigned long > nr_pages) > int nid; > int ret; > struct memory_notify arg; > - /* > - * mutex to protect zone->pageset when it's still shared > - * in onlined_pages() > - */ > - static DEFINE_MUTEX(zone_pageset_mutex); > > arg.start_pfn = pfn; > arg.nr_pages = nr_pages; > @@ -420,14 +415,14 @@ int online_pages(unsigned long pfn, unsigned long > nr_pages) > * This means the page allocator ignores this zone. > * So, zonelist must be updated after online. > */ > - mutex_lock(&zone_pageset_mutex); > + mutex_lock(&zonelists_mutex); > if (!populated_zone(zone)) > need_zonelists_rebuild = 1; > > ret = walk_system_ram_range(pfn, nr_pages, &onlined_pages, > online_pages_range); > if (ret) { > - mutex_unlock(&zone_pageset_mutex); > + mutex_unlock(&zonelists_mutex); > printk(KERN_DEBUG "online_pages %lx at %lx failed\n", > nr_pages, pfn); > memory_notify(MEM_CANCEL_ONLINE, &arg); > @@ -441,7 +436,7 @@ int online_pages(unsigned long pfn, unsigned long > nr_pages) > else > zone_pcp_update(zone); > > - mutex_unlock(&zone_pageset_mutex); > + mutex_unlock(&zonelists_mutex); > setup_per_zone_wmarks(); > calculate_zone_inactive_ratio(zone); > if (onlined_pages) { > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 72c1211..c8e6146 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2434,8 +2434,11 @@ int numa_zonelist_order_handler(ctl_table *table, int > write, > strncpy((char*)table->data, saved_string, > NUMA_ZONELIST_ORDER_LEN); > user_zonelist_order = oldval; > - } else if (oldval != user_zonelist_order) > + } else if (oldval != user_zonelist_order) { > + mutex_lock(&zonelists_mutex); > build_all_zonelists(NULL); > + mutex_unlock(&zonelists_mutex); > + } > } > out: > mutex_unlock(&zl_order_mutex); > @@ -2778,6 +2781,12 @@ static void setup_pageset(struct per_cpu_pageset *p, > unsigned long batch); > static DEFINE_PER_CPU(struct per_cpu_pageset, boot_pageset); > static void setup_zone_pageset(struct zone *zone); > > +/* > + * Global mutex to protect against size modification of zonelists > + * as well as to serialize pageset setup for the new populated zone. > + */ > +DEFINE_MUTEX(zonelists_mutex); > + > /* return values int ....just for stop_machine() */ > static __init_refok int __build_all_zonelists(void *data) > { > @@ -2821,6 +2830,10 @@ static __init_refok int __build_all_zonelists(void > *data) > return 0; > } > > +/* > + * Called with zonelists_mutex held always > + * unless system_state == SYSTEM_BOOTING. > + */ > void build_all_zonelists(void *data) > { > set_zonelist_order(); > -- 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] 7+ messages in thread
end of thread, other threads:[~2010-05-19 15:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-05-17 8:20 [PATCH 3/3] mem-hotplug: fix potential race while building zonelist for new populated zone Haicheng Li 2010-05-17 16:09 ` Christoph Lameter 2010-05-18 2:19 ` Wu Fengguang 2010-05-18 9:02 ` [RESEND][PATCH " Haicheng Li 2010-05-18 13:55 ` Christoph Lameter 2010-05-19 3:48 ` [RESEND -v2][PATCH " Haicheng Li 2010-05-19 15:21 ` Christoph Lameter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).