diff mbox series

[dwarves,v2,06/10] btf_encoder: switch to shared elf_functions table

Message ID 20241213223641.564002-7-ihor.solodrai@pm.me (mailing list archive)
State Not Applicable
Headers show
Series pahole: shared ELF and faster reproducible BTF encoding | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Ihor Solodrai Dec. 13, 2024, 10:37 p.m. UTC 
Do not collect functions from ELF for each new btf_encoder, and
instead set a pointer to a shared elf_functions table, built
beforehand by btf_encoder__pre_cus__load_module().

Set btf_encoder's ELF symbol table to one associated with the
corresponding elf_functions, instead of maintaining a copy.

For a single-threaded case call btf_encoder__add_saved_funcs() right
before btf_encoder__encode(), and not within it.

Signed-off-by: Ihor Solodrai <ihor.solodrai@pm.me>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Link: https://lore.kernel.org/bpf/20241128012341.4081072-8-ihor.solodrai@pm.me/
---
 btf_encoder.c | 49 +++++++++++++++++++++++++------------------------
 pahole.c      | 10 ++++++++--
 2 files changed, 33 insertions(+), 26 deletions(-)

Comments

Jiri Olsa Dec. 19, 2024, 2:58 p.m. UTC | #1 
On Fri, Dec 13, 2024 at 10:37:13PM +0000, Ihor Solodrai wrote:

SNIP

> @@ -2116,9 +2128,6 @@ int btf_encoder__encode(struct btf_encoder *encoder)
>  	int err;
>  	size_t shndx;
>  
> -	/* for single-threaded case, saved funcs are added here */
> -	btf_encoder__add_saved_funcs(encoder);
> -
>  	for (shndx = 1; shndx < encoder->seccnt; shndx++)
>  		if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
>  			btf_encoder__add_datasec(encoder, shndx);
> @@ -2477,14 +2486,13 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
>  			goto out_delete;
>  		}
>  
> -		encoder->symtab = elf_symtab__new(NULL, cu->elf);
> +		encoder->functions = elf_functions__get(cu->elf);

elf_functions__get should always return != NULL right? should we add assert call for that?

SNIP

> diff --git a/pahole.c b/pahole.c
> index 17af0b4..eb2e71a 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -3185,13 +3185,16 @@ 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(btf_encoder);
> +	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)
> +		if (!threads[i]->encoder || !threads[i]->btf)

is this related to this change? seems like separate fix

>  			continue;
>  		err = btf_encoder__add_encoder(btf_encoder, threads[i]->encoder);
>  		if (err < 0)
> @@ -3846,6 +3849,9 @@ try_sole_arg_as_class_names:
>  			exit(1);
>  		}
>  
> +		if (conf_load.nr_jobs <= 1 || conf_load.reproducible_build)
> +			btf_encoder__add_saved_funcs(btf_encoder);

should we check the return value here as well?

thanks,
jirka

> +
>  		err = btf_encoder__encode(btf_encoder);
>  		btf_encoder__delete(btf_encoder);
>  		if (err) {
> -- 
> 2.47.1
> 
> 
>
Ihor Solodrai Dec. 19, 2024, 7:06 p.m. UTC | #2 
On Thursday, December 19th, 2024 at 6:58 AM, Jiri Olsa <olsajiri@gmail.com> wrote:

> 
> 
> On Fri, Dec 13, 2024 at 10:37:13PM +0000, Ihor Solodrai wrote:
> 
> SNIP
> 
> > @@ -2116,9 +2128,6 @@ int btf_encoder__encode(struct btf_encoder *encoder)
> > int err;
> > size_t shndx;
> > 
> > - /* for single-threaded case, saved funcs are added here */
> > - btf_encoder__add_saved_funcs(encoder);
> > -
> > for (shndx = 1; shndx < encoder->seccnt; shndx++)
> > if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
> > btf_encoder__add_datasec(encoder, shndx);
> > @@ -2477,14 +2486,13 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
> > goto out_delete;
> > }
> > 
> > - encoder->symtab = elf_symtab__new(NULL, cu->elf);
> > + encoder->functions = elf_functions__get(cu->elf);
> 
> 
> elf_functions__get should always return != NULL right? should we add assert call for that?
> 
> SNIP
> 
> > diff --git a/pahole.c b/pahole.c
> > index 17af0b4..eb2e71a 100644
> > --- a/pahole.c
> > +++ b/pahole.c
> > @@ -3185,13 +3185,16 @@ 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(btf_encoder);
> > + 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)
> > + if (!threads[i]->encoder || !threads[i]->btf)
> 
> 
> is this related to this change? seems like separate fix
> 
> > continue;
> > err = btf_encoder__add_encoder(btf_encoder, threads[i]->encoder);
> > if (err < 0)
> > @@ -3846,6 +3849,9 @@ try_sole_arg_as_class_names:
> > exit(1);
> > }
> > 
> > + if (conf_load.nr_jobs <= 1 || conf_load.reproducible_build)
> > + btf_encoder__add_saved_funcs(btf_encoder);
> 
> 
> should we check the return value here as well?
> 
> thanks,
> jirka

