[Makedumpfile Patch v3 1/7] show_mem_usage(): calculate page offset after elf load

[Pratyush Anand]

x86_64 calculated page offset from PT_LOAD headers. Therefore call
get_page_offset() after get_elf_loads()

Signed-off-by: Pratyush Anand <***@redhat.com>
---
makedumpfile.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index e69b6df9a9ee..6942047199de 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -10944,15 +10944,15 @@ int show_mem_usage(void)

info->dump_level = MAX_DUMP_LEVEL;

- if (!get_page_offset())
- return FALSE;
-
if (!open_files_for_creating_dumpfile())
return FALSE;

if (!get_elf_loads(info->fd_memory, info->name_memory))
return FALSE;

+ if (!get_page_offset())
+ return FALSE;
+
if (!get_sys_kernel_vmcoreinfo(&vmcoreinfo_addr, &vmcoreinfo_len))
return FALSE;

[Pratyush Anand]

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 <***@redhat.com>
---
makedumpfile.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 6942047199de..3b8e9810468d 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -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;

[Pratyush Anand]

A kcore PT_LOAD can have a section from vmalloc region. However,
physical address in that header would be invalid (-1) after kernel
commit 464920104bf7 (/proc/kcore: update physical address for kcore ram
and text). Therefore, check for valid physical address while calculating
page_offset or phys_offset.

Signed-off-by: Pratyush Anand <***@redhat.com>
---
arch/x86_64.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86_64.c b/arch/x86_64.c
index 893cd516fc8b..e978a36f8878 100644
--- a/arch/x86_64.c
+++ b/arch/x86_64.c
@@ -41,7 +41,8 @@ get_page_offset_x86_64(void)
unsigned long long virt_start;

