Re: [PATCH v3 5/8] Add makedumpfile extension support

[Stephen Brennan <stephen.s.brennan@oracle.com>]

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 <ltao@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@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.

> +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