Re: [RFC PATCH 3/9] btf_encoder: separate elf function, saved function representations

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-11-28 at 01:23 +0000, Ihor Solodrai wrote:
> From: Alan Maguire <alan.maguire@oracle.com>
> 
> 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@oracle.com>
> Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

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);
> -}

[...]