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.
> 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.
> btf_encoder.c | 117 +++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 93 insertions(+), 24 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 9e05b56..f88c2e1 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -86,10 +86,15 @@ struct btf_encoder_func_state {
> };
>
> struct elf_function {
> + // bsearch key
> const char *name;
> - char *alias;
> - bool generated;
> - size_t prefixlen;
> + size_t prefixlen;
> + // for list search
> + struct elf_function *next;
> + const struct btf_encoder *owner;
Nit: 'encoder' might be a better name for this field.
> + // encoder-specific state
> + char *alias;
> + bool generated;
> struct btf_encoder_func_state state;
> };
>
> @@ -106,7 +111,7 @@ struct elf_functions {
> struct list_head node; // for elf_functions_list
> Elf *elf; // source ELF
> struct elf_symtab *symtab;
> - struct elf_function *entries;
> + struct elf_function *entries; // an array, each element is a head of a list
> int cnt;
> int suffix_cnt; /* number of .isra, .part etc */
> };
> @@ -169,10 +174,29 @@ static struct elf_functions *elf_functions__get(Elf *elf)
> return NULL;
> }
>
> -static inline void elf_functions__delete(struct elf_functions *funcs)
> +static void __elf_functions__delete(struct elf_functions *funcs)
> {
> + for (int i = 0; i < funcs->cnt; i++) {
> + // free all list elements except the head
> + struct elf_function *func = funcs->entries[i].next;
> +
> + while (func) {
> + struct elf_function *next = func->next;
> +
> + free(func->alias);
> + zfree(&func->state.annots);
> + zfree(&func->state.parms);
> + free(func);
> + func = next;
> + }
> + }
> free(funcs->entries);
> elf_symtab__delete(funcs->symtab);
> +}
> +
> +static inline void elf_functions__delete(struct elf_functions *funcs)
> +{
> + __elf_functions__delete(funcs);
> list_del(&funcs->node);
> free(funcs);
> }
> @@ -204,6 +228,7 @@ out_error:
> return NULL;
> }
>
> +
Nit: useless newline.
> int btf_encoder__pre_cus__load_module(struct process_dwflmod_parms *parms, Dwfl_Module *mod, Dwarf *dw, Elf *elf)
> {
> struct elf_functions *funcs = elf_functions__new(elf);
> @@ -1301,12 +1326,30 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct functio
> return 0;
> }
>
> +#define for_each_elf_function(func, head) \
> + for (struct elf_function *func = head; func; func = func->next)
>
> +static inline struct elf_function *find_elf_function_in_list(struct elf_function *list_head,
> + const struct btf_encoder *encoder)
Nit: just 'find_elf_function'?
> +{
> + for_each_elf_function(f, list_head) {
> + if (f->owner == encoder)
> + return f;
> + }
> +
> + return NULL;
> +}
> +
> static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
> {
> int i;
>
> for (i = 0; i < encoder->functions.cnt; i++) {
> - struct elf_function *func = &encoder->functions.entries[i];
> + struct elf_function *func = find_elf_function_in_list(&encoder->functions.entries[i], encoder);
> +
> + if (!func)
> + continue;
> +
> struct btf_encoder_func_state *state = &func->state;
> struct btf_encoder *other_encoder = NULL;
>
[...]
