V3 Implement crashkernel=auto

This patch implements shrinking the reserved memory for crash kernel, if it is more than enough.

For example, if you have already reserved 128M, now you just want 100M, you can do:

# echo $((100*1024*1024)) > /sys/kernel/kexec_crash_size

Signed-off-by WANG Cong <[email protected]>

Cc Neil Horman <[email protected]>

Cc Eric W. Biederman <[email protected]>

Cc Andi Kleen <[email protected]>

---

Index: linux-2.6/include/linux/kexec.h =================================> --- linux-2.6.orig/include/linux/kexec.h +++ linux-2.6/include/linux/kexec.h

(quoted diff) +3 -0

@@ -206,6 +206,9 @@ extern size_t vmcoreinfo_max_size;
 
 int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
+int shrink_crash_memory(unsigned long new_size);
+int kexec_crash_kernel_loaded(void);
+size_t get_crash_memory_size(void);
 
 #else /* !CONFIG_KEXEC */
 struct pt_regs;

Index: linux-2.6/kernel/kexec.c =================================> --- linux-2.6.orig/kernel/kexec.c +++ linux-2.6/kernel/kexec.c

(quoted diff) +6 -0

+	size = crashk_res.end - crashk_res.start + 1;
+	mutex_unlock(&kexec_mutex);
+	return size;
+}
+
+int shrink_crash_memory(unsigned long new_size)

(quoted diff) +15 -0

+	start = crashk_res.start;
+	end = crashk_res.end;
+
+	if (new_size >= end - start + 1) {
+		ret = -EINVAL;
+		if (new_size = end - start + 1)
+			ret = 0;
+		goto unlock;
+	}
+
+	start = roundup(start, PAGE_SIZE);
+	end = roundup(start + new_size, PAGE_SIZE) - 1;
+	npages = (end + 1 - start ) / PAGE_SIZE;
+
+	pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);

(quoted diff) +13 -0

+	if (!vaddr) {
+		ret = -ENOMEM;
+		goto free;
+	}
+	crashk_res.end = end;
+
+free:
+	kfree(pages);
+unlock:
+	mutex_unlock(&kexec_mutex);
+	return ret;
+}
+
 static u32 *append_elf_note(u32 *buf, char *name, unsigned type, void *data,
 			    size_t data_len)
 {

Index: linux-2.6/kernel/ksysfs.c =================================> --- linux-2.6.orig/kernel/ksysfs.c +++ linux-2.6/kernel/ksysfs.c

(quoted diff) +21 -0

@@ -100,6 +100,26 @@ static ssize_t kexec_crash_loaded_show(s
 }
 KERNEL_ATTR_RO(kexec_crash_loaded);
 
+static ssize_t kexec_crash_size_show(struct kobject *kobj,
+				       struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%lu\n", get_crash_memory_size());
+}
+static ssize_t kexec_crash_size_store(struct kobject *kobj,
+				   struct kobj_attribute *attr,
+				   const char *buf, size_t count)
+{
+	unsigned long cnt;
+	int ret;
+
+	if (kexec_crash_kernel_loaded())
+		return -ENOENT;
+	cnt = simple_strtoul(buf, NULL, 10);
+	ret = shrink_crash_memory(cnt);
+	return ret < 0 ? ret: count;
+}
+KERNEL_ATTR_RW(kexec_crash_size);
+
 static ssize_t vmcoreinfo_show(struct kobject *kobj,
 			       struct kobj_attribute *attr, char *buf)
 {

@@ -147,6 +167,7 @@ static struct attribute * kernel_attrs[]
 #ifdef CONFIG_KEXEC
 	&kexec_loaded_attr.attr,
 	&kexec_crash_loaded_attr.attr,
+	&kexec_crash_size_attr.attr,
 	&vmcoreinfo_attr.attr,
 #endif
 	NULL

Quoted 6 lines · Amerigo Wang

This patch implements shrinking the reserved memory for crash kernel, if it is more than enough.

For example, if you have already reserved 128M, now you just want 100M, you can do:

Quoted 2 lines

# echo $((100*1024*1024)) > /sys/kernel/kexec_crash_size

Getting closer (comments inline)

Semantically this patch is non-contriversial and pretty simple, but still needs a fair amount of review. Can you put this patch at the front of your patch set.

Quoted 8 lines

Index: linux-2.6/kernel/kexec.c
===================================================================

kernel/kexec.c +0 -0

@@ -1083,6 +1083,76 @@ void crash_kexec(struct pt_regs *regs)
 	}
 }
 

