On Monday, October 14th, 2024 at 3:30 PM, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
[...]
>
> Consider the following pahole execution log when patches #1-3 are applied:
>
> $ git log --oneline next..HEAD
> ce72b07 (HEAD) btf_encoder: collect elf_functions in pre_cus__load_module
> 4369607 btf_encoder: introduce elf_functions struct type
> e4ae1a0 dwarf_loader: introduce pre/post cus__load_module hooks to conf_load
>
> $ git diff
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 9e05b56..8527046 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -171,6 +171,7 @@ static struct elf_functions *elf_functions__get(Elf *elf)
>
> static inline void elf_functions__delete(struct elf_functions *funcs)
> {
> + fprintf(stderr, "elf_functions__delete(%p)\n", funcs);
> free(funcs->entries);
>
> elf_symtab__delete(funcs->symtab);
>
> list_del(&funcs->node);
>
> @@ -197,6 +198,7 @@ static struct elf_functions *elf_functions__new(Elf *elf)
> goto out_error;
>
> list_add_tail(&funcs->node, &elf_functions_list);
>
> + fprintf(stderr, "elf_functions__new(), returning %p\n", funcs);
> return funcs;
>
> out_error:
>
> $ pahole --btf_encode_detached=/dev/null ~/work/bpf-next/kernel/bpf/verifier.o ~/work/bpf-next/kernel/bpf/core.o
> elf_functions__new(), returning 0x307653c0
> elf_functions__new(), returning 0x30aa8e10
> elf_functions__delete(0x307653c0)
>
> Note that only one elf_function instance is freed.
Thank you for the example, I get the problem now.
I think this would be the fix:
diff --git a/btf_encoder.c b/btf_encoder.c
index cfe23d3..5317bce 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -177,6 +177,16 @@ static inline void elf_functions__delete(struct elf_functions *funcs)
free(funcs);
}
+static inline void elf_functions__delete_all()
+{
+ struct list_head *pos, *tmp;
+
+ list_for_each_safe(pos, tmp, &elf_functions_list) {
+ struct elf_functions *funcs = list_entry(pos, struct elf_functions, node);
+ elf_functions__delete(funcs);
+ }
+}
+
static int elf_functions__collect(struct elf_functions *functions);
int btf_encoder__pre_load_module(Dwfl_Module *mod, Elf *elf)
@@ -2136,8 +2146,7 @@ int btf_encoder__encode(struct btf_encoder *encoder)
err = btf_encoder__write_elf(encoder, encoder->btf, BTF_ELF_SEC);
}
- elf_functions__delete(elf_functions__get(encoder->functions.elf));
+ elf_functions__delete_all();
return err;
}
Can't think of a better place to call elf_functions__delete_all(). On
this patch, btf_encoder__add_saved_funcs() is still called at the
entry into btf_encoder__encode().
>
> [...]
>
> > > > > I think that elf_functions should be deleted in main() at the same
> > > > > level where btf_encoder__add_saved_funcs() is called.
> > > > >
> > > > > > /* index the ELF sections for later lookup */
> > > >
> > > > In this patch, the shared elf_functions is built but not used. It is
> > > > freed in btf_encoder__encode(), the line with TODO comment.
> > >
> > > My point was that the __delete call for all collected elf_function objects
> > > should be moved to main(). But maybe use `module_post_load()` hook instead,
> > > look for specific `elf` and delete it from the list? This way it would be
> > > both symmetrical to `module_pre_load()` and encapsulated within btf_encoder.c.
> >
> > I put the delete call in btf_encoder__encode() because I thought that
> > elf_functions information might be used there, but I haven't verified
> > that. It might be possible to do earlier.
> >
> > I don't think we can do it in post_module_load hook, because
> > threads_collect happens before cus__load_module is done, and merge of
> > the encoders happens in threads_collect.
>
>
> Good point.
