Re: [PATCH] kernel: fix start-up time degradation caused by strings command

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux