On Thursday, October 10th, 2024 at 1:57 AM, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > On Wed, 2024-10-09 at 23:35 +0000, Ihor Solodrai wrote: > > > btf_encoder__save_func() accumulates observations of DWARF functions, > > maintaining an elf_function per function name. > > > > Part of this routine is a funcs_match() call, which requires access to > > BTF implicitly referenced by btf_encoder_func_state. In case when > > elf_functions table is maintained for each btf_encoder this is not an > > issue, because every btf_encoder_func_state available to this encoder > > can only reference its own BTF. However, if elf_functions becomes > > shared, existing elf_function/btf_encoder_function_state structs can > > be produced by other encoders with their own BTFs. In this case > > funcs_match() can not be called unless BTFs are available and writes > > to them are synchronized in some manner (which is not desirable). > > > Nit: Something is missing in explanation at this point. > In the paragraph above you describe why funcs_match() > can't be called concurrently. > > I think a sentence is needed explaining that the solution is to > forgo concurrent funcs_match() calls and simply accumulate info > about all functions in DWARF. > Ok, I'll try making the explanation better. Just so it's clear, in this patchset there are no changes to btf_encoder__save_func() at all. This patch changes what's stored in the table (list of elf_function, not a single elf_function), however there is still only one elf_function per btf_encoder. This means that if a particular encoder encounters the same function more than once, its btf_encoder_func_state is going to be merged in btf_encoder__save_func(). And in this case calling funcs_match() is ok, because the functions are in the context of the same encoder (hence the same BTF). I remember we discussed off-list simple accumulation of elf_function structs, and removing merge logic from btf_encoder__save_func(). I am hesitant to switch to that, because this would mean that multiple elf_function objects for the same encoder might be stored in elf_functions table. This in turn would require more changes. For example, how btf_encoder__find_function() should behave then? I am inclined to stick with current (v2) approach, but want to make sure you're aware of this. Let me know what you think. > > Accumulated states are later merged between encoders in > > btf_encoder__add_saved_funcs(). To enable sharing of the elf_functions > > table between encoders, two members are introduced to struct > > elf_function: > > - elf_function *next - to enable linked-list > > - btf_encoder *owner - to find elf_function corresponding to a > > particular encoder in a list > > > > Each element of elf_functions->entries is a head of a list that stores > > elf_function structs one for each encoder that searched for this > > function name at least once. > > > > btf_encoder__find_function() is modified to perform the search on > > shared elf_functions table, and atomically update a corresponding list > > as necessary. > > > > At this point each btf_encoder is still maintaining it's own > > elf_functions table, but the updated implementation is used. > > > > Suggested-by: Eduard Zingerman eddyz87@xxxxxxxxx > > Signed-off-by: Ihor Solodrai ihor.solodrai@xxxxx > > --- > > > Aside from a few nits, I think the logic makes sense. > Not sure if it is worth it to separate patches #4 and #5. > [...]
