Re: [PATCH v2 dwarves 1/3] btf_encoder: record BTF-centric function state instead of DWARF-centric

[Alan Maguire <alan.maguire@oracle.com>]

On 23/09/2024 15:19, Jiri Olsa wrote:
> On Mon, Sep 16, 2024 at 02:49:44PM +0100, Alan Maguire wrote:
> 
> SNIP
> 
>> Baseline
>>
>> $ time pahole -J vmlinux -j1 --btf_features=default
>>
>> real	0m17.268s
>> user	0m15.808s
>> sys	0m1.415s
>>
>> $ time pahole -J vmlinux -j8 --btf_features=default
>>
>> real	0m10.768s
>> user	0m30.793s
>> sys	0m4.199s
>>
>> With these changes:
>>
>> $ time pahole -J vmlinux -j1 --btf_features=default
>>
>> real	0m16.564s
>> user	0m16.029s
>> sys	0m0.492s
>>
>> $ time pahole -J vmlinux -j8 --btf_features=default
>>
>> real	0m8.332s
>> user	0m30.627s
>> sys	0m0.714s
>>
>> In terms of functions encoded, 360 fewer functions make it into
>> BTF due to the different approach in consistency checking, but after
>> examining these cases, they do appear to be legitimately inconsistent
>> functions where the optimized versions have parameter mismatches
>> with the non-optimized expectations.
>>
>> Mileage may vary of course, and any testing folks could do would
>> be greatly appreciated!
> 
> looks good, some comments below.. I was hoping to find some way
> to split the change, but can't think of any ;-)
>

thanks for trying it out and looking at the code Jiri! Replies below..