kernel/kexec.c void crash_kexec(struct pt_regs *regs) · line 1091

Quoted 6 lines

kernel/kexec.c +5 -0

+int kexec_crash_kernel_loaded(void)
+{
+	int ret;
+	if (!mutex_trylock(&kexec_mutex))
+		return 1;
 

We don't need trylock on this code path

kernel/kexec.c void crash_kexec(struct pt_regs *regs) · line 1102

Quoted 12 lines

kernel/kexec.c +10 -0

+	ret = kexec_crash_image != NULL;
+	mutex_unlock(&kexec_mutex);
+	return ret;
+}
+
+size_t get_crash_memory_size(void)
+{
+	size_t size;
+	if (!mutex_trylock(&kexec_mutex))
+		return 1;
 

We don't need trylock on this code path

Quoted 7 lines

kernel/kexec.c +7 -0

+	size = crashk_res.end - crashk_res.start + 1;
+	mutex_unlock(&kexec_mutex);
+	return size;
+}
+
+int shrink_crash_memory(unsigned long new_size)
+{

kernel/kexec.c void crash_kexec(struct pt_regs *regs) · line 1119

Quoted 10 lines

kernel/kexec.c +9 -0

+	struct page **pages;
+	int ret = 0;
+	int  npages, i;
+	unsigned long addr;
+	unsigned long start, end;
+	void *vaddr;
+
+	if (!mutex_trylock(&kexec_mutex))
+		return -EBUSY;
 

We don't need trylock on this code path

We are missing the check to see if the crash_kernel is loaded under this lock instance. So I please move the kexec_crash_image != NULL test inline here and kill the kexec_crash_kernel_loaded function.

Quoted 23 lines

kernel/kexec.c +22 -0

+	start = crashk_res.start;
+	end = crashk_res.end;
+
+	if (new_size >= end - start + 1) {
+		ret = -EINVAL;
+		if (new_size == end - start + 1)
+			ret = 0;
+		goto unlock;
+	}
+
+	start = roundup(start, PAGE_SIZE);
+	end = roundup(start + new_size, PAGE_SIZE) - 1;
+	npages = (end + 1 - start ) / PAGE_SIZE;
+
+	pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);
+	if (!pages) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+	for (i = 0; i < npages; i++) {
+		addr = end + 1 + i * PAGE_SIZE;
+		pages[i] = virt_to_page(addr);

kernel/kexec.c void crash_kexec(struct pt_regs *regs) · line 1144

Quoted 4 lines

kernel/kexec.c +3 -0

+	}
+
+	vaddr = vm_map_ram(pages, npages, 0, PAGE_KERNEL);

This is the wrong kernel call to use. I expect this needs to look like a memory hotplug event. This does not put the pages into the free page pool.

Quoted 9 lines · Eric W. Biederman

Quoted 6 lines · Amerigo Wang

This patch implements shrinking the reserved memory for crash kernel, if it is more than enough.

For example, if you have already reserved 128M, now you just want 100M, you can do:

Quoted 10 lines

Quoted 2 lines

# echo $((100*1024*1024)) > /sys/kernel/kexec_crash_size

Getting closer (comments inline)

Semantically this patch is non-contriversial and pretty simple, but still needs a fair amount of review. Can you put this patch at the front of your patch set.

Sure, I will do it when I resend them next time.

I add mm people into Cc.

Quoted 6 lines

Quoted 6 lines

Index: linux-2.6/kernel/kexec.c =================================>>> --- linux-2.6.orig/kernel/kexec.c +++ linux-2.6/kernel/kexec.c

(quoted diff) +0 -0

@@ -1083,6 +1083,76 @@ void crash_kexec(struct pt_regs *regs)
 	}
 }

Quoted 8 lines

Review void crash_kexec(struct pt_regs *regs) · line 1090

Quoted 6 lines

(quoted diff) +4 -0

 +int kexec_crash_kernel_loaded(void)