Jiri, thanks for review.

This patch is going to be removed in the next version of the series,
following a discussion with Eduard and Andrii [1].

If you're interested you can inspect WIP v3 branch on github [2].

I am still testing it, and plan to add a patch removing global
btf_encoders list. Other than that it's close to what I am going to
submit.

[1] https://lore.kernel.org/dwarves/C82bYTvJaV4bfT15o25EsBiUvFsj5eTlm17933Hvva76CXjIcu3gvpaOCWPgeZ8g3cZ-RMa8Vp0y1o_QMR2LhPB-LEUYfZCGuCfR_HvkIP8=@pm.me/
[2] https://github.com/theihor/dwarves/tree/v3.workers

> 
> > +
> > err = btf_encoder__encode(btf_encoder);
> > btf_encoder__delete(btf_encoder);
> > if (err) {
> > --
> > 2.47.1
diff mbox series

Patch

diff --git a/btf_encoder.c b/btf_encoder.c
index 88d2872..4a4f6c8 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -136,7 +136,7 @@  struct btf_encoder {
 	size_t             seccnt;
 	int                encode_vars;
 	struct list_head   func_states;
-	struct elf_functions functions;
+	struct elf_functions *functions;
 };
 
 struct btf_func {
@@ -158,8 +158,23 @@  struct btf_kfunc_set_range {
  */
 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) {
+		struct elf_functions *funcs = list_entry(pos, struct elf_functions, node);
+
+		if (funcs->elf == elf)
+			return funcs;
+	}
+	return NULL;
+}
+
 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);
@@ -168,11 +183,11 @@  static inline void elf_functions__delete(struct elf_functions *funcs)
 
 static inline void elf_functions__delete_all(void)
 {
+	struct elf_functions *funcs;
 	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);
-
+		funcs = list_entry(pos, struct elf_functions, node);
 		elf_functions__delete(funcs);
 	}
 }
@@ -1316,9 +1331,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)
@@ -1406,7 +1418,7 @@  static struct elf_function *btf_encoder__find_function(const struct btf_encoder
 {
 	struct elf_function key = { .name = name, .prefixlen = prefixlen };
 
-	return bsearch(&key, encoder->functions.entries, encoder->functions.cnt, sizeof(key), functions_cmp);
+	return bsearch(&key, encoder->functions->entries, encoder->functions->cnt, sizeof(key), functions_cmp);
 }
 
 static bool btf_name_char_ok(char c, bool first)
@@ -2116,9 +2128,6 @@  int btf_encoder__encode(struct btf_encoder *encoder)
 	int err;
 	size_t shndx;
 
-	/* for single-threaded case, saved funcs are added here */
-	btf_encoder__add_saved_funcs(encoder);
-
 	for (shndx = 1; shndx < encoder->seccnt; shndx++)
 		if (gobuffer__size(&encoder->secinfo[shndx].secinfo))
 			btf_encoder__add_datasec(encoder, shndx);
@@ -2477,14 +2486,13 @@  struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
 			goto out_delete;
 		}
 
-		encoder->symtab = elf_symtab__new(NULL, cu->elf);
+		encoder->functions = elf_functions__get(cu->elf);
+		encoder->symtab = encoder->functions->symtab;
 		if (!encoder->symtab) {
 			if (encoder->verbose)
 				printf("%s: '%s' doesn't have symtab.\n", __func__, cu->filename);
 			goto out;
 		}
-		encoder->functions.symtab = encoder->symtab;
-		encoder->functions.elf = cu->elf;
 
 		/* index the ELF sections for later lookup */
 
@@ -2523,9 +2531,6 @@  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);
@@ -2553,12 +2558,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;
-
+	encoder->symtab = NULL;
+	encoder->functions = NULL;
 	btf_encoder__delete_saved_funcs(encoder);
 
 	free(encoder);
@@ -2657,7 +2658,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 (encoder->functions->cnt) {
 			const char *name;
 
 			name = function__name(fn);
@@ -2666,7 +2667,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 && 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
diff --git a/pahole.c b/pahole.c
index 17af0b4..eb2e71a 100644
--- a/pahole.c
+++ b/pahole.c
@@ -3185,13 +3185,16 @@  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(btf_encoder);
+	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)
+		if (!threads[i]->encoder || !threads[i]->btf)
 			continue;
 		err = btf_encoder__add_encoder(btf_encoder, threads[i]->encoder);
 		if (err < 0)
@@ -3846,6 +3849,9 @@  try_sole_arg_as_class_names:
 			exit(1);
 		}
 
+		if (conf_load.nr_jobs <= 1 || conf_load.reproducible_build)
+			btf_encoder__add_saved_funcs(btf_encoder);
+
 		err = btf_encoder__encode(btf_encoder);
 		btf_encoder__delete(btf_encoder);
 		if (err) {