Hi, lijiang <lijiang@xxxxxxxxxx> writes: > On Mon, Mar 28, 2022 at 5:04 PM <crash-utility-request@xxxxxxxxxx> wrote: > >> Date: Mon, 28 Mar 2022 11:03:38 +0200 >> From: Alexander Egorenkov <egorenar@xxxxxxxxxxxxx> >> To: "d.hatayama@xxxxxxxxxxx" <d.hatayama@xxxxxxxxxxx>, >> "crash-utility@xxxxxxxxxx" <crash-utility@xxxxxxxxxx> >> Subject: Re: [PATCH] kernel: fix start-up time >> degradation caused by strings command >> Message-ID: <87ee2m4ff9.fsf@xxxxxxxxxxxxxxxxxxxx> >> Content-Type: text/plain >> >> "d.hatayama@xxxxxxxxxxx" <d.hatayama@xxxxxxxxxxx> writes: >> >> > verify_namelist() uses strings command and scans full part of vmlinux >> > file to find linux_banner string. However, vmlinux file is quite large >> > these days, reaching over 500MB. As a result, this degradates start-up >> > time of crash command 10 or more seconds. (Of course, this depends on >> > machines you use for investigation, but I guess typically we cannot >> > use such powerful machines to investigate crash dump...) >> > >> > To resolve this issue, let's use bfd library and read linux_banner >> > string in vmlinux file directly. >> > >> > A simple benchmark shows the following result: >> > >> > Without the fix: >> > >> > # cat ./commands.txt >> > quit >> > # time ./crash -i ./commands.txt \ >> > /usr/lib/debug/lib/modules/5.16.15-201.fc35.x86_64/vmlinux \ >> > /var/crash/*/vmcore >/dev/null 2>&1 >> > >> > real 0m20.251s >> > user 0m19.022s >> > sys 0m1.054s >> > >> > With the fix: >> > >> > # time ./crash -i ./commands.txt \ >> > /usr/lib/debug/lib/modules/5.16.15-201.fc35.x86_64/vmlinux \ >> > /var/crash/*/vmcore >/dev/null 2>&1 >> > >> > real 0m6.528s >> > user 0m6.143s >> > sys 0m0.431s >> > >> > Note that this commit keeps the original logic that uses strings >> > command for backward compatibility for in case. >> > >> > Signed-off-by: HATAYAMA Daisuke <d.hatayama@xxxxxxxxxxx> >> > --- >> > Makefile | 2 +- >> > kernel.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 42 insertions(+), 1 deletion(-) >> > >> > diff --git a/Makefile b/Makefile >> > index 007d030..e520b12 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -364,7 +364,7 @@ task.o: ${GENERIC_HFILES} task.c >> > ${CC} -c ${CRASH_CFLAGS} task.c ${WARNING_OPTIONS} ${WARNING_ERROR} >> > >> > kernel.o: ${GENERIC_HFILES} kernel.c >> > - ${CC} -c ${CRASH_CFLAGS} kernel.c ${WARNING_OPTIONS} >> ${WARNING_ERROR} >> > + ${CC} -c ${CRASH_CFLAGS} kernel.c -I${BFD_DIRECTORY} >> -I${GDB_INCLUDE_DIRECTORY} ${WARNING_OPTIONS} ${WARNING_ERROR} >> > >> > printk.o: ${GENERIC_HFILES} printk.c >> > ${CC} -c ${CRASH_CFLAGS} printk.c ${WARNING_OPTIONS} >> ${WARNING_ERROR} >> > diff --git a/kernel.c b/kernel.c >> > index 1c63447..92434a3 100644 >> > --- a/kernel.c >> > +++ b/kernel.c >> > @@ -23,6 +23,11 @@ >> > #include <ctype.h> >> > #include <stdbool.h> >> > #include "xendump.h" >> > +#if defined(GDB_7_6) || defined(GDB_10_2) >> > +#define __CONFIG_H__ 1 >> > +#include "config.h" >> > +#endif >> > +#include "bfd.h" >> > >> > static void do_module_cmd(ulong, char *, ulong, char *, char *); >> > static void show_module_taint(void); >> > @@ -97,6 +102,7 @@ static void dump_printk_safe_seq_buf(int); >> > static char *vmcoreinfo_read_string(const char *); >> > static void check_vmcoreinfo(void); >> > static int is_pvops_xen(void); >> > +static int get_linux_banner_from_vmlinux(char *, size_t); >> > >> > >> > /* >> > @@ -1324,6 +1330,12 @@ verify_namelist() >> > target_smp = strstr(kt->utsname.version, " SMP ") ? TRUE : FALSE; >> > namelist_smp = FALSE; >> > >> > + if (get_linux_banner_from_vmlinux(buffer, sizeof(buffer)) && >> > + strstr(buffer, kt->proc_version)) { >> > + found = TRUE; >> > + goto found; >> > + } >> > + >> > sprintf(command, "/usr/bin/strings %s", namelist); >> > if ((pipe = popen(command, "r")) == NULL) { >> > error(INFO, "%s: %s\n", namelist, strerror(errno)); >> > @@ -1384,6 +1396,7 @@ verify_namelist() >> > } >> > } >> > >> > +found: >> > if (found) { >> > if (CRASHDEBUG(1)) { >> > fprintf(fp, "verify_namelist:\n"); >> > @@ -11770,3 +11783,31 @@ check_vmcoreinfo(void) >> > } >> > } >> > } >> > + >> > +static >> > +int get_linux_banner_from_vmlinux(char *buf, size_t size) >> > +{ >> > + struct bfd_section *sect; >> > + long offset; >> > + >> > + sect = bfd_get_section_by_name(st->bfd, ".rodata"); >> > + if (!sect) >> > + return FALSE; >> > + >> > + /* >> > + * Although symbol_value() returns dynamic symbol value that >> > + * is affected by kaslr, which is different from static symbol >> > + * value in vmlinux file, but relative offset to linux_banner >> > + * object in .rodata section is idential. >> > + */ >> > + offset = symbol_value("linux_banner") - symbol_value(".rodata"); >> > + >> > + if (!bfd_get_section_contents(st->bfd, >> > + sect, >> > + buf, >> > + offset, >> > + size)) >> > + return FALSE; >> > + >> > + return TRUE; >> > +} >> > -- >> > 2.31.1 >> > >> > >> > >> > -- >> > Crash-utility mailing list >> > Crash-utility@xxxxxxxxxx >> > https://listman.redhat.com/mailman/listinfo/crash-utility >> > Contribution Guidelines: https://github.com/crash-utility/crash/wiki >> >> Hi, >> >> this patch broke crash-utility on s390. >> When i try to open a dump file, then i get this error message: >> >> crash: cannot resolve ".rodata" >> >> > Thank you for pointing out this issue, Alex. > > Any idea why .rodata symbol is missing ? This symbol must be present in vmlinux ? I'm not yet familiar with this. I'll try to figure it out. >> Maybe test for its existence first ? >> > > Does the following patch work for you? > > diff --git a/kernel.c b/kernel.c > index 92434a3..b504564 100644 > --- a/kernel.c > +++ b/kernel.c > @@ -11790,6 +11790,9 @@ int get_linux_banner_from_vmlinux(char *buf, size_t > size) > struct bfd_section *sect; > long offset; > > + if (!symbol_exists(".rodata")) > + return FALSE; > + > sect = bfd_get_section_by_name(st->bfd, ".rodata"); > if (!sect) > return FALSE; > > Thanks. > Lianbo > > Regards yes, this works, thanks! Would be great if we had this fix on master soon :) >> Alex >> >> >> >> ------------------------------ >> >> Subject: Digest Footer >> >> -- >> Crash-utility mailing list >> Crash-utility@xxxxxxxxxx >> https://listman.redhat.com/mailman/listinfo/crash-utility >> >> >> ------------------------------ >> >> End of Crash-utility Digest, Vol 198, Issue 33 >> ********************************************** >> >> > This Message Is From an External Sender > This message came from outside your organization. > On Mon, Mar 28, 2022 at 5:04 PM <crash-utility-request@xxxxxxxxxx> wrote: > > Date: Mon, 28 Mar 2022 11:03:38 +0200 > From: Alexander Egorenkov <egorenar@xxxxxxxxxxxxx> > To: "d.hatayama@xxxxxxxxxxx" <d.hatayama@xxxxxxxxxxx>, > "crash-utility@xxxxxxxxxx" <crash-utility@xxxxxxxxxx> > Subject: Re: [PATCH] kernel: fix start-up time > degradation caused by strings command > Message-ID: <87ee2m4ff9.fsf@xxxxxxxxxxxxxxxxxxxx> > Content-Type: text/plain > > "d.hatayama@xxxxxxxxxxx" <d.hatayama@xxxxxxxxxxx> writes: > > > verify_namelist() uses strings command and scans full part of vmlinux > > file to find linux_banner string. However, vmlinux file is quite large > > these days, reaching over 500MB. As a result, this degradates start-up > > time of crash command 10 or more seconds. (Of course, this depends on > > machines you use for investigation, but I guess typically we cannot > > use such powerful machines to investigate crash dump...) > > > > To resolve this issue, let's use bfd library and read linux_banner > > string in vmlinux file directly. > > > > A simple benchmark shows the following result: > > > > Without the fix: > > > > # cat ./commands.txt > > quit > > # time ./crash -i ./commands.txt \ > > /usr/lib/debug/lib/modules/5.16.15-201.fc35.x86_64/vmlinux \ > > /var/crash/*/vmcore >/dev/null 2>&1 > > > > real 0m20.251s > > user 0m19.022s > > sys 0m1.054s > > > > With the fix: > > > > # time ./crash -i ./commands.txt \ > > /usr/lib/debug/lib/modules/5.16.15-201.fc35.x86_64/vmlinux \ > > /var/crash/*/vmcore >/dev/null 2>&1 > > > > real 0m6.528s > > user 0m6.143s > > sys 0m0.431s > > > > Note that this commit keeps the original logic that uses strings > > command for backward compatibility for in case. > > > > Signed-off-by: HATAYAMA Daisuke <d.hatayama@xxxxxxxxxxx> > > --- > > Makefile | 2 +- > > kernel.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 42 insertions(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index 007d030..e520b12 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -364,7 +364,7 @@ task.o: ${GENERIC_HFILES} task.c > > ${CC} -c ${CRASH_CFLAGS} task.c ${WARNING_OPTIONS} ${WARNING_ERROR} > > > > kernel.o: ${GENERIC_HFILES} kernel.c > > - ${CC} -c ${CRASH_CFLAGS} kernel.c ${WARNING_OPTIONS} ${WARNING_ERROR} > > + ${CC} -c ${CRASH_CFLAGS} kernel.c -I${BFD_DIRECTORY} -I${GDB_INCLUDE_DIRECTORY} ${WARNING_OPTIONS} ${WARNING_ERROR} > > > > printk.o: ${GENERIC_HFILES} printk.c > > ${CC} -c ${CRASH_CFLAGS} printk.c ${WARNING_OPTIONS} ${WARNING_ERROR} > > diff --git a/kernel.c b/kernel.c > > index 1c63447..92434a3 100644 > > --- a/kernel.c > > +++ b/kernel.c > > @@ -23,6 +23,11 @@ > > #include <ctype.h> > > #include <stdbool.h> > > #include "xendump.h" > > +#if defined(GDB_7_6) || defined(GDB_10_2) > > +#define __CONFIG_H__ 1 > > +#include "config.h" > > +#endif > > +#include "bfd.h" > > > > static void do_module_cmd(ulong, char *, ulong, char *, char *); > > static void show_module_taint(void); > > @@ -97,6 +102,7 @@ static void dump_printk_safe_seq_buf(int); > > static char *vmcoreinfo_read_string(const char *); > > static void check_vmcoreinfo(void); > > static int is_pvops_xen(void); > > +static int get_linux_banner_from_vmlinux(char *, size_t); > > > > > > /* > > @@ -1324,6 +1330,12 @@ verify_namelist() > > target_smp = strstr(kt->utsname.version, " SMP ") ? TRUE : FALSE; > > namelist_smp = FALSE; > > > > + if (get_linux_banner_from_vmlinux(buffer, sizeof(buffer)) && > > + strstr(buffer, kt->proc_version)) { > > + found = TRUE; > > + goto found; > > + } > > + > > sprintf(command, "/usr/bin/strings %s", namelist); > > if ((pipe = popen(command, "r")) == NULL) { > > error(INFO, "%s: %s\n", namelist, strerror(errno)); > > @@ -1384,6 +1396,7 @@ verify_namelist() > > } > > } > > > > +found: > > if (found) { > > if (CRASHDEBUG(1)) { > > fprintf(fp, "verify_namelist:\n"); > > @@ -11770,3 +11783,31 @@ check_vmcoreinfo(void) > > } > > } > > } > > + > > +static > > +int get_linux_banner_from_vmlinux(char *buf, size_t size) > > +{ > > + struct bfd_section *sect; > > + long offset; > > + > > + sect = bfd_get_section_by_name(st->bfd, ".rodata"); > > + if (!sect) > > + return FALSE; > > + > > + /* > > + * Although symbol_value() returns dynamic symbol value that > > + * is affected by kaslr, which is different from static symbol > > + * value in vmlinux file, but relative offset to linux_banner > > + * object in .rodata section is idential. > > + */ > > + offset = symbol_value("linux_banner") - symbol_value(".rodata"); > > + > > + if (!bfd_get_section_contents(st->bfd, > > + sect, > > + buf, > > + offset, > > + size)) > > + return FALSE; > > + > > + return TRUE; > > +} > > -- > > 2.31.1 > > > > > > > > -- > > Crash-utility mailing list > > Crash-utility@xxxxxxxxxx > > https://listman.redhat.com/mailman/listinfo/crash-utility > > Contribution Guidelines: https://github.com/crash-utility/crash/wiki > > Hi, > > this patch broke crash-utility on s390. > When i try to open a dump file, then i get this error message: > > crash: cannot resolve ".rodata" > > Thank you for pointing out this issue, Alex. > > Any idea why .rodata symbol is missing ? > Maybe test for its existence first ? > > Does the following patch work for you? > > diff --git a/kernel.c b/kernel.c > index 92434a3..b504564 100644 > --- a/kernel.c > +++ b/kernel.c > @@ -11790,6 +11790,9 @@ int get_linux_banner_from_vmlinux(char *buf, size_t size) > struct bfd_section *sect; > long offset; > > + if (!symbol_exists(".rodata")) > + return FALSE; > + > sect = bfd_get_section_by_name(st->bfd, ".rodata"); > if (!sect) > return FALSE; > > Thanks. > Lianbo > > Regards > Alex > > ------------------------------ > > Subject: Digest Footer > > -- > Crash-utility mailing list > Crash-utility@xxxxxxxxxx > https://listman.redhat.com/mailman/listinfo/crash-utility > > ------------------------------ > > End of Crash-utility Digest, Vol 198, Issue 33 > ********************************************** -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki