On Monday, October 14th, 2024 at 12:38 PM, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
>
> On Mon, 2024-10-14 at 19:03 +0000, Ihor Solodrai wrote:
>
> [...]
>
> > The elf_functions_list itself is static, this line initializes it:
> >
> > static LIST_HEAD(elf_functions_list);
> >
> > elf_functions__new() creates a new (single) elf_functions struct,
> > initializes it by collecting the functions, and adds it to the static
> > list.
> >
> > Maybe __new is too cryptic. What do you think about renaming to
> > elf_functions_list__add(), or maybe even split into
> > elf_functions__new() and _list__add()?
>
>
> On a second thought, it is called only from `btf_encoder__pre_cus__load_module`,
> hence I don't think it is needed as a separate function at all.
Right. No function, no need to name it :)
>
> [...]
>
> > > > @@ -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().
>
> In such scenario encoder->functions.elf would point to the last ELF processed,
>
> thus function tables corresponding to other ELFs won't be deleted.
> (but see my suggestion below).
>
> > If I merge patches #3-#5, these changes would disappear. I can try
> > that, but then patch #3 will be quite big to review. What do you think?
>
>
> I think patch #3 is good as a separate patch.
This is why I thought of merging the patches. There is auxiliary code
necessary to not break intermediate state of the patchset, but it
makes review harder, because it's not obvious why this code is
there... An alternative is a bigger patch, which is also harder to review
¯\_(ツ)_/¯
>
> > > 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.
