Hi Stephen, Thanks a lot for your quick reply and detailed information, I really appreciate it!
On Thu, Jan 22, 2026 at 1:51 PM Stephen Brennan <[email protected]> wrote: > > 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. Awesome, looking forward to your feedback on the code/API designs etc... > > 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. OK, I agree, mandatory kernel btf/kallsyms info + optional kernel module btf/kallsyms info is a reasonable design. So kernel modules' info can be loaded on demand. > > > +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 Sure, it also sounds reasonable. My original thoughts are, user customization on mm filtering are specified in .so, and if user don't need one .so, e.g. amdgpu mm filtering for a nvidia machine, then he doesn't pack the amdgpu_filter.so into kdump's initramfs. I agree adding extra makedumpfile cmdline option to receive those needed .so is a better design. > > > + 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. It is a clever idea! Though complex for code, I think it is doable. > > 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 again for your suggestions! I got your points and I think I can improve the code while waiting for maintainers ideas at the same time. I will let you know when done or encounter blockers if any. Thanks, Tao Liu > > 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 >
