On Mon, 2024-10-14 at 21:15 +0000, Ihor Solodrai wrote:
[...]
> > > > > @@ -2369,6 +2444,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > > > > goto out;
> > > > > }
> > > > > encoder->functions.symtab = encoder->symtab;
> > > > > + encoder->functions.elf = cu->elf;
> > > >
> > > > This line is removed in a subsequent patches, is it needed because of
> > > > the __delete call in the hunk above? Note, that if you want to account
> > > > for situation when multiple ELFs are processed the cu->elf might
> > > > change, so that __delete call would only delete elf_functions for the
> > > > last processed cu->elf.
> > >
> > > I wanted to keep intermediate patches small while maintaining that
> > > nothing is broken. These lines are necessary, because on this patch
> > > each encoder is still building it's own table, and each table needs a
> > > reference to the elf.
> >
> >
> > The accompanying __delete call would happen here:
> >
> > int btf_encoder__encode(struct btf_encoder *encoder)
> > ...
> > elf_functions__delete(elf_functions__get(encoder->functions.elf));
> >
> >
> > Called from here:
> >
> > int main(int argc, char *argv[])
> > ...
> > err = btf_encoder__encode(btf_encoder);
> >
> > Encoders are per thread and elf table is reassigned upon each call to
> > btf_encoder__new, so assume the following scenario:
> > - single encoder;
> > - multiple ELFs processed.
>
> This is not what happens on this patch. Each encoder has it's own copy
> of elf_functions allocated in btf_encoder__new():
>
> struct btf_encoder *encoder = zalloc(sizeof(*encoder));
>
> And then filled below by:
>
> if (elf_functions__collect(&encoder->functions))
> goto out_delete;
>
> This part:
>
> encoder->functions.symtab = encoder->symtab;
> encoder->functions.elf = cu->elf;
>
> Is necessary for elf_functions__collect() to work correctly.
> And then this is freed in btf_encoder__delete():
>
> for (i = 0; i < encoder->functions.cnt; i++)
> btf_encoder__delete_func(&encoder->functions.entries[i]);
> encoder->functions.cnt = 0;
> free(encoder->functions.entries);
> encoder->functions.entries = NULL;
>
> The table created by the hook call is built *in addition* to tables
> for each encoder, and then freed by elf_functions__delete() call in
> btf_encoder__encode().
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.
[...]
> > > > 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.
