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,
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?
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?
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)
> + 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
> - 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