+{
+	int ret;
+	if (!mutex_trylock(&kexec_mutex))
+		return 1;
 

We don't need trylock on this code path

OK.

Quoted 16 lines

Review void crash_kexec(struct pt_regs *regs) · line 1101

Quoted 12 lines

(quoted diff) +10 -0

+	ret = kexec_crash_image != NULL;
+	mutex_unlock(&kexec_mutex);
+	return ret;
+}
+
+size_t get_crash_memory_size(void)
+{
+	size_t size;
+	if (!mutex_trylock(&kexec_mutex))
+		return 1;
 

We don't need trylock on this code path

Hmm, crashk_res is a global struct, so other process can also change it... but currently no process does that, right?

Quoted 20 lines

Quoted 7 lines

(quoted diff) +7 -0

+	size = crashk_res.end - crashk_res.start + 1;
+	mutex_unlock(&kexec_mutex);
+	return size;
+}
+
+int shrink_crash_memory(unsigned long new_size)
+{

Review void crash_kexec(struct pt_regs *regs) · line 1118

Quoted 10 lines

(quoted diff) +9 -0

+	struct page **pages;
+	int ret = 0;
+	int  npages, i;
+	unsigned long addr;
+	unsigned long start, end;
+	void *vaddr;
+
+	if (!mutex_trylock(&kexec_mutex))
+		return -EBUSY;
 

We don't need trylock on this code path

Quoted 4 lines

We are missing the check to see if the crash_kernel is loaded under this lock instance. So I please move the kexec_crash_image != NULL test inline here and kill the kexec_crash_kernel_loaded function.

Ok, no problem.

Quoted 23 lines

Quoted 23 lines

(quoted diff) +22 -0

+	start = crashk_res.start;
+	end = crashk_res.end;
+
+	if (new_size >= end - start + 1) {
+		ret = -EINVAL;
+		if (new_size = end - start + 1)
+			ret = 0;
+		goto unlock;
+	}
+
+	start = roundup(start, PAGE_SIZE);
+	end = roundup(start + new_size, PAGE_SIZE) - 1;
+	npages = (end + 1 - start ) / PAGE_SIZE;
+
+	pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);
+	if (!pages) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+	for (i = 0; i < npages; i++) {
+		addr = end + 1 + i * PAGE_SIZE;
+		pages[i] = virt_to_page(addr);

Quoted 9 lines

Review void crash_kexec(struct pt_regs *regs) · line 1143

Quoted 4 lines

(quoted diff) +3 -0

+	}
+
+	vaddr = vm_map_ram(pages, npages, 0, PAGE_KERNEL);

This is the wrong kernel call to use. I expect this needs to look like a memory hotplug event. This does not put the pages into the free page pool.

Well, I also wanted to use an memory-hotplug API, but that will make the code depend on memory-hotplug, which certainly is not what we want...

I checked the mm code, actually what I need is an API which is similar to add_active_range(), but add_active_range() can't be used here since it is marked as "__init".

Do we have that kind of API in mm? I can't find one.

Quoted 18 lines · Amerigo Wang

Quoted 18 lines

