On Thursday, October 17th, 2024 at 1:13 PM, Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
> On Thu, Oct 17, 2024 at 11:56:04AM +0100, Alan Maguire wrote:
>
> > On 16/10/2024 01:10, Ihor Solodrai wrote:
> >
> > > Introduce a global elf_functions_list variable in btf_encoder.c that
> > > contains an elf_functions per ELF.
> >
> > Arnaldo can help provide context here, but at least notionally I think
> > the idea of maintaining libdwarves as a library has value. In that
>
>
> Yeah, but in all these years I'm not aware of any user, I even need to
> do some testing on building pahole statically with libdwarves to see if
> we get performance improvements...
>
> > context, avoiding global lists where possible is a good thing I think,
>
>
> Even without a library :-)
>
> > since if it was used as a library, multiple invokations could confuse
> > the elf_functions list. To that end, can we make the elf_functions_list
> > a field in the conf_load perhaps? It already contains base_btf so there
>
>
> conf_load looks with the structs we have now, so I would try to start
> there.
>
> - Arnaldo
>
> > is a precedent for storing data relevant to all encoders there, and
> > btf_encoder__new() has conf_load as a parameter, so the elf functions
> > list can still always be retrieved on encoder creation. In addition the
> > parameter to cus__process_dwflmod() has the parms structure which
> > contains the conf_load; you'd just need to pass that through to your
> > pre_load_module() callback I think.
> >
> > It shouldn't be a massive change but I think it would be worthwhile.
> >
> > Thanks!
Hi Alan, Arnaldo, thank you for review.
I understand your points, however adding elf_functions to conf_load
feels wrong to me. The global list I added was analogous to `encoders`
list in btf_encoder.c
I poked at the code a bit and came up with a context struct. It's
still a global variable, but with a clear init/exit interface.
Potentially other global BTF encoding-related state, for example
"main" btf_encoder and base_btf, could be stored in this context too.
Please see the diff below and let me know what you think.
---
btf_encoder.c | 119 ++++++++++++++++++++++++++++----------------------
btf_encoder.h | 3 ++
pahole.c | 16 ++++---
3 files changed, 80 insertions(+), 58 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index 1ec39fe..90aaa25 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -154,19 +154,76 @@ struct btf_kfunc_set_range {
uint64_t end;
};
+static struct {
+ bool initialized;
+
+ /* In principle, multiple ELFs can be processed in one pahole run,
+ * so we have to store elf_functions table per ELF.
+ * An element is added to the list on btf_encoder__pre_load_module,
+ * and removed after btf_encoder__encode is done.
+ */
+ struct list_head elf_functions_list;
+
+ /* mutex only needed for add/delete, as this can happen in multiple encoding
+ * threads. Traversal of the list is currently confined to thread collection.
+ */
+ pthread_mutex_t btf_encoder_list_lock;
+ struct list_head btf_encoder_list;
+
+} btf_encoding_context;
+
+int btf_encoding_context__init() {
+ int err = 0;
+
+ if (btf_encoding_context.initialized) {
+ fprintf(stderr, "btf_encoding_context__init() called while context is already initialized\n");
+ err = -1;
+ goto out;
+ }
+
+ INIT_LIST_HEAD(&btf_encoding_context.elf_functions_list);
+ INIT_LIST_HEAD(&btf_encoding_context.btf_encoder_list);
+ pthread_mutex_init(&btf_encoding_context.btf_encoder_list_lock, NULL);
+ btf_encoding_context.initialized = true;
+
+out:
+ return err;
+}
+
+static inline void elf_functions__delete(struct elf_functions *funcs);
+
+void btf_encoding_context__exit() {
+ struct list_head *pos, *tmp;
+
+ if (!btf_encoding_context.initialized) {
+ fprintf(stderr, "btf_encoding_context__exit() called while context is not initialized\n");
+ return;
+ }
+
+ list_for_each_safe(pos, tmp, &btf_encoding_context.elf_functions_list) {
+ struct elf_functions *funcs = list_entry(pos, struct elf_functions, node);
+ list_del(&funcs->node);
+ elf_functions__delete(funcs);
+ }
+
+ pthread_mutex_lock(&btf_encoding_context.btf_encoder_list_lock);
+ list_for_each_safe(pos, tmp, &btf_encoding_context.btf_encoder_list) {
+ struct btf_encoder *encoder = list_entry(pos, struct btf_encoder, node);
+ list_del(&encoder->node);
+ btf_encoder__delete(encoder);
+ }
+ pthread_mutex_unlock(&btf_encoding_context.btf_encoder_list_lock);
+
+ pthread_mutex_destroy(&btf_encoding_context.btf_encoder_list_lock);
+ btf_encoding_context.initialized = false;
+}
-/* In principle, multiple ELFs can be processed in one pahole run,
- * so we have to store elf_functions table per ELF.
- * An element is added to the list on btf_encoder__pre_load_module,
- * and removed after btf_encoder__encode is done.
- */
-static LIST_HEAD(elf_functions_list);
static struct elf_functions *elf_functions__get(Elf *elf)
{
struct list_head *pos;
- list_for_each(pos, &elf_functions_list) {
+ list_for_each(pos, &btf_encoding_context.elf_functions_list) {
struct elf_functions *funcs = list_entry(pos, struct elf_functions, node);
if (funcs->elf == elf)
@@ -198,21 +255,9 @@ static void __elf_functions__delete(struct elf_functions *funcs)
static inline void elf_functions__delete(struct elf_functions *funcs)
{
__elf_functions__delete(funcs);
- list_del(&funcs->node);
free(funcs);
}
-static inline void elf_functions__delete_all(void)
-{
- struct list_head *pos, *tmp;
-
- list_for_each_safe(pos, tmp, &elf_functions_list) {
- struct elf_functions *funcs = list_entry(pos, struct elf_functions, node);
-
- elf_functions__delete(funcs);
- }
-}
-
static int elf_functions__collect(struct elf_functions *functions);
int btf_encoder__pre_load_module(Dwfl_Module *mod, Elf *elf)
@@ -237,7 +282,7 @@ int btf_encoder__pre_load_module(Dwfl_Module *mod, Elf *elf)
if (err)
goto out_delete;
- list_add_tail(&funcs->node, &elf_functions_list);
+ list_add_tail(&funcs->node, &btf_encoding_context.elf_functions_list);
return 0;
@@ -246,37 +291,11 @@ out_delete:
return err;
}
-
-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);
+ pthread_mutex_lock(&btf_encoding_context.btf_encoder_list_lock);
+ list_add_tail(&encoder->node, &btf_encoding_context.btf_encoder_list);
+ pthread_mutex_unlock(&btf_encoding_context.btf_encoder_list_lock);
}
#define PERCPU_SECTION ".data..percpu"
@@ -2215,7 +2234,6 @@ int btf_encoder__encode(struct btf_encoder *encoder)
err = btf_encoder__write_elf(encoder, encoder->btf, BTF_ELF_SEC);
}
- elf_functions__delete_all();
return err;
}
@@ -2588,7 +2606,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);
zfree(&encoder->filename);
diff --git a/btf_encoder.h b/btf_encoder.h
index 29f652a..c3886b8 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -23,6 +23,9 @@ enum btf_var_option {
BTF_VAR_GLOBAL = 2,
};
+int btf_encoding_context__init();
+void btf_encoding_context__exit();
+
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);
diff --git a/pahole.c b/pahole.c
index 6f83636..2a6e012 100644
--- a/pahole.c
+++ b/pahole.c
@@ -3280,12 +3280,6 @@ static int pahole_threads_collect(struct conf_load *conf, int nr_threads, void *
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;
@@ -3818,8 +3812,12 @@ int main(int argc, char *argv[])
conf_load.threads_collect = pahole_threads_collect;
}
- if (btf_encode)
+ if (btf_encode) {
conf_load.pre_load_module = btf_encoder__pre_load_module;
+ int err = btf_encoding_context__init();
+ if (err < 0)
+ goto out;
+ }
// Make 'pahole --header type < file' a shorter form of 'pahole -C type --count 1 < file'
if (conf.header_type && !class_name && prettify_input) {
@@ -3928,7 +3926,11 @@ try_sole_arg_as_class_names:
goto out_cus_delete;
}
}
+
out_ok:
+ if (btf_encode)
+ btf_encoding_context__exit();
+
if (stats_formatter != NULL)
print_stats();
--
2.43.0
> > > [...]