for (i = 0; get_pt_load(i, &phys_start, NULL, &virt_start, NULL); i++) {
- if (virt_start < __START_KERNEL_map) {
+ if (virt_start < __START_KERNEL_map
+ && phys_start != NOT_PADDR) {
info->page_offset = virt_start - phys_start;
return TRUE;
}
@@ -76,7 +77,8 @@ get_phys_base_x86_64(void)
}

for (i = 0; get_pt_load(i, &phys_start, NULL, &virt_start, NULL); i++) {
- if (virt_start >= __START_KERNEL_map) {
+ if (virt_start >= __START_KERNEL_map
+ && phys_start != NOT_PADDR) {

info->phys_base = phys_start -
(virt_start & ~(__START_KERNEL_map));

[Pratyush Anand]

kcore passes correct phys_start for direct mapped region and an invalid
value (-1) for all other regions after the kernel commit
464920104bf7(/proc/kcore: update physical address for kcore ram and
text). arch specific function is_phys_addr() accepts only virt_start.
Therefore, check for valid phys_start in get_kcore_dump_loads().

Signed-off-by: Pratyush Anand <***@redhat.com>
---
elf_info.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/elf_info.c b/elf_info.c
index 65ff333cf33a..c5743b3cab28 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -881,7 +881,8 @@ int get_kcore_dump_loads(void)

for (i = 0; i < num_pt_loads; ++i) {
struct pt_load_segment *p = &pt_loads[i];
- if (!is_phys_addr(p->virt_start))
+ if (p->phys_start == NOT_PADDR
+ || !is_phys_addr(p->virt_start))
continue;
loads++;
}
@@ -901,7 +902,8 @@ int get_kcore_dump_loads(void)

for (i = 0, j = 0; i < num_pt_loads; ++i) {
struct pt_load_segment *p = &pt_loads[i];
- if (!is_phys_addr(p->virt_start))
+ if (p->phys_start == NOT_PADDR
+ || !is_phys_addr(p->virt_start))
continue;
if (j >= loads)
return FALSE;

[Pratyush Anand]

From: Baoquan He <***@redhat.com>

In set_kcore_vmcoreinfo, we calculate the virtual address of vmcoreinfo
by OR operation as below:

kvaddr = (ulong)vmcoreinfo_addr | PAGE_OFFSET;

When mm sections kaslr is not enabled, this is correct since the
starting address of direct mapping section is 0xffff880000000000 which
is 1T aligned. Usually system with memory below 1T won't cause problem.

However with mm section kaslr enabled, the starting address of direct
mapping is 1G aligned. The above code makes kvaddr unsure.

So change it to adding operation:
kvaddr = (ulong)vmcoreinfo_addr + PAGE_OFFSET;

Signed-off-by: Baoquan He <***@redhat.com>
---
elf_info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/elf_info.c b/elf_info.c
index c5743b3cab28..100272f83c48 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -372,7 +372,7 @@ int set_kcore_vmcoreinfo(uint64_t vmcoreinfo_addr, uint64_t vmcoreinfo_len)
off_t offset_desc;

offset = UNINITIALIZED;
- kvaddr = (ulong)vmcoreinfo_addr | PAGE_OFFSET;
+ kvaddr = (ulong)vmcoreinfo_addr + PAGE_OFFSET;

for (i = 0; i < num_pt_loads; ++i) {
struct pt_load_segment *p = &pt_loads[i];

[Pratyush Anand]

From: Baoquan He <***@redhat.com>

Kernel commit 464920104bf7 (/proc/kcore: update physical address for
kcore ram and text) provides physical address of direct mapping kcore
program segments. So no need to calculate it specifically now. And the
old code is not correct since it calls vaddr_to_paddr() which has not
been ready at that time.

Signed-off-by: Baoquan He <***@redhat.com>
---
elf_info.c | 17 -----------------
1 file changed, 17 deletions(-)

diff --git a/elf_info.c b/elf_info.c
index 100272f83c48..8e2437622141 100644
--- a/elf_info.c
+++ b/elf_info.c
@@ -857,22 +857,6 @@ static int exclude_segment(struct pt_load_segment **pt_loads,
return 0;
}

-static int
-process_dump_load(struct pt_load_segment *pls)
-{
- unsigned long long paddr;
-
- paddr = vaddr_to_paddr(pls->virt_start);
- pls->phys_start = paddr;
- pls->phys_end = paddr + (pls->virt_end - pls->virt_start);
- DEBUG_MSG("process_dump_load\n");
- DEBUG_MSG(" phys_start : %llx\n", pls->phys_start);
- DEBUG_MSG(" phys_end : %llx\n", pls->phys_end);
- DEBUG_MSG(" virt_start : %llx\n", pls->virt_start);
- DEBUG_MSG(" virt_end : %llx\n", pls->virt_end);
-
- return TRUE;
-}

int get_kcore_dump_loads(void)
{
@@ -917,7 +901,6 @@ int get_kcore_dump_loads(void)
}

pls[j] = *p;
- process_dump_load(&pls[j]);
j++;
}

[Pratyush Anand]

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 <***@redhat.com>
---
makedumpfile.8 | 9 ++++++++-
makedumpfile.c | 6 ++++++
print_info.c | 3 ++-
3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/makedumpfile.8 b/makedumpfile.8
index 9069fb18cdb6..993236486e77 100644
--- a/makedumpfile.8
+++ b/makedumpfile.8
@@ -235,13 +235,20 @@ the ELF format does not support compressed data.

.TP
\fB\-f\fR
-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 \-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
diff --git a/makedumpfile.c b/makedumpfile.c
index 3b8e9810468d..e3be1ab0a9ec 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -11269,6 +11269,12 @@ main(int argc, char *argv[])
MSG("Try `makedumpfile --help' for more information.\n");
goto out;
}
+ 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;
+ }

if (!show_mem_usage())
goto out;
diff --git a/print_info.c b/print_info.c
index 392d863a4227..72ed8fa0c059 100644
--- a/print_info.c
+++ b/print_info.c
@@ -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");

[Atsushi Kumagai]

Post by Pratyush Anand
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.
---
makedumpfile.8 | 9 ++++++++-
makedumpfile.c | 6 ++++++
print_info.c | 3 ++-
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/makedumpfile.8 b/makedumpfile.8
index 9069fb18cdb6..993236486e77 100644
--- a/makedumpfile.8
+++ b/makedumpfile.8
@@ -235,13 +235,20 @@ the ELF format does not support compressed data.
.TP
\fB\-f\fR
-Force existing DUMPFILE to be overwritten.
+Force existing DUMPFILE to be overwritten and mem-usage to work with older
+kernel as well.
.br
.br
# makedumpfile \-f \-d 31 \-x vmlinux /proc/vmcore dumpfile
.br
This command overwrites \fIDUMPFILE\fR even if it already exists.
+.br
+# 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
diff --git a/makedumpfile.c b/makedumpfile.c
index 3b8e9810468d..e3be1ab0a9ec 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -11269,6 +11269,12 @@ main(int argc, char *argv[])
MSG("Try `makedumpfile --help' for more information.\n");
goto out;
}
+ 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.


Thanks,
Atsushi Kumagai

Post by Pratyush Anand
if (!show_mem_usage())
goto out;
diff --git a/print_info.c b/print_info.c
index 392d863a4227..72ed8fa0c059 100644
--- a/print_info.c
+++ b/print_info.c
@@ -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

[Pratyush Anand]

Post by Atsushi Kumagai

Post by Pratyush Anand
+ 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?

~Pratyush

[b***@redhat.com]

Post by Pratyush Anand

Post by Atsushi Kumagai

Post by Pratyush Anand
+ 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?

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.

[Atsushi Kumagai]

Post by b***@redhat.com

Post by Pratyush Anand

Post by Atsushi Kumagai

Post by Pratyush Anand
+ 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?

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.


Regards,
Atsushi Kumagai

[Pratyush Anand]

Post by Atsushi Kumagai
When you change the design, please note it into changelog,
it's a review point of view.
- Fixed a memory leak issue and updated man page and usage info

Sorry about that!!

Post by Atsushi Kumagai
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.

and thanks :-)


~Pratyush