Hi Tao, This series looks really great -- I'm excited to see the switch to native .so extensions instead of epicc. I've applied the series locally and I'll rebuild my userspace stack inclusion feature based on it, to try it out myself.
In the meantime, I'll share some of my feedback on the patches (though I'm not a makedumpfile developer). This seems like the most important patch in terms of design, so I'll start here. Tao Liu <[email protected]> writes: > This patch will add .so extension support to makedumpfile, similar to crash > extension to crash utility. Currently only > "/usr/lib64/makedumpfile/extensions" > and "./extensions" are searched for extensions. Once found, kallsyms and btf > will be initialized so all extensions can benifit from it (Currently > makedumpfile > doesn't use these info, we can move the kallsyms/btf init code else where > later > if makedumpfile needs them). > > The makedumpfile extension is to help users to customize mm page filtering > upon > traditional mm page flag filtering, without make code modification on > makedumpfile > itself. > > Signed-off-by: Tao Liu <[email protected]> > --- > Makefile | 7 +++- > extension.c | 82 +++++++++++++++++++++++++++++++++++++++++++++ > extensions/Makefile | 10 ++++++ > makedumpfile.c | 4 +++ > 4 files changed, 102 insertions(+), 1 deletion(-) > create mode 100644 extension.c > create mode 100644 extensions/Makefile > > diff --git a/Makefile b/Makefile > index f3f4da8..7e29220 100644 > --- a/Makefile > +++ b/Makefile > @@ -45,7 +45,7 @@ CFLAGS_ARCH += -m32 > endif > > SRC_BASE = makedumpfile.c makedumpfile.h diskdump_mod.h sadump_mod.h > sadump_info.h > -SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c > cache.c tools.c printk.c detect_cycle.c kallsyms.c btf_info.c > +SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c > cache.c tools.c printk.c detect_cycle.c kallsyms.c btf_info.c extension.c > OBJ_PART=$(patsubst %.c,%.o,$(SRC_PART)) > SRC_ARCH = arch/arm.c arch/arm64.c arch/x86.c arch/x86_64.c arch/ia64.c > arch/ppc64.c arch/s390x.c arch/ppc.c arch/sparc64.c arch/mips64.c > arch/loongarch64.c arch/riscv64.c > OBJ_ARCH=$(patsubst %.c,%.o,$(SRC_ARCH)) > @@ -126,6 +126,7 @@ eppic_makedumpfile.so: extension_eppic.c > > clean: > rm -f $(OBJ) $(OBJ_PART) $(OBJ_ARCH) makedumpfile makedumpfile.8 > makedumpfile.conf.5 > + $(MAKE) -C extensions clean > > install: > install -m 755 -d ${DESTDIR}/${SBINDIR} ${DESTDIR}/usr/share/man/man5 > ${DESTDIR}/usr/share/man/man8 > @@ -135,3 +136,7 @@ install: > mkdir -p ${DESTDIR}/usr/share/makedumpfile/eppic_scripts > install -m 644 -D $(VPATH)makedumpfile.conf > ${DESTDIR}/usr/share/makedumpfile/makedumpfile.conf.sample > install -m 644 -t ${DESTDIR}/usr/share/makedumpfile/eppic_scripts/ > $(VPATH)eppic_scripts/* > + > +.PHONY: extensions > +extensions: > + $(MAKE) -C extensions CC=$(CC) > \ No newline at end of file > diff --git a/extension.c b/extension.c > new file mode 100644 > index 0000000..6ee7f4e > --- /dev/null > +++ b/extension.c > @@ -0,0 +1,82 @@ > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <dirent.h> > +#include <dlfcn.h> > +#include <stdbool.h> > +#include "kallsyms.h" > +#include "btf_info.h" > + > +static const char *dirs[] = { > + "/usr/lib64/makedumpfile/extensions", > + "./extensions", > +}; > + > +/* Will only init once */ > +static bool init_kallsyms_btf(void) > +{ > + static bool ret = false; > + static bool has_inited = false; > + > + if (has_inited) > + goto out; > + if (!init_kernel_kallsyms()) > + goto out; > + if (!init_kernel_btf()) > + goto out; > + if (!init_module_kallsyms()) > + goto out; > + if (!init_module_btf()) > + goto out; > + ret = true; I feel it would be good practice to load as little information as is necessary for the task. If "amdgpu" module is required, then load kernel kallsyms, BTF, and then the amdgpu module kallsyms & BTF. If no module debuginfo is required, then just the kernel would suffice. This would reduce memory usage and runtime, though I don't know if it would show up in profiling. The main benefit could be reliability: by handling less data, there are fewer chances to hit an error. > +out: > + has_inited = true; > + return ret; > +} > + > +static void cleanup_kallsyms_btf(void) > +{ > + cleanup_kallsyms(); > + cleanup_btf(); > +} > + > +void run_extensions(void) > +{ > + DIR *dir; > + struct dirent *entry; > + size_t len; > + int i; > + void *handle; > + char path[512]; > + > + for (i = 0; i < sizeof(dirs) / sizeof(char *); i++) { > + if ((dir = opendir(dirs[i])) != NULL) > + break; > + } > + > + if (!dir || i >= sizeof(dirs) / sizeof(char *)) > + /* No extensions found */ > + return; It could be confusing that makedumpfile would behave differently with the same command-line arguments depending on the presence or absence of these extensions on the filesystem. I think it may fit users' expectations better if they are required to specify extensions on the command line. Then we could load them by searching each directory in order. This allows: (a) more expected behavior (b) multiple extensions can exist without all being enabled, thus more flexibility (c) extensions can be present in the local "extensions/" directory, or in the system directory > + while ((entry = readdir(dir)) != NULL) { > + len = strlen(entry->d_name); > + if (len > 3 && strcmp(entry->d_name + len - 3, ".so") == 0) { > + /* Will only init when .so exist */ > + if (!init_kallsyms_btf()) > + goto out; > + > + snprintf(path, sizeof(path), "%s/%s", dirs[i], > entry->d_name); > + handle = dlopen(path, RTLD_NOW); > + if (!handle) { > + fprintf(stderr, "%s: Failed to load %s: %s\n", > + __func__, path, dlerror()); > + continue; > + } > + printf("Loaded extension: %s\n", path); > + dlclose(handle); Using the constructor/destructor of the shared object is clever! But we lose some flexibility: by the time the dlopen() returns, the constructor has executed and the plugin has thus executed. What if we instead use dlsym() to load some symbols from the DSO? In particular, I think it would be useful if extensions could declare a list of symbols and a list of structure information which they are interested in receiving. We could use these lists to know which kernel/module kallsyms & BTF we should load. We could even load the information into the local variables of the extension, so the extension would not need to manually load it. Of course this is more complex, but the benefit is: 1. Extensions can be written more simply, and would not need to manually load each symbol & type. 2. We could eliminate the hash tables for kallsyms & BTF, and eliminate the loading of unnecessary module information. Instead, we'd just populate the symbol addresses, struct offsets, and type sizes directly into the local variables which request them. Again, while I don't want to prematurely optimize -- it's good to avoid loading unnecessary information. I hope I've described my idea well. I would be happy to work on an implementation of it based on your patches here, if you're interested. Thanks, Stephen > + } > + } > +out: > + closedir(dir); > + cleanup_kallsyms_btf(); > +} > \ No newline at end of file > diff --git a/extensions/Makefile b/extensions/Makefile > new file mode 100644 > index 0000000..afbc61e > --- /dev/null > +++ b/extensions/Makefile > @@ -0,0 +1,10 @@ > +CC ?= gcc > +CONTRIB_SO := > + > +all: $(CONTRIB_SO) > + > +$(CONTRIB_SO): %.so: %.c > + $(CC) -O2 -g -fPIC -shared -o $@ $^ > + > +clean: > + rm -f $(CONTRIB_SO) > diff --git a/makedumpfile.c b/makedumpfile.c > index dba3628..ca8ed8a 100644 > --- a/makedumpfile.c > +++ b/makedumpfile.c > @@ -10847,6 +10847,8 @@ update_dump_level(void) > } > } > > +void run_extensions(void); > + > int > create_dumpfile(void) > { > @@ -10884,6 +10886,8 @@ retry: > if (info->flag_refiltering) > update_dump_level(); > > + run_extensions(); > + > if ((info->name_filterconfig || info->name_eppic_config) > && !gather_filter_info()) > return FALSE; > -- > 2.47.0
