[PATCH v3 5/8] Add makedumpfile extension support
Tao Liu
ltao at redhat.com
Tue Mar 17 08:31:20 PDT 2026
Hi Stephen,
Thanks a lot for your comments, I have made plenty of code
improvements based on your suggestions and your github code repo.
Sorry I didn't inline reply to all your comments, please check out the
latest v4 patch I posted upstream.
Some notable improvements:
1) I adjusted the extension callback function to amdgpu_filter, so a
same callback interface can serve both our extensions.
2) I followed your implementation of the INIT_OPT_(...) macro for
optional sym/types and the INIT_(...) macro for must-have sym/types.
If extension missing any must-have sym/types, it will load-fail and
exit as early(e.g. a machine doesn't have amdgpu). And optional
symbols checking during extension_init() (e.g. existence of kernel
symbol for kernel version detection)
3) kallsyms address calc for v7.0 kernel
On Fri, Mar 13, 2026 at 11:24 AM Stephen Brennan
<stephen.s.brennan at oracle.com> wrote:
>
> Tao Liu <ltao at redhat.com> writes:
>
> > Hi Stephen,
> >
> > On Wed, Mar 11, 2026 at 1:38 PM Stephen Brennan
> > <stephen.s.brennan at oracle.com> wrote:
> >>
> >> Tao Liu <ltao at redhat.com> writes:
> >> > Hi Stephen,
> >> >
> >> > Sorry I took some time to make modifications on v3 code. Please see a
> >> > drafted v4 [1] for a preview.
> >> >
> >> > I followed your suggestion to let extensions declare the kallsyms
> >> > symbol/btf types it needed, then during kallsyms/btf initialization,
> >> > it will only resolve the declared symbol/types, thus getting rid of
> >> > the hash table. Extensions now don't need to initial needed
> >> > symbols/types by itself.
> >> >
> >> > In addition, users can specify which extensions to load at
> >> > makedumpfile cmdline as:
> >> >
> >> > $ ./makedumpfile -d 31 -l
> >> > /var/crash/127.0.0.1-2025-06-10-18\:03\:12/vmcore /tmp/out --extension
> >> > amdgpu_filter.so
> >> >
> >> > Also "--extension" can be used several times, and amdgpu_filter.so can
> >> > be an absolute path as well. If no extensions are specified, the
> >> > btf/kallsyms of makedumpfile will not initialize.
> >> >
> >> > I don't know if this is what you wanted, or any suggestions for this?
> >> > Thanks in advance!
> >>
> >> Hi Tao,
> >>
> >> Please accept my apologies for the long delay on this review. I've gone through
> >> each commit with my feedback. This looks like a really excellent job and I
> >> think it is ready for the review of the maintainers. I've implemented my own
> >> userspace stack extension on top of this support, and I find it to be great for
> >> my use case as well, with one small change (adding flags for detecting whether a
> >> struct or member is found).
> >
> > No worries at all :) Thanks a lot for your detailed comments. I'm
> > looking through them as well as your code branch in github. This may
> > take a while...
> >
> >>
> >> Here is some more detailed feedback on a per-commit level; it's all pretty
> >> minor.
> >>
> >> d3aee7a ("Reserve sections for makedumpfile and extenions")
> >>
> >> * This is just a note, but I've found that the linker script may not be
> >> necessary. GCC automatically creates the __start_SECTION and __stop_SECTION
> >> symbols for sections it creates.
> >
> > I have tried to remove the makedumpfile.ld linker script. The
> > automatic symbol as __start/stop_SECTION are only generated for
> > makedumpfile, aka the main program, but not for .so extensions. This
> > is a breaker because I would expect all .so files have the
> > __start/stop_SECTION symbol so they can be resigtered to main program.
> > I'm not expert in GCC's default behaviours, so I guess I would prefer
> > the current approach of explicit symbol define within the linker
> > script, seems to be more robust... Perhaps you can share your code on
> > this, I will give it a try.
>
> The key to making GCC generate the symbols for the sections is that the
> section names should themselves be valid C identifiers (i.e. they should
> not start with a '.'). Here's a patch to give the idea:
>
> From 3618f51869e978328e49f6061eae94469bca43c5 Mon Sep 17 00:00:00 2001
> From: Stephen Brennan <stephen.s.brennan at oracle.com>
> Date: Wed, 11 Mar 2026 09:17:05 -0700
> Subject: [PATCH] Use automatically generated section start symbols
>
> GCC will create __start_SECTION and __stop_SECTION only if the section
> names are valid C identifiers. This means we cannot use a leading dot.
> Drop the leading dot from the section names so we can avoid defining a
> custom linker script.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan at oracle.com>
> ---
> Makefile | 2 +-
> btf_info.h | 4 ++--
> extensions/Makefile | 2 +-
> kallsyms.h | 2 +-
> makedumpfile.ld | 15 ---------------
> 5 files changed, 5 insertions(+), 20 deletions(-)
> delete mode 100644 makedumpfile.ld
>
> diff --git a/Makefile b/Makefile
> index 8196aa6..e2cde8f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -113,7 +113,7 @@ $(OBJ_ARCH): $(SRC_ARCH)
> $(CC) $(CFLAGS_ARCH) -c -o ./$@ $(VPATH)$(@:.o=.c)
>
> makedumpfile: $(SRC_BASE) $(OBJ_PART) $(OBJ_ARCH)
> - $(CC) $(CFLAGS) $(LDFLAGS) $(OBJ_PART) $(OBJ_ARCH) -rdynamic -Wl,-T,makedumpfile.ld -o $@ $< $(LIBS)
> + $(CC) $(CFLAGS) $(LDFLAGS) $(OBJ_PART) $(OBJ_ARCH) -rdynamic -o $@ $< $(LIBS)
> @sed -e "s/@DATE@/$(DATE)/" \
> -e "s/@VERSION@/$(VERSION)/" \
> $(VPATH)makedumpfile.8.in > $(VPATH)makedumpfile.8
> diff --git a/btf_info.h b/btf_info.h
> index 2a78fbf..555f21e 100644
> --- a/btf_info.h
> +++ b/btf_info.h
> @@ -29,7 +29,7 @@ void cleanup_btf(void);
> struct ktype_info _##MOD##_##S##_##M = { \
> QUATE(MOD), QUATE(S), QUATE(M), 0, 0, 0, 0, R, R\
> }; \
> - __attribute__((section(".init_ktypes"), used)) \
> + __attribute__((section("init_ktypes"), used)) \
> struct ktype_info * _ptr_##MOD##_##S##_##M = &_##MOD##_##S##_##M
>
> #define INIT_MOD_STRUCT_MEMBER(MOD, S, M) \
> @@ -60,7 +60,7 @@ void cleanup_btf(void);
> struct ktype_info _##MOD##_##S = { \
> QUATE(MOD), QUATE(S), 0, 0, 0, 0, 0, R, 0 \
> }; \
> - __attribute__((section(".init_ktypes"), used)) \
> + __attribute__((section("init_ktypes"), used)) \
> struct ktype_info * _ptr_##MOD##_##S = &_##MOD##_##S
>
> #define INIT_MOD_STRUCT(MOD, S) INIT_MOD_STRUCT_RQD(MOD, S, 1)
> diff --git a/extensions/Makefile b/extensions/Makefile
> index fc28577..a4dc86a 100644
> --- a/extensions/Makefile
> +++ b/extensions/Makefile
> @@ -8,7 +8,7 @@ amdgpu_filter.so: maple_tree.c
> userstack.so: maple_tree.c vma_rbtree.c
>
> $(CONTRIB_SO): %.so: %.c
> - $(CC) -O2 -g -fPIC -shared -Wl,-T,../makedumpfile.ld -o $@ $^
> + $(CC) -O2 -g -fPIC -shared -o $@ $^
>
> clean:
> rm -f $(CONTRIB_SO)
> diff --git a/kallsyms.h b/kallsyms.h
> index d94830e..1989dfe 100644
> --- a/kallsyms.h
> +++ b/kallsyms.h
> @@ -17,7 +17,7 @@ struct ksym_info {
> struct ksym_info _##MOD##_##SYM = { \
> QUATE(MOD), QUATE(SYM), 0 \
> }; \
> - __attribute__((section(".init_ksyms"), used)) \
> + __attribute__((section("init_ksyms"), used)) \
> struct ksym_info * _ptr_##MOD##_##SYM = &_##MOD##_##SYM
>
> #define GET_MOD_SYM(MOD, SYM) (_##MOD##_##SYM.value)
> diff --git a/makedumpfile.ld b/makedumpfile.ld
> deleted file mode 100644
> index 231a162..0000000
> --- a/makedumpfile.ld
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -SECTIONS
> -{
> - .init_ksyms ALIGN(8) : {
> - __start_init_ksyms = .;
> - KEEP(*(.init_ksyms*))
> - __stop_init_ksyms = .;
> - }
> -
> - .init_ktypes ALIGN(8) : {
> - __start_init_ktypes = .;
> - KEEP(*(.init_ktypes*))
> - __stop_init_ktypes = .;
> - }
> -}
> -INSERT AFTER .data;
> \ No newline at end of file
> --
> 2.47.3
Unfortunately with the patch applied I cannot see the
__start_init_ksyms/ktypes symbol for amdgpu_filter.so. So I had to
keep the linker script in v4...
Thanks,
Tao Liu
>
>
>
> >> * Otherwise, this is exactly what I was thinking!
> >>
> >> 102ae0b ("Implement kernel kallsyms resolving")
> >>
> >> * Again, this is looking very close to the design I hoped for, thanks!
> >> * I'm not sure whether is_unwanted_symbol() needs to exist anymore? Given that
> >> users opt-in to specific symbols, we don't need to filter out noisy ones that
> >> would waste memory. What do you think?
> >
> > Agreed, I can remove this function.
> >
> >> * Unfortunately, upstream has made some changes to the vmlinux kallsyms encoding
> >> in 7.0. You may want to check what we did in drgn to support those changes:
> >> https://github.com/osandov/drgn/commit/744f36ec3c3f64d7e1323a0037898158698585c4
> >> * In kallsyms.c:
> >
> > Thanks for the info, I will try this and update the code in v4.
> >
> >>
> >> +/*
> >> + * Makedumpfile's .init_ksyms section
> >> +*/
> >> +extern struct ksym_info __start_init_ksyms[];
> >> +extern struct ksym_info __stop_init_ksyms[];
> >>
> >> I'm not sure it matters, but the type here is wrong. It should be
> >> "extern struct ksym_info *" because you're storing pointers, not the actual
> >> struct. That said, the type isn't used so I don't know that it matters.
> >
> > Right, this is an error. Thanks for pointing that out.
> >
> >>
> >> 4187b33 ("Implement kernel btf resolving")
> >>
> >> * The get_ktype_info() function fails if either the struct or member is not
> >> found. This makes sense in a lot of cases, but there are other cases where we
> >> will want to use the presence or absence of a struct/struct member to detect
> >> which version of code to use. For example, my userspace stack code will use
> >> either maple or rbtree helper code for finding stack VMAs, depending on which
> >> is available. We can't know until runtime, when we check whether mm_rb or
> >> mm_mt is present.
> >>
> >> One solution is to handle this at runtime. We can have a macro like
> >> "HAVE_MEMBER(S, M)" and "HAVE_STRUCT(S)", and then each extension can check
> >> whether for the members it expects to be present. I think the major downside
> >> to this approach is that it requires manual effort, which is likely to be
> >> forgotten when writing extensions.
> >>
> >> Alternatively, we could set a flag in the struct ktype_info if the type is
> >> "required" (or optional), and only fail get_ktype_info() for required
> >
> > Agreed, an optional flag looks better.
> >
> >> structs/members. The concern with this approach is: what if plugin (A)
> >> requires a type which is not present, but plugin (B) does not? If both are
> >> loaded, the failure of A would cause B not to run. I'm not sure whether we
> >> should care about that situation... I don't know if we have a use case for
> >> using multiple plugins at the same time. Until we do, we probably won't have a
> >> good idea whether it should be allowed for one to fail, but the other to
> >> continue.
> >
> > Great question! I have thought about this previously. I suggest that
> > if plugin(A) fails, it should just fail and allow the execution of any
> > later plugins (B, C....) to continue. Each plugin is responsible for
> > one task, like plugin(A) for dealing with amdgpu's mm page
> > filtering,and plugin(B) for Intel's and plugin(C) for NV's. Plugin(A)
> > certainly will fail if one machine have no amdgpu, thus the amdgpu.ko
> > will never been loaded, so related symbol/types missing. This is
> > expected and shouldn't block the later plugins.
> >
> > But the "fail" should gentle, not like the ones as segfault, which
> > will crash the entire makedumpfile program.
>
> Totally agreed up to here! I think it makes sense for plugin failures to
> be independent.
>
> It seems like the BTF loading process should not fail if any type or
> member is not found. But once we've tried to load everything, we can
> then go through each plugin's list, and if we find a "required" type
> or member which is not present, we log that failure and don't run the
> plugin. This shouldn't be much work, because we already have all the
> types in an array for each plugin.
>
> > Since currently plugins
> > are native .so libraries, the quality of code is ensured by each
> > plugin authors, rather than makedumpfile maintainers. Idealy the the
> > plugins are well tested in 1st kernel before they are shipped to kdump
> > img, but who knows. From makedumpfile's view, do you think we need to
> > introduce a sandbox to isolate plugins from makedumpfile? This would
> > prevent serious plugin errors from stopping makedumpfile from
> > generating the vmcore.
>
> Personally, I don't believe we need sandbox. The plugins should be
> written to handle the clear error cases:
>
> - Optional types or symbols may not be present, so validate them.
> - Values read from /proc/vmcore may not be valid, or the read may fail,
> so check all return values.
>
> That said, I'm not set against it, so if you believe it's necessary I
> could be convinced :)
>
> Thanks again,
> Stephen
>
> >>
> >> I've implemented this second alternative in my branch.
> >>
> >> * Similar to the previous commit, __start_init_ktypes and __stop_init_ktypes are
> >> declared as structs but should probably be declared as pointers.
> >>
> >> 22097b7 ("Implement kernel module's kallsyms resolving")
> >> edfa698 ("Implement kernel module's btf resolving")
> >>
> >> * These commits are straightforward:
> >> Reviewed-by: Stephen Brennan <stephen.s.brennan at oracle.com>
> >>
> >> ede22e8 ("Add makedumpfile extensions support")
> >
> > I will address those later after reading through your code branch.
> > Thanks again for your detailed comments!
> >
> > Thanks,
> > Tao Liu
> >
> >>
> >> * nit: the array "handlers" actually contains "handles" (no r) returned from
> >> dlopen(). The word handlers usually implies a function pointer or something
> >> like that, called to handle a certain situation. Maybe rename this to
> >> "handles".
> >> * The run_extensions() function is called within a retry loop in
> >> create_dumpfile(). It seems possible that this could get called multiple
> >> times. But many of the global variables in the kallsyms, BTF, and extension
> >> code are not safe to be cleaned up and reinitialized. In particular, when
> >> array elements are freed, the lengths, capacities, and pointers are not reset
> >> to 0/NULL. I think it would be wise to make all the cleanup functions clear
> >> the globals, so that they may be reinitialized safely.
> >> * Further, I think it might be helpful to split extension loading, running, and
> >> cleanup. Something like this in create_dumpfile():
> >>
> >> load_extensions(); /* loads everything and calls entry() */
> >> retry:
> >> /* ... create bitmap and dump ... */
> >> if (status == NOSPACE) {
> >> /* ... */
> >> goto retry;
> >> }
> >> /* ... */
> >> cleanup_extensions();
> >> return;
> >>
> >> For two reasons: (1) this avoids unnecessary work re-loading and
> >> re-initializing the BTF, kallsyms, and extensions. (Though I still think it's
> >> safer to ensure they can be re-initialized safely.) And (2), this allows for
> >> future use of extensions in the rest of the dump operation. For my userspace
> >> stack extension, I plan to add a callback which allows extensions to override
> >> the decision to filter a page, since my logic can't be easily done via erase
> >> info. So the extensions need to remain loaded during the creation of the
> >> dumpfile, and cleaned up after. I have tweaked this in my own patches,
> >> but I just wanted to share the use case.
> >>
> >> c568635 ("btf/kallsyms based makedumpfile extension for mm page filtering")
> >>
> >> * This looks good to me!
> >> * I will say that for my userspace stack use case, while the "filter_page(...,
> >> false)" mechanism for specifying pages that are retained *looks* useful, I
> >> wouldn't be able to use it because I cannot determine the PFNs for each stack
> >> VMA. Instead, I have to use the page "mapping" and "index" fields to match the
> >> VMA and determine whether the PFN falls in a range I care to save. Of course,
> >> just because *I* won't use it doesn't mean it's not useful :)
> >>
> >> 2b252ec ("Filter amdgpu mm pages")
> >>
> >> * I'm no expert in amdgpu, but the overall approach makes sense to me, and the
> >> helpers look good.
> >>
> >>
> >> At a broader level, while the add_to_arr() function is useful, I do think the
> >> dynamic array/vector pattern could be captured with a dedicated data structure.
> >> For instance, drgn has this excellent LGPL-2.1+ header-only vector library:
> >> https://github.com/osandov/drgn/blob/main/libdrgn/vector.h
> >> I don't think it is high priority or must be addressed. Nor do I mean that this
> >> particular implementation choice be used. It's just something to share & think
> >> about.
> >>
> >> To provide some context on my comments related to the userspace stack tracing,
> >> here is a branch of mine which is based on yours, that adds my userspace stack
> >> extension and a few tweaks:
> >>
> >> https://github.com/brenns10/makedumpfile/commits/stepbren_userstack_v4/
> >>
> >> Thank you,
> >> Stephen
> >>
> >> > Thanks,
> >> > Tao Liu
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > [1]: https://github.com/liutgnu/makedumpfile/commits/v4/
> >> >
> >> > On Fri, Jan 23, 2026 at 2:43 AM Tao Liu <ltao at redhat.com> wrote:
> >> >>
> >> >> 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
> >> >> <stephen.s.brennan at oracle.com> 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 <ltao at redhat.com> 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 <ltao at redhat.com>
> >> >> > > ---
> >> >> > > 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
> >> >> >
> >>
>
More information about the kexec
mailing list