Quoted 4 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+	ret = kexec_crash_image != NULL;
+	mutex_unlock(&kexec_mutex);
+	return ret;
+}
+
+size_t get_crash_memory_size(void)
+{
+	size_t size;
+	if (!mutex_trylock(&kexec_mutex))
+		return 1;

We don't need trylock on this code path

Quoted 4 lines

Hmm, crashk_res is a global struct, so other process can also change it... but currently no process does that, right?

We still need the lock. Just doing trylock doesn't instead of just sleeping doesn't seem to make any sense on these code paths.

Quoted 41 lines

Quoted 24 lines

Quoted 24 lines

(quoted diff) +22 -0

+	start = crashk_res.start;
+	end = crashk_res.end;
+
+	if (new_size >= end - start + 1) {
+		ret = -EINVAL;
+		if (new_size == end - start + 1)
+			ret = 0;
+		goto unlock;
+	}
+
+	start = roundup(start, PAGE_SIZE);
+	end = roundup(start + new_size, PAGE_SIZE) - 1;
+	npages = (end + 1 - start ) / PAGE_SIZE;
+
+	pages = kmalloc(sizeof(struct page *) * npages, GFP_KERNEL);
+	if (!pages) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+	for (i = 0; i < npages; i++) {
+		addr = end + 1 + i * PAGE_SIZE;
+		pages[i] = virt_to_page(addr);

Quoted 10 lines

Review

Quoted 5 lines

(quoted diff) +3 -0

+	}
+
+	vaddr = vm_map_ram(pages, npages, 0, PAGE_KERNEL);

This is the wrong kernel call to use. I expect this needs to look like a memory hotplug event. This does not put the pages into the free page pool.

Well, I also wanted to use an memory-hotplug API, but that will make the code depend on memory-hotplug, which certainly is not what we want...

I checked the mm code, actually what I need is an API which is similar to add_active_range(), but add_active_range() can't be used here since it is marked as "__init".

Quoted 2 lines

Do we have that kind of API in mm? I can't find one.

Perhaps we will need to remove __init from add_active_range. I know the logic but I'm not up to speed on the mm pieces at the moment.

Not that simple, marking it as "__init" means it uses some "__init" data which will be dropped after initialization.

Quoted 3 lines · Amerigo Wang

Not that simple, marking it as "__init" means it uses some "__init" data which will be dropped after initialization.

If we start with the assumption that we will be reserving to much and will free the memory once we know how much we really need I see a very simple way to go about this. We ensure that the reservation of crash kernel memory is done through a normal allocation so that we have struct page entries for every page. On 32bit x86 that is an extra 1MB for a 128MB allocation.

Then when it comes time to release that memory we clear whatever magic flags we have on the page (like PG_reserve) and call free_page.

Quoted 14 lines · Eric W. Biederman

Quoted 3 lines · Amerigo Wang

Not that simple, marking it as "__init" means it uses some "__init" data which will be dropped after initialization.

If we start with the assumption that we will be reserving to much and will free the memory once we know how much we really need I see a very simple way to go about this. We ensure that the reservation of crash kernel memory is done through a normal allocation so that we have struct page entries for every page. On 32bit x86 that is an extra 1MB for a 128MB allocation.

Quoted 3 lines

Then when it comes time to release that memory we clear whatever magic flags we have on the page (like PG_reserve) and call free_page.

Hmm, my MM knowledge is not good enough to judge if this works... I need to check more MM source code.

Can any MM people help?

Quoted 21 lines · On Mon, 17 Aug 2009 17:50:21 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 19 lines · Eric W. Biederman

Quoted 4 lines · Amerigo Wang

Not that simple, marking it as "__init" means it uses some "__init" data which will be dropped after initialization.

If we start with the assumption that we will be reserving to much and will free the memory once we know how much we really need I see a very simple way to go about this. We ensure that the reservation of crash kernel memory is done through a normal allocation so that we have struct page entries for every page. On 32bit x86 that is an extra 1MB for a 128MB allocation.

Then when it comes time to release that memory we clear whatever magic flags we have on the page (like PG_reserve) and call free_page.

Quoted 6 lines

Hmm, my MM knowledge is not good enough to judge if this works... I need to check more MM source code.

Can any MM people help?

Hm, memory-hotplug guy is here.

Can I have a question ?

- How crash kernel's memory is preserved at boot ?

It's hidden from the system before mem_init() ?

Hi, thank you!

Quoted 4 lines

Can I have a question ?

- How crash kernel's memory is preserved at boot ?

Use bootmem, I think.

Quoted 2 lines

It's hidden from the system before mem_init() ?

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Quoted 10 lines · On Tue, 18 Aug 2009 14:31:23 +0800 Amerigo Wang <[email protected]> wrote:

Hi, thank you!

Quoted 5 lines

Can I have a question ?

- How crash kernel's memory is preserved at boot ?

Use bootmem, I think.

I see.

In x86,

setup_arch()

	-> reserve_crashkernel()
		-> find_and_reserve_crashkernel()
			-> reserve_bootmem_generic()

Then, all "active range" is already registered and there are memmap.

Quoted 8 lines

Quoted 3 lines

It's hidden from the system before mem_init() ?

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Let's see arch/x86/mm/init.c::free_initmem()

Maybe it's all you want.

- ClearPageReserved()
- init_page_count()
- free_page()
- totalram_pages++

But it has no argumetns. Maybe you need your own function or modification. online_pages() does very similar. But, hmm,.. writing something open coded one for crashkernel is not very bad, I think.

Quoted 9 lines · On Tue, 18 Aug 2009 14:31:23 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 3 lines

It's hidden from the system before mem_init() ?

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Let's see arch/x86/mm/init.c::free_initmem()

Maybe it's all you want.

- ClearPageReserved()
- init_page_count()
- free_page()
- totalram_pages++

Quoted 13 lines · KAMEZAWA Hiroyuki

Quoted 9 lines · On Tue, 18 Aug 2009 14:31:23 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 3 lines

It's hidden from the system before mem_init() ?

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Quoted 9 lines

Let's see arch/x86/mm/init.c::free_initmem()

Maybe it's all you want.

- ClearPageReserved()
- init_page_count()
- free_page()
- totalram_pages++

Just FYI: calling ClearPageReserved() caused an oops: "Unable to handle paging request".

I am trying to figure out why...

Quoted 26 lines · On Tue, 18 Aug 2009 18:35:32 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 24 lines · KAMEZAWA Hiroyuki

Quoted 10 lines · On Tue, 18 Aug 2009 14:31:23 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 4 lines

It's hidden from the system before mem_init() ?

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Let's see arch/x86/mm/init.c::free_initmem()

Maybe it's all you want.

- ClearPageReserved()
- init_page_count()
- free_page()
- totalram_pages++

Quoted 5 lines

Just FYI: calling ClearPageReserved() caused an oops: "Unable to handle paging request".

I am trying to figure out why...

Hmm...then....memmap is not there. pfn_valid() check will help you. What arch ? x86-64 ?

Quoted 37 lines · KAMEZAWA Hiroyuki

Quoted 33 lines · On Tue, 18 Aug 2009 18:35:32 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 15 lines · KAMEZAWA Hiroyuki

Quoted 11 lines · On Tue, 18 Aug 2009 14:31:23 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 4 lines

It's hidden from the system before mem_init() ?

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Quoted 10 lines

Let's see arch/x86/mm/init.c::free_initmem()

Maybe it's all you want.

- ClearPageReserved()
- init_page_count()
- free_page()
- totalram_pages++

Just FYI: calling ClearPageReserved() caused an oops: "Unable to handle paging request".

I am trying to figure out why...

Quoted 3 lines

Hmm...then....memmap is not there. pfn_valid() check will help you. What arch ? x86-64 ?

Hmm, yes, x86_64, but this code is arch-independent, I mean it should work or not work on all arch, no?

So I am afraid we need to use other API to free it...

Quoted 50 lines · On Wed, 19 Aug 2009 10:41:13 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 36 lines · KAMEZAWA Hiroyuki

Quoted 31 lines · On Tue, 18 Aug 2009 18:35:32 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 28 lines · KAMEZAWA Hiroyuki

Quoted 5 lines · On Tue, 18 Aug 2009 14:31:23 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 5 lines

It's hidden from the system before mem_init() ?

Quoted 7 lines

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Let's see arch/x86/mm/init.c::free_initmem()

Maybe it's all you want.

- ClearPageReserved()
- init_page_count()
- free_page()
- totalram_pages++

Quoted 10 lines

Quoted 6 lines

Just FYI: calling ClearPageReserved() caused an oops: "Unable to handle paging request".

I am trying to figure out why...

Hmm...then....memmap is not there. pfn_valid() check will help you. What arch ? x86-64 ?

Hmm, yes, x86_64, but this code is arch-independent, I mean it should work or not work on all arch, no?

Quoted 3 lines

So I am afraid we need to use other API to free it...

The, problem is whether memmap is there or not. That's all. plz see init sequence and check there are memmap. If memory-for-crash is obtained via bootmem,
Don't you try to free memory hole ?

Quoted 50 lines · On Wed, 19 Aug 2009 10:41:13 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 36 lines · KAMEZAWA Hiroyuki

Quoted 31 lines · On Tue, 18 Aug 2009 18:35:32 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 28 lines · KAMEZAWA Hiroyuki

Quoted 5 lines · On Tue, 18 Aug 2009 14:31:23 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 5 lines

It's hidden from the system before mem_init() ?

Quoted 7 lines

Not sure, but probably yes. It is reserved in setup_arch() which is before mm_init() which calls mem_init().

Do you have any advice to free that reserved memory after boot? :)