> thanks,
> jirka
> 
> 
> SNIP
> 
>> -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
>> +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, struct elf_function *func)
>>  {
>>  	struct btf *btf = encoder->btf;
>>  	const struct btf_type *t;
>>  	struct parameter *param;
>>  	uint16_t nr_params, param_idx;
>>  	int32_t id, type_id;
>> +	char tmp_name[KSYM_NAME_LEN];
>> +	const char *name;
>> +	struct btf_encoder_func_state *state = &func->state;
> 
> func could be NULL right?
>

good catch; I've moved the assignment to the code that deals with ELF
function prototype addition.

> SNIP
> 
>>  static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>>  {
>> -	fn->priv = encoder->cu;
>> -	if (func->function) {
>> -		struct function *existing = func->function;
>> -
>> -		/* If saving and we find an existing entry, we want to merge
>> -		 * observations across both functions, checking that the
>> -		 * "seen optimized parameters", "inconsistent prototype"
>> -		 * and "unexpected register" status is reflected in the
>> -		 * the func entry.
>> -		 * If the entry is new, record encoder state required
>> -		 * to add the local function later (encoder + type_id_off)
>> -		 * such that we can add the function later.
>> -		 */
>> -		existing->proto.optimized_parms |= fn->proto.optimized_parms;
>> -		existing->proto.unexpected_reg |= fn->proto.unexpected_reg;
>> -		if (!existing->proto.unexpected_reg && !existing->proto.inconsistent_proto &&
>> -		     !funcs__match(encoder, func, fn))
>> -			existing->proto.inconsistent_proto = 1;
>> -	} else {
>> -		func->state.type_id_off = encoder->type_id_off;
>> -		func->function = fn;
>> -		encoder->cu->functions_saved++;
> 
> we could remove functions_saved from cu now?
> 

good idea; removed that and the "processed" flag from ftypes.

> SNIP
> 
>> -static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
>> +static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>>  {
>> -	int btf_fnproto_id, btf_fn_id, tag_type_id;
>> -	struct llvm_annotation *annot;
>> +	int btf_fnproto_id, btf_fn_id, tag_type_id = 0;
>> +	int16_t component_idx = -1;
>>  	const char *name;
>> +	const char *value;
>> +	char tmp_value[KSYM_NAME_LEN];
>>  
>> -	btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
>> -	name = function__name(fn);
>> -	btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
>> +	btf_fnproto_id = btf_encoder__add_func_proto(encoder, fn ? &fn->proto : NULL, func);
>> +	name = func->alias ?: func->name;
>> +	if (btf_fnproto_id >= 0)
>> +		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
>>  	if (btf_fnproto_id < 0 || btf_fn_id < 0) {
>> -		printf("error: failed to encode function '%s'\n", function__name(fn));
>> +		printf("error: failed to encode function '%s': invalid %s\n", name, btf_fnproto_id < 0 ? "proto" : "func");
>>  		return -1;
>>  	}
>> -	list_for_each_entry(annot, &fn->annots, node) {
>> -		tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
>> -							annot->component_idx);
>> -		if (tag_type_id < 0) {
>> -			fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
>> -				annot->value, name, annot->component_idx);
>> -			return -1;
>> +	if (!fn) {
>> +		struct btf_encoder_func_state *state = &func->state;
>> +		uint16_t idx;
>> +
>> +		if (!state || state->nr_annots == 0)
> 
> it probably can't happen, but state will be allways != NULL in here.. should it be:
> 
> 		if (!func || state->nr_annots == 0)
> 

I've added assert()s to the btf_encoder__add_func[proto]() functions to
ensure that either the fn or func are non-NULL, since winding up with a
NULL fn/func is more of a programming error than a state we can wind up
in.
> 
>> +			return 0;
>> +
>> +		for (idx = 0; idx < state->nr_annots; idx++) {
>> +			struct btf_encoder_func_annot *a = &state->annots[idx];
>> +
>> +			value = btf__str_by_offset(encoder->btf, a->value);
>> +			/* adding BTF data may result in a mode of the
>> +			 * value string memory, so make a temporary copy.
>> +			 */
>> +			strncpy(tmp_value, value, sizeof(tmp_value));
>> +			component_idx = a->component_idx;
>> +
>> +			tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value, btf_fn_id, component_idx);
>> +			if (tag_type_id < 0)
>> +				break;
>> +		}
>> +	} else {
>> +		struct llvm_annotation *annot;
>> +
>> +		list_for_each_entry(annot, &fn->annots, node) {
>> +			value = annot->value;
>> +			component_idx = annot->component_idx;
>> +
>> +			tag_type_id = btf_encoder__add_decl_tag(encoder, value, btf_fn_id,
>> +								component_idx);
>> +			if (tag_type_id < 0)
>> +				break;
>>  		}
>>  	}
>> +	if (tag_type_id < 0) {
>> +		fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
>> +			value, name, component_idx);
>> +		return -1;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> -static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
>> +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 function *fn = func->function;
>> -		struct btf_encoder *other_encoder;
>> +		struct btf_encoder_func_state *state = &func->state;
>> +		struct btf_encoder *other_encoder = NULL;
>>  
>> -		if (!fn || fn->proto.processed)
>> +		if (!state || !state->initialized || state->processed)
>>  			continue;
> 
> state is now placed directly in elf_function so will allways be state != NULL
> 
>> -
>>  		/* merge optimized-out status across encoders; since each
>>  		 * encoder has the same elf symbol table we can use the
>>  		 * same index to access the same elf symbol.
>>  		 */
>>  		btf_encoders__for_each_encoder(other_encoder) {
>> -			struct function *other_fn;
>> +			struct elf_function *other_func;
>> +			struct btf_encoder_func_state *other_state;
>> +			uint8_t optimized, unexpected, inconsistent;
>>  
>>  			if (other_encoder == encoder)
>>  				continue;
>>  
>> -			other_fn = other_encoder->functions.entries[i].function;
>> -			if (!other_fn)
>> +			other_func = &other_encoder->functions.entries[i];
>> +			other_state = &other_func->state;
>> +			if (!other_state)
>>  				continue;
> 
> same as above it will allways be other_state != NULL
> 

both fixed now. thanks!


>> -			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
>> -			fn->proto.unexpected_reg |= other_fn->proto.unexpected_reg;
>> -			if (other_fn->proto.inconsistent_proto)
>> -				fn->proto.inconsistent_proto = 1;
>> -			if (!fn->proto.unexpected_reg && !fn->proto.inconsistent_proto &&
>> -			    !funcs__match(encoder, func, other_fn))
>> -				fn->proto.inconsistent_proto = 1;
>> -			other_fn->proto.processed = 1;
>> +			optimized = state->optimized_parms | other_state->optimized_parms;
>> +			unexpected = state->unexpected_reg | other_state->unexpected_reg;
>> +			inconsistent = state->inconsistent_proto | other_state->inconsistent_proto;
>> +			if (!unexpected && !inconsistent &&
>> +			    !funcs__match(encoder, func,
>> +					  encoder->btf, state,
>> +					  other_encoder->btf, other_state))
>> +				inconsistent = 1;
>> +			state->optimized_parms = other_state->optimized_parms = optimized;
>> +			state->unexpected_reg = other_state->unexpected_reg = unexpected;
>> +			state->inconsistent_proto = other_state->inconsistent_proto = inconsistent;
>> +
>> +			other_state->processed = 1;
> 
> SNIP