Fix --mem-usage /proc/kcore

PT_LOAD of kcore does not have valid p_paddr values for kernel version less that v4.11. Therefore, older kernel will no long work for mem-usage with current makedumpfile code. They can only work when they are patched with fix to "update physical address for kcore ram and text".

This patch fixes the makedumpfile so that it does not allow to work older kernel for --mem-usage until someone is sure that kernel is rightly patched and so uses -f in command line. It also updates man page and usage info accordingly.

Signed-off-by Pratyush Anand <[email protected]>

---

makedumpfile.8 | 9 ++++++++-
makedumpfile.c | 6 ++++++
print_info.c   | 3 ++-
3 files changed, 16 insertions(+), 2 deletions(-)

makedumpfile.8 +0 -0

index 9069fb18cdb6..993236486e77 100644
@@ -235,13 +235,20 @@ the ELF format does not support compressed data.
 

.TP
\fB\-f\fR

makedumpfile.8 +2 -1

-Force existing DUMPFILE to be overwritten.
+Force existing DUMPFILE to be overwritten and mem-usage to work with older
+kernel as well.

.br
.B Example:
.br

# makedumpfile \-f \-d 31 \-x vmlinux /proc/vmcore dumpfile
.br
This command overwrites \fIDUMPFILE\fR even if it already exists.
+.br

makedumpfile.8 +5 -0

+# makedumpfile \-f \-\-mem\-usage /proc/kcore
+.br
+Kernel version lesser than v4.11 will not work with \-\-mem\-usage
+functionality until it has been patched with upstream commit 464920104bf7.
+Therefore if you have patched your older kernel then use \-f.

.TP
\fB\-x\fR \fIVMLINUX\fR

makedumpfile.c +0 -0

index 3b8e9810468d..e3be1ab0a9ec 100644

makedumpfile.c +0 -0

 		if (!show_mem_usage())
 			goto out;

print_info.c +2 -2

index 392d863a4227..72ed8fa0c059 100644
@@ -309,7 +309,8 @@ print_usage(void)
 	MSG("      Print debugging message.\n");
 	MSG("\n");
 	MSG("  [-f]:\n");
-	MSG("      Overwrite DUMPFILE even if it already exists.\n");
+	MSG("      Overwrite DUMPFILE even if it already exists\n");
+	MSG("      Force mem-usage to work with older kernel as well.\n");
 	MSG("\n");
 	MSG("  [-h, --help]:\n");
 	MSG("      Show help message and LZO/snappy support status (enabled/disabled).\n");
--

2.9.3

makedumpfile.c main(int argc, char *argv[]) · line 11277

Quoted 6 lines

makedumpfile.c +6 -0

+		if (info->kernel_version < KERNEL_VERSION(4, 11, 0) &&
+				!info->flag_force) {
+			MSG("mem-usage not supported for this kernel.\n");
+			MSG("You can try with -f if your kernel's kcore has valid p_paddr\n");
+			goto out;
+		}

You forgot to set COMPLETED to retcd before goto.
The behavior will be different from the v2 patch.

Quoted 8 lines · Atsushi Kumagai

makedumpfile.c main(int argc, char *argv[]) · line 11277

Quoted 6 lines

makedumpfile.c +6 -0

+		if (info->kernel_version < KERNEL_VERSION(4, 11, 0) &&
+				!info->flag_force) {
+			MSG("mem-usage not supported for this kernel.\n");
+			MSG("You can try with -f if your kernel's kcore has valid p_paddr\n");
+			goto out;
+		}

You forgot to set COMPLETED to retcd before goto.
The behavior will be different from the v2 patch.

I had thought about it. Should not an unsupported feature be a FAILED case?

Quoted 11 lines · Atsushi Kumagai

Quoted 8 lines

makedumpfile.c main(int argc, char *argv[]) · line 11277

Quoted 6 lines

makedumpfile.c +6 -0

