add_memory() does all necessary check to avoid collision.
then, acpi layer doesn't have to check region by itself.
(*) pfn_valid() just returns page struct is valid or not. It returns 0
if a section has been already added even is ioresource is not added.
ioresource collision check in mm/memory_hotplug.c can do more precise
collistion check.
added enabled bit check just for sanity check..
Signed-Off-By: KAMEZAWA Hiroyuki <[email protected]>
drivers/acpi/acpi_memhotplug.c | 9 +--------
1 files changed, 1 insertion(+), 8 deletions(-)
Index: linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-2.6.18-rc3.orig/drivers/acpi/acpi_memhotplug.c 2006-08-01 16:11:47.000000000 +0900
+++ linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c 2006-08-02 14:12:45.000000000 +0900
@@ -230,17 +230,10 @@
* (i.e. memory-hot-remove function)
*/
list_for_each_entry(info, &mem_device->res_list, list) {
- u64 start_pfn, end_pfn;
-
- start_pfn = info->start_addr >> PAGE_SHIFT;
- end_pfn = (info->start_addr + info->length - 1) >> PAGE_SHIFT;
-
- if (pfn_valid(start_pfn) || pfn_valid(end_pfn)) {
- /* already enabled. try next area */
+ if (info->enabled) { /* just sanity check...*/
num_enabled++;
continue;
}
-
result = add_memory(node, info->start_addr, info->length);
if (result)
continue;
On Thu, 2006-08-03 at 12:36 +0900, KAMEZAWA Hiroyuki wrote:
> add_memory() does all necessary check to avoid collision.
> then, acpi layer doesn't have to check region by itself.
>
> (*) pfn_valid() just returns page struct is valid or not. It returns 0
> if a section has been already added even is ioresource is not added.
> ioresource collision check in mm/memory_hotplug.c can do more precise
> collistion check.
> added enabled bit check just for sanity check..
>
> Signed-Off-By: KAMEZAWA Hiroyuki <[email protected]>
>
>
> drivers/acpi/acpi_memhotplug.c | 9 +--------
> 1 files changed, 1 insertion(+), 8 deletions(-)
>
> Index: linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c
> ===================================================================
> --- linux-2.6.18-rc3.orig/drivers/acpi/acpi_memhotplug.c 2006-08-01 16:11:47.000000000 +0900
> +++ linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c 2006-08-02 14:12:45.000000000 +0900
> @@ -230,17 +230,10 @@
> * (i.e. memory-hot-remove function)
> */
> list_for_each_entry(info, &mem_device->res_list, list) {
> - u64 start_pfn, end_pfn;
> -
> - start_pfn = info->start_addr >> PAGE_SHIFT;
> - end_pfn = (info->start_addr + info->length - 1) >> PAGE_SHIFT;
> -
> - if (pfn_valid(start_pfn) || pfn_valid(end_pfn)) {
> - /* already enabled. try next area */
> + if (info->enabled) { /* just sanity check...*/
> num_enabled++;
> continue;
> }
This check needs to go. pfn_valid is a sparsemem specific check. Sanity
checking should be done it the the add_memory code.
I will test and let you know. This is going to expose some baddness I
see already with my RESERVE path work. (Extra add_memory calls from this
driver during boot....)
Thanks,
keith mannthey <[email protected]>
Linux Technology Center IBM
On Thu, 2006-08-03 at 11:28 -0700, keith mannthey wrote:
> On Thu, 2006-08-03 at 12:36 +0900, KAMEZAWA Hiroyuki wrote:
> > add_memory() does all necessary check to avoid collision.
> > then, acpi layer doesn't have to check region by itself.
> >
> > (*) pfn_valid() just returns page struct is valid or not. It returns 0
> > if a section has been already added even is ioresource is not added.
> > ioresource collision check in mm/memory_hotplug.c can do more precise
> > collistion check.
> > added enabled bit check just for sanity check..
> >
> > Signed-Off-By: KAMEZAWA Hiroyuki <[email protected]>
> >
> >
> > drivers/acpi/acpi_memhotplug.c | 9 +--------
> > 1 files changed, 1 insertion(+), 8 deletions(-)
> >
> > Index: linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c
> > ===================================================================
> > --- linux-2.6.18-rc3.orig/drivers/acpi/acpi_memhotplug.c 2006-08-01 16:11:47.000000000 +0900
> > +++ linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c 2006-08-02 14:12:45.000000000 +0900
> > @@ -230,17 +230,10 @@
> > * (i.e. memory-hot-remove function)
> > */
> > list_for_each_entry(info, &mem_device->res_list, list) {
> > - u64 start_pfn, end_pfn;
> > -
> > - start_pfn = info->start_addr >> PAGE_SHIFT;
> > - end_pfn = (info->start_addr + info->length - 1) >> PAGE_SHIFT;
> > -
> > - if (pfn_valid(start_pfn) || pfn_valid(end_pfn)) {
> > - /* already enabled. try next area */
> > + if (info->enabled) { /* just sanity check...*/
> > num_enabled++;
> > continue;
> > }
>
> This check needs to go. pfn_valid is a sparsemem specific check. Sanity
> checking should be done it the the add_memory code.
>
> I will test and let you know. This is going to expose some baddness I
> see already with my RESERVE path work. (Extra add_memory calls from this
> driver during boot....)
Ok. This pfn_valid check needs to be inserted somewhere in the code
path for sparsemem hotadd.
with a debug statement in add_memory
Hotplug Mem Device
add_memory 0 400000000 70000000
System RAM resource 400000000 - 46fffffff cannot be added
add_memory 0 380000000 80000000
System RAM resource 380000000 - 3ffffffff cannot be added
add_memory 0 300000000 80000000
System RAM resource 300000000 - 37fffffff cannot be added
add_memory 0 280000000 80000000
System RAM resource 280000000 - 2ffffffff cannot be added
add_memory 0 200000000 80000000
System RAM resource 200000000 - 27fffffff cannot be added
add_memory 0 180000000 80000000
System RAM resource 180000000 - 1ffffffff cannot be added
add_memory 0 100000000 80000000
System RAM resource 100000000 - 17fffffff cannot be added
add_memory 0 100000 7ff00000
The box doesn't boot. I am going to drop this patch and see about the
rest of the set. (They seem sane and look ok but I want to test)
The kernel needs to protect against bad calls to add_memory (and/or the
acpi driver needs to correctly id devices?)
keith mannthey <[email protected]>
Linux Technology Center IBM
Hi, Keith
Thank you for test.
On Thu, 03 Aug 2006 16:09:36 -0700
keith mannthey <[email protected]> wrote:
> > > drivers/acpi/acpi_memhotplug.c | 9 +--------
> > > 1 files changed, 1 insertion(+), 8 deletions(-)
> > >
> > > Index: linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c
> > > ===================================================================
> > > --- linux-2.6.18-rc3.orig/drivers/acpi/acpi_memhotplug.c 2006-08-01 16:11:47.000000000 +0900
> > > +++ linux-2.6.18-rc3/drivers/acpi/acpi_memhotplug.c 2006-08-02 14:12:45.000000000 +0900
> > > @@ -230,17 +230,10 @@
> > > * (i.e. memory-hot-remove function)
> > > */
> > > list_for_each_entry(info, &mem_device->res_list, list) {
> > > - u64 start_pfn, end_pfn;
> > > -
> > > - start_pfn = info->start_addr >> PAGE_SHIFT;
> > > - end_pfn = (info->start_addr + info->length - 1) >> PAGE_SHIFT;
> > > -
> > > - if (pfn_valid(start_pfn) || pfn_valid(end_pfn)) {
> > > - /* already enabled. try next area */
> > > + if (info->enabled) { /* just sanity check...*/
> > > num_enabled++;
> > > continue;
> > > }
> >
> > This check needs to go. pfn_valid is a sparsemem specific check. Sanity
> > checking should be done it the the add_memory code.
> >
> > I will test and let you know. This is going to expose some baddness I
> > see already with my RESERVE path work. (Extra add_memory calls from this
> > driver during boot....)
>
> Ok. This pfn_valid check needs to be inserted somewhere in the code
> path for sparsemem hotadd.
>
> with a debug statement in add_memory
>
> Hotplug Mem Device
> add_memory 0 400000000 70000000
> System RAM resource 400000000 - 46fffffff cannot be added
This messages is at ioresouce collision check. This says system has
memory resource between 400000000 - 46fffffff...before hotadd.
and sparse_add_one_seciton() returns -EEXIST if section exists.
==
int sparse_add_one_section(struct zone *zone, unsigned long start_pfn,
int nr_pages)
{
<snip>
if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
ret = -EEXIST;
goto out;
}
}
==
Ah... but x86_64 special (not depends on sparsemem..) __add_pages() call doesn't
do sanity check at online_page().
Here.
==
for (pfn = start_pfn; pfn < start_pfn + nr_pages; pfn++) {
if (pfn_valid(pfn)) {
online_page(pfn_to_page(pfn));
err = 0;
mem++;
}
==
So, panics ...maybe.
(System has memory between 40000000 - 46fffffff but it's onlined again)
Could you add sanity check in online_page() ?
==
if (PageReserved(page) {
online_page(pfn_to_page(pfn));
}
==
will be enough.
I don't have avaliable x86_64 box now.
-Kame
On Thu, 2006-08-03 at 12:36 +0900, KAMEZAWA Hiroyuki wrote:
> add_memory() does all necessary check to avoid collision.
> then, acpi layer doesn't have to check region by itself.
>
> (*) pfn_valid() just returns page struct is valid or not. It returns 0
> if a section has been already added even is ioresource is not added.
> ioresource collision check in mm/memory_hotplug.c can do more precise
> collistion check.
> added enabled bit check just for sanity check..
>
> Signed-Off-By: KAMEZAWA Hiroyuki <[email protected]>
> - start_pfn = info->start_addr >> PAGE_SHIFT;
> - end_pfn = (info->start_addr + info->length - 1) >> PAGE_SHIFT;
> -
> - if (pfn_valid(start_pfn) || pfn_valid(end_pfn)) {
This check needs to go somewhare in the add path. I am thinking of a
validate_add_memory_area call in add_memory (that can also be flexable
to enable the reserve check of (this memory area in add_nodes).
It is a useful protection for the sparsemem add path. I would rather
the kernel be able to stand up to odd acpi namespaces or other
mechanisms of invoking add_memory.
Thanks,
Keith
On Thu, 03 Aug 2006 17:13:16 -0700
keith mannthey <[email protected]> wrote:
> On Thu, 2006-08-03 at 12:36 +0900, KAMEZAWA Hiroyuki wrote:
> > add_memory() does all necessary check to avoid collision.
> > then, acpi layer doesn't have to check region by itself.
> >
> > (*) pfn_valid() just returns page struct is valid or not. It returns 0
> > if a section has been already added even is ioresource is not added.
> > ioresource collision check in mm/memory_hotplug.c can do more precise
> > collistion check.
> > added enabled bit check just for sanity check..
> >
> > Signed-Off-By: KAMEZAWA Hiroyuki <[email protected]>
>
> > - start_pfn = info->start_addr >> PAGE_SHIFT;
> > - end_pfn = (info->start_addr + info->length - 1) >> PAGE_SHIFT;
> > -
> > - if (pfn_valid(start_pfn) || pfn_valid(end_pfn)) {
>
> This check needs to go somewhare in the add path. I am thinking of a
> validate_add_memory_area call in add_memory (that can also be flexable
> to enable the reserve check of (this memory area in add_nodes).
>
> It is a useful protection for the sparsemem add path. I would rather
> the kernel be able to stand up to odd acpi namespaces or other
> mechanisms of invoking add_memory.
>
Hmm..Okay. I'll try some check patch today. please review it.
Maybe moving ioresouce collision check in early stage of add_memory() is good ?
Note:
I remove pfn_valid() here because pfn_valid() just says section exists or
not. When adding seveal small memory chunks in one section, Only the first
small chunk can be added.
Thanks,
-Kame
On Fri, 2006-08-04 at 09:44 +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 03 Aug 2006 17:13:16 -0700
> keith mannthey <[email protected]> wrote:
>
> > On Thu, 2006-08-03 at 12:36 +0900, KAMEZAWA Hiroyuki wrote:
> > > add_memory() does all necessary check to avoid collision.
> > > then, acpi layer doesn't have to check region by itself.
> > >
> > > (*) pfn_valid() just returns page struct is valid or not. It returns 0
> > > if a section has been already added even is ioresource is not added.
> > > ioresource collision check in mm/memory_hotplug.c can do more precise
> > > collistion check.
> > > added enabled bit check just for sanity check..
> > >
> > > Signed-Off-By: KAMEZAWA Hiroyuki <[email protected]>
> >
> > > - start_pfn = info->start_addr >> PAGE_SHIFT;
> > > - end_pfn = (info->start_addr + info->length - 1) >> PAGE_SHIFT;
> > > -
> > > - if (pfn_valid(start_pfn) || pfn_valid(end_pfn)) {
> >
> > This check needs to go somewhare in the add path. I am thinking of a
> > validate_add_memory_area call in add_memory (that can also be flexable
> > to enable the reserve check of (this memory area in add_nodes).
> >
> > It is a useful protection for the sparsemem add path. I would rather
> > the kernel be able to stand up to odd acpi namespaces or other
> > mechanisms of invoking add_memory.
> >
> Hmm..Okay. I'll try some check patch today. please review it.
> Maybe moving ioresouce collision check in early stage of add_memory() is good ?
Yea. I am working a a full patch set for but my sparsemem and reserve
add-based paths. It creates a valid_memory_add_range call at the start
of add_memory. I should be posting the set in the next few hours.
> Note:
> I remove pfn_valid() here because pfn_valid() just says section exists or
> not. When adding seveal small memory chunks in one section, Only the first
> small chunk can be added.
Hmm... I thought memory add areas needed to be section aligned for the arch?
What protecting is there for calling add_memory on an already present
memory range?
Thanks,
Keith
On Thu, 03 Aug 2006 18:54:32 -0700
keith mannthey <[email protected]> wrote:
> > Hmm..Okay. I'll try some check patch today. please review it.
> > Maybe moving ioresouce collision check in early stage of add_memory() is good ?
> Yea. I am working a a full patch set for but my sparsemem and reserve
> add-based paths. It creates a valid_memory_add_range call at the start
> of add_memory. I should be posting the set in the next few hours.
>
Ah..ok. but I wrote my own patch...and testing it now..
> > Note:
> > I remove pfn_valid() here because pfn_valid() just says section exists or
> > not. When adding seveal small memory chunks in one section, Only the first
> > small chunk can be added.
> Hmm... I thought memory add areas needed to be section aligned for the arch?
>
There are requests for memory-hot-add should allow to hot-add not-aligned memory.
Then, I wrote ioresouce collision check patch (before..but had bug..)
With ioresouce collistion check, alignments are not required at *add*.
(onlining is just for *offlined section*, now)
> What protecting is there for calling add_memory on an already present
> memory range?
>
For example, considering ia64, which has 1Gbytes section...
hot add following region.
==
(A) 0xc0000000 - 0xd7ffffff (section 3)
(B) 0xe0000000 - 0xffffffff (section 3)
==
(A) and (B) will go to the same section, but there is a memory hole between
(A) and (B). Considering memory (B) appears after (A) in DSDT.
After add_memory() against (A) is called, section 3 is ready.
Then, pfn_valid(0xe0000000) and pfn_valid(0xffffffff) returns true because
they are in section 3.
So, checking pfn_valid() for (B) will returns true and memory (B) cannot be
added. ioresouce collision check will help this situation.
The memory hole are not onlined because there are no ioresouce for it.
-Kame
Okay... here is 6/5 patch..
On Fri, 4 Aug 2006 11:15:50 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:
> > > Maybe moving ioresouce collision check in early stage of add_memory() is good ?
> > Yea. I am working a a full patch set for but my sparsemem and reserve
> > add-based paths. It creates a valid_memory_add_range call at the start
> > of add_memory. I should be posting the set in the next few hours.
> >
> Ah..ok. but I wrote my own patch...and testing it now..
>
This patch passed test with ia64. based on 5 patches already sent.
plz check.
Andrew, should I re-organize all patches if this is ok ?
-Kame
==
This patch is for collision check enhancement for memory hot add.
It's better to do resouce collision check before doing memory hot add,
which will touch memory management structures.
And add_section() should check section exists or not before calling
sparse_add_one_section(). (sparse_add_one_section() will do another
check anyway. but checking in memory_hotplug.c will be easy to understand.)
Signed-Off-By: KAMEZAWA Hiroyuki <[email protected]>
mm/memory_hotplug.c | 29 ++++++++++++++++++++++-------
1 files changed, 22 insertions(+), 7 deletions(-)
Index: linux-2.6.18-rc3/mm/memory_hotplug.c
===================================================================
--- linux-2.6.18-rc3.orig/mm/memory_hotplug.c 2006-08-02 15:30:53.000000000 +0900
+++ linux-2.6.18-rc3/mm/memory_hotplug.c 2006-08-04 10:19:20.000000000 +0900
@@ -52,6 +52,9 @@
int nr_pages = PAGES_PER_SECTION;
int ret;
+ if (pfn_valid(phys_start_pfn))
+ return -EEXIST;
+
ret = sparse_add_one_section(zone, phys_start_pfn, nr_pages);
if (ret < 0)
@@ -220,10 +223,9 @@
}
/* add this memory to iomem resource */
-static int register_memory_resource(u64 start, u64 size)
+static struct resource *register_memory_resource(u64 start, u64 size)
{
struct resource *res;
- int ret = 0;
res = kzalloc(sizeof(struct resource), GFP_KERNEL);
BUG_ON(!res);
@@ -235,9 +237,18 @@
printk("System RAM resource %llx - %llx cannot be added\n",
(unsigned long long)res->start, (unsigned long long)res->end);
kfree(res);
- ret = -EEXIST;
+ res = NULL;
}
- return ret;
+ return res;
+}
+
+static void release_memory_resource(struct resource *res)
+{
+ if (!res)
+ return;
+ release_resource(res);
+ kfree(res);
+ return;
}
@@ -246,8 +257,13 @@
{
pg_data_t *pgdat = NULL;
int new_pgdat = 0;
+ struct resource *res;
int ret;
+ res = register_memory_resource(start, size);
+ if (!res)
+ return -EEXIST;
+
if (!node_online(nid)) {
pgdat = hotadd_new_pgdat(nid, start);
if (!pgdat)
@@ -277,14 +293,13 @@
BUG_ON(ret);
}
- /* register this memory as resource */
- ret = register_memory_resource(start, size);
-
return ret;
error:
/* rollback pgdat allocation and others */
if (new_pgdat)
rollback_node_hotadd(nid, pgdat);
+ if (res)
+ release_memory_resource(res);
return ret;
}
On Fri, 2006-08-04 at 11:15 +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 03 Aug 2006 18:54:32 -0700
> keith mannthey <[email protected]> wrote:
>
> > > Hmm..Okay. I'll try some check patch today. please review it.
> > > Maybe moving ioresouce collision check in early stage of add_memory() is good ?
> > Yea. I am working a a full patch set for but my sparsemem and reserve
> > add-based paths. It creates a valid_memory_add_range call at the start
> > of add_memory. I should be posting the set in the next few hours.
> >
> Ah..ok. but I wrote my own patch...and testing it now..
Sure that is fine.
>
> > > Note:
> > > I remove pfn_valid() here because pfn_valid() just says section exists or
> > > not. When adding seveal small memory chunks in one section, Only the first
> > > small chunk can be added.
> > Hmm... I thought memory add areas needed to be section aligned for the arch?
> >
> There are requests for memory-hot-add should allow to hot-add not-aligned memory.
> Then, I wrote ioresouce collision check patch (before..but had bug..)
> With ioresouce collistion check, alignments are not required at *add*.
> (onlining is just for *offlined section*, now)
>
> > What protecting is there for calling add_memory on an already present
> > memory range?
> >
> For example, considering ia64, which has 1Gbytes section...
Maybe 1gb sections is too large?
> hot add following region.
> ==
> (A) 0xc0000000 - 0xd7ffffff (section 3)
> (B) 0xe0000000 - 0xffffffff (section 3)
> ==
> (A) and (B) will go to the same section, but there is a memory hole between
> (A) and (B). Considering memory (B) appears after (A) in DSDT.
>
> After add_memory() against (A) is called, section 3 is ready.
> Then, pfn_valid(0xe0000000) and pfn_valid(0xffffffff) returns true because
> they are in section 3.
> So, checking pfn_valid() for (B) will returns true and memory (B) cannot be
> added. ioresouce collision check will help this situation.
With iommus out there throwing aliment all off way the flexability is
good.
My question is this.
Assuming 0-0xbfffffff is present.
What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
to add_memory?
Thanks,
Keith
On Fri, 2006-08-04 at 11:32 +0900, KAMEZAWA Hiroyuki wrote:
> Okay... here is 6/5 patch..
>
> On Fri, 4 Aug 2006 11:15:50 +0900
> KAMEZAWA Hiroyuki <[email protected]> wrote:
> > > > Maybe moving ioresouce collision check in early stage of add_memory() is good ?
> > > Yea. I am working a a full patch set for but my sparsemem and reserve
> > > add-based paths. It creates a valid_memory_add_range call at the start
> > > of add_memory. I should be posting the set in the next few hours.
> > >
> > Ah..ok. but I wrote my own patch...and testing it now..
> >
>
> This patch passed test with ia64. based on 5 patches already sent.
> plz check.
<snip>
> pg_data_t *pgdat = NULL;
> int new_pgdat = 0;
> + struct resource *res;
> int ret;
>
> + res = register_memory_resource(start, size);
> + if (!res)
> + return -EEXIST;
> +
> if (!node_online(nid)) {
> pgdat = hotadd_new_pgdat(nid, start);
> if (!pgdat)
> @@ -277,14 +293,13 @@
> BUG_ON(ret);
> }
>
> - /* register this memory as resource */
> - ret = register_memory_resource(start, size);
> -
> return ret;
> error:
> /* rollback pgdat allocation and others */
> if (new_pgdat)
> rollback_node_hotadd(nid, pgdat);
> + if (res)
> + release_memory_resource(res);
>
I am trying to keep add_memory non-sparsemem specific. With reserve
memory the memory is already present and so using
register_memory_resource as a key to find out if it is already present
won't work.
I can build ontop of this if you want me to. I would like to test this
to make sure it works for me. (I will do this sometime today)
Thanks,
Keith
On Thu, 03 Aug 2006 20:00:08 -0700
keith mannthey <[email protected]> wrote:
> > > What protecting is there for calling add_memory on an already present
> > > memory range?
> > >
> > For example, considering ia64, which has 1Gbytes section...
>
> Maybe 1gb sections is too large?
>
ia64 machines sometimes to have crazy big memory...so 1gb section is requested.
Configurable section_size for small machines was rejected in old days.
> > hot add following region.
> > ==
> > (A) 0xc0000000 - 0xd7ffffff (section 3)
> > (B) 0xe0000000 - 0xffffffff (section 3)
> > ==
> > (A) and (B) will go to the same section, but there is a memory hole between
> > (A) and (B). Considering memory (B) appears after (A) in DSDT.
> >
> > After add_memory() against (A) is called, section 3 is ready.
> > Then, pfn_valid(0xe0000000) and pfn_valid(0xffffffff) returns true because
> > they are in section 3.
> > So, checking pfn_valid() for (B) will returns true and memory (B) cannot be
> > added. ioresouce collision check will help this situation.
>
> With iommus out there throwing aliment all off way the flexability is
> good.
>
> My question is this.
>
> Assuming 0-0xbfffffff is present.
>
> What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
> to add_memory?
Usual sparsemem's add_memory() checks whether there are sections in
sparse_add_one_section(). then add_pages() returns -EEXIST (nothing to do).
And ioresouce collision check will finally find collision because 0-0xbffffff
resource will conflict with 0xa0000000 to 0xa10000000 area.
But, x86_64 's (not sparsemem) add_pages() doen't do collision check, so it panics.
I posted patch to catch collision before calling arch_add_memory(), I think it will
help x86_64 users. and no-re-online.
-Kame
On Thu, 03 Aug 2006 20:09:42 -0700
keith mannthey <[email protected]> wrote:
> > pg_data_t *pgdat = NULL;
> > int new_pgdat = 0;
> > + struct resource *res;
> > int ret;
> >
> > + res = register_memory_resource(start, size);
> > + if (!res)
> > + return -EEXIST;
> > +
> > if (!node_online(nid)) {
> > pgdat = hotadd_new_pgdat(nid, start);
> > if (!pgdat)
> > @@ -277,14 +293,13 @@
> > BUG_ON(ret);
> > }
> >
> > - /* register this memory as resource */
> > - ret = register_memory_resource(start, size);
> > -
> > return ret;
> > error:
> > /* rollback pgdat allocation and others */
> > if (new_pgdat)
> > rollback_node_hotadd(nid, pgdat);
> > + if (res)
> > + release_memory_resource(res);
> >
>
>
> I am trying to keep add_memory non-sparsemem specific. With reserve
> memory the memory is already present and so using
> register_memory_resource as a key to find out if it is already present
> won't work.
>
Hm, okay. looks your help is necessary.
> I can build ontop of this if you want me to. I would like to test this
> to make sure it works for me. (I will do this sometime today)
>
Thanks!
-Kame
On Fri, 2006-08-04 at 12:13 +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 03 Aug 2006 20:00:08 -0700
> keith mannthey <[email protected]> wrote:
>
>
> > > > What protecting is there for calling add_memory on an already present
> > > > memory range?
> > > >
> > > For example, considering ia64, which has 1Gbytes section...
> >
> > Maybe 1gb sections is too large?
> >
> ia64 machines sometimes to have crazy big memory...so 1gb section is requested.
> Configurable section_size for small machines was rejected in old days.
My HW supports about 512gb......
What if you add a partial section. Then online in sysfs and add another
section? messy....
>
> > > hot add following region.
> > > ==
> > > (A) 0xc0000000 - 0xd7ffffff (section 3)
> > > (B) 0xe0000000 - 0xffffffff (section 3)
> > > ==
> > > (A) and (B) will go to the same section, but there is a memory hole between
> > > (A) and (B). Considering memory (B) appears after (A) in DSDT.
> > >
> > > After add_memory() against (A) is called, section 3 is ready.
> > > Then, pfn_valid(0xe0000000) and pfn_valid(0xffffffff) returns true because
> > > they are in section 3.
> > > So, checking pfn_valid() for (B) will returns true and memory (B) cannot be
> > > added. ioresouce collision check will help this situation.
> >
> > With iommus out there throwing aliment all off way the flexability is
> > good.
> >
> > My question is this.
> >
> > Assuming 0-0xbfffffff is present.
> >
> > What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
> > to add_memory?
>
> Usual sparsemem's add_memory() checks whether there are sections in
> sparse_add_one_section(). then add_pages() returns -EEXIST (nothing to do).
> And ioresouce collision check will finally find collision because 0-0xbffffff
> resource will conflict with 0xa0000000 to 0xa10000000 area.
> But, x86_64 's (not sparsemem) add_pages() doen't do collision check, so it panics.
I have paniced with your 5 patches while doing SPARSMEM.... I think
your 6th patch address the issues I was seeing.
Thanks,
Keith
On Thu, 03 Aug 2006 20:23:46 -0700
keith mannthey <[email protected]> wrote:
> On Fri, 2006-08-04 at 12:13 +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 03 Aug 2006 20:00:08 -0700
> > keith mannthey <[email protected]> wrote:
> >
> >
> > > > > What protecting is there for calling add_memory on an already present
> > > > > memory range?
> > > > >
> > > > For example, considering ia64, which has 1Gbytes section...
> > >
> > > Maybe 1gb sections is too large?
> > >
> > ia64 machines sometimes to have crazy big memory...so 1gb section is requested.
> > Configurable section_size for small machines was rejected in old days.
>
> My HW supports about 512gb......
>
> What if you add a partial section. Then online in sysfs and add another
> section? messy....
Once a section is onlined, it cannot be re-onlined. My patch just helps memory holes
in "a" memory hot add event.
Our firmware team tells us they may create small memory holes in contiguous memory...
>
> > > What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
> > > to add_memory?
> >
> > Usual sparsemem's add_memory() checks whether there are sections in
> > sparse_add_one_section(). then add_pages() returns -EEXIST (nothing to do).
> > And ioresouce collision check will finally find collision because 0-0xbffffff
> > resource will conflict with 0xa0000000 to 0xa10000000 area.
> > But, x86_64 's (not sparsemem) add_pages() doen't do collision check, so it panics.
>
> I have paniced with your 5 patches while doing SPARSMEM.... I think
> your 6th patch address the issues I was seeing.
>
Thank you for testing.
BTW, could you send your current config ? looks I should visit source code again..
-Kame
On Fri, 2006-08-04 at 12:48 +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 03 Aug 2006 20:23:46 -0700
> keith mannthey <[email protected]> wrote:
> > > > What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
> > > > to add_memory?
> > >
> > > Usual sparsemem's add_memory() checks whether there are sections in
> > > sparse_add_one_section(). then add_pages() returns -EEXIST (nothing to do).
> > > And ioresouce collision check will finally find collision because 0-0xbffffff
> > > resource will conflict with 0xa0000000 to 0xa10000000 area.
> > > But, x86_64 's (not sparsemem) add_pages() doen't do collision check, so it panics.
> >
> > I have paniced with your 5 patches while doing SPARSMEM.... I think
> > your 6th patch address the issues I was seeing.
> >
with the 6 patches things work as expected. It is nice to have the
sysfs devices online the correct amount of memory.
I was broken without this patch because invalid add_memory calls are
made on by box (yet another issue) during boot.
I will build my patch set on top of your 6 patches.
Thanks,
Keith
On Thu, 03 Aug 2006 21:25:34 -0700
keith mannthey <[email protected]> wrote:
> On Fri, 2006-08-04 at 12:48 +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 03 Aug 2006 20:23:46 -0700
> > keith mannthey <[email protected]> wrote:
>
> > > > > What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
> > > > > to add_memory?
> > > >
> > > > Usual sparsemem's add_memory() checks whether there are sections in
> > > > sparse_add_one_section(). then add_pages() returns -EEXIST (nothing to do).
> > > > And ioresouce collision check will finally find collision because 0-0xbffffff
> > > > resource will conflict with 0xa0000000 to 0xa10000000 area.
> > > > But, x86_64 's (not sparsemem) add_pages() doen't do collision check, so it panics.
> > >
> > > I have paniced with your 5 patches while doing SPARSMEM.... I think
> > > your 6th patch address the issues I was seeing.
> > >
>
>
> with the 6 patches things work as expected. It is nice to have the
> sysfs devices online the correct amount of memory.
>
> I was broken without this patch because invalid add_memory calls are
> made on by box (yet another issue) during boot.
>
> I will build my patch set on top of your 6 patches.
>
Okay, thank you very much !
-Kame
> On Thu, 03 Aug 2006 20:23:46 -0700
> keith mannthey <[email protected]> wrote:
>
> > On Fri, 2006-08-04 at 12:13 +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu, 03 Aug 2006 20:00:08 -0700
> > > keith mannthey <[email protected]> wrote:
> > >
> > >
> > > > > > What protecting is there for calling add_memory on an already present
> > > > > > memory range?
> > > > > >
> > > > > For example, considering ia64, which has 1Gbytes section...
> > > >
> > > > Maybe 1gb sections is too large?
> > > >
> > > ia64 machines sometimes to have crazy big memory...so 1gb section is requested.
> > > Configurable section_size for small machines was rejected in old days.
> >
> > My HW supports about 512gb......
> >
>
> > What if you add a partial section. Then online in sysfs and add another
> > section? messy....
> Once a section is onlined, it cannot be re-onlined. My patch just helps memory holes
> in "a" memory hot add event.
> Our firmware team tells us they may create small memory holes in contiguous memory...
I would like to mention about it more.
We asked not to make memory hole to firmware team before.
But, conclusion became NO.
Because this hole is made for memory partial broken case.
Current ACPI doesn't have any spec to tell which memory area is
broken. So, _CRS must return address range of only sane memory area.
In addition, partial broken area should be smaller as much as possible.
This is why he mention about memory hole.
BTW, I prefer that we should fix only bug at this time for 2.6.18.
But, I really confusing that current patches are for only bug fix or
including for small memory hole case.
IIRC, small memory hole need more works. So, it should be 2.6.19
or later. Right?
Kame-san, could you divide between just fix patch and considering
small hole case? Or all of patches are for only bug fix?
Bye.
--
Yasunori Goto
On Fri, 04 Aug 2006 14:46:53 +0900
Yasunori Goto <[email protected]> wrote:
> BTW, I prefer that we should fix only bug at this time for 2.6.18.
> But, I really confusing that current patches are for only bug fix or
> including for small memory hole case.
> IIRC, small memory hole need more works. So, it should be 2.6.19
> or later. Right?
> Kame-san, could you divide between just fix patch and considering
> small hole case? Or all of patches are for only bug fix?
>
Hm.
patch [1/5], [2/5], [3/5], [5/5] are *necessary* bug fixes.
patch [4/5] is for memory hot add with small chunks.
But in other view, it's bug that only the first memory chunk can be added
at hot-add if there is small memory hole in a section
Thanks,
-Kame
keith mannthey wrote:
> On Fri, 2006-08-04 at 12:48 +0900, KAMEZAWA Hiroyuki wrote:
>
>> On Thu, 03 Aug 2006 20:23:46 -0700
>> keith mannthey <[email protected]> wrote:
>>
>
>
>>>>> What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
>>>>> to add_memory?
>>>>>
>>>> Usual sparsemem's add_memory() checks whether there are sections in
>>>> sparse_add_one_section(). then add_pages() returns -EEXIST (nothing to do).
>>>> And ioresouce collision check will finally find collision because 0-0xbffffff
>>>> resource will conflict with 0xa0000000 to 0xa10000000 area.
>>>> But, x86_64 's (not sparsemem) add_pages() doen't do collision check, so it panics.
>>>>
>>> I have paniced with your 5 patches while doing SPARSMEM.... I think
>>> your 6th patch address the issues I was seeing.
>>>
>>>
>
>
> with the 6 patches things work as expected. It is nice to have the
> sysfs devices online the correct amount of memory.
>
> I was broken without this patch because invalid add_memory calls are
> made on by box (yet another issue) during boot.
>
> I will build my patch set on top of your 6 patches.
>
> Thanks,
> Keith
>
>
Keith, are you working on the reserve hotadd case? It looks really
broken, at the same time we both assume the hot add region contains RAM
per e820 (use of reserve_bootmem_node()) and at the same time in other
places (in reserve_hotadd()) that it may not contain RAM. And
nodes_cover_memory() is broken no matter what we assume.
--Mika
On Fri, 2006-08-04 at 11:23 +0300, Mika Penttilä wrote:
> keith mannthey wrote:
> > On Fri, 2006-08-04 at 12:48 +0900, KAMEZAWA Hiroyuki wrote:
> >
> >> On Thu, 03 Aug 2006 20:23:46 -0700
> >> keith mannthey <[email protected]> wrote:
> >>
> >
> >
> >>>>> What keeps 0xa0000000 to 0xa1000000 from being re-onlined by a bad call
> >>>>> to add_memory?
> >>>>>
> >>>> Usual sparsemem's add_memory() checks whether there are sections in
> >>>> sparse_add_one_section(). then add_pages() returns -EEXIST (nothing to do).
> >>>> And ioresouce collision check will finally find collision because 0-0xbffffff
> >>>> resource will conflict with 0xa0000000 to 0xa10000000 area.
> >>>> But, x86_64 's (not sparsemem) add_pages() doen't do collision check, so it panics.
> >>>>
> >>> I have paniced with your 5 patches while doing SPARSMEM.... I think
> >>> your 6th patch address the issues I was seeing.
> >>>
> >>>
> >
> >
> > with the 6 patches things work as expected. It is nice to have the
> > sysfs devices online the correct amount of memory.
> >
> > I was broken without this patch because invalid add_memory calls are
> > made on by box (yet another issue) during boot.
> >
> > I will build my patch set on top of your 6 patches.
> >
> > Thanks,
> > Keith
> >
> >
> Keith, are you working on the reserve hotadd case? It looks really
> broken, at the same time we both assume the hot add region contains RAM
> per e820 (use of reserve_bootmem_node()) and at the same time in other
> places (in reserve_hotadd()) that it may not contain RAM. And
> nodes_cover_memory() is broken no matter what we assume.
I am working that right now... There is handful of things that need
cleaned up with RESERVE. I am about 1 patch away for a patchset that
make both SPARSEMEM and RESERVE hot-add work on x86_64. It should be out
soon.
Reserve is in a non-compile state as it stands with 2.6.18.
Thanks,
Keith
On Fri, 2006-08-04 at 11:32 +0900, KAMEZAWA Hiroyuki wrote:
> Okay... here is 6/5 patch..
>
> On Fri, 4 Aug 2006 11:15:50 +0900
> KAMEZAWA Hiroyuki <[email protected]> wrote:
> > > > Maybe moving ioresouce collision check in early stage of add_memory() is good ?
> > > Yea. I am working a a full patch set for but my sparsemem and reserve
> > > add-based paths. It creates a valid_memory_add_range call at the start
> > > of add_memory. I should be posting the set in the next few hours.
> > >
> > Ah..ok. but I wrote my own patch...and testing it now..
> >
>
> This patch passed test with ia64. based on 5 patches already sent.
> plz check.
Looks good to me. Tested as works x86_64. It is required for the 4th
patch of this series.
Acked-by: Keith Mannthey <[email protected]>