From: Dave Hansen <hidden> Date: 2006-12-15 16:55:35
I think the comments added say it pretty well, but I'll repeat it here.
This fix is pretty similar in concept to the one that Arnd posted
as a temporary workaround, but I've added a few comments explaining
what the actual assumptions are, and improved it a wee little bit.
The end goal here is to simply avoid calling the early_*() functions
when it is _not_ early. Those functions stop working as soon as
free_initmem() is called. system_state is set to SYSTEM_RUNNING
just after free_initmem() is called, so it seems appropriate to use
here.
I did think twice about actually using SYSTEM_RUNNING because we
moved away from it in other parts of memory hotplug, but those
were actually for _allocations_ in favor of slab_is_available(),
and we really don't care about the slab here.
The only other assumption is that all memory-hotplug-time pages
given to memmap_init_zone() are valid and able to be onlined into
any any zone after the system is running. The "valid" part is
really just a question of whether or not a 'struct page' is there
for the pfn, and *not* whether there is actual memory. Since
all sparsemem sections have contiguous mem_map[]s within them,
and we only memory hotplug entire sparsemem sections, we can
be confident that this assumption will hold.
As for the memory being in the right node, we'll assume tha
memory hotplug is putting things in the right node.
Signed-off-by: Dave Hansen <redacted>
---
lxc-dave/init/main.c | 4 ++++
lxc-dave/mm/page_alloc.c | 28 +++++++++++++++++++++++++---
2 files changed, 29 insertions(+), 3 deletions(-)
diff -puN init/main.c~sparsemem-fix init/main.c
From: Michael Buesch <hidden> Date: 2006-12-15 17:23:19
On Friday 15 December 2006 17:53, Dave Hansen wrote:
quoted hunk
I think the comments added say it pretty well, but I'll repeat it here.
This fix is pretty similar in concept to the one that Arnd posted
as a temporary workaround, but I've added a few comments explaining
what the actual assumptions are, and improved it a wee little bit.
The end goal here is to simply avoid calling the early_*() functions
when it is _not_ early. Those functions stop working as soon as
free_initmem() is called. system_state is set to SYSTEM_RUNNING
just after free_initmem() is called, so it seems appropriate to use
here.
I did think twice about actually using SYSTEM_RUNNING because we
moved away from it in other parts of memory hotplug, but those
were actually for _allocations_ in favor of slab_is_available(),
and we really don't care about the slab here.
The only other assumption is that all memory-hotplug-time pages
given to memmap_init_zone() are valid and able to be onlined into
any any zone after the system is running. The "valid" part is
really just a question of whether or not a 'struct page' is there
for the pfn, and *not* whether there is actual memory. Since
all sparsemem sections have contiguous mem_map[]s within them,
and we only memory hotplug entire sparsemem sections, we can
be confident that this assumption will hold.
As for the memory being in the right node, we'll assume tha
memory hotplug is putting things in the right node.
Signed-off-by: Dave Hansen <redacted>
---
lxc-dave/init/main.c | 4 ++++
lxc-dave/mm/page_alloc.c | 28 +++++++++++++++++++++++++---
2 files changed, 29 insertions(+), 3 deletions(-)
diff -puN init/main.c~sparsemem-fix init/main.c
From: Dave Hansen <hidden> Date: 2006-12-15 17:24:07
On Fri, 2006-12-15 at 17:11 +0000, Andy Whitcroft wrote:
ie. that if hotplug is pushing this memory into a zone on a node it
really does know what its doing, and its putting it in the right place.
Obviously that code needs to be 'overlap' aware but thats ok for this
interface.
I'm not sure if this, especially if we're only doing one section at a
time.
+ /*+ * There are two things that make this work:+ * 1. The early_pfn...() functions are __init and+ * use __initdata. If the system is < SYSTEM_RUNNING,+ * those functions and their data will still exist.+ * 2. We also assume that all actual memory hotplug+ * (as opposed to boot-time) calls to this are only+ * for contiguous memory regions. With sparsemem,+ * this guaranteed is easy because all sections are+ * contiguous and we never online more than one+ * section at a time. Boot-time memory can have holes+ * anywhere.+ */+ if (system_state >= SYSTEM_RUNNING)+ return 1;
Is there any value in codifying the assumption here, as a safety net?
if (system_state >= SYSTEM_RUNNING)
#ifdef CONFIG_SPARSEMEM
return 1;
#else
return 0;
#endif
Dunno. The normal case is that it isn't even called without memory
hotplug. The only non-sparsemem memory hotplug is Keith's baby, and
they lay out all of their mem_map at boot-time anyway, so I don't think
this gets used.
Keith, do you know whether you even do memmap_init_zone() at runtime,
and if you ever have holes if/when you do?
quoted
+ if (!early_pfn_valid(pfn))+ return 0;+ if (!early_pfn_in_nid(pfn, nid))+ return 0;+ return 1;+}+ /* * Initially all pages are reserved - free ones are freed * up by free_all_bootmem() once the early boot process is
@@ -2069,9 +2093,7 @@ void __meminit memmap_init_zone(unsigned unsigned long pfn; for (pfn = start_pfn; pfn < end_pfn; pfn++) {- if (!early_pfn_valid(pfn))- continue;- if (!early_pfn_in_nid(pfn, nid))+ if (!can_online_pfn_into_nid(pfn))
We're not passing nid here?
No, because my ppc64 cross compiler has magically broken. :(
Fixed patch appended.
-- Dave
I think the comments added say it pretty well, but I'll repeat it here.
This fix is pretty similar in concept to the one that Arnd posted
as a temporary workaround, but I've added a few comments explaining
what the actual assumptions are, and improved it a wee little bit.
The end goal here is to simply avoid calling the early_*() functions
when it is _not_ early. Those functions stop working as soon as
free_initmem() is called. system_state is set to SYSTEM_RUNNING
just after free_initmem() is called, so it seems appropriate to use
here.
I did think twice about actually using SYSTEM_RUNNING because we
moved away from it in other parts of memory hotplug, but those
were actually for _allocations_ in favor of slab_is_available(),
and we really don't care about the slab here.
The only other assumption is that all memory-hotplug-time pages
given to memmap_init_zone() are valid and able to be onlined into
any any zone after the system is running. The "valid" part is
really just a question of whether or not a 'struct page' is there
for the pfn, and *not* whether there is actual memory. Since
all sparsemem sections have contiguous mem_map[]s within them,
and we only memory hotplug entire sparsemem sections, we can
be confident that this assumption will hold.
As for the memory being in the right node, we'll assume tha
memory hotplug is putting things in the right node.
Signed-off-by: Dave Hansen <redacted>
---
lxc-dave/init/main.c | 4 ++++
lxc-dave/mm/page_alloc.c | 28 +++++++++++++++++++++++++---
2 files changed, 29 insertions(+), 3 deletions(-)
diff -puN init/main.c~sparsemem-fix init/main.c
From: Andy Whitcroft <hidden> Date: 2006-12-15 17:57:43
Dave Hansen wrote:
I think the comments added say it pretty well, but I'll repeat it here.
This fix is pretty similar in concept to the one that Arnd posted
as a temporary workaround, but I've added a few comments explaining
what the actual assumptions are, and improved it a wee little bit.
The end goal here is to simply avoid calling the early_*() functions
when it is _not_ early. Those functions stop working as soon as
free_initmem() is called. system_state is set to SYSTEM_RUNNING
just after free_initmem() is called, so it seems appropriate to use
here.
I did think twice about actually using SYSTEM_RUNNING because we
moved away from it in other parts of memory hotplug, but those
were actually for _allocations_ in favor of slab_is_available(),
and we really don't care about the slab here.
The only other assumption is that all memory-hotplug-time pages given to memmap_init_zone() are valid and able to be onlined into
any any zone after the system is running. The "valid" part is
really just a question of whether or not a 'struct page' is there
for the pfn, and *not* whether there is actual memory. Since
all sparsemem sections have contiguous mem_map[]s within them,
and we only memory hotplug entire sparsemem sections, we can
be confident that this assumption will hold.
ie. that if hotplug is pushing this memory into a zone on a node it really does know what its doing, and its putting it in the right place. Obviously that code needs to be 'overlap' aware but thats ok for this interface.
quoted hunk
As for the memory being in the right node, we'll assume tha
memory hotplug is putting things in the right node.
Signed-off-by: Dave Hansen <redacted>
---
lxc-dave/init/main.c | 4 ++++
lxc-dave/mm/page_alloc.c | 28 +++++++++++++++++++++++++---
2 files changed, 29 insertions(+), 3 deletions(-)
diff -puN init/main.c~sparsemem-fix init/main.c
+ /*+ * There are two things that make this work:+ * 1. The early_pfn...() functions are __init and+ * use __initdata. If the system is < SYSTEM_RUNNING,+ * those functions and their data will still exist.+ * 2. We also assume that all actual memory hotplug+ * (as opposed to boot-time) calls to this are only+ * for contiguous memory regions. With sparsemem,+ * this guaranteed is easy because all sections are+ * contiguous and we never online more than one+ * section at a time. Boot-time memory can have holes+ * anywhere.+ */+ if (system_state >= SYSTEM_RUNNING)+ return 1;
Is there any value in codifying the assumption here, as a safety net?
if (system_state >= SYSTEM_RUNNING)
#ifdef CONFIG_SPARSEMEM
return 1;
#else
return 0;
#endif
quoted hunk
+ if (!early_pfn_valid(pfn))+ return 0;+ if (!early_pfn_in_nid(pfn, nid))+ return 0;+ return 1;+}+ /* * Initially all pages are reserved - free ones are freed * up by free_all_bootmem() once the early boot process is
@@ -2069,9 +2093,7 @@ void __meminit memmap_init_zone(unsigned unsigned long pfn; for (pfn = start_pfn; pfn < end_pfn; pfn++) {- if (!early_pfn_valid(pfn))- continue;- if (!early_pfn_in_nid(pfn, nid))+ if (!can_online_pfn_into_nid(pfn))
@@ -770,6 +770,10 @@ static int init(void * unused)free_initmem();unlock_kernel();mark_rodata_ro();+/*+*Memoryhotplugrequiresthatthissystem_statetransition+*happerafterfree_initmem().(seememmap_init_zone())
s/happer/happens/
Other than that, can't this possibly race and crash here?
I mean, it's not a big race window, but it can happen, no?
That's a good point. Nice eye.
There are three routes in here: boot-time init, an ACPI call, and a
write to a sysfs file. Bootmem is taken care of. The write to a sysfs
file can't happen yet because userspace isn't up.
The only question would be about ACPI. I _guess_ an ACPI event could
come in at any time, and could hit this race window.
One other thought I had was to add an argument to memmap_init_zone() to
indicate that the memory being fed to it was contiguous and did not need
the validation checks.
Anybody have thoughts on that?
-- Dave
From: Andrew Morton <hidden> Date: 2006-12-15 19:49:46
On Fri, 15 Dec 2006 09:24:00 -0800
Dave Hansen [off-list ref] wrote:
...
I think the comments added say it pretty well, but I'll repeat it here.
This fix is pretty similar in concept to the one that Arnd posted
as a temporary workaround, but I've added a few comments explaining
what the actual assumptions are, and improved it a wee little bit.
The end goal here is to simply avoid calling the early_*() functions
when it is _not_ early. Those functions stop working as soon as
free_initmem() is called. system_state is set to SYSTEM_RUNNING
just after free_initmem() is called, so it seems appropriate to use
here.
Would really prefer not to do this. system_state is evil. Its semantics
are poorly-defined and if someone changes them a bit, or changes memory
initialisation order, you get whacked.
I think an mm-private flag with /*documented*/ semantics would be better.
It's only a byte.
+static int __meminit can_online_pfn_into_nid(unsigned long pfn, int nid)
I spent some time trying to work out what "can_online_pfn_into_nid" can
possibly mean and failed. "We can bring a pfn online then turn it into a
NID"? Don't think so. "We can bring this page online and allocate it to
this node"? Maybe.
Perhaps if the function's role in the world was commented it would be clearer.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2006-12-15 20:25:36
The only other assumption is that all memory-hotplug-time pages
given to memmap_init_zone() are valid and able to be onlined into
any any zone after the system is running. The "valid" part is
really just a question of whether or not a 'struct page' is there
for the pfn, and *not* whether there is actual memory. Since
all sparsemem sections have contiguous mem_map[]s within them,
and we only memory hotplug entire sparsemem sections, we can
be confident that this assumption will hold.
As for the memory being in the right node, we'll assume tha
memory hotplug is putting things in the right node.
BTW, just that people know, what we are adding isn't even memory :-) We
are calling __add_pages() to create struct page for the SPE local stores
and register space as we use them later from a nopage() handler (and no,
we can't use no_pfn just yet for various reasons, notably we need to
handle races with unmap_mapping_ranges() and thus have the truncate
logic in).
Those pages, thus, must never be onlined. Ever. It might make sense to
create a way to inform memory hotplug of that fact, but on the other
hand, I wouldn't bother as I have a plan to get rid of those
__add_pages() completely and work without struct page, maybe in a 2.6.21
timeframe.
Ben.
On Fri, 15 Dec 2006 11:45:36 -0800
Andrew Morton [off-list ref] wrote:
Perhaps if the function's role in the world was commented it would be clearer.
How about patch like this ? (this one is not tested.)
Already-exisiting-more-generic-flag is available ?
-Kame
==
include/linux/memory_hotplug.h | 8 ++++++++
mm/memory_hotplug.c | 14 ++++++++++++++
mm/page_alloc.c | 11 +++++++----
3 files changed, 29 insertions(+), 4 deletions(-)
Index: linux-2.6.20-rc1-mm1/include/linux/memory_hotplug.h
===================================================================
@@ -2069,10 +2069,13 @@unsignedlongpfn;for(pfn=start_pfn;pfn<end_pfn;pfn++){-if(!early_pfn_valid(pfn))-continue;-if(!early_pfn_in_nid(pfn,nid))-continue;+if(!under_memory_hotadd()){+/* we are in boot */+if(!early_pfn_valid(pfn))+continue;+if(!early_pfn_in_nid(pfn,nid))+continue;+}page=pfn_to_page(pfn);set_page_links(page,zone,nid,pfn);init_page_count(page);
I'd be willing to be that this will work just fine. But, I think we can
do it without any static state at all, if we just pass a runtime-or-not
flag down into the arch_add_memory() call chain.
I'll code that up so we can compare to yours.
-- Dave
I'd be willing to be that this will work just fine. But, I think we can
do it without any static state at all, if we just pass a runtime-or-not
flag down into the arch_add_memory() call chain.
I'll code that up so we can compare to yours.
Yes, I stronly concur that passing an explicit flag is much much better
than any hack involving global state.
This also doesn't fix the problem on cell, since the time when the bug
happens, we're not calling through this function, or arch_add_memory,
at all, but rather invoke __add_pages directly. As BenH already mentioned,
we shouldn't do really call __add_pages at all.
Let me attempt another fix that might address all cases. This is completely
untested as of now, but also addresses Dave's latest comment.
Arnd <><
This is what I was thinking of. Sometimes I find these kinds of calls a
bit annoying:
foo(0, 1, 1, 0, 99, 22)
It only takes a minute to look up what all of the numbers do, but that
is one minute too many. :)
How about an enum, or a pair of #defines?
enum context
{
EARLY,
HOTPLUG
};
extern void memmap_init_zone(unsigned long, int, unsigned long, unsigned long,
enum call_context);
...
So, the call I quoted above would become:
memmap_init_zone((size), (nid), (zone), (start_pfn), EARLY)
-- Dave
On Tuesday 19 December 2006 00:16, Dave Hansen wrote:
How about an enum, or a pair of #defines?
=20
enum context
{
=A0 =A0 =A0 =A0 EARLY,
=A0 =A0 =A0 =A0 HOTPLUG
};
Sounds good, but since this is in a global header file, it needs
to be in an appropriate name space, like
enum memmap_context {
MEMMAP_EARLY,
MEMMAP_HOTPLUG,
};
Arnd <><
From: Dave Hansen <hidden> Date: 2006-12-19 19:34:59
Pulling cbe... list off the cc because it is giving annoying 'too many
recipients' warnings.
On Tue, 2006-12-19 at 09:59 +0100, Arnd Bergmann wrote:
On Tuesday 19 December 2006 00:16, Dave Hansen wrote:
quoted
How about an enum, or a pair of #defines?
enum context
{
EARLY,
HOTPLUG
};
This patch should do the trick. Arnd, can you try this to make sure it
solves the issue? Also, if you get a chance, could you do a quick s390
boot of it? It was the only arch touched by this whole thing.
It compiles on all of my i386 .configs.
--
The following patch fixes an oops experienced on the Cell architecture
when init-time functions, early_*(), are called at runtime. It alters
the call paths to make sure that the callers explicitly say whether the
call is being made on behalf of a hotplug even, or happening at
boot-time.
Signed-off-by: Dave Hansen <redacted>
---
lxc-dave/arch/s390/mm/vmem.c | 3 ++-
lxc-dave/include/linux/mm.h | 7 ++++++-
lxc-dave/include/linux/mmzone.h | 3 ++-
lxc-dave/mm/memory_hotplug.c | 6 ++++--
lxc-dave/mm/page_alloc.c | 25 +++++++++++++++++--------
5 files changed, 31 insertions(+), 13 deletions(-)
diff -puN mm/page_alloc.c~sparsemem-enum1 mm/page_alloc.c
@@ -67,11 +67,13 @@ static int __add_zone(struct zone *zone,zone_type=zone-pgdat->node_zones;if(!populated_zone(zone)){intret=0;-ret=init_currently_empty_zone(zone,phys_start_pfn,nr_pages);+ret=init_currently_empty_zone(zone,phys_start_pfn,+nr_pages,MEMMAP_HOTPLUG);if(ret<0)returnret;}-memmap_init_zone(nr_pages,nid,zone_type,phys_start_pfn);+memmap_init_zone(nr_pages,nid,zone_type,+phys_start_pfn,MEMMAP_HOTPLUG);return0;}
@@ -474,7 +474,8 @@ int zone_watermark_ok(struct zone *z, inintclasszone_idx,intalloc_flags);externintinit_currently_empty_zone(structzone*zone,unsignedlongstart_pfn,-unsignedlongsize);+unsignedlongsize,+enummemmap_contextcontext);#ifdef CONFIG_HAVE_MEMORY_PRESENTvoidmemory_present(intnid,unsignedlongstart,unsignedlongend);
From: Dave Hansen <hidden> Date: 2007-01-06 01:10:12
I dropped this on the floor over Christmas. This has had a few smoke
tests on ppc64 and i386 and is ready for -mm. Against 2.6.20-rc2-mm1.
The following patch fixes an oops experienced on the Cell architecture
when init-time functions, early_*(), are called at runtime. It alters
the call paths to make sure that the callers explicitly say whether the
call is being made on behalf of a hotplug even, or happening at
boot-time.
Signed-off-by: Dave Hansen <redacted>
---
lxc-dave/arch/s390/mm/vmem.c | 3 ++-
lxc-dave/include/linux/mm.h | 7 ++++++-
lxc-dave/include/linux/mmzone.h | 3 ++-
lxc-dave/mm/memory_hotplug.c | 6 ++++--
lxc-dave/mm/page_alloc.c | 25 +++++++++++++++++--------
5 files changed, 31 insertions(+), 13 deletions(-)
diff -puN mm/page_alloc.c~sparsemem-enum1 mm/page_alloc.c
@@ -67,11 +67,13 @@ static int __add_zone(struct zone *zone,zone_type=zone-pgdat->node_zones;if(!populated_zone(zone)){intret=0;-ret=init_currently_empty_zone(zone,phys_start_pfn,nr_pages);+ret=init_currently_empty_zone(zone,phys_start_pfn,+nr_pages,MEMMAP_HOTPLUG);if(ret<0)returnret;}-memmap_init_zone(nr_pages,nid,zone_type,phys_start_pfn);+memmap_init_zone(nr_pages,nid,zone_type,+phys_start_pfn,MEMMAP_HOTPLUG);return0;}
@@ -474,7 +474,8 @@ int zone_watermark_ok(struct zone *z, inintclasszone_idx,intalloc_flags);externintinit_currently_empty_zone(structzone*zone,unsignedlongstart_pfn,-unsignedlongsize);+unsignedlongsize,+enummemmap_contextcontext);#ifdef CONFIG_HAVE_MEMORY_PRESENTvoidmemory_present(intnid,unsignedlongstart,unsignedlongend);
From: Dave Hansen <hidden> Date: 2007-01-07 08:58:37
On Fri, 2007-01-05 at 22:52 -0600, John Rose wrote:
quoted
I dropped this on the floor over Christmas. This has had a few smoke
tests on ppc64 and i386 and is ready for -mm. Against 2.6.20-rc2-mm1.
Could this break ia64, given that it uses memmap_init_zone()?
You are right, I think it does.
Here's an updated patch to replace the earlier one. I had to move the
enum definition over to a different header because ia64 evidently has a
different include order.
---
The following patch fixes an oops experienced on the Cell architecture
when init-time functions, early_*(), are called at runtime. It alters
the call paths to make sure that the callers explicitly say whether the
call is being made on behalf of a hotplug even, or happening at
boot-time.
It has been compile tested on ia64, s390, i386 and x86_64.
Signed-off-by: Dave Hansen <redacted>
---
lxc-dave/arch/ia64/mm/init.c | 5 +++--
lxc-dave/arch/s390/mm/vmem.c | 3 ++-
lxc-dave/include/linux/mm.h | 3 ++-
lxc-dave/include/linux/mmzone.h | 8 ++++++--
lxc-dave/mm/memory_hotplug.c | 6 ++++--
lxc-dave/mm/page_alloc.c | 25 +++++++++++++++++--------
6 files changed, 34 insertions(+), 16 deletions(-)
diff -puN arch/s390/mm/vmem.c~Re-_PATCH_Fix_sparsemem_on_Cell arch/s390/mm/vmem.c
@@ -67,11 +67,13 @@ static int __add_zone(struct zone *zone,zone_type=zone-pgdat->node_zones;if(!populated_zone(zone)){intret=0;-ret=init_currently_empty_zone(zone,phys_start_pfn,nr_pages);+ret=init_currently_empty_zone(zone,phys_start_pfn,+nr_pages,MEMMAP_HOTPLUG);if(ret<0)returnret;}-memmap_init_zone(nr_pages,nid,zone_type,phys_start_pfn);+memmap_init_zone(nr_pages,nid,zone_type,+phys_start_pfn,MEMMAP_HOTPLUG);return0;}
@@ -550,7 +551,7 @@ memmap_init (unsigned long size, int nidunsignedlongstart_pfn){if(!vmem_map)-memmap_init_zone(size,nid,zone,start_pfn);+memmap_init_zone(size,nid,zone,start_pfn,MEMMAP_EARLY);else{structpage*start;structmemmap_init_callback_dataargs;
On Sunday 07 January 2007 09:58, Dave Hansen wrote:
The following patch fixes an oops experienced on the Cell architecture
when init-time functions, early_*(), are called at runtime. =A0It alters
the call paths to make sure that the callers explicitly say whether the
call is being made on behalf of a hotplug even, or happening at
boot-time.=20
=20
It has been compile tested on ia64, s390, i386 and x86_64.
I can't test it here, since I'm travelling at the moment, but
this version looks good to me. Thanks for picking it up again!
From: Tim Pepper <hidden> Date: 2007-01-08 06:31:14
On 1/7/07, Dave Hansen [off-list ref] wrote:
On Fri, 2007-01-05 at 22:52 -0600, John Rose wrote:
quoted
Could this break ia64, given that it uses memmap_init_zone()?
You are right, I think it does.
Here's an updated patch to replace the earlier one. I had to move the
enum definition over to a different header because ia64 evidently has a
different include order.
Boot tested OK on ia64 with this latest version of the patch.
Tim
From: Tim Pepper <hidden> Date: 2007-01-08 06:47:50
On 1/7/07, Dave Hansen [off-list ref] wrote:
On Fri, 2007-01-05 at 22:52 -0600, John Rose wrote:
quoted
Could this break ia64, given that it uses memmap_init_zone()?
You are right, I think it does.
Boot tested OK on ia64 with this latest version of the patch.
(forgot to click plain text on gmail the first time..sorry if you got
html mail or repeat)
Tim