+		if (info->kernel_version < KERNEL_VERSION(4, 11, 0) &&
+				!info->flag_force) {
+			MSG("mem-usage not supported for this kernel.\n");
+			MSG("You can try with -f if your kernel's kcore has valid p_paddr\n");
+			goto out;
+		}

You forgot to set COMPLETED to retcd before goto.
The behavior will be different from the v2 patch.

Quoted 1 line

I had thought about it. Should not an unsupported feature be a FAILED case?

Judging from the result, it should be a failed case. People expect to get a dumped vmcore, it failed to collect at last because of unmatched kernel and tool.

When you change the design, please note it into changelog, it's a review point of view.

Changes since v2:
- Fixed a memory leak issue and updated man page and usage info

As for the return value, I guess both is OK if there is a clear intention. If this is what you intended, I have no objection.
(it actually sounds more reasonable)

So I'll merge the v3 patch into v1.6.2, thanks for your work.

Call cach_init() before get_kcore_dump_loads(), because latter uses cache_search().

Call path is like this :
get_kcore_dump_loads() -> process_dump_load() -> vaddr_to_paddr() -> vtop4_x86_64() -> readmem() -> cache_search()

Signed-off-by Pratyush Anand <[email protected]>

---
makedumpfile.c | 6 +++---

1 file changed, 3 insertions(+), 3 deletions(-)

makedumpfile.c +3 -4

index 6942047199de..3b8e9810468d 100644
@@ -3878,6 +3878,9 @@ initial(void)
 	if (!get_value_for_old_linux())
 		return FALSE;
 
+	if (!is_xen_memory() && !cache_init())
+		return FALSE;
+
 	if (info->flag_mem_usage && !get_kcore_dump_loads())
 		return FALSE;
 

@@ -4000,9 +4003,6 @@ out:
 		}
 	}
 
-	if (!is_xen_memory() && !cache_init())
-		return FALSE;
-
 	if (debug_info && !get_machdep_info())
 		return FALSE;
 
--

2.9.3

Hello Pratyush,

Quoted 3 lines

Call cach_init() before get_kcore_dump_loads(), because latter uses cache_search().

Quoted 3 lines

Call path is like this :
get_kcore_dump_loads() -> process_dump_load() -> vaddr_to_paddr() -> vtop4_x86_64() -> readmem() -> cache_search()

I found that the current cach_init() is too early, cache_init() should be called after fallback_to_current_page_size() for older kernel which doesn't have VMCOREINFO.
Could you confirm that the patch below is fine with you ?

Thanks,
Atsushi Kumagai

From: Atsushi Kumagai <[email protected]>
Date: Wed, 19 Jul 2017 15:05:58 +0900
Subject: [PATCH] Fix the regression caused by moving cache_init() earlier

info->page_size should be set before calling cache_init() since it allocates some buffers based on info->page_size. However, cache_init() was moved before fallback_to_current_page_size() while it set info->page_size for older kernel which doesn't have VMCOREINFO.

This patch moves cache_init() after fallback_to_current_page_size() and reverts the commit: b98e6e0d2a(sadump: set info->page_size before cache_init())

Signed-off-by Atsushi Kumagai <[email protected]>

---
 makedumpfile.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

makedumpfile.c +16 -12

index a3f0697..f85003a 100644
@@ -3905,15 +3905,6 @@ initial(void)
 	if (!get_value_for_old_linux())
 		return FALSE;
 
-	if (info->flag_sadump && !set_page_size(sadump_page_size()))
-			return FALSE;
-
-	if (!is_xen_memory() && !cache_init())
-		return FALSE;
-
-	if (info->flag_mem_usage && !get_kcore_dump_loads())
-		return FALSE;
-
 	if (info->flag_refiltering) {
 		if (info->flag_elf_dumpfile) {
 			MSG("'-E' option is disable, ");

@@ -3936,6 +3927,9 @@ initial(void)
 			return FALSE;
 		}
 
+		if (!set_page_size(sadump_page_size()))
+			return FALSE;
+
 		if (!sadump_initialize_bitmap_memory())
 			return FALSE;
 

@@ -3949,9 +3943,7 @@ initial(void)
 		 * postponed until debug information becomes
 		 * available.
 		 */
-
-	} else if (!get_phys_base())
-		return FALSE;
+	}
 
 out:
 	if (!info->page_size) {

@@ -3962,6 +3954,18 @@ out:
 		if (!fallback_to_current_page_size())
 			return FALSE;
 	}