Let's see arch/x86/mm/init.c::free_initmem()

Maybe it's all you want.

- ClearPageReserved()
- init_page_count()
- free_page()
- totalram_pages++

Quoted 10 lines

Quoted 6 lines

Just FYI: calling ClearPageReserved() caused an oops: "Unable to handle paging request".

I am trying to figure out why...

Hmm...then....memmap is not there. pfn_valid() check will help you. What arch ? x86-64 ?

Hmm, yes, x86_64, but this code is arch-independent, I mean it should work or not work on all arch, no?

Quoted 3 lines

So I am afraid we need to use other API to free it...

The, problem is whether memmap is there or not. That's all. plz see init sequence and check there are memmap. If memory-for-crash is obtained via bootmem,
Don't you try to free memory hole ?

Quoted 5 lines

The, problem is whether memmap is there or not. That's all. plz see init sequence and check there are memmap. If memory-for-crash is obtained via bootmem,
Don't you try to free memory hole ?

Hi,

It looks like that mem_map has 'struct page' for the reserved memory, I checked my "early_node_map[] active PFN ranges" output, the reserved memory area for crash kernel is right in one range. Am I missing something here?

I don't know why that oops comes out, maybe because of no PTE for thoese pages?

Quoted 13 lines · On Thu, 20 Aug 2009 17:15:56 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 5 lines

The, problem is whether memmap is there or not. That's all. plz see init sequence and check there are memmap. If memory-for-crash is obtained via bootmem,
Don't you try to free memory hole ?

Hi,

It looks like that mem_map has 'struct page' for the reserved memory, I checked my "early_node_map[] active PFN ranges" output, the reserved memory area for crash kernel is right in one range. Am I missing something here?

Quoted 3 lines

I don't know why that oops comes out, maybe because of no PTE for thoese pages?

Hmm ? Could you show me the code you use ?

Quoted 16 lines · KAMEZAWA Hiroyuki

Quoted 13 lines · On Thu, 20 Aug 2009 17:15:56 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 5 lines

The, problem is whether memmap is there or not. That's all. plz see init sequence and check there are memmap. If memory-for-crash is obtained via bootmem,
Don't you try to free memory hole ?

Hi,

It looks like that mem_map has 'struct page' for the reserved memory, I checked my "early_node_map[] active PFN ranges" output, the reserved memory area for crash kernel is right in one range. Am I missing something here?

Quoted 4 lines

Quoted 3 lines

I don't know why that oops comes out, maybe because of no PTE for thoese pages?

Hmm ? Could you show me the code you use ?

(Sorry that I reply to you with my gmail, my work email can't send out this message, probably because one of the destinations is broken... I am the same person, don't be confused. :-)

Sure. Below is it:

(quoted diff) +7 -0

+    for (addr = end + 1; addr < crashk_res.end; addr += PAGE_SIZE) {
+        printk(KERN_DEBUG "PFN is valid? %d\n", pfn_valid(addr>>PAGE_SHIFT));
+        ClearPageReserved(virt_to_page(addr));
+        init_page_count(virt_to_page(addr));
+        free_page(addr);
+        totalram_pages++;
+    }

pfn_valid() returns 1, and oops happens at ClearPageReserved(). ('addr' is right between crashk_res.start and crashk_res.end)

Thank you!

Quoted 37 lines · On Fri, 21 Aug 2009 09:59:27 +0800 Amerigo Wang <[email protected]>…

Quoted 16 lines · KAMEZAWA Hiroyuki

Quoted 13 lines · On Thu, 20 Aug 2009 17:15:56 +0800 Amerigo Wang <[email protected]> wrote:

Quoted 5 lines

The, problem is whether memmap is there or not. That's all. plz see init sequence and check there are memmap. If memory-for-crash is obtained via bootmem,
Don't you try to free memory hole ?

Hi,

It looks like that mem_map has 'struct page' for the reserved memory, I checked my "early_node_map[] active PFN ranges" output, the reserved memory area for crash kernel is right in one range. Am I missing something here?

Quoted 4 lines

Quoted 3 lines

I don't know why that oops comes out, maybe because of no PTE for thoese pages?

Hmm ? Could you show me the code you use ?

(Sorry that I reply to you with my gmail, my work email can't send out this message, probably because one of the destinations is broken... I am the same person, don't be confused. :-)

