Date: Wed, 23 Mar 2022 08:09:49 +0000
From: "d.hatayama@fujitsu.com" <d.hatayama@fujitsu.com>
To: "crash-utility@redhat.com" <crash-utility@redhat.com>
Subject: [Crash-utility] [PATCH] kernel: fix start-up time degradation
        caused by strings command
Message-ID:
        <TYAPR01MB6507E11837E16B365D7743B695189@TYAPR01MB6507.jpnprd01.prod.outlook.com>

Content-Type: text/plain; charset="iso-2022-jp"

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@fujitsu.com>
---
 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