+
+	if (!is_xen_memory() && !cache_init())
+		return FALSE;
+
+	if (info->flag_mem_usage && !get_kcore_dump_loads())
+		return FALSE;
+
+	if (!info->flag_refiltering && !info->flag_sadump) {
+		if (!get_phys_base())
+			return FALSE;
+	}
+
 	if (!get_max_mapnr())
 		return FALSE;
 

Hi Atsushi,

Quoted 9 lines · Atsushi Kumagai

Hello Pratyush,

Quoted 6 lines

Call cach_init() before get_kcore_dump_loads(), because latter uses cache_search().

Call path is like this :
get_kcore_dump_loads() -> process_dump_load() -> vaddr_to_paddr() -> vtop4_x86_64() -> readmem() -> cache_search()

Quoted 4 lines

I found that the current cach_init() is too early, cache_init() should be called after fallback_to_current_page_size() for older kernel which doesn't have VMCOREINFO.
Could you confirm that the patch below is fine with you ?

Its fine and works.

Quoted 78 lines

Thanks,
Atsushi Kumagai

From: Atsushi Kumagai <[email protected]>
Date: Wed, 19 Jul 2017 15:05:58 +0900
Subject: [PATCH] Fix the regression caused by moving cache_init() earlier

info->page_size should be set before calling cache_init() since it allocates some buffers based on info->page_size. However, cache_init() was moved before fallback_to_current_page_size() while it set info->page_size for older kernel which doesn't have VMCOREINFO.

This patch moves cache_init() after fallback_to_current_page_size() and reverts the commit: b98e6e0d2a(sadump: set info->page_size before cache_init())

Signed-off-by Atsushi Kumagai <[email protected]>

---
 makedumpfile.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

makedumpfile.c +16 -12

index a3f0697..f85003a 100644
@@ -3905,15 +3905,6 @@ initial(void)
 	if (!get_value_for_old_linux())
 		return FALSE;
 
-	if (info->flag_sadump && !set_page_size(sadump_page_size()))
-			return FALSE;
-
-	if (!is_xen_memory() && !cache_init())
-		return FALSE;
-
-	if (info->flag_mem_usage && !get_kcore_dump_loads())
-		return FALSE;
-
 	if (info->flag_refiltering) {
 		if (info->flag_elf_dumpfile) {
 			MSG("'-E' option is disable, ");

@@ -3936,6 +3927,9 @@ initial(void)
 			return FALSE;
 		}
 
+		if (!set_page_size(sadump_page_size()))
+			return FALSE;
+
 		if (!sadump_initialize_bitmap_memory())
 			return FALSE;
 

@@ -3949,9 +3943,7 @@ initial(void)
 		 * postponed until debug information becomes
 		 * available.
 		 */
-
-	} else if (!get_phys_base())
-		return FALSE;
+	}
 
 out:
 	if (!info->page_size) {

@@ -3962,6 +3954,18 @@ out:
 		if (!fallback_to_current_page_size())
 			return FALSE;
 	}
+
+	if (!is_xen_memory() && !cache_init())
+		return FALSE;
+
+	if (info->flag_mem_usage && !get_kcore_dump_loads())
+		return FALSE;
+
+	if (!info->flag_refiltering && !info->flag_sadump) {
+		if (!get_phys_base())
+			return FALSE;
+	}
+

makedumpfile.c out: · line 3971

Quoted 4 lines