Sure. Below is it:

(quoted diff) +7 -0

+    for (addr = end + 1; addr < crashk_res.end; addr += PAGE_SIZE) {
+        printk(KERN_DEBUG "PFN is valid? %d\n", pfn_valid(addr>>PAGE_SHIFT));
+        ClearPageReserved(virt_to_page(addr));
+        init_page_count(virt_to_page(addr));
+        free_page(addr);
+        totalram_pages++;
+    }

Quoted 2 lines

pfn_valid() returns 1, and oops happens at ClearPageReserved(). ('addr' is right between crashk_res.start and crashk_res.end)

Confused,
if pfn_valid(addr >> PAGE_SHIFT) == true

you should do

	ClearPageReserved(pfn_to_page(addr >> PAGE_SHIFT));

because addr is physical address, not virtual.
I guess crashk_res.end is physical address....

This series of patch implements automatically reserved memory for crashkernel, by introducing a new boot option "crashkernel=auto". This idea is from Neil.

Quoted 3 lines · Amerigo Wang

This series of patch implements automatically reserved memory for crashkernel, by introducing a new boot option "crashkernel=auto". This idea is from Neil.

Honestly I don't see why everything is guarded by CONFIG_KEXEC_AUTO_RESERVE. We do we need that new configuration option? I mean, if I don't specify 'crashkernel=auto', then the patch does nothing, right? Then the option CONFIG_KEXEC_AUTO_RESERVE would only be needed so save some bytes of code. Is that really worth it?

Quoted 6 lines · Bernhard Walle

Honestly I don't see why everything is guarded by CONFIG_KEXEC_AUTO_RESERVE. We do we need that new configuration option? I mean, if I don't specify 'crashkernel=auto', then the patch does nothing, right? Then the option CONFIG_KEXEC_AUTO_RESERVE would only be needed so save some bytes of code. Is that really worth it?

Hi, CONFIG_KEXEC_AUTO_RESERVE is not for saving bytes, it just provides a choice for the user to decide to enable it or not.

Hi,

Quoted 8 lines · Amerigo Wang

Quoted 6 lines · Bernhard Walle

Honestly I don't see why everything is guarded by CONFIG_KEXEC_AUTO_RESERVE. We do we need that new configuration option? I mean, if I don't specify 'crashkernel=auto', then the patch does nothing, right? Then the option CONFIG_KEXEC_AUTO_RESERVE would only be needed so save some bytes of code. Is that really worth it?

Quoted 3 lines

Hi, CONFIG_KEXEC_AUTO_RESERVE is not for saving bytes, it just provides a choice for the user to decide to enable it or not.

Still, I don't understand it. When I don't say "crashkernel=auto" on command line, then nothing is done, right? So the choice for the user is the "crashkernel=auto". Why do we need CONFIG_KEXEC_AUTO_RESERVE then? Maybe I just missed something in my logic ...

Sure.

But if we disable CONFIG_KEXEC_AUTO_RESERVE, that means crashkernel=auto will be invalid, this is the same as it is now.

Quoted 4 lines · Amerigo Wang

Sure.

But if we disable CONFIG_KEXEC_AUTO_RESERVE, that means crashkernel=auto will be invalid, this is the same as it is now.

Ok, but since 'crashkernel=auto' is not used today, nobody has 'crashkernel=auto' in the bootloader configuration. So I don't see any practial advantage of that config option.

Eric, what's your opinion on that, do we need a config option CONFIG_KEXEC_AUTO_RESERVE or could we just implement that feature unconditionally (if CONFIG_KEXEC is enabled, of course).

Quoted 11 lines · Bernhard Walle

Quoted 5 lines · Amerigo Wang

Sure.

But if we disable CONFIG_KEXEC_AUTO_RESERVE, that means crashkernel=auto will be invalid, this is the same as it is now.

Ok, but since 'crashkernel=auto' is not used today, nobody has 'crashkernel=auto' in the bootloader configuration. So I don't see any practial advantage of that config option.

Quoted 4 lines

Eric, what's your opinion on that, do we need a config option CONFIG_KEXEC_AUTO_RESERVE or could we just implement that feature unconditionally (if CONFIG_KEXEC is enabled, of course).

The only reason I can see the option going away would be a dependency on CONFIG_HOTPLUG_MEM.