On Thu, 2024-11-28 at 01:23 +0000, Ihor Solodrai wrote:
> From: Alan Maguire <alan.maguire@xxxxxxxxxx>
>
> Have saved function representation point back at immutable ELF function
> table. This will make sharing the ELF function table across encoders
> easier. Simply accumulate saved functions for each encoder, and on
> completion combine them into a name-sorted list. Then carry out
> comparisons to check for inconsistent representations, skipping functions
> that are inconsistent in their representation.
>
> Thre is a small growth in maximum resident set size due to saving
> more functions; it grows from
>
> Maximum resident set size (kbytes): 701888
>
> to:
>
> Maximum resident set size (kbytes): 704168
>
> ...with this patch for -j1.
>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxx>
> ---
Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
I like what this patch does, a few nits below.
Note:
this patch leads to 58 less functions being generated,
compared to a previous patch, for my test configuration.
For example, functions like:
- hid_map_usage_clear
- jhash
- nlmsg_parse_deprecated_strict
Are not in the BTF anymore. It would be good if patch message could
explain why this happens.
[...]
> +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
> +{
> + struct btf_encoder_func_state **saved_fns, *s;
> + struct btf_encoder *e = NULL;
> + int i = 0, j, nr_saved_fns = 0;
> +
> + /* Retrieve function states from each encoder, combine them
> + * and sort by name, addr.
> + */
> + btf_encoders__for_each_encoder(e) {
> + list_for_each_entry(s, &e->func_states, node)
> + nr_saved_fns++;
> + }
> + /* Another thread already did this work */
> + if (nr_saved_fns == 0) {
> + printf("nothing to do for encoder...\n");
> + return 0;
> + }
Nit: this function is called from pahole_threads_collect():
static int pahole_threads_collect(...)
for (i = 0; i < nr_threads; i++)
...
err = btf_encoder__add_encoder(btf_encoder, threads[i]->encoder);
...
int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other)
...
btf_encoder__add_saved_funcs(other);
...
maybe move call to btf_encoder__add_saved_funcs() to pahole_threads_collect()
outside of the loop? So that comment about another thread won't be necessary.
> +
> + printf("got %d saved functions...\n", nr_saved_fns);
> + saved_fns = calloc(nr_saved_fns, sizeof(*saved_fns));
> + btf_encoders__for_each_encoder(e) {
> + list_for_each_entry(s, &e->func_states, node)
> + saved_fns[i++] = s;
> + }
> + printf("added %d saved fns\n", i);
> + qsort(saved_fns, nr_saved_fns, sizeof(*saved_fns), saved_functions_cmp);
> +
> + for (i = 0; i < nr_saved_fns; i = j) {
> + struct btf_encoder_func_state *state = saved_fns[i];
> +
> + /* Compare across sorted functions that match by name/prefix;
> + * share inconsistent/unexpected reg state between them.
> + */
> + j = i + 1;
> +
> + while (j < nr_saved_fns &&
> + saved_functions_combine(saved_fns[i], saved_fns[j]) == 0)
> + j++;
> +
> + /* do not exclude functions with optimized-out parameters; they
> + * may still be _called_ with the right parameter values, they
> + * just do not _use_ them. Only exclude functions with
> + * unexpected register use or multiple inconsistent prototypes.
> + */
> + if (!encoder->skip_encoding_inconsistent_proto ||
> + (!state->unexpected_reg && !state->inconsistent_proto)) {
> + if (btf_encoder__add_func(state->encoder, state)) {
> + free(saved_fns);
> + return -1;
> + }
> + }
> + }
> + /* Now that we are done with function states, free them. */
> + free(saved_fns);
> + btf_encoders__for_each_encoder(e)
> + btf_encoder__delete_saved_funcs(e);
> + return 0;
> +}
[...]
> @@ -2437,16 +2470,8 @@ out_delete:
> return NULL;
> }
>
> -void btf_encoder__delete_func(struct elf_function *func)
> -{
> - free(func->alias);
Nit: it looks like func->alias is never freed after this change.
> - zfree(&func->state.annots);
> - zfree(&func->state.parms);
> -}
[...]
