mem-hotplug: fix potential race while building zonelist for new populated zone
Subsystems (5)
PATCH 3/3 mem-hotplug: fix potential race while building zonelist for new populated zone
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.
--- include/linux/memory_hotplug.h | 8 ++++++++ mm/memory_hotplug.c | 15 +++++++++------ mm/page_alloc.c | 20 ++++++++++++++++---- 3 files changed, 33 insertions(+), 10 deletions(-)
include/linux/memory_hotplug.h +8 -0
@@ -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
mm/memory_hotplug.c +9 -6
@@ -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);
mm/page_alloc.c +16 -4
@@ -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 /*
Signature · 8 lines
1.6.0.rc1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 14 lines · Haicheng Li
mm/page_alloc.c +5
-0@@ -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
mm/page_alloc.c static __init_refok int __build_all_zonelists(void · line 2801
Quoted 10 lines
mm/page_alloc.c +9
-0+* 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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 22 lines · Christoph Lameter
Quoted 20 lines · Haicheng Li
mm/page_alloc.c +11
-0@@ -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;
Quoted 8 lines
mm/page_alloc.c static __init_refok int __build_all_zonelists(void *data) · line 2799
Quoted 4 lines
mm/page_alloc.c +3
-0+ 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.
Quoted 2 lines
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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH RESEND 3/3 mem-hotplug: fix potential race while building zonelist for new populated zone
Quoted 4 lines · Wu Fengguang
Quoted 2 lines · Christoph Lameter
Building a zonelist now has the potential side effect of changes to the size of the zone?
Quoted 8 lines
Yeah, this sounds a bit hacky.
Quoted 2 lines
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_mutexa global mutex and take it in all the functions that callbuild_all_zonelists()-- currently onlyonline_pages()andnuma_zonelist_order_handler().
Yes, if we don't mind adding a global mutex, it's another way to fix the race.
Quoted 11 lines
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();
Quoted 4 lines
In step (3,4),
zone->pagesetstill points toboot_pageset, so bad things may happen if 2+ nodes are in this state. Even if only 1 node is accessing theboot_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 <[email protected]>
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).
--- include/linux/mmzone.h | 1 + mm/memory_hotplug.c | 11 +++-------- mm/page_alloc.c | 15 ++++++++++++++- 3 files changed, 18 insertions(+), 9 deletions(-)
include/linux/mmzone.h +1 -0
@@ -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);
mm/memory_hotplug.c +3 -8
@@ -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) {
mm/page_alloc.c +14 -1
@@ -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 ();
Signature · 8 lines
1.5.6.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 1 line · Haicheng Li
+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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH RESEND 3/3 mem-hotplug: fix potential race while building zonelist for new populated zone
Quoted 6 lines · Christoph Lameter
Quoted 1 line · Haicheng Li
+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.
Quoted 7 lines
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 <[email protected]>
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).
--- include/linux/mmzone.h | 1 + mm/memory_hotplug.c | 11 +++-------- mm/page_alloc.c | 15 ++++++++++++++- 3 files changed, 18 insertions(+), 9 deletions(-)
include/linux/mmzone.h +1 -0
@@ -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);
mm/memory_hotplug.c +3 -8
@@ -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) {
mm/page_alloc.c +14 -1
@@ -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 ();
Signature · 12 lines
1.5.6.1 -haicheng -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Looks good on its own....
Quoted 158 lines · Haicheng Li
Quoted 6 lines · Christoph Lameter
Quoted 1 line · Haicheng Li
+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.
Quoted 7 lines
So
zones_mutex?or
zonelists_mutex?I prefer
zonelists_mutex.Christoph, how about below patch? Thanks.
---
From3ccfd04d6ae9127bf0f5472db0b266e7b3f158bdMon Sep 17 00:00:00 2001
From: Haicheng Li <[email protected]>
Date: Wed, 19 May 2010 11:07:21 +0800
Subject: [PATCH] mem-hotplug: fix potential race while building zonelist for new populated zoneAdd global mutex
zonelists_mutexto 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->pagesetstill points toboot_pageset, so bad things may happen if 2+ nodes are in this state. Even if only 1 node is accessing theboot_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 <[email protected]>Signed-off-by Wu Fengguang <[email protected]>Reviewed-by Andi Kleen <[email protected]>--- include/linux/mmzone.h | 1 + mm/memory_hotplug.c | 11 +++-------- mm/page_alloc.c | 15 ++++++++++++++- 3 files changed, 18 insertions(+), 9 deletions(-)include/linux/mmzone.h +1
-0@@ -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);mm/memory_hotplug.c +0
-5@@ -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;mm/memory_hotplug.c +2
-2arg.nr_pages = nr_pages;@@ -420,14 +415,14 @@ int online_pages(unsigned long pfn, unsigned longnr_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);mm/memory_hotplug.c +1
-1memory_notify (MEM_CANCEL_ONLINE , &arg);@@ -441,7 +436,7 @@ int online_pages(unsigned long pfn, unsigned long nr_pages) elsezone_pcp_update (zone ); -mutex_unlock (&zone_pageset_mutex); +mutex_unlock (&zonelists_mutex);setup_per_zone_wmarks ();calculate_zone_inactive_ratio (zone);mm/memory_hotplug.c +0
-0if (onlined_pages) {mm/page_alloc.c +4
-1@@ -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:mm/page_alloc.c +6
-0mutex_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){
mm/page_alloc.c +4
-0@@ -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) {
Quoted 2 lines
set_zonelist_order();
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>