makedumpfile.c +0 -0

 	if (!get_max_mapnr())
 		return FALSE;
 

--
Regards
Pratyush

-----Original Message-----
From: Atsushi Kumagai [mailto:[email protected]]
Sent: Thursday, July 20, 2017 3:54 PM
To: Pratyush Anand <[email protected]>
Cc: [email protected]; Hatayama, Daisuke <[email protected]>; [email protected]; [email protected]
Subject: RE: [Makedumpfile Patch v3 2/7] initial(): call cach_init() a bit early

Quoted 12 lines

Hi Atsushi,

Quoted 9 lines · Atsushi Kumagai

Hello Pratyush,

Quoted 6 lines

Call cach_init() before get_kcore_dump_loads(), because latter uses cache_search().

Call path is like this :
get_kcore_dump_loads() -> process_dump_load() -> vaddr_to_paddr() -> vtop4_x86_64() -> readmem() -> cache_search()

Quoted 6 lines

Quoted 4 lines

I found that the current cach_init() is too early, cache_init() should be called after fallback_to_current_page_size() for older kernel which doesn't have VMCOREINFO.
Could you confirm that the patch below is fine with you ?

Its fine and works.

Thanks, I'll merge this fix into v1.6.2.

Regards,
Atsushi Kumagai

Quoted 8 lines

Quoted 8 lines

Thanks,
Atsushi Kumagai

From: Atsushi Kumagai <[email protected]>
Date: Wed, 19 Jul 2017 15:05:58 +0900
Subject: [PATCH] Fix the regression caused by moving cache_init() earlier

Quoted 7 lines

Quoted 7 lines

info->page_size should be set before calling cache_init() since it allocates some buffers based on info->page_size. However, cache_init() was moved before fallback_to_current_page_size() while it set info->page_size for older kernel which doesn't have VMCOREINFO.

This patch moves cache_init() after fallback_to_current_page_size() and reverts the commit: b98e6e0d2a(sadump: set info->page_size before

cache_init())

Quoted 71 lines

Quoted 57 lines

Signed-off-by Atsushi Kumagai <[email protected]>

---
 makedumpfile.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

makedumpfile.c +9 -12

index a3f0697..f85003a 100644
@@ -3905,15 +3905,6 @@ initial(void)
 	if (!get_value_for_old_linux())
 		return FALSE;
 
-	if (info->flag_sadump && !set_page_size(sadump_page_size()))
-			return FALSE;
-
-	if (!is_xen_memory() && !cache_init())
-		return FALSE;
-
-	if (info->flag_mem_usage && !get_kcore_dump_loads())
-		return FALSE;
-
 	if (info->flag_refiltering) {
 		if (info->flag_elf_dumpfile) {
 			MSG("'-E' option is disable, ");

@@ -3936,6 +3927,9 @@ initial(void)
 			return FALSE;
 		}
 
+		if (!set_page_size(sadump_page_size()))
+			return FALSE;
+
 		if (!sadump_initialize_bitmap_memory())
 			return FALSE;
 

@@ -3949,9 +3943,7 @@ initial(void)
 		 * postponed until debug information becomes
 		 * available.
 		 */
-
-	} else if (!get_phys_base())
-		return FALSE;
+	}
 
 out:
 	if (!info->page_size) {

@@ -3962,6 +3954,18 @@ out:
 		if (!fallback_to_current_page_size())
 			return FALSE;
 	}
+
+	if (!is_xen_memory() && !cache_init())
+		return FALSE;
+
+	if (info->flag_mem_usage && !get_kcore_dump_loads())

makedumpfile.c out: · line 3971

Quoted 10 lines

makedumpfile.c +7 -0

+		return FALSE;
+
+	if (!info->flag_refiltering && !info->flag_sadump) {
+		if (!get_phys_base())
+			return FALSE;
+	}
+
 	if (!get_max_mapnr())
 		return FALSE;
 

--
Regards
Pratyush