* [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding
@ 2024-12-21 1:22 Ihor Solodrai
2024-12-21 1:22 ` [PATCH dwarves v3 1/8] btf_encoder: simplify function encoding Ihor Solodrai
` (7 more replies)
0 siblings, 8 replies; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:22 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
This is a v3 of the patchset aiming to speed up parallel reproducible
BTF encoding.
In comparison to v2:
- removed patch v2 03 adding pre_load_module hook
- removed patch v2 05 making use of the hook
- since we will have a single btf_encoder, there is no need to
collect ELF tables before encoders are created
- removed patch v2 07 adding btf_encoder_context
- patch v3 04 is a rewritten patch v2 06
- each btf_encoder now maintains it's own list of function
tables per ELF
- patch v3 07 is an updated patch v2 10
- dwarf_loader multithreading is adjusted attempting to minimize
blocking on locks
- new patch v3 08 increases the cu->obstack chunk size
- new patch v3 09 cleans up global list of encoders in btf_encoder.c
Testing:
- ./tests/tests pass on vmlinux built from bpf-next
- bpftool dump of reproducible BTF is identical to v1.28
Sample perf runs on 6.9 kernel with a production-like config, on a
machine with nproc=176:
This patchset:
Performance counter stats for '/home/isolodrai/dwarves/build/pahole -J -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs --lang_exclude=rust --btf_encode_detached=/dev/null .tmp_vmlinux.btf' (13 runs):
17,911.11 msec cpu-clock # 4.412 CPUs utilized ( +- 0.46% )
4.0600 +- 0.0116 seconds time elapsed ( +- 0.29% )
pahole/next (v1.28):
Performance counter stats for '/home/isolodrai/dwarves/build/pahole -J -j --btf_features=encode_force,var,float,enum64,decl_tag,type_tag,optimized_func,consistent_func,decl_tag_kfuncs --lang_exclude=rust --btf_encode_detached=/dev/null .tmp_vmlinux.btf' (13 runs):
82,289.12 msec cpu-clock # 17.427 CPUs utilized ( +- 0.54% )
4.7219 +- 0.0270 seconds time elapsed ( +- 0.57% )
v2: https://lore.kernel.org/dwarves/20241213223641.564002-1-ihor.solodrai@pm.me/
v1 RFC: https://lore.kernel.org/dwarves/20241128012341.4081072-1-ihor.solodrai@pm.me/
Alan Maguire (2):
btf_encoder: simplify function encoding
btf_encoder: separate elf function, saved function representations
Ihor Solodrai (6):
btf_encoder: introduce elf_functions struct type
btf_encoder: introduce elf_functions_list
btf_encoder: remove skip_encoding_inconsistent_proto
dwarf_loader: introduce cu->id
dwarf_loader: multithreading with a job/worker model
btf_encoder: clean up global encoders list
btf_encoder.c | 643 +++++++++++++++++++-----------------
btf_encoder.h | 7 +-
btf_loader.c | 2 +-
ctf_loader.c | 2 +-
dwarf_loader.c | 335 +++++++++++++------
dwarves.c | 44 ---
dwarves.h | 21 +-
pahole.c | 230 ++-----------
pdwtags.c | 3 +-
pfunct.c | 3 +-
tests/reproducible_build.sh | 5 +-
11 files changed, 605 insertions(+), 690 deletions(-)
--
2.47.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 1/8] btf_encoder: simplify function encoding
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
@ 2024-12-21 1:22 ` Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations Ihor Solodrai
` (6 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:22 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
From: Alan Maguire <alan.maguire@oracle.com>
Currently we have two modes of function encoding; one adds functions
based upon the first instance found and ignores inconsistent
representations. The second saves function representations and later
finds inconsistencies. The mode chosen is determined by
conf_load->skip_encoding_btf_inconsistent_proto.
The knock-on effect is that we need to support two modes in
btf_encoder__add_func(); one for each case. Simplify by using
the "save function" approach for both cases; only difference is
that we allow inconsistent representations if
skip_encoding_btf_inconsistent_proto is not set (it is set by default
for upstream kernels and has been for a while).
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>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Link: https://lore.kernel.org/dwarves/20241128012341.4081072-2-ihor.solodrai@pm.me/
---
btf_encoder.c | 79 +++++++++++++++++----------------------------------
1 file changed, 26 insertions(+), 53 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 3754884..0835848 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -88,7 +88,6 @@ struct btf_encoder_func_state {
struct elf_function {
const char *name;
char *alias;
- bool generated;
size_t prefixlen;
struct btf_encoder_func_state state;
};
@@ -120,6 +119,7 @@ struct btf_encoder {
force,
gen_floats,
skip_encoding_decl_tag,
+ skip_encoding_inconsistent_proto,
tag_kfuncs,
gen_distilled_base;
uint32_t array_index_id;
@@ -1165,18 +1165,18 @@ out:
return err;
}
-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 elf_function *func)
{
+ struct btf_encoder_func_state *state = &func->state;
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];
+ uint16_t idx;
- assert(fn != NULL || func != NULL);
-
- btf_fnproto_id = btf_encoder__add_func_proto(encoder, fn ? &fn->proto : NULL, func);
+ btf_fnproto_id = btf_encoder__add_func_proto(encoder, 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,
@@ -1186,40 +1186,23 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct functio
name, btf_fnproto_id < 0 ? "proto" : "func");
return -1;
}
- if (!fn) {
- struct btf_encoder_func_state *state = &func->state;
- uint16_t idx;
-
- if (state->nr_annots == 0)
- return 0;
+ if (state->nr_annots == 0)
+ return 0;
- for (idx = 0; idx < state->nr_annots; idx++) {
- struct btf_encoder_func_annot *a = &state->annots[idx];
+ 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) - 1);
- 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;
+ 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) - 1);
+ component_idx = a->component_idx;
- tag_type_id = btf_encoder__add_decl_tag(encoder, value, btf_fn_id,
- component_idx);
- if (tag_type_id < 0)
- break;
- }
+ tag_type_id = btf_encoder__add_decl_tag(encoder, tmp_value,
+ btf_fn_id, component_idx);
+ if (tag_type_id < 0)
+ break;
}
if (tag_type_id < 0) {
fprintf(stderr,
@@ -1277,8 +1260,9 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
* just do not _use_ them. Only exclude functions with
* unexpected register use or multiple inconsistent prototypes.
*/
- if (!state->unexpected_reg && !state->inconsistent_proto) {
- if (btf_encoder__add_func(encoder, NULL, func))
+ if (!encoder->skip_encoding_inconsistent_proto ||
+ (!state->unexpected_reg && !state->inconsistent_proto)) {
+ if (btf_encoder__add_func(encoder, func))
return -1;
}
state->processed = 1;
@@ -2353,6 +2337,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->force = conf_load->btf_encode_force;
encoder->gen_floats = conf_load->btf_gen_floats;
encoder->skip_encoding_decl_tag = conf_load->skip_encoding_btf_decl_tag;
+ encoder->skip_encoding_inconsistent_proto = conf_load->skip_encoding_btf_inconsistent_proto;
encoder->tag_kfuncs = conf_load->btf_decl_tag_kfuncs;
encoder->gen_distilled_base = conf_load->btf_gen_distilled_base;
encoder->verbose = verbose;
@@ -2558,7 +2543,6 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
cu__for_each_function(cu, core_id, fn) {
struct elf_function *func = NULL;
- bool save = false;
/*
* Skip functions that:
@@ -2580,15 +2564,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
/* prefer exact function name match... */
func = btf_encoder__find_function(encoder, name, 0);
- if (func) {
- if (func->generated)
- continue;
- if (conf_load->skip_encoding_btf_inconsistent_proto)
- save = true;
- else
- func->generated = true;
- } else if (encoder->functions.suffix_cnt &&
- conf_load->btf_gen_optimized) {
+ if (!func && encoder->functions.suffix_cnt &&
+ conf_load->btf_gen_optimized) {
/* falling back to name.isra.0 match if no exact
* match is found; only bother if we found any
* .suffix function names. The function
@@ -2599,7 +2576,6 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
func = btf_encoder__find_function(encoder, name,
strlen(name));
if (func) {
- save = true;
if (encoder->verbose)
printf("matched function '%s' with '%s'%s\n",
name, func->name,
@@ -2617,10 +2593,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
if (!func)
continue;
- if (save)
- err = btf_encoder__save_func(encoder, fn, func);
- else
- err = btf_encoder__add_func(encoder, fn, func);
+ err = btf_encoder__save_func(encoder, fn, func);
if (err)
goto out;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
2024-12-21 1:22 ` [PATCH dwarves v3 1/8] btf_encoder: simplify function encoding Ihor Solodrai
@ 2024-12-21 1:23 ` Ihor Solodrai
2025-01-01 16:56 ` Jiri Olsa
2024-12-21 1:23 ` [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type Ihor Solodrai
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:23 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
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.
/usr/bin/time samples with this change:
jobs 1, mem 837844 Kb, time 6.40 sec
jobs 2, mem 936204 Kb, time 3.88 sec
jobs 4, mem 1023120 Kb, time 2.75 sec
jobs 8, mem 1163824 Kb, time 2.31 sec
jobs 16, mem 1190588 Kb, time 2.08 sec
jobs 32, mem 1341180 Kb, time 2.36 sec
/usr/bin/time samples on next (3ddadc1):
jobs 1, mem 834100 Kb, time 6.20 sec
jobs 2, mem 925048 Kb, time 3.81 sec
jobs 4, mem 1025424 Kb, time 2.88 sec
jobs 8, mem 1178480 Kb, time 2.21 sec
jobs 16, mem 1241780 Kb, time 2.07 sec
jobs 32, mem 1442316 Kb, time 2.33 sec
Link: https://lore.kernel.org/dwarves/20241128012341.4081072-4-ihor.solodrai@pm.me/
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Co-developed-by: Ihor Solodrai <ihor.solodrai@pm.me>
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
---
btf_encoder.c | 293 +++++++++++++++++++++++++++-----------------------
btf_encoder.h | 1 +
pahole.c | 2 +
3 files changed, 161 insertions(+), 135 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 0835848..f558346 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -72,14 +72,15 @@ struct btf_encoder_func_annot {
/* state used to do later encoding of saved functions */
struct btf_encoder_func_state {
+ struct list_head node;
+ struct btf_encoder *encoder;
+ struct elf_function *elf;
uint32_t type_id_off;
uint16_t nr_parms;
uint16_t nr_annots;
- uint8_t initialized:1;
uint8_t optimized_parms:1;
uint8_t unexpected_reg:1;
uint8_t inconsistent_proto:1;
- uint8_t processed:1;
int ret_type_id;
struct btf_encoder_func_parm *parms;
struct btf_encoder_func_annot *annots;
@@ -89,7 +90,6 @@ struct elf_function {
const char *name;
char *alias;
size_t prefixlen;
- struct btf_encoder_func_state state;
};
struct elf_secinfo {
@@ -126,6 +126,7 @@ struct btf_encoder {
struct elf_secinfo *secinfo;
size_t seccnt;
int encode_vars;
+ struct list_head func_states;
struct {
struct elf_function *entries;
int allocated;
@@ -148,8 +149,6 @@ struct btf_kfunc_set_range {
static LIST_HEAD(encoders);
static pthread_mutex_t encoders__lock = PTHREAD_MUTEX_INITIALIZER;
-static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder);
-
/* mutex only needed for add/delete, as this can happen in multiple encoding
* threads. Traversal of the list is currently confined to thread collection.
*/
@@ -693,25 +692,26 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
}
static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype,
- struct elf_function *func)
+ struct btf_encoder_func_state *state)
{
- struct btf *btf = encoder->btf;
const struct btf_type *t;
+ struct btf *btf;
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;
- assert(ftype != NULL || func != NULL);
+ assert(ftype != NULL || state != NULL);
/* add btf_type for func_proto */
if (ftype) {
+ btf = encoder->btf;
nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
- } else if (func) {
- state = &func->state;
+ } else if (state) {
+ encoder = state->encoder;
+ btf = state->encoder->btf;
nr_params = state->nr_parms;
type_id = state->ret_type_id;
} else {
@@ -801,8 +801,6 @@ int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder
if (encoder == other)
return 0;
- btf_encoder__add_saved_funcs(other);
-
for (shndx = 1; shndx < other->seccnt; shndx++) {
struct gobuffer *var_secinfo_buf = &other->secinfo[shndx].secinfo;
size_t sz = gobuffer__size(var_secinfo_buf);
@@ -1031,10 +1029,13 @@ static bool types__match(struct btf_encoder *encoder,
return false;
}
-static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
- struct btf *btf1, struct btf_encoder_func_state *s1,
- struct btf *btf2, struct btf_encoder_func_state *s2)
+static bool funcs__match(struct btf_encoder_func_state *s1,
+ struct btf_encoder_func_state *s2)
{
+ struct btf_encoder *encoder = s1->encoder;
+ struct elf_function *func = s1->elf;
+ struct btf *btf1 = s1->encoder->btf;
+ struct btf *btf2 = s2->encoder->btf;
uint8_t i;
if (s1->nr_parms != s2->nr_parms) {
@@ -1072,8 +1073,7 @@ static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func,
static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
{
- struct btf_encoder_func_state *existing = &func->state;
- struct btf_encoder_func_state state = { 0 };
+ struct btf_encoder_func_state *state = zalloc(sizeof(*state));
struct ftype *ftype = &fn->proto;
struct btf *btf = encoder->btf;
struct llvm_annotation *annot;
@@ -1081,22 +1081,23 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
uint8_t param_idx = 0;
int str_off, err = 0;
- /* if already skipping this function, no need to proceed. */
- if (existing->unexpected_reg || existing->inconsistent_proto)
- return 0;
+ if (!state)
+ return -ENOMEM;
- state.nr_parms = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
- state.ret_type_id = ftype->tag.type == 0 ? 0 : encoder->type_id_off + ftype->tag.type;
- if (state.nr_parms > 0) {
- state.parms = zalloc(state.nr_parms * sizeof(*state.parms));
- if (!state.parms) {
+ state->encoder = encoder;
+ state->elf = func;
+ state->nr_parms = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
+ state->ret_type_id = ftype->tag.type == 0 ? 0 : encoder->type_id_off + ftype->tag.type;
+ if (state->nr_parms > 0) {
+ state->parms = zalloc(state->nr_parms * sizeof(*state->parms));
+ if (!state->parms) {
err = -ENOMEM;
goto out;
}
}
- state.inconsistent_proto = ftype->inconsistent_proto;
- state.unexpected_reg = ftype->unexpected_reg;
- state.optimized_parms = ftype->optimized_parms;
+ state->inconsistent_proto = ftype->inconsistent_proto;
+ state->unexpected_reg = ftype->unexpected_reg;
+ state->optimized_parms = ftype->optimized_parms;
ftype__for_each_parameter(ftype, param) {
const char *name = parameter__name(param) ?: "";
@@ -1105,21 +1106,21 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
err = str_off;
goto out;
}
- state.parms[param_idx].name_off = str_off;
- state.parms[param_idx].type_id = param->tag.type == 0 ? 0 :
- encoder->type_id_off + param->tag.type;
+ state->parms[param_idx].name_off = str_off;
+ state->parms[param_idx].type_id = param->tag.type == 0 ? 0 :
+ encoder->type_id_off + param->tag.type;
param_idx++;
}
if (ftype->unspec_parms)
- state.parms[param_idx].type_id = 0;
+ state->parms[param_idx].type_id = 0;
list_for_each_entry(annot, &fn->annots, node)
- state.nr_annots++;
- if (state.nr_annots) {
+ state->nr_annots++;
+ if (state->nr_annots) {
uint8_t idx = 0;
- state.annots = zalloc(state.nr_annots * sizeof(*state.annots));
- if (!state.annots) {
+ state->annots = zalloc(state->nr_annots * sizeof(*state->annots));
+ if (!state->annots) {
err = -ENOMEM;
goto out;
}
@@ -1129,46 +1130,24 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
err = str_off;
goto out;
}
- state.annots[idx].value = str_off;
- state.annots[idx].component_idx = annot->component_idx;
+ state->annots[idx].value = str_off;
+ state->annots[idx].component_idx = annot->component_idx;
idx++;
}
}
- state.initialized = 1;
-
- if (state.unexpected_reg)
- btf_encoder__log_func_skip(encoder, func,
- "unexpected register used for parameter\n");
- if (!existing->initialized) {
- memcpy(existing, &state, sizeof(*existing));
- return 0;
- }
-
- /* 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
- * 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->optimized_parms |= state.optimized_parms;
- existing->unexpected_reg |= state.unexpected_reg;
- if (!existing->unexpected_reg &&
- !funcs__match(encoder, func, encoder->btf, &state,
- encoder->btf, existing))
- existing->inconsistent_proto = 1;
+ list_add_tail(&state->node, &encoder->func_states);
+ return 0;
out:
- zfree(&state.annots);
- zfree(&state.parms);
+ zfree(&state->annots);
+ zfree(&state->parms);
+ free(state);
return err;
}
static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
- struct elf_function *func)
+ struct btf_encoder_func_state *state)
{
- struct btf_encoder_func_state *state = &func->state;
+ struct elf_function *func = state->elf;
int btf_fnproto_id, btf_fn_id, tag_type_id = 0;
int16_t component_idx = -1;
const char *name;
@@ -1176,7 +1155,7 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
char tmp_value[KSYM_NAME_LEN];
uint16_t idx;
- btf_fnproto_id = btf_encoder__add_func_proto(encoder, NULL, func);
+ btf_fnproto_id = btf_encoder__add_func_proto(encoder, NULL, state);
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,
@@ -1214,62 +1193,6 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
return 0;
}
-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 btf_encoder_func_state *state = &func->state;
- struct btf_encoder *other_encoder = NULL;
-
- if (!state->initialized || state->processed)
- continue;
- /* 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 elf_function *other_func;
- struct btf_encoder_func_state *other_state;
- uint8_t optimized, unexpected, inconsistent;
-
- if (other_encoder == encoder)
- continue;
-
- other_func = &other_encoder->functions.entries[i];
- other_state = &other_func->state;
- if (!other_state->initialized)
- continue;
- 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;
- }
- /* 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(encoder, func))
- return -1;
- }
- state->processed = 1;
- }
- return 0;
-}
-
static int functions_cmp(const void *_a, const void *_b)
{
const struct elf_function *a = _a;
@@ -1297,6 +1220,110 @@ static void *reallocarray_grow(void *ptr, int *nmemb, size_t size)
return new;
}
+static int saved_functions_cmp(const void *_a, const void *_b)
+{
+ struct btf_encoder_func_state * const *a = _a;
+ struct btf_encoder_func_state * const *b = _b;
+
+ return functions_cmp((*a)->elf, (*b)->elf);
+}
+
+static int saved_functions_combine(void *_a, void *_b)
+{
+ uint8_t optimized, unexpected, inconsistent;
+ struct btf_encoder_func_state *a = _a;
+ struct btf_encoder_func_state *b = _b;
+ int ret;
+
+ ret = strncmp(a->elf->name, b->elf->name,
+ max(a->elf->prefixlen, b->elf->prefixlen));
+ if (ret != 0)
+ return ret;
+ optimized = a->optimized_parms | b->optimized_parms;
+ unexpected = a->unexpected_reg | b->unexpected_reg;
+ inconsistent = a->inconsistent_proto | b->inconsistent_proto;
+ if (!unexpected && !inconsistent && !funcs__match(a, b))
+ inconsistent = 1;
+ a->optimized_parms = b->optimized_parms = optimized;
+ a->unexpected_reg = b->unexpected_reg = unexpected;
+ a->inconsistent_proto = b->inconsistent_proto = inconsistent;
+
+ return 0;
+}
+
+static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
+{
+ struct btf_encoder_func_state *pos, *s;
+
+ list_for_each_entry_safe(pos, s, &encoder->func_states, node) {
+ list_del(&pos->node);
+ free(pos->parms);
+ free(pos->annots);
+ free(pos);
+ }
+
+ for (int i = 0; i < encoder->functions.cnt; i++)
+ free(encoder->functions.entries[i].alias);
+}
+
+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++;
+ }
+
+ if (nr_saved_fns == 0)
+ return 0;
+
+ 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;
+ }
+ 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;
+}
+
static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *sym)
{
struct elf_function *new;
@@ -1330,6 +1357,8 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
encoder->functions.suffix_cnt++;
encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
+ } else {
+ encoder->functions.entries[encoder->functions.cnt].prefixlen = strlen(name);
}
encoder->functions.cnt++;
return 0;
@@ -2345,6 +2374,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->need_index_type = false;
encoder->array_index_id = 0;
encoder->encode_vars = 0;
+ INIT_LIST_HEAD(&encoder->func_states);
if (!conf_load->skip_encoding_btf_vars)
encoder->encode_vars |= BTF_VAR_PERCPU;
if (conf_load->encode_btf_global_vars)
@@ -2429,16 +2459,8 @@ out_delete:
return NULL;
}
-void btf_encoder__delete_func(struct elf_function *func)
-{
- free(func->alias);
- zfree(&func->state.annots);
- zfree(&func->state.parms);
-}
-
void btf_encoder__delete(struct btf_encoder *encoder)
{
- int i;
size_t shndx;
if (encoder == NULL)
@@ -2447,18 +2469,19 @@ void btf_encoder__delete(struct btf_encoder *encoder)
btf_encoders__delete(encoder);
for (shndx = 0; shndx < encoder->seccnt; shndx++)
__gobuffer__delete(&encoder->secinfo[shndx].secinfo);
+ free(encoder->secinfo);
zfree(&encoder->filename);
zfree(&encoder->source_filename);
btf__free(encoder->btf);
encoder->btf = NULL;
elf_symtab__delete(encoder->symtab);
- for (i = 0; i < encoder->functions.cnt; i++)
- btf_encoder__delete_func(&encoder->functions.entries[i]);
encoder->functions.allocated = encoder->functions.cnt = 0;
free(encoder->functions.entries);
encoder->functions.entries = NULL;
+ btf_encoder__delete_saved_funcs(encoder);
+
free(encoder);
}
diff --git a/btf_encoder.h b/btf_encoder.h
index 824963b..9b26162 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -33,5 +33,6 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
struct btf *btf_encoder__btf(struct btf_encoder *encoder);
int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
+int btf_encoder__add_saved_funcs(struct btf_encoder *encoder);
#endif /* _BTF_ENCODER_H_ */
diff --git a/pahole.c b/pahole.c
index fa5d8c7..a36b732 100644
--- a/pahole.c
+++ b/pahole.c
@@ -3185,6 +3185,7 @@ static int pahole_threads_collect(struct conf_load *conf, int nr_threads, void *
if (error)
goto out;
+ btf_encoder__add_saved_funcs(btf_encoder);
for (i = 0; i < nr_threads; i++) {
/*
* Merge content of the btf instances of worker threads to the btf
@@ -3843,6 +3844,7 @@ try_sole_arg_as_class_names:
}
err = btf_encoder__encode(btf_encoder);
+ btf_encoder__delete(btf_encoder);
if (err) {
fputs("Failed to encode BTF\n", stderr);
goto out_cus_delete;
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
2024-12-21 1:22 ` [PATCH dwarves v3 1/8] btf_encoder: simplify function encoding Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations Ihor Solodrai
@ 2024-12-21 1:23 ` Ihor Solodrai
2025-01-01 16:56 ` Jiri Olsa
2024-12-21 1:23 ` [PATCH dwarves v3 4/8] btf_encoder: introduce elf_functions_list Ihor Solodrai
` (4 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:23 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
Extract elf_functions struct type from btf_encoder.
Replace routines operating on functions table in btf_encoder by
routines operating on elf_functions:
- btf_encoder__collect_function -> elf_functions__collect_function
- btf_encoder__collect_symbols -> elf_functions__collect
Now these functions do not depend on btf_encoder being passed to them
as a parameter.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
Link: https://lore.kernel.org/dwarves/20241128012341.4081072-6-ihor.solodrai@pm.me/
---
btf_encoder.c | 120 +++++++++++++++++++++++++++-----------------------
1 file changed, 64 insertions(+), 56 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index f558346..1467e09 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -101,6 +101,13 @@ struct elf_secinfo {
struct gobuffer secinfo;
};
+struct elf_functions {
+ struct elf_symtab *symtab;
+ struct elf_function *entries;
+ int cnt;
+ int suffix_cnt; /* number of .isra, .part etc */
+};
+
/*
* cu: cu being processed.
*/
@@ -127,12 +134,7 @@ struct btf_encoder {
size_t seccnt;
int encode_vars;
struct list_head func_states;
- struct {
- struct elf_function *entries;
- int allocated;
- int cnt;
- int suffix_cnt; /* number of .isra, .part etc */
- } functions;
+ struct elf_functions functions;
};
struct btf_func {
@@ -1210,16 +1212,6 @@ static int functions_cmp(const void *_a, const void *_b)
#define max(x, y) ((x) < (y) ? (y) : (x))
#endif
-static void *reallocarray_grow(void *ptr, int *nmemb, size_t size)
-{
- int new_nmemb = max(1000, *nmemb * 3 / 2);
- void *new = realloc(ptr, new_nmemb * size);
-
- if (new)
- *nmemb = new_nmemb;
- return new;
-}
-
static int saved_functions_cmp(const void *_a, const void *_b)
{
struct btf_encoder_func_state * const *a = _a;
@@ -1324,44 +1316,29 @@ int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
return 0;
}
-static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *sym)
+static void elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
{
- struct elf_function *new;
+ struct elf_function *func;
const char *name;
if (elf_sym__type(sym) != STT_FUNC)
- return 0;
- name = elf_sym__name(sym, encoder->symtab);
- if (!name)
- return 0;
+ return;
- if (encoder->functions.cnt == encoder->functions.allocated) {
- new = reallocarray_grow(encoder->functions.entries,
- &encoder->functions.allocated,
- sizeof(*encoder->functions.entries));
- if (!new) {
- /*
- * The cleanup - delete_functions is called
- * in btf_encoder__encode_cu error path.
- */
- return -1;
- }
- encoder->functions.entries = new;
- }
+ name = elf_sym__name(sym, functions->symtab);
+ if (!name)
+ return;
- memset(&encoder->functions.entries[encoder->functions.cnt], 0,
- sizeof(*new));
- encoder->functions.entries[encoder->functions.cnt].name = name;
+ func = &functions->entries[functions->cnt];
+ func->name = name;
if (strchr(name, '.')) {
const char *suffix = strchr(name, '.');
-
- encoder->functions.suffix_cnt++;
- encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
+ functions->suffix_cnt++;
+ func->prefixlen = suffix - name;
} else {
- encoder->functions.entries[encoder->functions.cnt].prefixlen = strlen(name);
+ func->prefixlen = strlen(name);
}
- encoder->functions.cnt++;
- return 0;
+
+ functions->cnt++;
}
static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
@@ -2126,26 +2103,56 @@ int btf_encoder__encode(struct btf_encoder *encoder)
return err;
}
-
-static int btf_encoder__collect_symbols(struct btf_encoder *encoder)
+static int elf_functions__collect(struct elf_functions *functions)
{
- uint32_t sym_sec_idx;
+ uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab);
+ struct elf_function *tmp;
+ Elf32_Word sym_sec_idx;
uint32_t core_id;
GElf_Sym sym;
+ int err;
- elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) {
- if (btf_encoder__collect_function(encoder, &sym))
- return -1;
+ /* We know that number of functions is less than number of symbols,
+ * so we can overallocate temporarily.
+ */
+ functions->entries = calloc(nr_symbols, sizeof(*functions->entries));
+ if (!functions->entries) {
+ err = -ENOMEM;
+ goto out_free;
+ }
+
+ functions->cnt = 0;
+ elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {
+ elf_functions__collect_function(functions, &sym);
}
- if (encoder->functions.cnt) {
- qsort(encoder->functions.entries, encoder->functions.cnt, sizeof(encoder->functions.entries[0]),
+ if (functions->cnt) {
+ qsort(functions->entries,
+ functions->cnt,
+ sizeof(*functions->entries),
functions_cmp);
- if (encoder->verbose)
- printf("Found %d functions!\n", encoder->functions.cnt);
+ } else {
+ err = 0;
+ goto out_free;
+ }
+
+ /* Reallocate to the exact size */
+ tmp = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
+ if (tmp) {
+ functions->entries = tmp;
+ } else {
+ fprintf(stderr, "could not reallocate memory for elf_functions table\n");
+ err = -ENOMEM;
+ goto out_free;
}
return 0;
+
+out_free:
+ free(functions->entries);
+ functions->entries = NULL;
+ functions->cnt = 0;
+ return err;
}
static bool ftype__has_arg_names(const struct ftype *ftype)
@@ -2406,6 +2413,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
printf("%s: '%s' doesn't have symtab.\n", __func__, cu->filename);
goto out;
}
+ encoder->functions.symtab = encoder->symtab;
/* index the ELF sections for later lookup */
@@ -2444,7 +2452,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
if (!found_percpu && encoder->verbose)
printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
- if (btf_encoder__collect_symbols(encoder))
+ if (elf_functions__collect(&encoder->functions))
goto out_delete;
if (encoder->verbose)
@@ -2476,7 +2484,7 @@ void btf_encoder__delete(struct btf_encoder *encoder)
encoder->btf = NULL;
elf_symtab__delete(encoder->symtab);
- encoder->functions.allocated = encoder->functions.cnt = 0;
+ encoder->functions.cnt = 0;
free(encoder->functions.entries);
encoder->functions.entries = NULL;
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 4/8] btf_encoder: introduce elf_functions_list
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
` (2 preceding siblings ...)
2024-12-21 1:23 ` [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type Ihor Solodrai
@ 2024-12-21 1:23 ` Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 5/8] btf_encoder: remove skip_encoding_inconsistent_proto Ihor Solodrai
` (3 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:23 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
In case of processing of multiple DWARF modules, multiple ELFs are
read. This requires maintaining and elf_functions table per ELF.
Replace btf_encoder.functions with btf_encoder.elf_functions_list,
that contains all necessary elf_functions tables.
The list is initialized when btf_encoder is created. When a new CU is
assigned to the encoder in btf_encoder__encode_cu, an elf_functions
table will be created if the CU is coming from an unknown Elf.
This patch is a variant of [1], following a discussion at [2].
[1] https://lore.kernel.org/bpf/20241213223641.564002-7-ihor.solodrai@pm.me/
[2] https://lore.kernel.org/bpf/C82bYTvJaV4bfT15o25EsBiUvFsj5eTlm17933Hvva76CXjIcu3gvpaOCWPgeZ8g3cZ-RMa8Vp0y1o_QMR2LhPB-LEUYfZCGuCfR_HvkIP8=@pm.me/
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
---
btf_encoder.c | 138 +++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 115 insertions(+), 23 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 1467e09..566ecfe 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -102,6 +102,8 @@ struct elf_secinfo {
};
struct elf_functions {
+ struct list_head node; /* for elf_functions_list */
+ Elf *elf; /* source ELF */
struct elf_symtab *symtab;
struct elf_function *entries;
int cnt;
@@ -134,7 +136,11 @@ struct btf_encoder {
size_t seccnt;
int encode_vars;
struct list_head func_states;
- struct elf_functions functions;
+ /* This is a list of elf_functions tables, one per ELF.
+ * Multiple ELF modules can be processed in one pahole run,
+ * so we have to store elf_functions tables per ELF.
+ */
+ struct list_head elf_functions_list;
};
struct btf_func {
@@ -148,6 +154,72 @@ struct btf_kfunc_set_range {
uint64_t end;
};
+static inline void elf_functions__delete(struct elf_functions *funcs)
+{
+ for (int i = 0; i < funcs->cnt; i++)
+ free(funcs->entries[i].alias);
+ free(funcs->entries);
+ elf_symtab__delete(funcs->symtab);
+ list_del(&funcs->node);
+ free(funcs);
+}
+
+static int elf_functions__collect(struct elf_functions *functions);
+
+struct elf_functions *elf_functions__new(Elf *elf)
+{
+ struct elf_functions *funcs;
+ int err;
+
+ funcs = calloc(1, sizeof(*funcs));
+ if (!funcs) {
+ err = -ENOMEM;
+ goto out_delete;
+ }
+
+ funcs->symtab = elf_symtab__new(NULL, elf);
+ if (!funcs->symtab) {
+ err = -1;
+ goto out_delete;
+ }
+
+ funcs->elf = elf;
+ err = elf_functions__collect(funcs);
+ if (err < 0)
+ goto out_delete;
+
+ return funcs;
+
+out_delete:
+ elf_functions__delete(funcs);
+ return NULL;
+}
+
+static inline void elf_functions_list__clear(struct list_head *elf_functions_list)
+{
+ struct elf_functions *funcs;
+ struct list_head *pos, *tmp;
+
+ list_for_each_safe(pos, tmp, elf_functions_list) {
+ funcs = list_entry(pos, struct elf_functions, node);
+ elf_functions__delete(funcs);
+ }
+}
+
+static struct elf_functions *elf_functions__find(const Elf *elf, const struct list_head *elf_functions_list)
+{
+ struct elf_functions *funcs;
+ struct list_head *pos;
+
+ list_for_each(pos, elf_functions_list) {
+ funcs = list_entry(pos, struct elf_functions, node);
+ if (funcs->elf == elf)
+ return funcs;
+ }
+ return NULL;
+}
+
+
static LIST_HEAD(encoders);
static pthread_mutex_t encoders__lock = PTHREAD_MUTEX_INITIALIZER;
@@ -1253,9 +1325,6 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
free(pos->annots);
free(pos);
}
-
- for (int i = 0; i < encoder->functions.cnt; i++)
- free(encoder->functions.entries[i].alias);
}
int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
@@ -1341,12 +1410,30 @@ static void elf_functions__collect_function(struct elf_functions *functions, GEl
functions->cnt++;
}
+static struct elf_functions *btf_encoder__elf_functions(struct btf_encoder *encoder)
+{
+ struct elf_functions *funcs = NULL;
+
+ if (!encoder->cu || !encoder->cu->elf)
+ return NULL;
+
+ funcs = elf_functions__find(encoder->cu->elf, &encoder->elf_functions_list);
+ if (!funcs) {
+ funcs = elf_functions__new(encoder->cu->elf);
+ if (funcs)
+ list_add(&funcs->node, &encoder->elf_functions_list);
+ }
+
+ return funcs;
+}
+
static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
const char *name, size_t prefixlen)
{
+ struct elf_functions *funcs = elf_functions__find(encoder->cu->elf, &encoder->elf_functions_list);
struct elf_function key = { .name = name, .prefixlen = prefixlen };
- return bsearch(&key, encoder->functions.entries, encoder->functions.cnt, sizeof(key), functions_cmp);
+ return bsearch(&key, funcs->entries, funcs->cnt, sizeof(key), functions_cmp);
}
static bool btf_name_char_ok(char c, bool first)
@@ -2100,6 +2187,8 @@ int btf_encoder__encode(struct btf_encoder *encoder)
#endif
err = btf_encoder__write_elf(encoder, encoder->btf, BTF_ELF_SEC);
}
+
+ elf_functions_list__clear(&encoder->elf_functions_list);
return err;
}
@@ -2358,8 +2447,10 @@ out:
struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load)
{
struct btf_encoder *encoder = zalloc(sizeof(*encoder));
+ struct elf_functions *funcs = NULL;
if (encoder) {
+ encoder->cu = cu;
encoder->raw_output = detached_filename != NULL;
encoder->source_filename = strdup(cu->filename);
encoder->filename = strdup(encoder->raw_output ? detached_filename : cu->filename);
@@ -2387,6 +2478,13 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
if (conf_load->encode_btf_global_vars)
encoder->encode_vars |= BTF_VAR_GLOBAL;
+ INIT_LIST_HEAD(&encoder->elf_functions_list);
+ funcs = btf_encoder__elf_functions(encoder);
+ if (!funcs)
+ goto out_delete;
+
+ encoder->symtab = funcs->symtab;
+
GElf_Ehdr ehdr;
if (gelf_getehdr(cu->elf, &ehdr) == NULL) {
@@ -2407,14 +2505,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
goto out_delete;
}
- encoder->symtab = elf_symtab__new(NULL, cu->elf);
- if (!encoder->symtab) {
- if (encoder->verbose)
- printf("%s: '%s' doesn't have symtab.\n", __func__, cu->filename);
- goto out;
- }
- encoder->functions.symtab = encoder->symtab;
-
/* index the ELF sections for later lookup */
GElf_Shdr shdr;
@@ -2452,14 +2542,11 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
if (!found_percpu && encoder->verbose)
printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
- if (elf_functions__collect(&encoder->functions))
- goto out_delete;
-
if (encoder->verbose)
printf("File %s:\n", cu->filename);
btf_encoders__add(encoder);
}
-out:
+
return encoder;
out_delete:
@@ -2482,11 +2569,8 @@ void btf_encoder__delete(struct btf_encoder *encoder)
zfree(&encoder->source_filename);
btf__free(encoder->btf);
encoder->btf = NULL;
- elf_symtab__delete(encoder->symtab);
- encoder->functions.cnt = 0;
- free(encoder->functions.entries);
- encoder->functions.entries = NULL;
+ elf_functions_list__clear(&encoder->elf_functions_list);
btf_encoder__delete_saved_funcs(encoder);
@@ -2497,12 +2581,20 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
{
struct llvm_annotation *annot;
int btf_type_id, tag_type_id, skipped_types = 0;
+ struct elf_functions *funcs;
uint32_t core_id;
struct function *fn;
struct tag *pos;
int err = 0;
encoder->cu = cu;
+ funcs = btf_encoder__elf_functions(encoder);
+ if (!funcs) {
+ err = -1;
+ goto out;
+ }
+ encoder->symtab = funcs->symtab;
+
encoder->type_id_off = btf__type_cnt(encoder->btf) - 1;
if (!encoder->has_index_type) {
@@ -2586,7 +2678,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
continue;
if (!ftype__has_arg_names(&fn->proto))
continue;
- if (encoder->functions.cnt) {
+ if (funcs->cnt) {
const char *name;
name = function__name(fn);
@@ -2595,7 +2687,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
/* prefer exact function name match... */
func = btf_encoder__find_function(encoder, name, 0);
- if (!func && encoder->functions.suffix_cnt &&
+ if (!func && funcs->suffix_cnt &&
conf_load->btf_gen_optimized) {
/* falling back to name.isra.0 match if no exact
* match is found; only bother if we found any
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 5/8] btf_encoder: remove skip_encoding_inconsistent_proto
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
` (3 preceding siblings ...)
2024-12-21 1:23 ` [PATCH dwarves v3 4/8] btf_encoder: introduce elf_functions_list Ihor Solodrai
@ 2024-12-21 1:23 ` Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 6/8] dwarf_loader: introduce cu->id Ihor Solodrai
` (2 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:23 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
This flag is needed only for btf_encoder__add_saved_funcs(), so there
is no reason to keep it in each btf_encoder.
Link: https://lore.kernel.org/dwarves/e1df45360963d265ea5e0b3634f0a3dae0c9c343.camel@gmail.com/
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
---
btf_encoder.c | 10 ++++------
btf_encoder.h | 4 ++--
pahole.c | 7 +++++--
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 566ecfe..90f1b9a 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -128,7 +128,6 @@ struct btf_encoder {
force,
gen_floats,
skip_encoding_decl_tag,
- skip_encoding_inconsistent_proto,
tag_kfuncs,
gen_distilled_base;
uint32_t array_index_id;
@@ -1327,7 +1326,7 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
}
}
-int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
+int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
{
struct btf_encoder_func_state **saved_fns, *s;
struct btf_encoder *e = NULL;
@@ -1367,7 +1366,7 @@ int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
* just do not _use_ them. Only exclude functions with
* unexpected register use or multiple inconsistent prototypes.
*/
- if (!encoder->skip_encoding_inconsistent_proto ||
+ if (!skip_encoding_inconsistent_proto ||
(!state->unexpected_reg && !state->inconsistent_proto)) {
if (btf_encoder__add_func(state->encoder, state)) {
free(saved_fns);
@@ -2129,14 +2128,14 @@ out:
return err;
}
-int btf_encoder__encode(struct btf_encoder *encoder)
+int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
{
bool should_tag_kfuncs;
int err;
size_t shndx;
/* for single-threaded case, saved funcs are added here */
- btf_encoder__add_saved_funcs(encoder);
+ btf_encoder__add_saved_funcs(conf->skip_encoding_btf_inconsistent_proto);
for (shndx = 1; shndx < encoder->seccnt; shndx++)
if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
@@ -2464,7 +2463,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
encoder->force = conf_load->btf_encode_force;
encoder->gen_floats = conf_load->btf_gen_floats;
encoder->skip_encoding_decl_tag = conf_load->skip_encoding_btf_decl_tag;
- encoder->skip_encoding_inconsistent_proto = conf_load->skip_encoding_btf_inconsistent_proto;
encoder->tag_kfuncs = conf_load->btf_decl_tag_kfuncs;
encoder->gen_distilled_base = conf_load->btf_gen_distilled_base;
encoder->verbose = verbose;
diff --git a/btf_encoder.h b/btf_encoder.h
index 9b26162..b95f2f3 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -26,13 +26,13 @@ enum btf_var_option {
struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
void btf_encoder__delete(struct btf_encoder *encoder);
-int btf_encoder__encode(struct btf_encoder *encoder);
+int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf);
int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load);
struct btf *btf_encoder__btf(struct btf_encoder *encoder);
int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
-int btf_encoder__add_saved_funcs(struct btf_encoder *encoder);
+int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto);
#endif /* _BTF_ENCODER_H_ */
diff --git a/pahole.c b/pahole.c
index a36b732..37d76b1 100644
--- a/pahole.c
+++ b/pahole.c
@@ -3185,7 +3185,10 @@ static int pahole_threads_collect(struct conf_load *conf, int nr_threads, void *
if (error)
goto out;
- btf_encoder__add_saved_funcs(btf_encoder);
+ err = btf_encoder__add_saved_funcs(conf_load.skip_encoding_btf_inconsistent_proto);
+ if (err < 0)
+ goto out;
+
for (i = 0; i < nr_threads; i++) {
/*
* Merge content of the btf instances of worker threads to the btf
@@ -3843,7 +3846,7 @@ try_sole_arg_as_class_names:
exit(1);
}
- err = btf_encoder__encode(btf_encoder);
+ err = btf_encoder__encode(btf_encoder, &conf_load);
btf_encoder__delete(btf_encoder);
if (err) {
fputs("Failed to encode BTF\n", stderr);
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 6/8] dwarf_loader: introduce cu->id
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
` (4 preceding siblings ...)
2024-12-21 1:23 ` [PATCH dwarves v3 5/8] btf_encoder: remove skip_encoding_inconsistent_proto Ihor Solodrai
@ 2024-12-21 1:23 ` Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list Ihor Solodrai
7 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:23 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
Add an id member to the struct cu.
An id is an index of a CU, in order they are created in dwarf_loader.c
This allows for an easy identification of a CU, particularly when they
need to be processed in order.
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
---
dwarf_loader.c | 4 ++++
dwarves.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/dwarf_loader.c b/dwarf_loader.c
index 598fde4..4f07e17 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -3440,6 +3440,7 @@ struct dwarf_cus {
int build_id_len;
int error;
struct dwarf_cu *type_dcu;
+ uint32_t nr_cus_created;
};
struct dwarf_thread {
@@ -3472,6 +3473,9 @@ static struct dwarf_cu *dwarf_cus__create_cu(struct dwarf_cus *dcus, Dwarf_Die *
cu->priv = dcu;
cu->dfops = &dwarf__ops;
+ cu->id = dcus->nr_cus_created;
+ dcus->nr_cus_created++;
+
return dcu;
}
diff --git a/dwarves.h b/dwarves.h
index 1cb0d62..2d08883 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -290,6 +290,7 @@ struct cu {
struct ptr_table functions_table;
struct ptr_table tags_table;
struct rb_root functions;
+ uint32_t id;
const char *name;
char *filename;
void *priv;
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
` (5 preceding siblings ...)
2024-12-21 1:23 ` [PATCH dwarves v3 6/8] dwarf_loader: introduce cu->id Ihor Solodrai
@ 2024-12-21 1:23 ` Ihor Solodrai
2024-12-30 16:07 ` Arnaldo Carvalho de Melo
2025-01-01 16:56 ` Jiri Olsa
2024-12-21 1:23 ` [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list Ihor Solodrai
7 siblings, 2 replies; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:23 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
Multithreading is now contained in dwarf_loader.c, and is implemented
using a jobs queue and a pool of worker threads. As a consequence,
multithreading-related code is removed from pahole.c.
A single-thread special case is removed: queueing setup works fine
with a single worker, which will switch between jobs as appropriate.
Code supporting previous version of the multithreading, such as
cu_state, thread_data and related functions, is also removed.
reproducible_build flag is now moot: the BTF encoding is always
reproducible with these changes.
The goal outlined in the RFC [1] - making parallel reproducible BTF
generation as fast as non-reproducible - is achieved by implementing
the requirement of ordered CU encoding (stealing) directly in the job
queue in dwarf_loader.c
The synchronization in the queue is implemented by a mutex (which
ensures consistency of queue state) and a job_added condition
variable. Motivation behind using condition variables is a classic
one: we want to avoid the threads checking the state of the queue in a
busy loop, competing for a single mutex.
In comparison to the previous version of this patch [2], job_taken
condition variable is removed. The number of decoded CUs in memory is
now limited by initial JOB_DECODE jobs. The enqueue/dequeue interface
is changed aiming to reduce locking. See relevant discussion [3].
[1] https://lore.kernel.org/dwarves/20241128012341.4081072-1-ihor.solodrai@pm.me/
[2] https://lore.kernel.org/dwarves/20241213223641.564002-11-ihor.solodrai@pm.me/
[3] https://lore.kernel.org/dwarves/58dc053c9d47a18124d8711604b08acbc6400340.camel@gmail.com/
Co-developed-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
---
btf_encoder.c | 3 +-
btf_encoder.h | 2 -
btf_loader.c | 2 +-
ctf_loader.c | 2 +-
dwarf_loader.c | 331 +++++++++++++++++++++++++-----------
dwarves.c | 44 -----
dwarves.h | 20 +--
pahole.c | 231 +++----------------------
pdwtags.c | 3 +-
pfunct.c | 3 +-
tests/reproducible_build.sh | 5 +-
11 files changed, 266 insertions(+), 380 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 90f1b9a..7e03ba4 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1326,7 +1326,7 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
}
}
-int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
+static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
{
struct btf_encoder_func_state **saved_fns, *s;
struct btf_encoder *e = NULL;
@@ -2134,7 +2134,6 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
int err;
size_t shndx;
- /* for single-threaded case, saved funcs are added here */
btf_encoder__add_saved_funcs(conf->skip_encoding_btf_inconsistent_proto);
for (shndx = 1; shndx < encoder->seccnt; shndx++)
diff --git a/btf_encoder.h b/btf_encoder.h
index b95f2f3..0081a99 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -33,6 +33,4 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
struct btf *btf_encoder__btf(struct btf_encoder *encoder);
int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
-int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto);
-
#endif /* _BTF_ENCODER_H_ */
diff --git a/btf_loader.c b/btf_loader.c
index 4814f29..cff0011 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -730,7 +730,7 @@ static int cus__load_btf(struct cus *cus, struct conf_load *conf, const char *fi
* The app stole this cu, possibly deleting it,
* so forget about it
*/
- if (conf && conf->steal && conf->steal(cu, conf, NULL))
+ if (conf && conf->steal && conf->steal(cu, conf))
return 0;
cus__add(cus, cu);
diff --git a/ctf_loader.c b/ctf_loader.c
index 944bf6e..501c4ab 100644
--- a/ctf_loader.c
+++ b/ctf_loader.c
@@ -728,7 +728,7 @@ int ctf__load_file(struct cus *cus, struct conf_load *conf,
* The app stole this cu, possibly deleting it,
* so forget about it
*/
- if (conf && conf->steal && conf->steal(cu, conf, NULL))
+ if (conf && conf->steal && conf->steal(cu, conf))
return 0;
cus__add(cus, cu);
diff --git a/dwarf_loader.c b/dwarf_loader.c
index 4f07e17..526e3d9 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -3250,24 +3250,20 @@ static void cu__sort_types_by_offset(struct cu *cu, struct conf_load *conf)
cu__for_all_tags(cu, type__sort_by_offset, conf);
}
-static int cu__finalize(struct cu *cu, struct cus *cus, struct conf_load *conf, void *thr_data)
+static void cu__finalize(struct cu *cu, struct cus *cus, struct conf_load *conf)
{
cu__for_all_tags(cu, class_member__cache_byte_size, conf);
if (cu__language_reorders_offsets(cu))
cu__sort_types_by_offset(cu, conf);
-
- cus__set_cu_state(cus, cu, CU__LOADED);
-
- if (conf && conf->steal) {
- return conf->steal(cu, conf, thr_data);
- }
- return LSK__KEEPIT;
}
-static int cus__finalize(struct cus *cus, struct cu *cu, struct conf_load *conf, void *thr_data)
+static int cus__steal_now(struct cus *cus, struct cu *cu, struct conf_load *conf)
{
- int lsk = cu__finalize(cu, cus, conf, thr_data);
+ if (!conf || !conf->steal)
+ return 0;
+
+ int lsk = conf->steal(cu, conf);
switch (lsk) {
case LSK__DELETE:
cus__remove(cus, cu);
@@ -3443,11 +3439,110 @@ struct dwarf_cus {
uint32_t nr_cus_created;
};
-struct dwarf_thread {
- struct dwarf_cus *dcus;
- void *data;
+/* Multithreading is implemented using a job/worker model.
+ * cus_processing_queue represents a collection of jobs to be
+ * completed by workers.
+ * dwarf_loader__worker_thread is the worker loop, taking jobs from
+ * the queue, executing them and re-enqueuing new jobs as necessary.
+ * Implementation of this queue ensures two constraints:
+ * * JOB_STEAL jobs for a CU are executed in the order of cu->id, as
+ * a consequence JOB_STEAL jobs always run one at a time.
+ * * Initial number of JOB_DECODE jobs in the queue is effectively a
+ * limit on how many decoded CUs can be held in memory.
+ * See dwarf_loader__decoded_cus_limit()
+ */
+static struct {
+ pthread_mutex_t mutex;
+ pthread_cond_t job_added;
+ /* next_cu_id determines the next CU ready to be stealed
+ * This enforces the order of CU stealing.
+ */
+ uint32_t next_cu_id;
+ struct list_head jobs;
+} cus_processing_queue;
+
+enum job_type {
+ JOB_NONE = 0,
+ JOB_DECODE = 1,
+ JOB_STEAL = 2,
+};
+
+struct cu_processing_job {
+ struct list_head node;
+ enum job_type type;
+ struct cu *cu; /* for JOB_STEAL */
};
+static void cus_queue__init(void)
+{
+ pthread_mutex_init(&cus_processing_queue.mutex, NULL);
+ pthread_cond_init(&cus_processing_queue.job_added, NULL);
+ INIT_LIST_HEAD(&cus_processing_queue.jobs);
+ cus_processing_queue.next_cu_id = 0;
+}
+
+static void cus_queue__destroy(void)
+{
+ pthread_mutex_destroy(&cus_processing_queue.mutex);
+ pthread_cond_destroy(&cus_processing_queue.job_added);
+}
+
+static inline void cus_queue__inc_next_cu_id(void)
+{
+ pthread_mutex_lock(&cus_processing_queue.mutex);
+ cus_processing_queue.next_cu_id++;
+ pthread_mutex_unlock(&cus_processing_queue.mutex);
+}
+
+static struct cu_processing_job *cus_queue__try_dequeue(void)
+{
+ struct cu_processing_job *job, *dequeued_job = NULL;
+ struct list_head *pos, *tmp;
+
+ list_for_each_safe(pos, tmp, &cus_processing_queue.jobs) {
+ job = list_entry(pos, struct cu_processing_job, node);
+ if (job->type == JOB_STEAL && job->cu->id == cus_processing_queue.next_cu_id) {
+ dequeued_job = job;
+ break;
+ }
+ if (job->type == JOB_DECODE) {
+ /* all JOB_STEALs are added to the head, so no viable JOB_STEAL available */
+ dequeued_job = job;
+ break;
+ }
+ }
+ /* No jobs or only steals out of order */
+ if (!dequeued_job)
+ return NULL;
+
+ list_del(&dequeued_job->node);
+ return job;
+}
+
+static struct cu_processing_job *cus_queue__enqdeq_job(struct cu_processing_job *job)
+{
+ pthread_mutex_lock(&cus_processing_queue.mutex);
+ if (job) {
+ /* JOB_STEAL have higher priority, add them to the head so
+ * they can be found faster
+ */
+ if (job->type == JOB_STEAL)
+ list_add(&job->node, &cus_processing_queue.jobs);
+ else
+ list_add_tail(&job->node, &cus_processing_queue.jobs);
+ pthread_cond_signal(&cus_processing_queue.job_added);
+ }
+ for (;;) {
+ job = cus_queue__try_dequeue();
+ if (job)
+ break;
+ /* No jobs or only steals out of order */
+ pthread_cond_wait(&cus_processing_queue.job_added, &cus_processing_queue.mutex);
+ }
+ pthread_mutex_unlock(&cus_processing_queue.mutex);
+ return job;
+}
+
static struct dwarf_cu *dwarf_cus__create_cu(struct dwarf_cus *dcus, Dwarf_Die *cu_die, uint8_t pointer_size)
{
/*
@@ -3479,28 +3574,6 @@ static struct dwarf_cu *dwarf_cus__create_cu(struct dwarf_cus *dcus, Dwarf_Die *
return dcu;
}
-static int dwarf_cus__process_cu(struct dwarf_cus *dcus, Dwarf_Die *cu_die,
- struct cu *cu, void *thr_data)
-{
- if (die__process_and_recode(cu_die, cu, dcus->conf) != 0 ||
- cus__finalize(dcus->cus, cu, dcus->conf, thr_data) == LSK__STOP_LOADING)
- return DWARF_CB_ABORT;
-
- return DWARF_CB_OK;
-}
-
-static int dwarf_cus__create_and_process_cu(struct dwarf_cus *dcus, Dwarf_Die *cu_die, uint8_t pointer_size)
-{
- struct dwarf_cu *dcu = dwarf_cus__create_cu(dcus, cu_die, pointer_size);
-
- if (dcu == NULL)
- return DWARF_CB_ABORT;
-
- cus__add(dcus->cus, dcu->cu);
-
- return dwarf_cus__process_cu(dcus, cu_die, dcu->cu, NULL);
-}
-
static int dwarf_cus__nextcu(struct dwarf_cus *dcus, struct dwarf_cu **dcu,
Dwarf_Die *die_mem, Dwarf_Die **cu_die,
uint8_t *pointer_size, uint8_t *offset_size)
@@ -3541,24 +3614,76 @@ out_unlock:
return ret;
}
-static void *dwarf_cus__process_cu_thread(void *arg)
+static struct cu *dwarf_loader__decode_next_cu(struct dwarf_cus *dcus)
{
- struct dwarf_thread *dthr = arg;
- struct dwarf_cus *dcus = dthr->dcus;
uint8_t pointer_size, offset_size;
+ struct dwarf_cu *dcu = NULL;
Dwarf_Die die_mem, *cu_die;
- struct dwarf_cu *dcu;
+ int err;
- while (dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size) == 0) {
- if (cu_die == NULL)
+ err = dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size);
+
+ if (err < 0)
+ goto out_error;
+ else if (err == 1) /* no more CUs */
+ return NULL;
+
+ err = die__process_and_recode(cu_die, dcu->cu, dcus->conf);
+ if (err)
+ goto out_error;
+ if (cu_die == NULL)
+ return NULL;
+
+ cu__finalize(dcu->cu, dcus->cus, dcus->conf);
+
+ return dcu->cu;
+
+out_error:
+ dcus->error = err;
+ fprintf(stderr, "error decoding cu %s\n", dcu && dcu->cu ? dcu->cu->name : "");
+ return NULL;
+}
+
+static void *dwarf_loader__worker_thread(void *arg)
+{
+ struct cu_processing_job *job = NULL;
+ struct dwarf_cus *dcus = arg;
+ bool stop = false;
+ struct cu *cu;
+
+ while (!stop) {
+ job = cus_queue__enqdeq_job(job);
+ switch (job->type) {
+
+ case JOB_DECODE:
+ cu = dwarf_loader__decode_next_cu(dcus);
+
+ if (cu == NULL) {
+ free(job);
+ stop = true;
+ break;
+ }
+
+ job->type = JOB_STEAL;
+ job->cu = cu;
break;
- if (dwarf_cus__process_cu(dcus, cu_die, dcu->cu, dthr->data) == DWARF_CB_ABORT)
+ case JOB_STEAL:
+ if (cus__steal_now(dcus->cus, job->cu, dcus->conf) == LSK__STOP_LOADING)
+ goto out_abort;
+ cus_queue__inc_next_cu_id();
+ /* re-enqueue JOB_DECODE so that next CU is decoded from DWARF */
+ job->type = JOB_DECODE;
+ job->cu = NULL;
+ break;
+
+ default:
+ fprintf(stderr, "Unknown dwarf_loader job type %d\n", job->type);
goto out_abort;
+ }
}
- if (dcus->conf->thread_exit &&
- dcus->conf->thread_exit(dcus->conf, dthr->data) != 0)
+ if (dcus->error)
goto out_abort;
return (void *)DWARF_CB_OK;
@@ -3566,29 +3691,64 @@ out_abort:
return (void *)DWARF_CB_ABORT;
}
-static int dwarf_cus__threaded_process_cus(struct dwarf_cus *dcus)
+/*
+ * If workers pick up next cu for decoding as soon as they're ready,
+ * then the memory usage may greatly increase, if the stealer can't
+ * keep up with incoming work.
+ * If we want to avoid this there needs to be a limit on how many
+ * decoded, but not yet stolen, CUs we allow to hold in memory. When
+ * this limit is reached the workers will wait for more CUs to get
+ * stolen.
+ * The limit is enforced by the number of JOB_DECODE jobs we enqueue
+ * before the workers have started decoding. A job serves as a
+ * "ticket": worker can proceed with decoding only if it has a ticket.
+ *
+ * As for the value of this limit, it must be at least N, where N is
+ * the number of workers. If the limit < N, some workers will never
+ * work. Setting the limit higher, while allows for higher memory
+ * consumption, does not necessarily improves the total pahole
+ * runtime, likely due to increased concurrent memory allocation.
+ *
+ * Here are some data points that led to the chosen value:
+ *
+ * perf stat -e cpu-clock -r13 ... pahole -J -j$(nproc) ... vmlinux
+ * limit=N 1.58878 +- 0.00859 seconds time elapsed ( +- 0.54% )
+ * limit=Nx2 1.58532 +- 0.00405 seconds time elapsed ( +- 0.26% ) # best
+ * limit=Nx4 1.59415 +- 0.00413 seconds time elapsed ( +- 0.26% )
+ * limit=Nx8 1.62584 +- 0.00448 seconds time elapsed ( +- 0.28% )
+ * limit=Nx1024 1.92333 +- 0.00765 seconds time elapsed ( +- 0.40% )
+ */
+static inline int dwarf_loader__decoded_cus_limit(const struct conf_load *conf)
{
- pthread_t threads[dcus->conf->nr_jobs];
- struct dwarf_thread dthr[dcus->conf->nr_jobs];
- void *thread_data[dcus->conf->nr_jobs];
- int res;
+ return conf->nr_jobs > 0 ? conf->nr_jobs * 2 : 2;
+}
+
+static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
+{
+ int nr_workers = dcus->conf->nr_jobs > 0 ? dcus->conf->nr_jobs : 1;
+ pthread_t workers[nr_workers];
+ struct cu_processing_job *job;
int i;
- if (dcus->conf->threads_prepare) {
- res = dcus->conf->threads_prepare(dcus->conf, dcus->conf->nr_jobs, thread_data);
- if (res != 0)
- return res;
- } else {
- memset(thread_data, 0, sizeof(void *) * dcus->conf->nr_jobs);
- }
+ cus_queue__init();
- for (i = 0; i < dcus->conf->nr_jobs; ++i) {
- dthr[i].dcus = dcus;
- dthr[i].data = thread_data[i];
+ /* Fill up the queue with JOB_DECODE jobs.
+ */
+ for (i = 0; i < dwarf_loader__decoded_cus_limit(dcus->conf); i++) {
+ job = calloc(1, sizeof(*job));
+ if (!job) {
+ dcus->error = -ENOMEM;
+ goto out_error;
+ }
+ job->type = JOB_DECODE;
+ /* no need for locks, workers were not started yet */
+ list_add(&job->node, &cus_processing_queue.jobs);
+ }
- dcus->error = pthread_create(&threads[i], NULL,
- dwarf_cus__process_cu_thread,
- &dthr[i]);
+ for (i = 0; i < nr_workers; ++i) {
+ dcus->error = pthread_create(&workers[i], NULL,
+ dwarf_loader__worker_thread,
+ dcus);
if (dcus->error)
goto out_join;
}
@@ -3598,52 +3758,18 @@ static int dwarf_cus__threaded_process_cus(struct dwarf_cus *dcus)
out_join:
while (--i >= 0) {
void *res;
- int err = pthread_join(threads[i], &res);
+ int err = pthread_join(workers[i], &res);
if (err == 0 && res != NULL)
dcus->error = (long)res;
}
- if (dcus->conf->threads_collect) {
- res = dcus->conf->threads_collect(dcus->conf, dcus->conf->nr_jobs,
- thread_data, dcus->error);
- if (dcus->error == 0)
- dcus->error = res;
- }
+out_error:
+ cus_queue__destroy();
return dcus->error;
}
-static int __dwarf_cus__process_cus(struct dwarf_cus *dcus)
-{
- uint8_t pointer_size, offset_size;
- Dwarf_Off noff;
- size_t cuhl;
-
- while (dwarf_nextcu(dcus->dw, dcus->off, &noff, &cuhl, NULL, &pointer_size, &offset_size) == 0) {
- Dwarf_Die die_mem;
- Dwarf_Die *cu_die = dwarf_offdie(dcus->dw, dcus->off + cuhl, &die_mem);
-
- if (cu_die == NULL)
- break;
-
- if (dwarf_cus__create_and_process_cu(dcus, cu_die, pointer_size) == DWARF_CB_ABORT)
- return DWARF_CB_ABORT;
-
- dcus->off = noff;
- }
-
- return 0;
-}
-
-static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
-{
- if (dcus->conf->nr_jobs > 1)
- return dwarf_cus__threaded_process_cus(dcus);
-
- return __dwarf_cus__process_cus(dcus);
-}
-
static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
Dwfl_Module *mod, Dwarf *dw, Elf *elf,
const char *filename,
@@ -3733,7 +3859,8 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
if (cu__resolve_func_ret_types_optimized(cu) != LSK__KEEPIT)
goto out_abort;
- if (cus__finalize(cus, cu, conf, NULL) == LSK__STOP_LOADING)
+ cu__finalize(cu, cus, conf);
+ if (cus__steal_now(cus, cu, conf) == LSK__STOP_LOADING)
goto out_abort;
return 0;
@@ -3765,7 +3892,8 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
}
if (type_cu != NULL) {
- type_lsk = cu__finalize(type_cu, cus, conf, NULL);
+ cu__finalize(type_cu, cus, conf);
+ type_lsk = cus__steal_now(cus, type_cu, conf);
if (type_lsk == LSK__DELETE) {
cus__remove(cus, type_cu);
}
@@ -3787,6 +3915,7 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
.type_dcu = type_cu ? &type_dcu : NULL,
.build_id = build_id,
.build_id_len = build_id_len,
+ .nr_cus_created = 0,
};
res = dwarf_cus__process_cus(&dcus);
}
diff --git a/dwarves.c b/dwarves.c
index ae512b9..7c3e878 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -530,48 +530,6 @@ void cus__unlock(struct cus *cus)
pthread_mutex_unlock(&cus->mutex);
}
-void cus__set_cu_state(struct cus *cus, struct cu *cu, enum cu_state state)
-{
- cus__lock(cus);
- cu->state = state;
- cus__unlock(cus);
-}
-
-// Used only when reproducible builds are desired
-struct cu *cus__get_next_processable_cu(struct cus *cus)
-{
- struct cu *cu;
-
- cus__lock(cus);
-
- list_for_each_entry(cu, &cus->cus, node) {
- switch (cu->state) {
- case CU__LOADED:
- cu->state = CU__PROCESSING;
- goto found;
- case CU__PROCESSING:
- // This will happen when we get to parallel
- // reproducible BTF encoding, libbpf dedup work needed
- // here. The other possibility is when we're flushing
- // the DWARF processed CUs when the parallel DWARF
- // loading stoped and we still have CUs to encode to
- // BTF because of ordering requirements.
- continue;
- case CU__UNPROCESSED:
- // The first entry isn't loaded, signal the
- // caller to return and try another day, as we
- // need to respect the original DWARF CU ordering.
- goto out;
- }
- }
-out:
- cu = NULL;
-found:
- cus__unlock(cus);
-
- return cu;
-}
-
bool cus__empty(const struct cus *cus)
{
return list_empty(&cus->cus);
@@ -808,8 +766,6 @@ struct cu *cu__new(const char *name, uint8_t addr_size,
cu->addr_size = addr_size;
cu->extra_dbg_info = 0;
- cu->state = CU__UNPROCESSED;
-
cu->nr_inline_expansions = 0;
cu->size_inline_expansions = 0;
cu->nr_structures_changed = 0;
diff --git a/dwarves.h b/dwarves.h
index 2d08883..70b4ddf 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -44,12 +44,6 @@ enum load_steal_kind {
LSK__STOP_LOADING,
};
-enum cu_state {
- CU__UNPROCESSED,
- CU__LOADED,
- CU__PROCESSING,
-};
-
/*
* BTF combines all the types into one big CU using btf_dedup(), so for something
* like a allyesconfig vmlinux kernel we can get over 65535 types.
@@ -60,7 +54,6 @@ struct btf;
struct conf_fprintf;
/** struct conf_load - load configuration
- * @thread_exit - called at the end of a thread, 1st user: BTF encoder dedup
* @extra_dbg_info - keep original debugging format extra info
* (e.g. DWARF's decl_{line,file}, id, etc)
* @fixup_silly_bitfields - Fixup silly things such as "int foo:32;"
@@ -70,11 +63,9 @@ struct conf_fprintf;
* @skip_missing - skip missing types rather than bailing out.
*/
struct conf_load {
- enum load_steal_kind (*steal)(struct cu *cu,
- struct conf_load *conf,
- void *thr_data);
+ enum load_steal_kind (*steal)(struct cu *cu, struct conf_load *conf);
struct cu * (*early_cu_filter)(struct cu *cu);
- int (*thread_exit)(struct conf_load *conf, void *thr_data);
+ int (*pre_load_module)(Dwfl_Module *mod, Elf *elf);
void *cookie;
char *format_path;
int nr_jobs;
@@ -105,8 +96,6 @@ struct conf_load {
const char *kabi_prefix;
struct btf *base_btf;
struct conf_fprintf *conf_fprintf;
- int (*threads_prepare)(struct conf_load *conf, int nr_threads, void **thr_data);
- int (*threads_collect)(struct conf_load *conf, int nr_threads, void **thr_data, int error);
};
/** struct conf_fprintf - hints to the __fprintf routines
@@ -188,10 +177,6 @@ void cus__add(struct cus *cus, struct cu *cu);
void __cus__remove(struct cus *cus, struct cu *cu);
void cus__remove(struct cus *cus, struct cu *cu);
-struct cu *cus__get_next_processable_cu(struct cus *cus);
-
-void cus__set_cu_state(struct cus *cus, struct cu *cu, enum cu_state state);
-
void cus__print_error_msg(const char *progname, const struct cus *cus,
const char *filename, const int err);
struct cu *cus__find_pair(struct cus *cus, const char *name);
@@ -308,7 +293,6 @@ struct cu {
uint8_t nr_register_params;
int register_params[ARCH_MAX_REGISTER_PARAMS];
int functions_saved;
- enum cu_state state;
uint16_t language;
unsigned long nr_inline_expansions;
size_t size_inline_expansions;
diff --git a/pahole.c b/pahole.c
index 37d76b1..af3e1cf 100644
--- a/pahole.c
+++ b/pahole.c
@@ -3118,6 +3118,32 @@ out:
return ret;
}
+static enum load_steal_kind pahole_stealer__btf_encode(struct cu *cu, struct conf_load *conf_load)
+{
+ int err;
+
+ if (!btf_encoder)
+ btf_encoder = btf_encoder__new(cu,
+ detached_btf_filename,
+ conf_load->base_btf,
+ global_verbose,
+ conf_load);
+
+ if (!btf_encoder) {
+ fprintf(stderr, "Error creating BTF encoder.\n");
+ return LSK__STOP_LOADING;
+ }
+
+ err = btf_encoder__encode_cu(btf_encoder, cu, conf_load);
+ if (err < 0) {
+ fprintf(stderr, "Error while encoding BTF.\n");
+ return LSK__STOP_LOADING;
+ }
+
+ return LSK__DELETE;
+}
+
+
static struct type_instance *header;
static bool print_enumeration_with_enumerator(struct cu *cu, const char *name)
@@ -3136,87 +3162,7 @@ static bool print_enumeration_with_enumerator(struct cu *cu, const char *name)
return false;
}
-struct thread_data {
- struct btf *btf;
- struct btf_encoder *encoder;
-};
-
-static int pahole_threads_prepare_reproducible_build(struct conf_load *conf, int nr_threads, void **thr_data)
-{
- for (int i = 0; i < nr_threads; i++)
- thr_data[i] = NULL;
-
- return 0;
-}
-
-static int pahole_threads_prepare(struct conf_load *conf, int nr_threads, void **thr_data)
-{
- int i;
- struct thread_data *threads = calloc(sizeof(struct thread_data), nr_threads);
-
- for (i = 0; i < nr_threads; i++)
- thr_data[i] = threads + i;
-
- return 0;
-}
-
-static int pahole_thread_exit(struct conf_load *conf, void *thr_data)
-{
- struct thread_data *thread = thr_data;
-
- if (thread == NULL)
- return 0;
-
- /*
- * Here we will call btf__dedup() here once we extend
- * btf__dedup().
- */
-
- return 0;
-}
-
-static int pahole_threads_collect(struct conf_load *conf, int nr_threads, void **thr_data,
- int error)
-{
- struct thread_data **threads = (struct thread_data **)thr_data;
- int i;
- int err = 0;
-
- if (error)
- goto out;
-
- err = btf_encoder__add_saved_funcs(conf_load.skip_encoding_btf_inconsistent_proto);
- if (err < 0)
- goto out;
-
- for (i = 0; i < nr_threads; i++) {
- /*
- * Merge content of the btf instances of worker threads to the btf
- * instance of the primary btf_encoder.
- */
- if (!threads[i]->btf)
- continue;
- err = btf_encoder__add_encoder(btf_encoder, threads[i]->encoder);
- if (err < 0)
- goto out;
- }
- err = 0;
-
-out:
- for (i = 0; i < nr_threads; i++) {
- if (threads[i]->encoder && threads[i]->encoder != btf_encoder) {
- btf_encoder__delete(threads[i]->encoder);
- threads[i]->encoder = NULL;
- }
- }
- free(threads[0]);
-
- return err;
-}
-
-static enum load_steal_kind pahole_stealer(struct cu *cu,
- struct conf_load *conf_load,
- void *thr_data)
+static enum load_steal_kind pahole_stealer(struct cu *cu, struct conf_load *conf_load)
{
int ret = LSK__DELETE;
@@ -3238,94 +3184,7 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
return LSK__DELETE; // Maybe we can find this in several CUs, so don't stop it
if (btf_encode) {
- static pthread_mutex_t btf_lock = PTHREAD_MUTEX_INITIALIZER;
- struct btf_encoder *encoder;
-
- pthread_mutex_lock(&btf_lock);
- /*
- * FIXME:
- *
- * This should be really done at main(), but since in the current codebase only at this
- * point we'll have cu->elf setup...
- */
- if (!btf_encoder) {
- /*
- * btf_encoder is the primary encoder.
- * And, it is used by the thread
- * create it.
- */
- btf_encoder = btf_encoder__new(cu, detached_btf_filename, conf_load->base_btf,
- global_verbose, conf_load);
- if (btf_encoder && thr_data) {
- struct thread_data *thread = thr_data;
-
- thread->encoder = btf_encoder;
- thread->btf = btf_encoder__btf(btf_encoder);
- }
- }
-
- // Reproducible builds don't have multiple btf_encoders, so we need to keep the lock until we encode BTF for this CU.
- if (thr_data)
- pthread_mutex_unlock(&btf_lock);
-
- if (!btf_encoder) {
- ret = LSK__STOP_LOADING;
- goto out_btf;
- }
-
- /*
- * thr_data keeps per-thread data for worker threads. Each worker thread
- * has an encoder. The main thread will merge the data collected by all
- * these encoders to btf_encoder. However, the first thread reaching this
- * function creates btf_encoder and reuses it as its local encoder. It
- * avoids copying the data collected by the first thread.
- */
- if (thr_data) {
- struct thread_data *thread = thr_data;
-
- if (thread->encoder == NULL) {
- thread->encoder =
- btf_encoder__new(cu, detached_btf_filename,
- NULL,
- global_verbose,
- conf_load);
- thread->btf = btf_encoder__btf(thread->encoder);
- }
- encoder = thread->encoder;
- } else {
- encoder = btf_encoder;
- }
-
- // Since we don't have yet a way to parallelize the BTF encoding, we
- // need to ask the loader for the next CU that we can process, one
- // that is loaded and is in order, if the next one isn't yet loaded,
- // then return to let the DWARF loader thread to load the next one,
- // eventually all will get processed, even if when all DWARF loading
- // threads finish.
- if (conf_load->reproducible_build) {
- ret = LSK__KEEPIT; // we're not processing the cu passed to this
- // function, so keep it.
- cu = cus__get_next_processable_cu(cus);
- if (cu == NULL)
- goto out_btf;
- }
-
- ret = btf_encoder__encode_cu(encoder, cu, conf_load);
- if (ret < 0) {
- fprintf(stderr, "Encountered error while encoding BTF.\n");
- exit(1);
- }
-
- if (conf_load->reproducible_build) {
- ret = LSK__KEEPIT; // we're not processing the cu passed to this function, so keep it.
- // Kinda equivalent to LSK__DELETE since we processed this, but we can't delete it
- // as we stash references to entries in CUs for 'struct function' in btf_encoder__add_saved_funcs()
- // and btf_encoder__save_func(), so we can't delete them here. - Alan Maguire
- }
-out_btf:
- if (!thr_data) // See comment about reproducibe_build above
- pthread_mutex_unlock(&btf_lock);
- return ret;
+ return pahole_stealer__btf_encode(cu, conf_load);
}
#if 0
if (ctf_encode) {
@@ -3625,24 +3484,6 @@ out_free:
return ret;
}
-static int cus__flush_reproducible_build(struct cus *cus, struct btf_encoder *encoder, struct conf_load *conf_load)
-{
- int err = 0;
-
- while (true) {
- struct cu *cu = cus__get_next_processable_cu(cus);
-
- if (cu == NULL)
- break;
-
- err = btf_encoder__encode_cu(encoder, cu, conf_load);
- if (err < 0)
- break;
- }
-
- return err;
-}
-
int main(int argc, char *argv[])
{
int err, remaining, rc = EXIT_FAILURE;
@@ -3731,16 +3572,6 @@ int main(int argc, char *argv[])
if (languages.exclude)
conf_load.early_cu_filter = cu__filter;
- conf_load.thread_exit = pahole_thread_exit;
-
- if (conf_load.reproducible_build) {
- conf_load.threads_prepare = pahole_threads_prepare_reproducible_build;
- conf_load.threads_collect = NULL;
- } else {
- conf_load.threads_prepare = pahole_threads_prepare;
- conf_load.threads_collect = pahole_threads_collect;
- }
-
// Make 'pahole --header type < file' a shorter form of 'pahole -C type --count 1 < file'
if (conf.header_type && !class_name && prettify_input) {
conf.count = 1;
@@ -3840,12 +3671,6 @@ try_sole_arg_as_class_names:
header = NULL;
if (btf_encode && btf_encoder) { // maybe all CUs were filtered out and thus we don't have an encoder?
- if (conf_load.reproducible_build &&
- cus__flush_reproducible_build(cus, btf_encoder, &conf_load) < 0) {
- fprintf(stderr, "Encountered error while encoding BTF.\n");
- exit(1);
- }
-
err = btf_encoder__encode(btf_encoder, &conf_load);
btf_encoder__delete(btf_encoder);
if (err) {
diff --git a/pdwtags.c b/pdwtags.c
index 67982af..962883d 100644
--- a/pdwtags.c
+++ b/pdwtags.c
@@ -91,8 +91,7 @@ static int cu__emit_tags(struct cu *cu)
}
static enum load_steal_kind pdwtags_stealer(struct cu *cu,
- struct conf_load *conf_load __maybe_unused,
- void *thr_data __maybe_unused)
+ struct conf_load *conf_load __maybe_unused)
{
cu__emit_tags(cu);
return LSK__DELETE;
diff --git a/pfunct.c b/pfunct.c
index 1d74ece..55eafe8 100644
--- a/pfunct.c
+++ b/pfunct.c
@@ -510,8 +510,7 @@ int elf_symtabs__show(char *filenames[])
}
static enum load_steal_kind pfunct_stealer(struct cu *cu,
- struct conf_load *conf_load __maybe_unused,
- void *thr_data __maybe_unused)
+ struct conf_load *conf_load __maybe_unused)
{
if (function_name) {
diff --git a/tests/reproducible_build.sh b/tests/reproducible_build.sh
index f10f834..a940d93 100755
--- a/tests/reproducible_build.sh
+++ b/tests/reproducible_build.sh
@@ -37,10 +37,7 @@ for threads in $(seq $nr_proc) ; do
sleep 0.3s
# PID part to remove ps output headers
nr_threads_started=$(ps -L -C pahole | grep -v PID | wc -l)
-
- if [ $threads -gt 1 ] ; then
- ((nr_threads_started -= 1))
- fi
+ ((nr_threads_started -= 1)) # main thread doesn't count, it waits to join
if [ $threads != $nr_threads_started ] ; then
echo "ERROR: pahole asked to start $threads encoding threads, started $nr_threads_started"
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
` (6 preceding siblings ...)
2024-12-21 1:23 ` [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model Ihor Solodrai
@ 2024-12-21 1:23 ` Ihor Solodrai
2025-01-01 16:56 ` Jiri Olsa
7 siblings, 1 reply; 22+ messages in thread
From: Ihor Solodrai @ 2024-12-21 1:23 UTC (permalink / raw)
To: dwarves; +Cc: acme, alan.maguire, eddyz87, andrii, mykolal, bpf
With multithreading moved entirely to the dwarf_loader, now there is
only one btf_encoder. Hence there is no need to maintain a global list
of encoders anymore.
Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
---
btf_encoder.c | 106 ++++++++------------------------------------------
btf_encoder.h | 4 --
2 files changed, 17 insertions(+), 93 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 7e03ba4..88e32e4 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -218,39 +218,6 @@ static struct elf_functions *elf_functions__find(const Elf *elf, const struct li
return NULL;
}
-
-static LIST_HEAD(encoders);
-static pthread_mutex_t encoders__lock = PTHREAD_MUTEX_INITIALIZER;
-
-/* mutex only needed for add/delete, as this can happen in multiple encoding
- * threads. Traversal of the list is currently confined to thread collection.
- */
-
-#define btf_encoders__for_each_encoder(encoder) \
- list_for_each_entry(encoder, &encoders, node)
-
-static void btf_encoders__add(struct btf_encoder *encoder)
-{
- pthread_mutex_lock(&encoders__lock);
- list_add_tail(&encoder->node, &encoders);
- pthread_mutex_unlock(&encoders__lock);
-}
-
-static void btf_encoders__delete(struct btf_encoder *encoder)
-{
- struct btf_encoder *existing = NULL;
-
- pthread_mutex_lock(&encoders__lock);
- /* encoder may not have been added to list yet; check. */
- btf_encoders__for_each_encoder(existing) {
- if (encoder == existing)
- break;
- }
- if (encoder == existing)
- list_del(&encoder->node);
- pthread_mutex_unlock(&encoders__lock);
-}
-
#define PERCPU_SECTION ".data..percpu"
/*
@@ -868,39 +835,6 @@ static int32_t btf_encoder__add_var_secinfo(struct btf_encoder *encoder, size_t
return gobuffer__add(&encoder->secinfo[shndx].secinfo, &si, sizeof(si));
}
-int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other)
-{
- size_t shndx;
- if (encoder == other)
- return 0;
-
- for (shndx = 1; shndx < other->seccnt; shndx++) {
- struct gobuffer *var_secinfo_buf = &other->secinfo[shndx].secinfo;
- size_t sz = gobuffer__size(var_secinfo_buf);
- uint16_t nr_var_secinfo = sz / sizeof(struct btf_var_secinfo);
- uint32_t type_id;
- uint32_t next_type_id = btf__type_cnt(encoder->btf);
- int32_t i, id;
- struct btf_var_secinfo *vsi;
-
- if (strcmp(encoder->secinfo[shndx].name, other->secinfo[shndx].name)) {
- fprintf(stderr, "mismatched ELF sections at index %zu: \"%s\", \"%s\"\n",
- shndx, encoder->secinfo[shndx].name, other->secinfo[shndx].name);
- return -1;
- }
-
- for (i = 0; i < nr_var_secinfo; i++) {
- vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
- type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
- id = btf_encoder__add_var_secinfo(encoder, shndx, type_id, vsi->offset, vsi->size);
- if (id < 0)
- return id;
- }
- }
-
- return btf__add_btf(encoder->btf, other->btf);
-}
-
static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, size_t shndx)
{
struct gobuffer *var_secinfo_buf = &encoder->secinfo[shndx].secinfo;
@@ -1326,27 +1260,29 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
}
}
-static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
+static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_encoding_inconsistent_proto)
{
struct btf_encoder_func_state **saved_fns, *s;
- struct btf_encoder *e = NULL;
- int i = 0, j, nr_saved_fns = 0;
+ int err = 0, i = 0, j, nr_saved_fns = 0;
- /* Retrieve function states from each encoder, combine them
+ /* Retrieve function states from the 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++;
+ list_for_each_entry(s, &encoder->func_states, node) {
+ nr_saved_fns++;
}
if (nr_saved_fns == 0)
- return 0;
+ goto out;
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;
+ if (!saved_fns) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ list_for_each_entry(s, &encoder->func_states, node) {
+ saved_fns[i++] = s;
}
qsort(saved_fns, nr_saved_fns, sizeof(*saved_fns), saved_functions_cmp);
@@ -1377,11 +1313,10 @@ static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
/* 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);
- }
+ btf_encoder__delete_saved_funcs(encoder);
- return 0;
+out:
+ return err;
}
static void elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
@@ -2134,7 +2069,7 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
int err;
size_t shndx;
- btf_encoder__add_saved_funcs(conf->skip_encoding_btf_inconsistent_proto);
+ btf_encoder__add_saved_funcs(encoder, conf->skip_encoding_btf_inconsistent_proto);
for (shndx = 1; shndx < encoder->seccnt; shndx++)
if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
@@ -2541,7 +2476,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
if (encoder->verbose)
printf("File %s:\n", cu->filename);
- btf_encoders__add(encoder);
}
return encoder;
@@ -2558,7 +2492,6 @@ void btf_encoder__delete(struct btf_encoder *encoder)
if (encoder == NULL)
return;
- btf_encoders__delete(encoder);
for (shndx = 0; shndx < encoder->seccnt; shndx++)
__gobuffer__delete(&encoder->secinfo[shndx].secinfo);
free(encoder->secinfo);
@@ -2727,8 +2660,3 @@ out:
encoder->cu = NULL;
return err;
}
-
-struct btf *btf_encoder__btf(struct btf_encoder *encoder)
-{
- return encoder->btf;
-}
diff --git a/btf_encoder.h b/btf_encoder.h
index 0081a99..0f345e2 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -27,10 +27,6 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
void btf_encoder__delete(struct btf_encoder *encoder);
int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf);
-
int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load);
-struct btf *btf_encoder__btf(struct btf_encoder *encoder);
-
-int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
#endif /* _BTF_ENCODER_H_ */
--
2.47.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model
2024-12-21 1:23 ` [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model Ihor Solodrai
@ 2024-12-30 16:07 ` Arnaldo Carvalho de Melo
2024-12-30 16:11 ` Arnaldo Carvalho de Melo
2025-01-01 16:56 ` Jiri Olsa
1 sibling, 1 reply; 22+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-12-30 16:07 UTC (permalink / raw)
To: Ihor Solodrai; +Cc: dwarves, alan.maguire, eddyz87, andrii, mykolal, bpf
On Sat, Dec 21, 2024 at 01:23:38AM +0000, Ihor Solodrai wrote:
> Multithreading is now contained in dwarf_loader.c, and is implemented
> using a jobs queue and a pool of worker threads. As a consequence,
> multithreading-related code is removed from pahole.c.
>
> A single-thread special case is removed: queueing setup works fine
> with a single worker, which will switch between jobs as appropriate.
>
> Code supporting previous version of the multithreading, such as
> cu_state, thread_data and related functions, is also removed.
>
> reproducible_build flag is now moot: the BTF encoding is always
> reproducible with these changes.
>
> The goal outlined in the RFC [1] - making parallel reproducible BTF
> generation as fast as non-reproducible - is achieved by implementing
> the requirement of ordered CU encoding (stealing) directly in the job
> queue in dwarf_loader.c
>
> The synchronization in the queue is implemented by a mutex (which
> ensures consistency of queue state) and a job_added condition
> variable. Motivation behind using condition variables is a classic
> one: we want to avoid the threads checking the state of the queue in a
> busy loop, competing for a single mutex.
>
> In comparison to the previous version of this patch [2], job_taken
> condition variable is removed. The number of decoded CUs in memory is
> now limited by initial JOB_DECODE jobs. The enqueue/dequeue interface
> is changed aiming to reduce locking. See relevant discussion [3].
So I'm running tests/tests after each of these patches and at this point
I got:
root@number:/home/acme/git/pahole# tests/tests
1: Validation of BTF encoding of functions; this may take some time: grep: /tmp/btf_functions.sh.OgxoO4/dwarf.funcs: binary file matches
ERROR: mismatch : BTF 'bool evtchn_fifo_is_pending(evtchn_port_t);' not found; DWARF ''
Test data is in /tmp/btf_functions.sh.OgxoO4
2: Default BTF on a system without BTF: Ok
3: Flexible arrays accounting: Ok
4: Check that pfunct can print btf_decl_tags read from BTF: Ok
5: Pretty printing of files using DWARF type information: Ok
6: Parallel reproducible DWARF Loading/Serial BTF encoding: Ok
/home/acme/git/pahole
root@number:/home/acme/git/pahole#
root@number:/home/acme/git/pahole# grep evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/dwarf.funcs
bool evtchn_fifo_is_pending(evtchn_port_t);
root@number:/home/acme/git/pahole# grep evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/btf.funcs
bool evtchn_fifo_is_pending(evtchn_port_t);
root@number:/home/acme/git/pahole#
Which seems like a test artifact:
root@number:/home/acme/git/pahole# pfunct -f evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/vmlinux.btf
bool evtchn_fifo_is_pending(evtchn_port_t port);
root@number:/home/acme/git/pahole# pfunct -F btf -f evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/vmlinux.btf
bool evtchn_fifo_is_pending(evtchn_port_t port);
root@number:/home/acme/git/pahole#
root@number:/home/acme/git/pahole# pfunct -F btf -f evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
bool evtchn_fifo_is_pending(evtchn_port_t port);
root@number:/home/acme/git/pahole# pfunct -F dwarf evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
bool evtchn_fifo_is_pending(evtchn_port_t port);
bool evtchn_fifo_is_pending(evtchn_port_t port);
root@number:/home/acme/git/pahole#
Maybe because DWARF has two such functions? Alan?
But:
root@number:/home/acme/git/pahole# readelf -wi /lib/modules/6.13.0-rc2/build/vmlinux | grep -B2 -A12 evtchn_fifo_is_pending
<2><946502d>: Abbrev Number: 0
<1><946502e>: Abbrev Number: 128 (DW_TAG_subprogram)
<9465030> DW_AT_name : (indirect string, offset: 0x39779e): evtchn_fifo_is_pending
<9465034> DW_AT_decl_file : 1
<9465034> DW_AT_decl_line : 206
<9465035> DW_AT_decl_column : 13
<9465036> DW_AT_prototyped : 1
<9465036> DW_AT_type : <0x9456abf>
<946503a> DW_AT_low_pc : 0xffffffff81b6d330
<9465042> DW_AT_high_pc : 0x65
<946504a> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa)
<946504c> DW_AT_call_all_calls: 1
<946504c> DW_AT_sibling : <0x946517a>
<2><9465050>: Abbrev Number: 81 (DW_TAG_formal_parameter)
<9465051> DW_AT_name : (indirect string, offset: 0x3abde1): port
root@number:/home/acme/git/pahole#
I.e. there is just one, but when pfunct prints it must be seeing some
other usage that refers back to 946502e...
root@number:/home/acme/git/pahole# readelf -wi /lib/modules/6.13.0-rc2/build/vmlinux | grep -w -B2 -A12 0x946502e
<4><9464825>: Abbrev Number: 60 (DW_TAG_call_site)
<9464826> DW_AT_call_return_pc: 0xffffffff81b6d4f5
<946482e> DW_AT_call_origin : <0x946502e>
<9464832> DW_AT_sibling : <0x946483d>
<5><9464836>: Abbrev Number: 13 (DW_TAG_call_site_parameter)
<9464837> DW_AT_location : 1 byte block: 55 (DW_OP_reg5 (rdi))
<9464839> DW_AT_call_value : 2 byte block: 7f 0 (DW_OP_breg15 (r15): 0)
<5><946483c>: Abbrev Number: 0
<4><946483d>: Abbrev Number: 60 (DW_TAG_call_site)
<946483e> DW_AT_call_return_pc: 0xffffffff81b6d590
<9464846> DW_AT_call_origin : <0x9463caa>
<946484a> DW_AT_sibling : <0x946485d>
<5><946484e>: Abbrev Number: 13 (DW_TAG_call_site_parameter)
<946484f> DW_AT_location : 1 byte block: 55 (DW_OP_reg5 (rdi))
<9464851> DW_AT_call_value : 2 byte block: 7f 0 (DW_OP_breg15 (r15): 0)
--
<9464f6b> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa)
<9464f6d> DW_AT_call_all_calls: 1
<9464f6d> DW_AT_sibling : <0x946502e>
<2><9464f71>: Abbrev Number: 81 (DW_TAG_formal_parameter)
<9464f72> DW_AT_name : (indirect string, offset: 0x3abde1): port
<9464f76> DW_AT_decl_file : 1
<9464f76> DW_AT_decl_line : 212
<9464f77> DW_AT_decl_column : 44
<9464f78> DW_AT_type : <0x9463263>
<9464f7c> DW_AT_location : 0x181ea81 (location list)
<9464f80> DW_AT_GNU_locviews: 0x181ea79
<2><9464f84>: Abbrev Number: 64 (DW_TAG_variable)
<9464f85> DW_AT_name : (indirect string, offset: 0x9f2cd): word
<9464f89> DW_AT_decl_file : 1
<9464f89> DW_AT_decl_line : 214
root@number:/home/acme/git/pahole#
Not really :-\
root@number:/home/acme/git/pahole# pfunct --decl_info -F dwarf evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
/* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
/* <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 */
bool evtchn_fifo_is_pending(evtchn_port_t port);
/* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
/* <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 */
bool evtchn_fifo_is_pending(evtchn_port_t port);
root@number:/home/acme/git/pahole#
So far I couldn't find an explanation for this oddity... Lets see if
after applying all patches we get past this.
- Arnaldo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model
2024-12-30 16:07 ` Arnaldo Carvalho de Melo
@ 2024-12-30 16:11 ` Arnaldo Carvalho de Melo
2024-12-30 18:37 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 22+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-12-30 16:11 UTC (permalink / raw)
To: Ihor Solodrai; +Cc: dwarves, alan.maguire, eddyz87, andrii, mykolal, bpf
On Mon, Dec 30, 2024 at 01:07:36PM -0300, Arnaldo Carvalho de Melo wrote:
> On Sat, Dec 21, 2024 at 01:23:38AM +0000, Ihor Solodrai wrote:
> > Multithreading is now contained in dwarf_loader.c, and is implemented
> > using a jobs queue and a pool of worker threads. As a consequence,
> > multithreading-related code is removed from pahole.c.
> >
> > A single-thread special case is removed: queueing setup works fine
> > with a single worker, which will switch between jobs as appropriate.
> >
> > Code supporting previous version of the multithreading, such as
> > cu_state, thread_data and related functions, is also removed.
> >
> > reproducible_build flag is now moot: the BTF encoding is always
> > reproducible with these changes.
> >
> > The goal outlined in the RFC [1] - making parallel reproducible BTF
> > generation as fast as non-reproducible - is achieved by implementing
> > the requirement of ordered CU encoding (stealing) directly in the job
> > queue in dwarf_loader.c
> >
> > The synchronization in the queue is implemented by a mutex (which
> > ensures consistency of queue state) and a job_added condition
> > variable. Motivation behind using condition variables is a classic
> > one: we want to avoid the threads checking the state of the queue in a
> > busy loop, competing for a single mutex.
> >
> > In comparison to the previous version of this patch [2], job_taken
> > condition variable is removed. The number of decoded CUs in memory is
> > now limited by initial JOB_DECODE jobs. The enqueue/dequeue interface
> > is changed aiming to reduce locking. See relevant discussion [3].
>
> So I'm running tests/tests after each of these patches and at this point
> I got:
>
> root@number:/home/acme/git/pahole# tests/tests
> 1: Validation of BTF encoding of functions; this may take some time: grep: /tmp/btf_functions.sh.OgxoO4/dwarf.funcs: binary file matches
> ERROR: mismatch : BTF 'bool evtchn_fifo_is_pending(evtchn_port_t);' not found; DWARF ''
> Test data is in /tmp/btf_functions.sh.OgxoO4
> 2: Default BTF on a system without BTF: Ok
> 3: Flexible arrays accounting: Ok
> 4: Check that pfunct can print btf_decl_tags read from BTF: Ok
> 5: Pretty printing of files using DWARF type information: Ok
> 6: Parallel reproducible DWARF Loading/Serial BTF encoding: Ok
> /home/acme/git/pahole
> root@number:/home/acme/git/pahole#
>
> root@number:/home/acme/git/pahole# grep evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/dwarf.funcs
> bool evtchn_fifo_is_pending(evtchn_port_t);
> root@number:/home/acme/git/pahole# grep evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/btf.funcs
> bool evtchn_fifo_is_pending(evtchn_port_t);
> root@number:/home/acme/git/pahole#
>
> Which seems like a test artifact:
>
> root@number:/home/acme/git/pahole# pfunct -f evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/vmlinux.btf
> bool evtchn_fifo_is_pending(evtchn_port_t port);
> root@number:/home/acme/git/pahole# pfunct -F btf -f evtchn_fifo_is_pending /tmp/btf_functions.sh.OgxoO4/vmlinux.btf
> bool evtchn_fifo_is_pending(evtchn_port_t port);
> root@number:/home/acme/git/pahole#
>
> root@number:/home/acme/git/pahole# pfunct -F btf -f evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
> bool evtchn_fifo_is_pending(evtchn_port_t port);
> root@number:/home/acme/git/pahole# pfunct -F dwarf evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
> bool evtchn_fifo_is_pending(evtchn_port_t port);
> bool evtchn_fifo_is_pending(evtchn_port_t port);
> root@number:/home/acme/git/pahole#
>
> Maybe because DWARF has two such functions? Alan?
>
> But:
>
> root@number:/home/acme/git/pahole# readelf -wi /lib/modules/6.13.0-rc2/build/vmlinux | grep -B2 -A12 evtchn_fifo_is_pending
> <2><946502d>: Abbrev Number: 0
> <1><946502e>: Abbrev Number: 128 (DW_TAG_subprogram)
> <9465030> DW_AT_name : (indirect string, offset: 0x39779e): evtchn_fifo_is_pending
> <9465034> DW_AT_decl_file : 1
> <9465034> DW_AT_decl_line : 206
> <9465035> DW_AT_decl_column : 13
> <9465036> DW_AT_prototyped : 1
> <9465036> DW_AT_type : <0x9456abf>
> <946503a> DW_AT_low_pc : 0xffffffff81b6d330
> <9465042> DW_AT_high_pc : 0x65
> <946504a> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa)
> <946504c> DW_AT_call_all_calls: 1
> <946504c> DW_AT_sibling : <0x946517a>
> <2><9465050>: Abbrev Number: 81 (DW_TAG_formal_parameter)
> <9465051> DW_AT_name : (indirect string, offset: 0x3abde1): port
> root@number:/home/acme/git/pahole#
>
> I.e. there is just one, but when pfunct prints it must be seeing some
> other usage that refers back to 946502e...
>
> root@number:/home/acme/git/pahole# readelf -wi /lib/modules/6.13.0-rc2/build/vmlinux | grep -w -B2 -A12 0x946502e
> <4><9464825>: Abbrev Number: 60 (DW_TAG_call_site)
> <9464826> DW_AT_call_return_pc: 0xffffffff81b6d4f5
> <946482e> DW_AT_call_origin : <0x946502e>
> <9464832> DW_AT_sibling : <0x946483d>
> <5><9464836>: Abbrev Number: 13 (DW_TAG_call_site_parameter)
> <9464837> DW_AT_location : 1 byte block: 55 (DW_OP_reg5 (rdi))
> <9464839> DW_AT_call_value : 2 byte block: 7f 0 (DW_OP_breg15 (r15): 0)
> <5><946483c>: Abbrev Number: 0
> <4><946483d>: Abbrev Number: 60 (DW_TAG_call_site)
> <946483e> DW_AT_call_return_pc: 0xffffffff81b6d590
> <9464846> DW_AT_call_origin : <0x9463caa>
> <946484a> DW_AT_sibling : <0x946485d>
> <5><946484e>: Abbrev Number: 13 (DW_TAG_call_site_parameter)
> <946484f> DW_AT_location : 1 byte block: 55 (DW_OP_reg5 (rdi))
> <9464851> DW_AT_call_value : 2 byte block: 7f 0 (DW_OP_breg15 (r15): 0)
> --
> <9464f6b> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa)
> <9464f6d> DW_AT_call_all_calls: 1
> <9464f6d> DW_AT_sibling : <0x946502e>
> <2><9464f71>: Abbrev Number: 81 (DW_TAG_formal_parameter)
> <9464f72> DW_AT_name : (indirect string, offset: 0x3abde1): port
> <9464f76> DW_AT_decl_file : 1
> <9464f76> DW_AT_decl_line : 212
> <9464f77> DW_AT_decl_column : 44
> <9464f78> DW_AT_type : <0x9463263>
> <9464f7c> DW_AT_location : 0x181ea81 (location list)
> <9464f80> DW_AT_GNU_locviews: 0x181ea79
> <2><9464f84>: Abbrev Number: 64 (DW_TAG_variable)
> <9464f85> DW_AT_name : (indirect string, offset: 0x9f2cd): word
> <9464f89> DW_AT_decl_file : 1
> <9464f89> DW_AT_decl_line : 214
> root@number:/home/acme/git/pahole#
>
> Not really :-\
>
> root@number:/home/acme/git/pahole# pfunct --decl_info -F dwarf evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
> /* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
> /* <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 */
> bool evtchn_fifo_is_pending(evtchn_port_t port);
> /* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
> /* <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 */
> bool evtchn_fifo_is_pending(evtchn_port_t port);
> root@number:/home/acme/git/pahole#
>
> So far I couldn't find an explanation for this oddity... Lets see if
> after applying all patches we get past this.
Its not related to this patch series, before we get two outputs for
these (and other functions in
/home/acme/git/linux/drivers/xen/events/events_fifo.c).
Still investigating.
- Arnaldo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model
2024-12-30 16:11 ` Arnaldo Carvalho de Melo
@ 2024-12-30 18:37 ` Arnaldo Carvalho de Melo
2025-01-02 23:09 ` Ihor Solodrai
0 siblings, 1 reply; 22+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-12-30 18:37 UTC (permalink / raw)
To: Ihor Solodrai; +Cc: dwarves, alan.maguire, eddyz87, andrii, mykolal, bpf
On Mon, Dec 30, 2024 at 01:11:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > Not really :-\
> >
> > root@number:/home/acme/git/pahole# pfunct --decl_info -F dwarf evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
> > /* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
> > /* <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 */
> > bool evtchn_fifo_is_pending(evtchn_port_t port);
> > /* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
> > /* <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 */
> > bool evtchn_fifo_is_pending(evtchn_port_t port);
> > root@number:/home/acme/git/pahole#
> >
> > So far I couldn't find an explanation for this oddity... Lets see if
> > after applying all patches we get past this.
> Its not related to this patch series, before we get two outputs for
> these (and other functions in
> /home/acme/git/linux/drivers/xen/events/events_fifo.c).
>
> Still investigating.
root@number:/home/acme/git/pahole# perf probe -x ~/bin/pfunct function__show
Added new event:
probe_pfunct:function_show (on function__show in /home/acme/git/pahole/build/pfunct)
You can now use it in all perf tools, such as:
perf record -e probe_pfunct:function_show -aR sleep 1
root@number:/home/acme/git/pahole# perf trace -e probe_pfunct:function_show --call-graph dwarf pfunct --decl_info -F dwarf evtchn_fifo_set_pending /lib/modules/6.13.0-rc2/build/vmlinux
/* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
/* <946517a> /home/acme/git/linux/drivers/xen/events/events_fifo.c:200 */
void evtchn_fifo_set_pending(evtchn_port_t port);
/* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c */
/* <946517a> /home/acme/git/linux/drivers/xen/events/events_fifo.c:200 */
void evtchn_fifo_set_pending(evtchn_port_t port);
0.000 pfunct/2006089 probe_pfunct:function_show(__probe_ip: 4208235)
function__show (/home/acme/git/pahole/build/pfunct)
pfunct_stealer (/home/acme/git/pahole/build/pfunct)
cus__steal_now (/home/acme/git/pahole/build/libdwarves.so.1.0.0)
dwarf_loader__worker_thread (/home/acme/git/pahole/build/libdwarves.so.1.0.0)
start_thread (/usr/lib64/libc.so.6)
clone3 (/usr/lib64/libc.so.6)
0.134 pfunct/2006088 probe_pfunct:function_show(__probe_ip: 4208235)
function__show (/home/acme/git/pahole/build/pfunct)
cu_function_iterator (/home/acme/git/pahole/build/pfunct)
cus__for_each_cu (/home/acme/git/pahole/build/libdwarves.so.1.0.0)
main (/home/acme/git/pahole/build/pfunct)
__libc_start_call_main (/usr/lib64/libc.so.6)
__libc_start_main@@GLIBC_2.34 (/usr/lib64/libc.so.6)
_start (/home/acme/git/pahole/build/pfunct)
root@number:/home/acme/git/pahole#
With the following patch we get just one output for this case, but that
isn't the right solution... I'll look on removing the
cu_function_iterator() based printing, otherwise when printing all
matches we'll still duplicate the printings.
Anyway, doesn't seem related to the problem that tests/tests was
catching, that I'm not being able to reproduce anymore after having the
whole series applied, probably some race?
- Arnaldo
diff --git a/pfunct.c b/pfunct.c
index 55eafe8a8e790dcb..9645b004381a7e1e 100644
--- a/pfunct.c
+++ b/pfunct.c
@@ -518,7 +518,13 @@ static enum load_steal_kind pfunct_stealer(struct cu *cu,
if (tag) {
function__show(tag__function(tag), cu);
- return show_all_matches ? LSK__DELETE : LSK__STOP_LOADING;
+ if (!show_all_matches) {
+ // Expedite exit, since we already did what was requested:
+ // print the first occurrence of a given function
+ exit(0);
+ }
+
+ return LSK__DELETE;
}
} else if (class_name) {
cu_class_iterator(cu, class_name);
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations
2024-12-21 1:23 ` [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations Ihor Solodrai
@ 2025-01-01 16:56 ` Jiri Olsa
2025-01-02 19:54 ` Ihor Solodrai
0 siblings, 1 reply; 22+ messages in thread
From: Jiri Olsa @ 2025-01-01 16:56 UTC (permalink / raw)
To: Ihor Solodrai; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Sat, Dec 21, 2024 at 01:23:01AM +0000, Ihor Solodrai wrote:
SNIP
> +static int saved_functions_combine(void *_a, void *_b)
> +{
> + uint8_t optimized, unexpected, inconsistent;
> + struct btf_encoder_func_state *a = _a;
> + struct btf_encoder_func_state *b = _b;
> + int ret;
> +
> + ret = strncmp(a->elf->name, b->elf->name,
> + max(a->elf->prefixlen, b->elf->prefixlen));
> + if (ret != 0)
> + return ret;
> + optimized = a->optimized_parms | b->optimized_parms;
> + unexpected = a->unexpected_reg | b->unexpected_reg;
> + inconsistent = a->inconsistent_proto | b->inconsistent_proto;
> + if (!unexpected && !inconsistent && !funcs__match(a, b))
> + inconsistent = 1;
> + a->optimized_parms = b->optimized_parms = optimized;
> + a->unexpected_reg = b->unexpected_reg = unexpected;
> + a->inconsistent_proto = b->inconsistent_proto = inconsistent;
do we still need to update the 'b' state object?
> +
> + return 0;
> +}
> +
> +static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> +{
> + struct btf_encoder_func_state *pos, *s;
> +
> + list_for_each_entry_safe(pos, s, &encoder->func_states, node) {
> + list_del(&pos->node);
> + free(pos->parms);
> + free(pos->annots);
> + free(pos);
> + }
> +
> + for (int i = 0; i < encoder->functions.cnt; i++)
> + free(encoder->functions.entries[i].alias);
> +}
> +
> +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++;
> + }
the encoder loop goes eventualy away, but still would it make to store
func_states count instead of the loop?
now when there's just single place that stores 'state' it seems like it
should be straighforward
SNIP
> void btf_encoder__delete(struct btf_encoder *encoder)
> {
> - int i;
> size_t shndx;
>
> if (encoder == NULL)
> @@ -2447,18 +2469,19 @@ void btf_encoder__delete(struct btf_encoder *encoder)
> btf_encoders__delete(encoder);
> for (shndx = 0; shndx < encoder->seccnt; shndx++)
> __gobuffer__delete(&encoder->secinfo[shndx].secinfo);
> + free(encoder->secinfo);
nit, this seems unrelated to this change, should be in separate fix?
thanks,
jirka
> zfree(&encoder->filename);
> zfree(&encoder->source_filename);
> btf__free(encoder->btf);
> encoder->btf = NULL;
> elf_symtab__delete(encoder->symtab);
>
> - for (i = 0; i < encoder->functions.cnt; i++)
> - btf_encoder__delete_func(&encoder->functions.entries[i]);
> encoder->functions.allocated = encoder->functions.cnt = 0;
> free(encoder->functions.entries);
> encoder->functions.entries = NULL;
>
> + btf_encoder__delete_saved_funcs(encoder);
> +
> free(encoder);
> }
>
SNIP
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model
2024-12-21 1:23 ` [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model Ihor Solodrai
2024-12-30 16:07 ` Arnaldo Carvalho de Melo
@ 2025-01-01 16:56 ` Jiri Olsa
2025-01-03 0:24 ` Ihor Solodrai
1 sibling, 1 reply; 22+ messages in thread
From: Jiri Olsa @ 2025-01-01 16:56 UTC (permalink / raw)
To: Ihor Solodrai; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Sat, Dec 21, 2024 at 01:23:38AM +0000, Ihor Solodrai wrote:
SNIP
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 90f1b9a..7e03ba4 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -1326,7 +1326,7 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> }
> }
>
> -int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> +static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> {
> struct btf_encoder_func_state **saved_fns, *s;
> struct btf_encoder *e = NULL;
> @@ -2134,7 +2134,6 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
> int err;
> size_t shndx;
>
> - /* for single-threaded case, saved funcs are added here */
> btf_encoder__add_saved_funcs(conf->skip_encoding_btf_inconsistent_proto);
should we check the return value in here? now it's the only caller
SNIP
> -struct dwarf_thread {
> - struct dwarf_cus *dcus;
> - void *data;
> +/* Multithreading is implemented using a job/worker model.
> + * cus_processing_queue represents a collection of jobs to be
> + * completed by workers.
> + * dwarf_loader__worker_thread is the worker loop, taking jobs from
> + * the queue, executing them and re-enqueuing new jobs as necessary.
> + * Implementation of this queue ensures two constraints:
> + * * JOB_STEAL jobs for a CU are executed in the order of cu->id, as
> + * a consequence JOB_STEAL jobs always run one at a time.
> + * * Initial number of JOB_DECODE jobs in the queue is effectively a
> + * limit on how many decoded CUs can be held in memory.
> + * See dwarf_loader__decoded_cus_limit()
> + */
> +static struct {
> + pthread_mutex_t mutex;
> + pthread_cond_t job_added;
> + /* next_cu_id determines the next CU ready to be stealed
> + * This enforces the order of CU stealing.
> + */
> + uint32_t next_cu_id;
> + struct list_head jobs;
> +} cus_processing_queue;
> +
> +enum job_type {
> + JOB_NONE = 0,
nit, no need for JOB_NONE?
SNIP
> +static struct cu_processing_job *cus_queue__try_dequeue(void)
> +{
> + struct cu_processing_job *job, *dequeued_job = NULL;
> + struct list_head *pos, *tmp;
> +
> + list_for_each_safe(pos, tmp, &cus_processing_queue.jobs) {
> + job = list_entry(pos, struct cu_processing_job, node);
> + if (job->type == JOB_STEAL && job->cu->id == cus_processing_queue.next_cu_id) {
> + dequeued_job = job;
> + break;
> + }
> + if (job->type == JOB_DECODE) {
> + /* all JOB_STEALs are added to the head, so no viable JOB_STEAL available */
> + dequeued_job = job;
> + break;
> + }
> + }
> + /* No jobs or only steals out of order */
> + if (!dequeued_job)
> + return NULL;
> +
> + list_del(&dequeued_job->node);
> + return job;
IIUC job == dequeued_job at this point, but I think we should return
dequeued_job to be clear
SNIP
> -static void *dwarf_cus__process_cu_thread(void *arg)
> +static struct cu *dwarf_loader__decode_next_cu(struct dwarf_cus *dcus)
> {
> - struct dwarf_thread *dthr = arg;
> - struct dwarf_cus *dcus = dthr->dcus;
> uint8_t pointer_size, offset_size;
> + struct dwarf_cu *dcu = NULL;
> Dwarf_Die die_mem, *cu_die;
> - struct dwarf_cu *dcu;
> + int err;
>
> - while (dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size) == 0) {
> - if (cu_die == NULL)
> + err = dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size);
> +
> + if (err < 0)
> + goto out_error;
> + else if (err == 1) /* no more CUs */
> + return NULL;
> +
> + err = die__process_and_recode(cu_die, dcu->cu, dcus->conf);
> + if (err)
> + goto out_error;
> + if (cu_die == NULL)
> + return NULL;
should this check be placed before calling die__process_and_recode ?
SNIP
> -static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
> -{
> - if (dcus->conf->nr_jobs > 1)
> - return dwarf_cus__threaded_process_cus(dcus);
> -
> - return __dwarf_cus__process_cus(dcus);
> -}
> -
> static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> Dwfl_Module *mod, Dwarf *dw, Elf *elf,
> const char *filename,
> @@ -3733,7 +3859,8 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> if (cu__resolve_func_ret_types_optimized(cu) != LSK__KEEPIT)
> goto out_abort;
>
> - if (cus__finalize(cus, cu, conf, NULL) == LSK__STOP_LOADING)
> + cu__finalize(cu, cus, conf);
> + if (cus__steal_now(cus, cu, conf) == LSK__STOP_LOADING)
> goto out_abort;
>
> return 0;
> @@ -3765,7 +3892,8 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
> }
>
> if (type_cu != NULL) {
> - type_lsk = cu__finalize(type_cu, cus, conf, NULL);
> + cu__finalize(type_cu, cus, conf);
> + type_lsk = cus__steal_now(cus, type_cu, conf);
> if (type_lsk == LSK__DELETE) {
> cus__remove(cus, type_cu);
> }
> @@ -3787,6 +3915,7 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
> .type_dcu = type_cu ? &type_dcu : NULL,
> .build_id = build_id,
> .build_id_len = build_id_len,
> + .nr_cus_created = 0,
should go to the previous patch? if needed at all..
thanks,
jirka
> };
> res = dwarf_cus__process_cus(&dcus);
> }
> diff --git a/dwarves.c b/dwarves.c
> index ae512b9..7c3e878 100644
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -530,48 +530,6 @@ void cus__unlock(struct cus *cus)
> pthread_mutex_unlock(&cus->mutex);
> }
>
SNIP
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list
2024-12-21 1:23 ` [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list Ihor Solodrai
@ 2025-01-01 16:56 ` Jiri Olsa
2025-01-03 0:43 ` Ihor Solodrai
0 siblings, 1 reply; 22+ messages in thread
From: Jiri Olsa @ 2025-01-01 16:56 UTC (permalink / raw)
To: Ihor Solodrai; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Sat, Dec 21, 2024 at 01:23:45AM +0000, Ihor Solodrai wrote:
SNIP
> -static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_encoding_inconsistent_proto)
> {
> struct btf_encoder_func_state **saved_fns, *s;
> - struct btf_encoder *e = NULL;
> - int i = 0, j, nr_saved_fns = 0;
> + int err = 0, i = 0, j, nr_saved_fns = 0;
>
> - /* Retrieve function states from each encoder, combine them
> + /* Retrieve function states from the 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++;
> + list_for_each_entry(s, &encoder->func_states, node) {
> + nr_saved_fns++;
> }
>
> if (nr_saved_fns == 0)
> - return 0;
> + goto out;
>
> 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;
> + if (!saved_fns) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + list_for_each_entry(s, &encoder->func_states, node) {
> + saved_fns[i++] = s;
> }
> qsort(saved_fns, nr_saved_fns, sizeof(*saved_fns), saved_functions_cmp);
>
> @@ -1377,11 +1313,10 @@ static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
>
> /* 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);
> - }
> + btf_encoder__delete_saved_funcs(encoder);
is this call necessary? there's btf_encoder__delete call right after
same for elf_functions_list__clear in btf_encoder__encode
thanks,
jirka
>
> - return 0;
> +out:
> + return err;
> }
>
SNIP
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type
2024-12-21 1:23 ` [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type Ihor Solodrai
@ 2025-01-01 16:56 ` Jiri Olsa
2025-01-02 20:06 ` Ihor Solodrai
0 siblings, 1 reply; 22+ messages in thread
From: Jiri Olsa @ 2025-01-01 16:56 UTC (permalink / raw)
To: Ihor Solodrai; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Sat, Dec 21, 2024 at 01:23:10AM +0000, Ihor Solodrai wrote:
SNIP
> -static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *sym)
> +static void elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
> {
> - struct elf_function *new;
> + struct elf_function *func;
> const char *name;
>
> if (elf_sym__type(sym) != STT_FUNC)
> - return 0;
> - name = elf_sym__name(sym, encoder->symtab);
> - if (!name)
> - return 0;
> + return;
>
> - if (encoder->functions.cnt == encoder->functions.allocated) {
> - new = reallocarray_grow(encoder->functions.entries,
> - &encoder->functions.allocated,
> - sizeof(*encoder->functions.entries));
> - if (!new) {
> - /*
> - * The cleanup - delete_functions is called
> - * in btf_encoder__encode_cu error path.
> - */
> - return -1;
> - }
> - encoder->functions.entries = new;
> - }
> + name = elf_sym__name(sym, functions->symtab);
> + if (!name)
> + return;
>
> - memset(&encoder->functions.entries[encoder->functions.cnt], 0,
> - sizeof(*new));
> - encoder->functions.entries[encoder->functions.cnt].name = name;
> + func = &functions->entries[functions->cnt];
> + func->name = name;
> if (strchr(name, '.')) {
> const char *suffix = strchr(name, '.');
> -
nit, let's keep that new line after declaration
> - encoder->functions.suffix_cnt++;
> - encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
> + functions->suffix_cnt++;
> + func->prefixlen = suffix - name;
> } else {
> - encoder->functions.entries[encoder->functions.cnt].prefixlen = strlen(name);
> + func->prefixlen = strlen(name);
> }
> - encoder->functions.cnt++;
> - return 0;
> +
> + functions->cnt++;
> }
>
> static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
> @@ -2126,26 +2103,56 @@ int btf_encoder__encode(struct btf_encoder *encoder)
> return err;
> }
>
> -
> -static int btf_encoder__collect_symbols(struct btf_encoder *encoder)
> +static int elf_functions__collect(struct elf_functions *functions)
> {
> - uint32_t sym_sec_idx;
> + uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab);
> + struct elf_function *tmp;
> + Elf32_Word sym_sec_idx;
> uint32_t core_id;
> GElf_Sym sym;
> + int err;
>
> - elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) {
> - if (btf_encoder__collect_function(encoder, &sym))
> - return -1;
> + /* We know that number of functions is less than number of symbols,
> + * so we can overallocate temporarily.
> + */
> + functions->entries = calloc(nr_symbols, sizeof(*functions->entries));
> + if (!functions->entries) {
> + err = -ENOMEM;
> + goto out_free;
you could just return -ENOMEM here
> + }
> +
> + functions->cnt = 0;
> + elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {
> + elf_functions__collect_function(functions, &sym);
> }
>
> - if (encoder->functions.cnt) {
> - qsort(encoder->functions.entries, encoder->functions.cnt, sizeof(encoder->functions.entries[0]),
> + if (functions->cnt) {
> + qsort(functions->entries,
> + functions->cnt,
> + sizeof(*functions->entries),
> functions_cmp);
nit, why not keep the single line?
> - if (encoder->verbose)
> - printf("Found %d functions!\n", encoder->functions.cnt);
> + } else {
> + err = 0;
> + goto out_free;
> + }
> +
> + /* Reallocate to the exact size */
> + tmp = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
> + if (tmp) {
> + functions->entries = tmp;
> + } else {
> + fprintf(stderr, "could not reallocate memory for elf_functions table\n");
> + err = -ENOMEM;
> + goto out_free;
> }
>
> return 0;
> +
> +out_free:
> + free(functions->entries);
> + functions->entries = NULL;
> + functions->cnt = 0;
> + return err;
> }
>
> static bool ftype__has_arg_names(const struct ftype *ftype)
> @@ -2406,6 +2413,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> printf("%s: '%s' doesn't have symtab.\n", __func__, cu->filename);
> goto out;
> }
> + encoder->functions.symtab = encoder->symtab;
I was wondering if we need to keep both symtab pointers, but it's sorted
out in the next patch ;-)
thanks,
jirka
>
> /* index the ELF sections for later lookup */
>
> @@ -2444,7 +2452,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> if (!found_percpu && encoder->verbose)
> printf("%s: '%s' doesn't have '%s' section\n", __func__, cu->filename, PERCPU_SECTION);
>
> - if (btf_encoder__collect_symbols(encoder))
> + if (elf_functions__collect(&encoder->functions))
> goto out_delete;
>
> if (encoder->verbose)
> @@ -2476,7 +2484,7 @@ void btf_encoder__delete(struct btf_encoder *encoder)
> encoder->btf = NULL;
> elf_symtab__delete(encoder->symtab);
>
> - encoder->functions.allocated = encoder->functions.cnt = 0;
> + encoder->functions.cnt = 0;
> free(encoder->functions.entries);
> encoder->functions.entries = NULL;
>
> --
> 2.47.1
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations
2025-01-01 16:56 ` Jiri Olsa
@ 2025-01-02 19:54 ` Ihor Solodrai
0 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2025-01-02 19:54 UTC (permalink / raw)
To: Jiri Olsa; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
>
>
> On Sat, Dec 21, 2024 at 01:23:01AM +0000, Ihor Solodrai wrote:
>
> SNIP
>
> > +static int saved_functions_combine(void *_a, void *_b)
> > +{
> > + uint8_t optimized, unexpected, inconsistent;
> > + struct btf_encoder_func_state *a = _a;
> > + struct btf_encoder_func_state *b = _b;
> > + int ret;
> > +
> > + ret = strncmp(a->elf->name, b->elf->name,
> > + max(a->elf->prefixlen, b->elf->prefixlen));
> > + if (ret != 0)
> > + return ret;
> > + optimized = a->optimized_parms | b->optimized_parms;
> > + unexpected = a->unexpected_reg | b->unexpected_reg;
> > + inconsistent = a->inconsistent_proto | b->inconsistent_proto;
> > + if (!unexpected && !inconsistent && !funcs__match(a, b))
> > + inconsistent = 1;
> > + a->optimized_parms = b->optimized_parms = optimized;
> > + a->unexpected_reg = b->unexpected_reg = unexpected;
> > + a->inconsistent_proto = b->inconsistent_proto = inconsistent;
>
>
> do we still need to update the 'b' state object?
Hi Jiri, thanks for review.
Given how this function is used, we don't need to. But this is a kind
of a "merge", and IMO having both states equal on the way out makes
sense.
while (j < nr_saved_fns && saved_functions_combine(saved_fns[i], saved_fns[j]) == 0)
j++;
>
> > +
> > + return 0;
> > +}
> > +
> > +static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> > +{
> > + struct btf_encoder_func_state *pos, *s;
> > +
> > + list_for_each_entry_safe(pos, s, &encoder->func_states, node) {
> > + list_del(&pos->node);
> > + free(pos->parms);
> > + free(pos->annots);
> > + free(pos);
> > + }
> > +
> > + for (int i = 0; i < encoder->functions.cnt; i++)
> > + free(encoder->functions.entries[i].alias);
> > +}
> > +
> > +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++;
> > + }
>
>
> the encoder loop goes eventualy away, but still would it make to store
> func_states count instead of the loop?
>
> now when there's just single place that stores 'state' it seems like it
> should be straighforward
Yeah.
>
> SNIP
>
> > void btf_encoder__delete(struct btf_encoder *encoder)
> > {
> > - int i;
> > size_t shndx;
> >
> > if (encoder == NULL)
> > @@ -2447,18 +2469,19 @@ void btf_encoder__delete(struct btf_encoder *encoder)
> > btf_encoders__delete(encoder);
> > for (shndx = 0; shndx < encoder->seccnt; shndx++)
> > __gobuffer__delete(&encoder->secinfo[shndx].secinfo);
> > + free(encoder->secinfo);
>
>
> nit, this seems unrelated to this change, should be in separate fix?
IIRC this was also caught by the sanitizer. I'll sliver this into a
separate patch in the next iteration.
>
> thanks,
> jirka
>
> > zfree(&encoder->filename);
> > zfree(&encoder->source_filename);
> > btf__free(encoder->btf);
> > encoder->btf = NULL;
> > elf_symtab__delete(encoder->symtab);
> >
> > - for (i = 0; i < encoder->functions.cnt; i++)
> > - btf_encoder__delete_func(&encoder->functions.entries[i]);
> > encoder->functions.allocated = encoder->functions.cnt = 0;
> > free(encoder->functions.entries);
> > encoder->functions.entries = NULL;
> >
> > + btf_encoder__delete_saved_funcs(encoder);
> > +
> > free(encoder);
> > }
>
>
> SNIP
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type
2025-01-01 16:56 ` Jiri Olsa
@ 2025-01-02 20:06 ` Ihor Solodrai
0 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2025-01-02 20:06 UTC (permalink / raw)
To: Jiri Olsa; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Sat, Dec 21, 2024 at 01:23:10AM +0000, Ihor Solodrai wrote:
>
> SNIP
>
> > -static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *sym)
> > +static void elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
> > {
> > - struct elf_function *new;
> > + struct elf_function *func;
> > const char *name;
> >
> > if (elf_sym__type(sym) != STT_FUNC)
> > - return 0;
> > - name = elf_sym__name(sym, encoder->symtab);
> > - if (!name)
> > - return 0;
> > + return;
> >
> > - if (encoder->functions.cnt == encoder->functions.allocated) {
> > - new = reallocarray_grow(encoder->functions.entries,
> > - &encoder->functions.allocated,
> > - sizeof(encoder->functions.entries));
> > - if (!new) {
> > - /
> > - * The cleanup - delete_functions is called
> > - * in btf_encoder__encode_cu error path.
> > - */
> > - return -1;
> > - }
> > - encoder->functions.entries = new;
> > - }
> > + name = elf_sym__name(sym, functions->symtab);
> > + if (!name)
> > + return;
> >
> > - memset(&encoder->functions.entries[encoder->functions.cnt], 0,
> > - sizeof(*new));
> > - encoder->functions.entries[encoder->functions.cnt].name = name;
> > + func = &functions->entries[functions->cnt];
> > + func->name = name;
> > if (strchr(name, '.')) {
> > const char *suffix = strchr(name, '.');
> > -
>
>
> nit, let's keep that new line after declaration
ok
>
> > - encoder->functions.suffix_cnt++;
> > - encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
> > + functions->suffix_cnt++;
> > + func->prefixlen = suffix - name;
> > } else {
> > - encoder->functions.entries[encoder->functions.cnt].prefixlen = strlen(name);
> > + func->prefixlen = strlen(name);
> > }
> > - encoder->functions.cnt++;
> > - return 0;
> > +
> > + functions->cnt++;
> > }
> >
> > static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
> > @@ -2126,26 +2103,56 @@ int btf_encoder__encode(struct btf_encoder *encoder)
> > return err;
> > }
> >
> > -
> > -static int btf_encoder__collect_symbols(struct btf_encoder *encoder)
> > +static int elf_functions__collect(struct elf_functions *functions)
> > {
> > - uint32_t sym_sec_idx;
> > + uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab);
> > + struct elf_function *tmp;
> > + Elf32_Word sym_sec_idx;
> > uint32_t core_id;
> > GElf_Sym sym;
> > + int err;
> >
> > - elf_symtab__for_each_symbol_index(encoder->symtab, core_id, sym, sym_sec_idx) {
> > - if (btf_encoder__collect_function(encoder, &sym))
> > - return -1;
> > + /* We know that number of functions is less than number of symbols,
> > + * so we can overallocate temporarily.
> > + */
> > + functions->entries = calloc(nr_symbols, sizeof(*functions->entries));
> > + if (!functions->entries) {
> > + err = -ENOMEM;
> > + goto out_free;
>
>
> you could just return -ENOMEM here
I am trying to adhere to the kernel style, although not very strictly.
It's recommended [1] to have a single exit from a function when there
is cleanup work.
I usually check my patches with a script [2] before submitting.
[1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions
[2] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
>
> > + }
> > +
> > + functions->cnt = 0;
> > + elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {
> > + elf_functions__collect_function(functions, &sym);
> > }
> >
> > - if (encoder->functions.cnt) {
> > - qsort(encoder->functions.entries, encoder->functions.cnt, sizeof(encoder->functions.entries[0]),
> > + if (functions->cnt) {
> > + qsort(functions->entries,
> > + functions->cnt,
> > + sizeof(*functions->entries),
> > functions_cmp);
>
>
> nit, why not keep the single line?
How many chars in a line is too many? :)
>
> > - if (encoder->verbose)
> > - printf("Found %d functions!\n", encoder->functions.cnt);
> > + } else {
> > + err = 0;
> > + goto out_free;
> > + }
> > +
> > + /* Reallocate to the exact size */
> > + tmp = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
> > + if (tmp) {
> > + functions->entries = tmp;
> > + } else {
> > + fprintf(stderr, "could not reallocate memory for elf_functions table\n");
> > + err = -ENOMEM;
> > + goto out_free;
> > }
> >
> > return 0;
> > +
> > +out_free:
> > + free(functions->entries);
> > + functions->entries = NULL;
> > + functions->cnt = 0;
> > + return err;
> > }
> >
> > static bool ftype__has_arg_names(const struct ftype *ftype)
> > @@ -2406,6 +2413,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > printf("%s: '%s' doesn't have symtab.\n", func, cu->filename);
> > goto out;
> > }
> > + encoder->functions.symtab = encoder->symtab;
>
>
> I was wondering if we need to keep both symtab pointers, but it's sorted
> out in the next patch ;-)
>
> thanks,
> jirka
>
> [...]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model
2024-12-30 18:37 ` Arnaldo Carvalho de Melo
@ 2025-01-02 23:09 ` Ihor Solodrai
0 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2025-01-02 23:09 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: dwarves, alan.maguire, eddyz87, andrii, mykolal, bpf
On Monday, December 30th, 2024 at 10:37 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
>
> On Mon, Dec 30, 2024 at 01:11:41PM -0300, Arnaldo Carvalho de Melo wrote:
>
> > > Not really :-\
> > >
> > > root@number:/home/acme/git/pahole# pfunct --decl_info -F dwarf evtchn_fifo_is_pending /lib/modules/6.13.0-rc2/build/vmlinux
> > > /* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c /
> > > / <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 /
> > > bool evtchn_fifo_is_pending(evtchn_port_t port);
> > > / Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c /
> > > / <946502e> /home/acme/git/linux/drivers/xen/events/events_fifo.c:206 */
> > > bool evtchn_fifo_is_pending(evtchn_port_t port);
> > > root@number:/home/acme/git/pahole#
> > >
> > > So far I couldn't find an explanation for this oddity... Lets see if
> > > after applying all patches we get past this.
>
> > Its not related to this patch series, before we get two outputs for
> > these (and other functions in
> > /home/acme/git/linux/drivers/xen/events/events_fifo.c).
> >
> > Still investigating.
>
>
> root@number:/home/acme/git/pahole# perf probe -x ~/bin/pfunct function__show
> Added new event:
> probe_pfunct:function_show (on function__show in /home/acme/git/pahole/build/pfunct)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe_pfunct:function_show -aR sleep 1
>
> root@number:/home/acme/git/pahole# perf trace -e probe_pfunct:function_show --call-graph dwarf pfunct --decl_info -F dwarf evtchn_fifo_set_pending /lib/modules/6.13.0-rc2/build/vmlinux
> /* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c /
> / <946517a> /home/acme/git/linux/drivers/xen/events/events_fifo.c:200 */
>
> void evtchn_fifo_set_pending(evtchn_port_t port);
> /* Used at: /home/acme/git/linux/drivers/xen/events/events_fifo.c /
> / <946517a> /home/acme/git/linux/drivers/xen/events/events_fifo.c:200 */
>
> void evtchn_fifo_set_pending(evtchn_port_t port);
> 0.000 pfunct/2006089 probe_pfunct:function_show(__probe_ip: 4208235)
> function__show (/home/acme/git/pahole/build/pfunct)
> pfunct_stealer (/home/acme/git/pahole/build/pfunct)
> cus__steal_now (/home/acme/git/pahole/build/libdwarves.so.1.0.0)
> dwarf_loader__worker_thread (/home/acme/git/pahole/build/libdwarves.so.1.0.0)
> start_thread (/usr/lib64/libc.so.6)
> clone3 (/usr/lib64/libc.so.6)
> 0.134 pfunct/2006088 probe_pfunct:function_show(__probe_ip: 4208235)
> function__show (/home/acme/git/pahole/build/pfunct)
> cu_function_iterator (/home/acme/git/pahole/build/pfunct)
> cus__for_each_cu (/home/acme/git/pahole/build/libdwarves.so.1.0.0)
> main (/home/acme/git/pahole/build/pfunct)
> __libc_start_call_main (/usr/lib64/libc.so.6)
> __libc_start_main@@GLIBC_2.34 (/usr/lib64/libc.so.6)
> _start (/home/acme/git/pahole/build/pfunct)
> root@number:/home/acme/git/pahole#
>
> With the following patch we get just one output for this case, but that
> isn't the right solution... I'll look on removing the
> cu_function_iterator() based printing, otherwise when printing all
> matches we'll still duplicate the printings.
>
> Anyway, doesn't seem related to the problem that tests/tests was
> catching, that I'm not being able to reproduce anymore after having the
> whole series applied, probably some race?
Hi Arnaldo, thank you for testing.
I couldn't reproduce the mismatch error that you saw:
root@number:/home/acme/git/pahole# tests/tests
1: Validation of BTF encoding of functions; this may take some time: grep: /tmp/btf_functions.sh.OgxoO4/dwarf.funcs: binary file matches
ERROR: mismatch : BTF 'bool evtchn_fifo_is_pending(evtchn_port_t);' not found; DWARF ''
Test data is in /tmp/btf_functions.sh.OgxoO4
I've tried a couple of kernels:
* 6.12 with selftests/bpf config
* Fedora 6.12.6-100.fc40.x86_64 (my workstation)
* 6.13-rc2 with Fedora-like config
I saw warnings that don't seem to be related to this series:
theihor@qube:~/dev/dwarves$ PATH=$(realpath build):$PATH vmlinux=~/git/kernel.org/linux-for-pahole/vmlinux ./tests/tests
1: Validation of BTF encoding of functions; this may take some time: Ok
2: Default BTF on a system without BTF: Ok
3: Flexible arrays accounting: WARNING: still unsuported BTF_KIND_DECL_TAG(bpf_fastcall) for bpf_cast_to_kern_ctx already with attribute (bpf_kfunc), ignoring
WARNING: still unsuported BTF_KIND_DECL_TAG(bpf_fastcall) for bpf_rdonly_cast already with attribute (bpf_kfunc), ignoring
Ok
4: Pretty printing of files using DWARF type information: Ok
5: Parallel reproducible DWARF Loading/Serial BTF encoding: Ok
As for potential race, since btf_encoder is sequential and a unit of
work is a CU, I don't see how a single function could have been
missed. Other declarations in events_fifo.c would've been affected I
think.
If you see this again, please let me know.
>
> - Arnaldo
>
> diff --git a/pfunct.c b/pfunct.c
> index 55eafe8a8e790dcb..9645b004381a7e1e 100644
> --- a/pfunct.c
> +++ b/pfunct.c
> @@ -518,7 +518,13 @@ static enum load_steal_kind pfunct_stealer(struct cu *cu,
>
> if (tag) {
> function__show(tag__function(tag), cu);
> - return show_all_matches ? LSK__DELETE : LSK__STOP_LOADING;
> + if (!show_all_matches) {
> + // Expedite exit, since we already did what was requested:
> + // print the first occurrence of a given function
> + exit(0);
> + }
> +
> + return LSK__DELETE;
> }
> } else if (class_name) {
> cu_class_iterator(cu, class_name);
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model
2025-01-01 16:56 ` Jiri Olsa
@ 2025-01-03 0:24 ` Ihor Solodrai
0 siblings, 0 replies; 22+ messages in thread
From: Ihor Solodrai @ 2025-01-03 0:24 UTC (permalink / raw)
To: Jiri Olsa; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
>
>
> On Sat, Dec 21, 2024 at 01:23:38AM +0000, Ihor Solodrai wrote:
>
> SNIP
>
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 90f1b9a..7e03ba4 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -1326,7 +1326,7 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> > }
> > }
> >
> > -int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > +static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > {
> > struct btf_encoder_func_state **saved_fns, *s;
> > struct btf_encoder *e = NULL;
> > @@ -2134,7 +2134,6 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
> > int err;
> > size_t shndx;
> >
> > - /* for single-threaded case, saved funcs are added here */
> > btf_encoder__add_saved_funcs(conf->skip_encoding_btf_inconsistent_proto);
>
>
> should we check the return value in here? now it's the only caller
Yes, thanks.
>
> SNIP
>
> > -struct dwarf_thread {
> > - struct dwarf_cus *dcus;
> > - void data;
> > +/ Multithreading is implemented using a job/worker model.
> > + * cus_processing_queue represents a collection of jobs to be
> > + * completed by workers.
> > + * dwarf_loader__worker_thread is the worker loop, taking jobs from
> > + * the queue, executing them and re-enqueuing new jobs as necessary.
> > + * Implementation of this queue ensures two constraints:
> > + * * JOB_STEAL jobs for a CU are executed in the order of cu->id, as
> > + * a consequence JOB_STEAL jobs always run one at a time.
> > + * * Initial number of JOB_DECODE jobs in the queue is effectively a
> > + * limit on how many decoded CUs can be held in memory.
> > + * See dwarf_loader__decoded_cus_limit()
> > + /
> > +static struct {
> > + pthread_mutex_t mutex;
> > + pthread_cond_t job_added;
> > + / next_cu_id determines the next CU ready to be stealed
> > + * This enforces the order of CU stealing.
> > + */
> > + uint32_t next_cu_id;
> > + struct list_head jobs;
> > +} cus_processing_queue;
> > +
> > +enum job_type {
> > + JOB_NONE = 0,
>
>
> nit, no need for JOB_NONE?
I find it useful for the default value to not be a valid "type". This
helps to avoid bugs when an object is initialized, but the type has
not been set explicitly (even though it should be).
>
> SNIP
>
> > +static struct cu_processing_job *cus_queue__try_dequeue(void)
> > +{
> > + struct cu_processing_job *job, *dequeued_job = NULL;
> > + struct list_head *pos, tmp;
> > +
> > + list_for_each_safe(pos, tmp, &cus_processing_queue.jobs) {
> > + job = list_entry(pos, struct cu_processing_job, node);
> > + if (job->type == JOB_STEAL && job->cu->id == cus_processing_queue.next_cu_id) {
> > + dequeued_job = job;
> > + break;
> > + }
> > + if (job->type == JOB_DECODE) {
> > + / all JOB_STEALs are added to the head, so no viable JOB_STEAL available /
> > + dequeued_job = job;
> > + break;
> > + }
> > + }
> > + / No jobs or only steals out of order */
> > + if (!dequeued_job)
> > + return NULL;
> > +
> > + list_del(&dequeued_job->node);
> > + return job;
>
>
> IIUC job == dequeued_job at this point, but I think we should return
> dequeued_job to be clear
Agree.
>
> SNIP
>
> > -static void *dwarf_cus__process_cu_thread(void *arg)
> > +static struct cu *dwarf_loader__decode_next_cu(struct dwarf_cus *dcus)
> > {
> > - struct dwarf_thread *dthr = arg;
> > - struct dwarf_cus *dcus = dthr->dcus;
> > uint8_t pointer_size, offset_size;
> > + struct dwarf_cu *dcu = NULL;
> > Dwarf_Die die_mem, *cu_die;
> > - struct dwarf_cu *dcu;
> > + int err;
> >
> > - while (dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size) == 0) {
> > - if (cu_die == NULL)
> > + err = dwarf_cus__nextcu(dcus, &dcu, &die_mem, &cu_die, &pointer_size, &offset_size);
> > +
> > + if (err < 0)
> > + goto out_error;
> > + else if (err == 1) /* no more CUs */
> > + return NULL;
> > +
> > + err = die__process_and_recode(cu_die, dcu->cu, dcus->conf);
> > + if (err)
> > + goto out_error;
> > + if (cu_die == NULL)
> > + return NULL;
>
>
> should this check be placed before calling die__process_and_recode ?
Yes, but actually this check is redundant. If dwarf_cus__nextcu
returns 0, then the cu_die is valid. There are null checks within
dwarf_cus__nextcu:
if (ret == 0 && *cu_die != NULL) {
*dcu = dwarf_cus__create_cu(dcus, *cu_die, *pointer_size);
if (*dcu == NULL) {
dcus->error = ENOMEM;
ret = -1;
goto out_unlock;
}
// Do it here to keep all CUs in cus->cus in the same
// order as in the DWARF file being loaded (e.g. vmlinux)
__cus__add(dcus->cus, (*dcu)->cu);
}
>
>
> SNIP
>
> > -static int dwarf_cus__process_cus(struct dwarf_cus *dcus)
> > -{
> > - if (dcus->conf->nr_jobs > 1)
> > - return dwarf_cus__threaded_process_cus(dcus);
> > -
> > - return __dwarf_cus__process_cus(dcus);
> > -}
> > -
> > static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> > Dwfl_Module *mod, Dwarf *dw, Elf *elf,
> > const char *filename,
> > @@ -3733,7 +3859,8 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> > if (cu__resolve_func_ret_types_optimized(cu) != LSK__KEEPIT)
> > goto out_abort;
> >
> > - if (cus__finalize(cus, cu, conf, NULL) == LSK__STOP_LOADING)
> > + cu__finalize(cu, cus, conf);
> > + if (cus__steal_now(cus, cu, conf) == LSK__STOP_LOADING)
> > goto out_abort;
> >
> > return 0;
> > @@ -3765,7 +3892,8 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
> > }
> >
> > if (type_cu != NULL) {
> > - type_lsk = cu__finalize(type_cu, cus, conf, NULL);
> > + cu__finalize(type_cu, cus, conf);
> > + type_lsk = cus__steal_now(cus, type_cu, conf);
> > if (type_lsk == LSK__DELETE) {
> > cus__remove(cus, type_cu);
> > }
> > @@ -3787,6 +3915,7 @@ static int cus__load_module(struct cus *cus, struct conf_load *conf,
> > .type_dcu = type_cu ? &type_dcu : NULL,
> > .build_id = build_id,
> > .build_id_len = build_id_len,
> > + .nr_cus_created = 0,
>
>
> should go to the previous patch? if needed at all..
Yeah. I think it's better to have an explicit assignment.
>
> thanks,
> jirka
>
> > };
> > res = dwarf_cus__process_cus(&dcus);
> > }
> > diff --git a/dwarves.c b/dwarves.c
> > index ae512b9..7c3e878 100644
> > --- a/dwarves.c
> > +++ b/dwarves.c
> > @@ -530,48 +530,6 @@ void cus__unlock(struct cus *cus)
> > pthread_mutex_unlock(&cus->mutex);
> > }
>
>
> SNIP
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list
2025-01-01 16:56 ` Jiri Olsa
@ 2025-01-03 0:43 ` Ihor Solodrai
2025-01-03 9:27 ` Jiri Olsa
0 siblings, 1 reply; 22+ messages in thread
From: Ihor Solodrai @ 2025-01-03 0:43 UTC (permalink / raw)
To: Jiri Olsa; +Cc: dwarves, acme, alan.maguire, eddyz87, andrii, mykolal, bpf
On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Sat, Dec 21, 2024 at 01:23:45AM +0000, Ihor Solodrai wrote:
>
> SNIP
>
> > -static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_encoding_inconsistent_proto)
> > {
> > struct btf_encoder_func_state **saved_fns, *s;
> > - struct btf_encoder *e = NULL;
> > - int i = 0, j, nr_saved_fns = 0;
> > + int err = 0, i = 0, j, nr_saved_fns = 0;
> >
> > - /* Retrieve function states from each encoder, combine them
> > + /* Retrieve function states from the 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++;
> > + list_for_each_entry(s, &encoder->func_states, node) {
> > + nr_saved_fns++;
> > }
> >
> > if (nr_saved_fns == 0)
> > - return 0;
> > + goto out;
> >
> > 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;
> > + if (!saved_fns) {
> > + err = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + list_for_each_entry(s, &encoder->func_states, node) {
> > + saved_fns[i++] = s;
> > }
> > qsort(saved_fns, nr_saved_fns, sizeof(*saved_fns), saved_functions_cmp);
> >
> > @@ -1377,11 +1313,10 @@ static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> >
> > /* 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);
> > - }
> > + btf_encoder__delete_saved_funcs(encoder);
>
>
> is this call necessary? there's btf_encoder__delete call right after
> same for elf_functions_list__clear in btf_encoder__encode
At this point we know that the function information is no longer
needed, so we can free up some memory.
I remember when I wrote a "context" patch [1] (now discarded), there
was a significant difference in MAX RSS between freeing this right
away and delaying until encoding is finished. Now it might not be as
significant, but still there is no reason to keep the stuff we know is
not used later.
[1] https://lore.kernel.org/dwarves/20241213223641.564002-8-ihor.solodrai@pm.me/
>
> thanks,
> jirka
>
> > - return 0;
> > +out:
> > + return err;
> > }
>
>
> SNIP
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list
2025-01-03 0:43 ` Ihor Solodrai
@ 2025-01-03 9:27 ` Jiri Olsa
0 siblings, 0 replies; 22+ messages in thread
From: Jiri Olsa @ 2025-01-03 9:27 UTC (permalink / raw)
To: Ihor Solodrai
Cc: Jiri Olsa, dwarves, acme, alan.maguire, eddyz87, andrii, mykolal,
bpf
On Fri, Jan 03, 2025 at 12:43:42AM +0000, Ihor Solodrai wrote:
> On Wednesday, January 1st, 2025 at 8:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
>
> >
> > On Sat, Dec 21, 2024 at 01:23:45AM +0000, Ihor Solodrai wrote:
> >
> > SNIP
> >
> > > -static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > > +static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_encoding_inconsistent_proto)
> > > {
> > > struct btf_encoder_func_state **saved_fns, *s;
> > > - struct btf_encoder *e = NULL;
> > > - int i = 0, j, nr_saved_fns = 0;
> > > + int err = 0, i = 0, j, nr_saved_fns = 0;
> > >
> > > - /* Retrieve function states from each encoder, combine them
> > > + /* Retrieve function states from the 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++;
> > > + list_for_each_entry(s, &encoder->func_states, node) {
> > > + nr_saved_fns++;
> > > }
> > >
> > > if (nr_saved_fns == 0)
> > > - return 0;
> > > + goto out;
> > >
> > > 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;
> > > + if (!saved_fns) {
> > > + err = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > + list_for_each_entry(s, &encoder->func_states, node) {
> > > + saved_fns[i++] = s;
> > > }
> > > qsort(saved_fns, nr_saved_fns, sizeof(*saved_fns), saved_functions_cmp);
> > >
> > > @@ -1377,11 +1313,10 @@ static int btf_encoder__add_saved_funcs(bool skip_encoding_inconsistent_proto)
> > >
> > > /* 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);
> > > - }
> > > + btf_encoder__delete_saved_funcs(encoder);
> >
> >
> > is this call necessary? there's btf_encoder__delete call right after
> > same for elf_functions_list__clear in btf_encoder__encode
>
> At this point we know that the function information is no longer
> needed, so we can free up some memory.
>
> I remember when I wrote a "context" patch [1] (now discarded), there
> was a significant difference in MAX RSS between freeing this right
> away and delaying until encoding is finished. Now it might not be as
> significant, but still there is no reason to keep the stuff we know is
> not used later.
ok, makes sense
jirka
>
> [1] https://lore.kernel.org/dwarves/20241213223641.564002-8-ihor.solodrai@pm.me/
>
> >
> > thanks,
> > jirka
> >
> > > - return 0;
> > > +out:
> > > + return err;
> > > }
> >
> >
> > SNIP
>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-01-03 9:27 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-21 1:22 [PATCH dwarves v3 0/8] pahole: faster reproducible BTF encoding Ihor Solodrai
2024-12-21 1:22 ` [PATCH dwarves v3 1/8] btf_encoder: simplify function encoding Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations Ihor Solodrai
2025-01-01 16:56 ` Jiri Olsa
2025-01-02 19:54 ` Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 3/8] btf_encoder: introduce elf_functions struct type Ihor Solodrai
2025-01-01 16:56 ` Jiri Olsa
2025-01-02 20:06 ` Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 4/8] btf_encoder: introduce elf_functions_list Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 5/8] btf_encoder: remove skip_encoding_inconsistent_proto Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 6/8] dwarf_loader: introduce cu->id Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 7/8] dwarf_loader: multithreading with a job/worker model Ihor Solodrai
2024-12-30 16:07 ` Arnaldo Carvalho de Melo
2024-12-30 16:11 ` Arnaldo Carvalho de Melo
2024-12-30 18:37 ` Arnaldo Carvalho de Melo
2025-01-02 23:09 ` Ihor Solodrai
2025-01-01 16:56 ` Jiri Olsa
2025-01-03 0:24 ` Ihor Solodrai
2024-12-21 1:23 ` [PATCH dwarves v3 8/8] btf_encoder: clean up global encoders list Ihor Solodrai
2025-01-01 16:56 ` Jiri Olsa
2025-01-03 0:43 ` Ihor Solodrai
2025-01-03 9:27 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox