[PATCH 0/4] perf tools: Fix MMAP2 build-ID synthesis in namespaces (v1)

[PATCH 0/4] perf tools: Fix MMAP2 build-ID synthesis in namespaces (v1)

[Namhyung Kim]

Hello,

The perf record --buildid-mmap enables PERF_RECORD_MISC_MMAP_BUILD_ID
which includes a build-ID of the DSO.  It requires to read the info from the
file when synthesizing.  But I found some issues in the current code that it
didn't check duplicate files and didn't handle namespaces.

I think chroot is not a concern since perf will see it from the outside so
that it can see the full path.

The code is available at 'perf/mmap2-buildid-fix-v1' branch on

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (4):
  perf tools: Move dsos functions to util/dsos.c
  perf tools: Add perf_event__synthesize_{start,stop}()
  perf record: Save DSO build-ID for synthesizing
  perf tools: Honor namespace when synthesizing build-id

 tools/perf/builtin-inject.c        |  3 ++
 tools/perf/builtin-record.c        |  3 ++
 tools/perf/builtin-stat.c          |  2 +
 tools/perf/builtin-top.c           |  4 ++
 tools/perf/util/auxtrace.c         |  2 +
 tools/perf/util/dsos.c             | 29 +++++++++++++
 tools/perf/util/dsos.h             |  3 ++
 tools/perf/util/machine.c          | 29 -------------
 tools/perf/util/synthetic-events.c | 65 ++++++++++++++++++++++++++----
 tools/perf/util/synthetic-events.h |  3 ++
 10 files changed, 107 insertions(+), 36 deletions(-)


base-commit: 62e64c9d2fd12839c02f1b3e8b873e7cb34e8720
-- 
2.37.3.968.ga6b4b080e4-goog

[PATCH 1/4] perf tools: Move dsos functions to util/dsos.c

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dsos.c    | 29 +++++++++++++++++++++++++++++
 tools/perf/util/dsos.h    |  3 +++
 tools/perf/util/machine.c | 29 -----------------------------
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index 2bd23e4cf19e..90a800625110 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -12,6 +12,35 @@
 #include <symbol.h> // filename__read_build_id
 #include <unistd.h>
 
+void dsos__init(struct dsos *dsos)
+{
+	INIT_LIST_HEAD(&dsos->head);
+	dsos->root = RB_ROOT;
+	init_rwsem(&dsos->lock);
+}
+
+static void dsos__purge(struct dsos *dsos)
+{
+	struct dso *pos, *n;
+
+	down_write(&dsos->lock);
+
+	list_for_each_entry_safe(pos, n, &dsos->head, node) {
+		RB_CLEAR_NODE(&pos->rb_node);
+		pos->root = NULL;
+		list_del_init(&pos->node);
+		dso__put(pos);
+	}
+
+	up_write(&dsos->lock);
+}
+
+void dsos__exit(struct dsos *dsos)
+{
+	dsos__purge(dsos);
+	exit_rwsem(&dsos->lock);
+}
+
 static int __dso_id__cmp(struct dso_id *a, struct dso_id *b)
 {
 	if (a->maj > b->maj) return -1;
diff --git a/tools/perf/util/dsos.h b/tools/perf/util/dsos.h
index 5dbec2bc6966..49f448f106f8 100644
--- a/tools/perf/util/dsos.h
+++ b/tools/perf/util/dsos.h
@@ -21,6 +21,9 @@ struct dsos {
 	struct rw_semaphore lock;
 };
 
+void dsos__init(struct dsos *dsos);
+void dsos__exit(struct dsos *dsos);
+
 void __dsos__add(struct dsos *dsos, struct dso *dso);
 void dsos__add(struct dsos *dsos, struct dso *dso);
 struct dso *__dsos__addnew(struct dsos *dsos, const char *name);
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 2a16cae28407..4c5540f5c753 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -50,13 +50,6 @@ static struct dso *machine__kernel_dso(struct machine *machine)
 	return machine->vmlinux_map->dso;
 }
 
-static void dsos__init(struct dsos *dsos)
-{
-	INIT_LIST_HEAD(&dsos->head);
-	dsos->root = RB_ROOT;
-	init_rwsem(&dsos->lock);
-}
-
 static void machine__threads_init(struct machine *machine)
 {
 	int i;
@@ -181,28 +174,6 @@ struct machine *machine__new_kallsyms(void)
 	return machine;
 }
 
-static void dsos__purge(struct dsos *dsos)
-{
-	struct dso *pos, *n;
-
-	down_write(&dsos->lock);
-
-	list_for_each_entry_safe(pos, n, &dsos->head, node) {
-		RB_CLEAR_NODE(&pos->rb_node);
-		pos->root = NULL;
-		list_del_init(&pos->node);
-		dso__put(pos);
-	}
-
-	up_write(&dsos->lock);
-}
-
-static void dsos__exit(struct dsos *dsos)
-{
-	dsos__purge(dsos);
-	exit_rwsem(&dsos->lock);
-}
-
 void machine__delete_threads(struct machine *machine)
 {
 	struct rb_node *nd;
-- 
2.37.3.968.ga6b4b080e4-goog

[PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}()

[Namhyung Kim]

These functions are to prepare and cleanup necessary work for synthesizing.
It doesn't do anything yet but later patch will add it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-inject.c        | 3 +++
 tools/perf/builtin-record.c        | 3 +++
 tools/perf/builtin-stat.c          | 2 ++
 tools/perf/builtin-top.c           | 4 ++++
 tools/perf/util/auxtrace.c         | 2 ++
 tools/perf/util/synthetic-events.c | 8 ++++++++
 tools/perf/util/synthetic-events.h | 3 +++
 7 files changed, 25 insertions(+)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index e254f18986f7..2e91a887919b 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -2368,9 +2368,12 @@ int cmd_inject(int argc, const char **argv)
 	if (ret < 0)
 		goto out_delete;
 
+	perf_event__synthesize_start();
+
 	ret = __cmd_inject(&inject);
 
 	guest_session__exit(&inject.guest_session);
+	perf_event__synthesize_stop();
 
 out_delete:
 	strlist__delete(inject.known_build_ids);
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 02e38f50a138..5b7b9ad2a280 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1966,6 +1966,8 @@ static int record__synthesize(struct record *rec, bool tail)
 	if (rec->opts.tail_synthesize != tail)
 		return 0;
 
+	perf_event__synthesize_start();
+
 	if (data->is_pipe) {
 		err = perf_event__synthesize_for_pipe(tool, session, data,
 						      process_synthesized_event);
@@ -2072,6 +2074,7 @@ static int record__synthesize(struct record *rec, bool tail)
 	}
 
 out:
+	perf_event__synthesize_stop();
 	return err;
 }
 
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e05fe72c1d87..f6f61e08f4c2 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -962,6 +962,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		if (err < 0)
 			return err;
 
+		perf_event__synthesize_start();
 		err = perf_event__synthesize_stat_events(&stat_config, NULL, evsel_list,
 							 process_synthesized_event, is_pipe);
 		if (err < 0)
@@ -2641,6 +2642,7 @@ int cmd_stat(int argc, const char **argv)
 			perf_session__write_header(perf_stat.session, evsel_list, fd, true);
 		}
 
+		perf_event__synthesize_stop();
 		evlist__close(evsel_list);
 		perf_session__delete(perf_stat.session);
 	}
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index e89208b4ad4b..1eff894e6b5f 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1258,6 +1258,8 @@ static int __cmd_top(struct perf_top *top)
 #endif
 	}
 
+	perf_event__synthesize_start();
+
 	ret = perf_event__synthesize_bpf_events(top->session, perf_event__process,
 						&top->session->machines.host,
 						&top->record_opts);
@@ -1273,6 +1275,8 @@ static int __cmd_top(struct perf_top *top)
 				    top->evlist->core.threads, true, false,
 				    top->nr_threads_synthesize);
 
+	perf_event__synthesize_stop();
+
 	if (top->nr_threads_synthesize > 1)
 		perf_set_singlethreaded();
 
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index b59c278fe9ed..1bfe076c22fb 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -1328,6 +1328,7 @@ int perf_event__process_auxtrace_info(struct perf_session *session,
 	if (err)
 		return err;
 
+	perf_event__synthesize_start();
 	unleader_auxtrace(session);
 
 	return 0;
@@ -2834,6 +2835,7 @@ void auxtrace__free(struct perf_session *session)
 	if (!session->auxtrace)
 		return;
 
+	perf_event__synthesize_stop();
 	return session->auxtrace->free(session);
 }
 
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 0ff57ca24577..9d4f5dacd154 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -47,6 +47,14 @@
 
 unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
 
+void perf_event__synthesize_start(void)
+{
+}
+
+void perf_event__synthesize_stop(void)
+{
+}
+
 int perf_tool__process_synth_event(struct perf_tool *tool,
 				   union perf_event *event,
 				   struct machine *machine,
diff --git a/tools/perf/util/synthetic-events.h b/tools/perf/util/synthetic-events.h
index 53737d1619a4..e4414616080c 100644
--- a/tools/perf/util/synthetic-events.h
+++ b/tools/perf/util/synthetic-events.h
@@ -43,6 +43,9 @@ int parse_synth_opt(char *str);
 typedef int (*perf_event__handler_t)(struct perf_tool *tool, union perf_event *event,
 				     struct perf_sample *sample, struct machine *machine);
 
+void perf_event__synthesize_start(void);
+void perf_event__synthesize_stop(void);
+
 int perf_event__synthesize_attrs(struct perf_tool *tool, struct evlist *evlist, perf_event__handler_t process);
 int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *attr, u32 ids, u64 *id, perf_event__handler_t process);
 int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16 misc, perf_event__handler_t process, struct machine *machine);
-- 
2.37.3.968.ga6b4b080e4-goog

[PATCH 3/4] perf record: Save DSO build-ID for synthesizing

[Namhyung Kim]

When synthesizing MMAP2 with build-id, it'd read the same file repeatedly as
it has no idea if it's done already.  Maintain a dsos to check that and skip
the file access if possible.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/synthetic-events.c | 49 +++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 9d4f5dacd154..e6978b2dee8f 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -4,6 +4,7 @@
 #include "util/data.h"
 #include "util/debug.h"
 #include "util/dso.h"
+#include "util/dsos.h"
 #include "util/event.h"
 #include "util/evlist.h"
 #include "util/machine.h"
@@ -47,12 +48,25 @@
 
 unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
 
+static bool synth_init_done;
+static struct dsos synth_dsos;
+
 void perf_event__synthesize_start(void)
 {
+	if (synth_init_done)
+		return;
+
+	dsos__init(&synth_dsos);
+
+	synth_init_done = true;
 }
 
 void perf_event__synthesize_stop(void)
 {
+	if (!synth_init_done)
+		return;
+
+	dsos__exit(&synth_dsos);
 }
 
 int perf_tool__process_synth_event(struct perf_tool *tool,
@@ -374,26 +388,47 @@ static bool read_proc_maps_line(struct io *io, __u64 *start, __u64 *end,
 static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
 					     bool is_kernel)
 {
-	struct build_id bid;
+	struct build_id _bid, *bid = &_bid;
+	struct dso *dso = NULL;
+	struct dso_id id;
 	int rc;
 
-	if (is_kernel)
-		rc = sysfs__read_build_id("/sys/kernel/notes", &bid);
-	else
-		rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
+	if (is_kernel) {
+		rc = sysfs__read_build_id("/sys/kernel/notes", bid);
+		goto out;
+	}
+
+	id.maj = event->maj;
+	id.min = event->min;
+	id.ino = event->ino;
+	id.ino_generation = event->ino_generation;
 
+	dso = dsos__findnew_id(&synth_dsos, event->filename, &id);
+	if (dso && dso->has_build_id) {
+		bid = &dso->bid;
+		rc = 0;
+		goto out;
+	}
+
+	rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
+
+out:
 	if (rc == 0) {
-		memcpy(event->build_id, bid.data, sizeof(bid.data));
-		event->build_id_size = (u8) bid.size;
+		memcpy(event->build_id, bid->data, sizeof(bid->data));
+		event->build_id_size = (u8) bid->size;
 		event->header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
 		event->__reserved_1 = 0;
 		event->__reserved_2 = 0;
+
+		if (dso && !dso->has_build_id)
+			dso__set_build_id(dso, bid);
 	} else {
 		if (event->filename[0] == '/') {
 			pr_debug2("Failed to read build ID for %s\n",
 				  event->filename);
 		}
 	}
+	dso__put(dso);
 }
 
 int perf_event__synthesize_mmap_events(struct perf_tool *tool,
-- 
2.37.3.968.ga6b4b080e4-goog

[PATCH 4/4] perf tools: Honor namespace when synthesizing build-id

[Namhyung Kim]

It needs to go into a namespace before reading a file.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/synthetic-events.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index e6978b2dee8f..d0d540d09196 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -391,6 +391,8 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
 	struct build_id _bid, *bid = &_bid;
 	struct dso *dso = NULL;
 	struct dso_id id;
+	struct nsinfo *nsi;
+	struct nscookie nc;
 	int rc;
 
 	if (is_kernel) {
@@ -410,8 +412,14 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
 		goto out;
 	}
 
+	nsi = nsinfo__new(event->pid);
+	nsinfo__mountns_enter(nsi, &nc);
+
 	rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
 
+	nsinfo__mountns_exit(&nc);
+	nsinfo__put(nsi);
+
 out:
 	if (rc == 0) {
 		memcpy(event->build_id, bid->data, sizeof(bid->data));
-- 
2.37.3.968.ga6b4b080e4-goog

Re: [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id

[Adrian Hunter]

On 16/09/22 20:59, Namhyung Kim wrote:
> It needs to go into a namespace before reading a file.

This looks like a fix, in which case make it the first patch
and add a fixes tag?

> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/synthetic-events.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index e6978b2dee8f..d0d540d09196 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -391,6 +391,8 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
>  	struct build_id _bid, *bid = &_bid;
>  	struct dso *dso = NULL;
>  	struct dso_id id;
> +	struct nsinfo *nsi;
> +	struct nscookie nc;
>  	int rc;
>  
>  	if (is_kernel) {
> @@ -410,8 +412,14 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
>  		goto out;
>  	}
>  
> +	nsi = nsinfo__new(event->pid);
> +	nsinfo__mountns_enter(nsi, &nc);
> +
>  	rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
>  
> +	nsinfo__mountns_exit(&nc);
> +	nsinfo__put(nsi);
> +
>  out:
>  	if (rc == 0) {
>  		memcpy(event->build_id, bid->data, sizeof(bid->data));

Re: [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}()

[Adrian Hunter]

On 16/09/22 20:59, Namhyung Kim wrote:
> These functions are to prepare and cleanup necessary work for synthesizing.
> It doesn't do anything yet but later patch will add it.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-inject.c        | 3 +++
>  tools/perf/builtin-record.c        | 3 +++
>  tools/perf/builtin-stat.c          | 2 ++
>  tools/perf/builtin-top.c           | 4 ++++
>  tools/perf/util/auxtrace.c         | 2 ++
>  tools/perf/util/synthetic-events.c | 8 ++++++++
>  tools/perf/util/synthetic-events.h | 3 +++
>  7 files changed, 25 insertions(+)
> 
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index e254f18986f7..2e91a887919b 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -2368,9 +2368,12 @@ int cmd_inject(int argc, const char **argv)
>  	if (ret < 0)
>  		goto out_delete;
>  
> +	perf_event__synthesize_start();
> +
>  	ret = __cmd_inject(&inject);
>  
>  	guest_session__exit(&inject.guest_session);
> +	perf_event__synthesize_stop();

AFAICT perf inject synthesizes mmap events only for JIT and that is
open-coded in jitdump.c. i.e. perf_event__synthesize_start / stop
not needed

>  
>  out_delete:
>  	strlist__delete(inject.known_build_ids);
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 02e38f50a138..5b7b9ad2a280 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1966,6 +1966,8 @@ static int record__synthesize(struct record *rec, bool tail)
>  	if (rec->opts.tail_synthesize != tail)
>  		return 0;
>  
> +	perf_event__synthesize_start();

Perhaps also record__synthesize_workload() ?

> +
>  	if (data->is_pipe) {
>  		err = perf_event__synthesize_for_pipe(tool, session, data,
>  						      process_synthesized_event);
> @@ -2072,6 +2074,7 @@ static int record__synthesize(struct record *rec, bool tail)
>  	}
>  
>  out:
> +	perf_event__synthesize_stop();
>  	return err;
>  }
>  
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index e05fe72c1d87..f6f61e08f4c2 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -962,6 +962,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  		if (err < 0)
>  			return err;
>  
> +		perf_event__synthesize_start();
>  		err = perf_event__synthesize_stat_events(&stat_config, NULL, evsel_list,
>  							 process_synthesized_event, is_pipe);
>  		if (err < 0)
> @@ -2641,6 +2642,7 @@ int cmd_stat(int argc, const char **argv)
>  			perf_session__write_header(perf_stat.session, evsel_list, fd, true);
>  		}
>  
> +		perf_event__synthesize_stop();
>  		evlist__close(evsel_list);
>  		perf_session__delete(perf_stat.session);
>  	}
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index e89208b4ad4b..1eff894e6b5f 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1258,6 +1258,8 @@ static int __cmd_top(struct perf_top *top)
>  #endif
>  	}
>  
> +	perf_event__synthesize_start();
> +
>  	ret = perf_event__synthesize_bpf_events(top->session, perf_event__process,
>  						&top->session->machines.host,
>  						&top->record_opts);
> @@ -1273,6 +1275,8 @@ static int __cmd_top(struct perf_top *top)
>  				    top->evlist->core.threads, true, false,
>  				    top->nr_threads_synthesize);
>  
> +	perf_event__synthesize_stop();
> +
>  	if (top->nr_threads_synthesize > 1)
>  		perf_set_singlethreaded();
>  
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index b59c278fe9ed..1bfe076c22fb 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -1328,6 +1328,7 @@ int perf_event__process_auxtrace_info(struct perf_session *session,
>  	if (err)
>  		return err;
>  
> +	perf_event__synthesize_start();
>  	unleader_auxtrace(session);
>  
>  	return 0;
> @@ -2834,6 +2835,7 @@ void auxtrace__free(struct perf_session *session)
>  	if (!session->auxtrace)
>  		return;
>  
> +	perf_event__synthesize_stop();

auxtrace does not synthesize mmap events

>  	return session->auxtrace->free(session);
>  }
>  
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 0ff57ca24577..9d4f5dacd154 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -47,6 +47,14 @@
>  
>  unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
>  
> +void perf_event__synthesize_start(void)
> +{
> +}
> +
> +void perf_event__synthesize_stop(void)
> +{
> +}
> +
>  int perf_tool__process_synth_event(struct perf_tool *tool,
>  				   union perf_event *event,
>  				   struct machine *machine,
> diff --git a/tools/perf/util/synthetic-events.h b/tools/perf/util/synthetic-events.h
> index 53737d1619a4..e4414616080c 100644
> --- a/tools/perf/util/synthetic-events.h
> +++ b/tools/perf/util/synthetic-events.h
> @@ -43,6 +43,9 @@ int parse_synth_opt(char *str);
>  typedef int (*perf_event__handler_t)(struct perf_tool *tool, union perf_event *event,
>  				     struct perf_sample *sample, struct machine *machine);
>  
> +void perf_event__synthesize_start(void);
> +void perf_event__synthesize_stop(void);
> +
>  int perf_event__synthesize_attrs(struct perf_tool *tool, struct evlist *evlist, perf_event__handler_t process);
>  int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *attr, u32 ids, u64 *id, perf_event__handler_t process);
>  int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16 misc, perf_event__handler_t process, struct machine *machine);

Re: [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c

[Adrian Hunter]

On 16/09/22 20:58, Namhyung Kim wrote:

Maybe say why the move.

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/dsos.c    | 29 +++++++++++++++++++++++++++++
>  tools/perf/util/dsos.h    |  3 +++
>  tools/perf/util/machine.c | 29 -----------------------------
>  3 files changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> index 2bd23e4cf19e..90a800625110 100644
> --- a/tools/perf/util/dsos.c
> +++ b/tools/perf/util/dsos.c
> @@ -12,6 +12,35 @@
>  #include <symbol.h> // filename__read_build_id
>  #include <unistd.h>
>  
> +void dsos__init(struct dsos *dsos)
> +{
> +	INIT_LIST_HEAD(&dsos->head);
> +	dsos->root = RB_ROOT;
> +	init_rwsem(&dsos->lock);
> +}
> +
> +static void dsos__purge(struct dsos *dsos)
> +{
> +	struct dso *pos, *n;
> +
> +	down_write(&dsos->lock);
> +
> +	list_for_each_entry_safe(pos, n, &dsos->head, node) {
> +		RB_CLEAR_NODE(&pos->rb_node);
> +		pos->root = NULL;
> +		list_del_init(&pos->node);
> +		dso__put(pos);
> +	}
> +
> +	up_write(&dsos->lock);
> +}
> +
> +void dsos__exit(struct dsos *dsos)
> +{
> +	dsos__purge(dsos);
> +	exit_rwsem(&dsos->lock);
> +}
> +
>  static int __dso_id__cmp(struct dso_id *a, struct dso_id *b)
>  {
>  	if (a->maj > b->maj) return -1;
> diff --git a/tools/perf/util/dsos.h b/tools/perf/util/dsos.h
> index 5dbec2bc6966..49f448f106f8 100644
> --- a/tools/perf/util/dsos.h
> +++ b/tools/perf/util/dsos.h
> @@ -21,6 +21,9 @@ struct dsos {
>  	struct rw_semaphore lock;
>  };
>  
> +void dsos__init(struct dsos *dsos);
> +void dsos__exit(struct dsos *dsos);
> +
>  void __dsos__add(struct dsos *dsos, struct dso *dso);
>  void dsos__add(struct dsos *dsos, struct dso *dso);
>  struct dso *__dsos__addnew(struct dsos *dsos, const char *name);
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 2a16cae28407..4c5540f5c753 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -50,13 +50,6 @@ static struct dso *machine__kernel_dso(struct machine *machine)
>  	return machine->vmlinux_map->dso;
>  }
>  
> -static void dsos__init(struct dsos *dsos)
> -{
> -	INIT_LIST_HEAD(&dsos->head);
> -	dsos->root = RB_ROOT;
> -	init_rwsem(&dsos->lock);
> -}
> -
>  static void machine__threads_init(struct machine *machine)
>  {
>  	int i;
> @@ -181,28 +174,6 @@ struct machine *machine__new_kallsyms(void)
>  	return machine;
>  }
>  
> -static void dsos__purge(struct dsos *dsos)
> -{
> -	struct dso *pos, *n;
> -
> -	down_write(&dsos->lock);
> -
> -	list_for_each_entry_safe(pos, n, &dsos->head, node) {
> -		RB_CLEAR_NODE(&pos->rb_node);
> -		pos->root = NULL;
> -		list_del_init(&pos->node);
> -		dso__put(pos);
> -	}
> -
> -	up_write(&dsos->lock);
> -}
> -
> -static void dsos__exit(struct dsos *dsos)
> -{
> -	dsos__purge(dsos);
> -	exit_rwsem(&dsos->lock);
> -}
> -
>  void machine__delete_threads(struct machine *machine)
>  {
>  	struct rb_node *nd;

Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing

[Adrian Hunter]

On 16/09/22 20:59, Namhyung Kim wrote:
> When synthesizing MMAP2 with build-id, it'd read the same file repeatedly as
> it has no idea if it's done already.  Maintain a dsos to check that and skip
> the file access if possible.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/synthetic-events.c | 49 +++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 9d4f5dacd154..e6978b2dee8f 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -4,6 +4,7 @@
>  #include "util/data.h"
>  #include "util/debug.h"
>  #include "util/dso.h"
> +#include "util/dsos.h"
>  #include "util/event.h"
>  #include "util/evlist.h"
>  #include "util/machine.h"
> @@ -47,12 +48,25 @@
>  
>  unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
>  
> +static bool synth_init_done;
> +static struct dsos synth_dsos;
> +
>  void perf_event__synthesize_start(void)
>  {
> +	if (synth_init_done)
> +		return;
> +
> +	dsos__init(&synth_dsos);
> +
> +	synth_init_done = true;
>  }
>  
>  void perf_event__synthesize_stop(void)
>  {
> +	if (!synth_init_done)
> +		return;
> +
> +	dsos__exit(&synth_dsos);
>  }
>  
>  int perf_tool__process_synth_event(struct perf_tool *tool,
> @@ -374,26 +388,47 @@ static bool read_proc_maps_line(struct io *io, __u64 *start, __u64 *end,
>  static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
>  					     bool is_kernel)
>  {
> -	struct build_id bid;
> +	struct build_id _bid, *bid = &_bid;
> +	struct dso *dso = NULL;
> +	struct dso_id id;
>  	int rc;
>  
> -	if (is_kernel)
> -		rc = sysfs__read_build_id("/sys/kernel/notes", &bid);
> -	else
> -		rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
> +	if (is_kernel) {
> +		rc = sysfs__read_build_id("/sys/kernel/notes", bid);
> +		goto out;
> +	}
> +
> +	id.maj = event->maj;
> +	id.min = event->min;
> +	id.ino = event->ino;
> +	id.ino_generation = event->ino_generation;
>  

There might be some paths missing perf_event__synthesize_start()
e.g. perf trace
So it would probably be safer to use lazy initialization for
synth_dsos.

Also, callers of perf_record_mmap2__read_build_id() have a struct
machine so I wonder about putting synth_dsos in struct machine ?
Or even just using machine->dsos ?

> +	dso = dsos__findnew_id(&synth_dsos, event->filename, &id);
> +	if (dso && dso->has_build_id) {
> +		bid = &dso->bid;
> +		rc = 0;
> +		goto out;
> +	}
> +
> +	rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
> +
> +out:
>  	if (rc == 0) {
> -		memcpy(event->build_id, bid.data, sizeof(bid.data));
> -		event->build_id_size = (u8) bid.size;
> +		memcpy(event->build_id, bid->data, sizeof(bid->data));
> +		event->build_id_size = (u8) bid->size;
>  		event->header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
>  		event->__reserved_1 = 0;
>  		event->__reserved_2 = 0;
> +
> +		if (dso && !dso->has_build_id)
> +			dso__set_build_id(dso, bid);
>  	} else {
>  		if (event->filename[0] == '/') {
>  			pr_debug2("Failed to read build ID for %s\n",
>  				  event->filename);
>  		}
>  	}
> +	dso__put(dso);
>  }
>  
>  int perf_event__synthesize_mmap_events(struct perf_tool *tool,

Re: [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c

[Namhyung Kim]

Hi Adrian,

On Tue, Sep 20, 2022 at 6:51 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 16/09/22 20:58, Namhyung Kim wrote:
>
> Maybe say why the move.

Sure, I'll add something like this.

"The dsos routines will be used in other places soon.  As they have a
separate file already, let's move them to a proper place."


Thanks for your review!
Namhyung

Re: [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}()

[Namhyung Kim]

On Tue, Sep 20, 2022 at 6:50 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 16/09/22 20:59, Namhyung Kim wrote:
> > These functions are to prepare and cleanup necessary work for synthesizing.
> > It doesn't do anything yet but later patch will add it.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-inject.c        | 3 +++
> >  tools/perf/builtin-record.c        | 3 +++
> >  tools/perf/builtin-stat.c          | 2 ++
> >  tools/perf/builtin-top.c           | 4 ++++
> >  tools/perf/util/auxtrace.c         | 2 ++
> >  tools/perf/util/synthetic-events.c | 8 ++++++++
> >  tools/perf/util/synthetic-events.h | 3 +++
> >  7 files changed, 25 insertions(+)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index e254f18986f7..2e91a887919b 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -2368,9 +2368,12 @@ int cmd_inject(int argc, const char **argv)
> >       if (ret < 0)
> >               goto out_delete;
> >
> > +     perf_event__synthesize_start();
> > +
> >       ret = __cmd_inject(&inject);
> >
> >       guest_session__exit(&inject.guest_session);
> > +     perf_event__synthesize_stop();
>
> AFAICT perf inject synthesizes mmap events only for JIT and that is
> open-coded in jitdump.c. i.e. perf_event__synthesize_start / stop
> not needed

Right, it's not strictly necessary.  While perf inject -b can synthesize
some build-id events but it also doesn't require this even with the
patch 3.

Originally I thought this might be a good place to add something
we want to do for synthesis.  But I only need it for DSO build-IDs
in MMAP2 and we can remove it from other places.

>
> >
> >  out_delete:
> >       strlist__delete(inject.known_build_ids);
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 02e38f50a138..5b7b9ad2a280 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -1966,6 +1966,8 @@ static int record__synthesize(struct record *rec, bool tail)
> >       if (rec->opts.tail_synthesize != tail)
> >               return 0;
> >
> > +     perf_event__synthesize_start();
>
> Perhaps also record__synthesize_workload() ?

Oh, right.  I thought it's handled during exec but it seems
switch-output things need it additionally.  Will add.

>
> > +
> >       if (data->is_pipe) {
> >               err = perf_event__synthesize_for_pipe(tool, session, data,
> >                                                     process_synthesized_event);
> > @@ -2072,6 +2074,7 @@ static int record__synthesize(struct record *rec, bool tail)
> >       }
> >
> >  out:
> > +     perf_event__synthesize_stop();
> >       return err;
> >  }
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index e05fe72c1d87..f6f61e08f4c2 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -962,6 +962,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >               if (err < 0)
> >                       return err;
> >
> > +             perf_event__synthesize_start();
> >               err = perf_event__synthesize_stat_events(&stat_config, NULL, evsel_list,
> >                                                        process_synthesized_event, is_pipe);
> >               if (err < 0)
> > @@ -2641,6 +2642,7 @@ int cmd_stat(int argc, const char **argv)
> >                       perf_session__write_header(perf_stat.session, evsel_list, fd, true);
> >               }
> >
> > +             perf_event__synthesize_stop();
> >               evlist__close(evsel_list);
> >               perf_session__delete(perf_stat.session);
> >       }
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index e89208b4ad4b..1eff894e6b5f 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -1258,6 +1258,8 @@ static int __cmd_top(struct perf_top *top)
> >  #endif
> >       }
> >
> > +     perf_event__synthesize_start();
> > +
> >       ret = perf_event__synthesize_bpf_events(top->session, perf_event__process,
> >                                               &top->session->machines.host,
> >                                               &top->record_opts);
> > @@ -1273,6 +1275,8 @@ static int __cmd_top(struct perf_top *top)
> >                                   top->evlist->core.threads, true, false,
> >                                   top->nr_threads_synthesize);
> >
> > +     perf_event__synthesize_stop();
> > +
> >       if (top->nr_threads_synthesize > 1)
> >               perf_set_singlethreaded();
> >
> > diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> > index b59c278fe9ed..1bfe076c22fb 100644
> > --- a/tools/perf/util/auxtrace.c
> > +++ b/tools/perf/util/auxtrace.c
> > @@ -1328,6 +1328,7 @@ int perf_event__process_auxtrace_info(struct perf_session *session,
> >       if (err)
> >               return err;
> >
> > +     perf_event__synthesize_start();
> >       unleader_auxtrace(session);
> >
> >       return 0;
> > @@ -2834,6 +2835,7 @@ void auxtrace__free(struct perf_session *session)
> >       if (!session->auxtrace)
> >               return;
> >
> > +     perf_event__synthesize_stop();
>
> auxtrace does not synthesize mmap events

Yeah, I don't have a strong need for it.  I just added it in case
we need something for synthesis later.  But I will remove it
if you don't think it's worth it.

Thanks,
Namhyung

>
> >       return session->auxtrace->free(session);
> >  }
> >
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 0ff57ca24577..9d4f5dacd154 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -47,6 +47,14 @@
> >
> >  unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
> >
> > +void perf_event__synthesize_start(void)
> > +{
> > +}
> > +
> > +void perf_event__synthesize_stop(void)
> > +{
> > +}
> > +
> >  int perf_tool__process_synth_event(struct perf_tool *tool,
> >                                  union perf_event *event,
> >                                  struct machine *machine,
> > diff --git a/tools/perf/util/synthetic-events.h b/tools/perf/util/synthetic-events.h
> > index 53737d1619a4..e4414616080c 100644
> > --- a/tools/perf/util/synthetic-events.h
> > +++ b/tools/perf/util/synthetic-events.h
> > @@ -43,6 +43,9 @@ int parse_synth_opt(char *str);
> >  typedef int (*perf_event__handler_t)(struct perf_tool *tool, union perf_event *event,
> >                                    struct perf_sample *sample, struct machine *machine);
> >
> > +void perf_event__synthesize_start(void);
> > +void perf_event__synthesize_stop(void);
> > +
> >  int perf_event__synthesize_attrs(struct perf_tool *tool, struct evlist *evlist, perf_event__handler_t process);
> >  int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *attr, u32 ids, u64 *id, perf_event__handler_t process);
> >  int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16 misc, perf_event__handler_t process, struct machine *machine);
>

Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing

[Namhyung Kim]

On Tue, Sep 20, 2022 at 7:00 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 16/09/22 20:59, Namhyung Kim wrote:
> > When synthesizing MMAP2 with build-id, it'd read the same file repeatedly as
> > it has no idea if it's done already.  Maintain a dsos to check that and skip
> > the file access if possible.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/synthetic-events.c | 49 +++++++++++++++++++++++++-----
> >  1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 9d4f5dacd154..e6978b2dee8f 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -4,6 +4,7 @@
> >  #include "util/data.h"
> >  #include "util/debug.h"
> >  #include "util/dso.h"
> > +#include "util/dsos.h"
> >  #include "util/event.h"
> >  #include "util/evlist.h"
> >  #include "util/machine.h"
> > @@ -47,12 +48,25 @@
> >
> >  unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
> >
> > +static bool synth_init_done;
> > +static struct dsos synth_dsos;
> > +
> >  void perf_event__synthesize_start(void)
> >  {
> > +     if (synth_init_done)
> > +             return;
> > +
> > +     dsos__init(&synth_dsos);
> > +
> > +     synth_init_done = true;
> >  }
> >
> >  void perf_event__synthesize_stop(void)
> >  {
> > +     if (!synth_init_done)
> > +             return;
> > +
> > +     dsos__exit(&synth_dsos);
> >  }
> >
> >  int perf_tool__process_synth_event(struct perf_tool *tool,
> > @@ -374,26 +388,47 @@ static bool read_proc_maps_line(struct io *io, __u64 *start, __u64 *end,
> >  static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
> >                                            bool is_kernel)
> >  {
> > -     struct build_id bid;
> > +     struct build_id _bid, *bid = &_bid;
> > +     struct dso *dso = NULL;
> > +     struct dso_id id;
> >       int rc;
> >
> > -     if (is_kernel)
> > -             rc = sysfs__read_build_id("/sys/kernel/notes", &bid);
> > -     else
> > -             rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
> > +     if (is_kernel) {
> > +             rc = sysfs__read_build_id("/sys/kernel/notes", bid);
> > +             goto out;
> > +     }
> > +
> > +     id.maj = event->maj;
> > +     id.min = event->min;
> > +     id.ino = event->ino;
> > +     id.ino_generation = event->ino_generation;
> >
>
> There might be some paths missing perf_event__synthesize_start()
> e.g. perf trace
> So it would probably be safer to use lazy initialization for
> synth_dsos.

I thought about that too, but it'd need a cleanup routine at the end
anyway.  So I ended up having a pair of start/stop routines.

>
> Also, callers of perf_record_mmap2__read_build_id() have a struct
> machine so I wonder about putting synth_dsos in struct machine ?
> Or even just using machine->dsos ?

My intention was to use minimal info from struct dso - name, id and
build-id.  But as it already uses dsos/dso routines, it's not much
different from using the existing machine->dsos.

So yeah, I think it's good to reuse the existing one as it'd benefit
the build-id processing at the end.  Will change.

Thanks,
Namhyung


>
> > +     dso = dsos__findnew_id(&synth_dsos, event->filename, &id);
> > +     if (dso && dso->has_build_id) {
> > +             bid = &dso->bid;
> > +             rc = 0;
> > +             goto out;
> > +     }
> > +
> > +     rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
> > +
> > +out:
> >       if (rc == 0) {
> > -             memcpy(event->build_id, bid.data, sizeof(bid.data));
> > -             event->build_id_size = (u8) bid.size;
> > +             memcpy(event->build_id, bid->data, sizeof(bid->data));
> > +             event->build_id_size = (u8) bid->size;
> >               event->header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
> >               event->__reserved_1 = 0;
> >               event->__reserved_2 = 0;
> > +
> > +             if (dso && !dso->has_build_id)
> > +                     dso__set_build_id(dso, bid);
> >       } else {
> >               if (event->filename[0] == '/') {
> >                       pr_debug2("Failed to read build ID for %s\n",
> >                                 event->filename);
> >               }
> >       }
> > +     dso__put(dso);
> >  }
> >
> >  int perf_event__synthesize_mmap_events(struct perf_tool *tool,
>

Re: [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id

[Namhyung Kim]

On Tue, Sep 20, 2022 at 6:36 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 16/09/22 20:59, Namhyung Kim wrote:
> > It needs to go into a namespace before reading a file.
>
> This looks like a fix, in which case make it the first patch
> and add a fixes tag?

Good point, will do.

Thanks,
Namhyung


>
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/synthetic-events.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index e6978b2dee8f..d0d540d09196 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -391,6 +391,8 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
> >       struct build_id _bid, *bid = &_bid;
> >       struct dso *dso = NULL;
> >       struct dso_id id;
> > +     struct nsinfo *nsi;
> > +     struct nscookie nc;
> >       int rc;
> >
> >       if (is_kernel) {
> > @@ -410,8 +412,14 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
> >               goto out;
> >       }
> >
> > +     nsi = nsinfo__new(event->pid);
> > +     nsinfo__mountns_enter(nsi, &nc);
> > +
> >       rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
> >
> > +     nsinfo__mountns_exit(&nc);
> > +     nsinfo__put(nsi);
> > +
> >  out:
> >       if (rc == 0) {
> >               memcpy(event->build_id, bid->data, sizeof(bid->data));
>

Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing

[Adrian Hunter]

On 20/09/22 21:30, Namhyung Kim wrote:
> On Tue, Sep 20, 2022 at 7:00 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 16/09/22 20:59, Namhyung Kim wrote:
>>> When synthesizing MMAP2 with build-id, it'd read the same file repeatedly as
>>> it has no idea if it's done already.  Maintain a dsos to check that and skip
>>> the file access if possible.
>>>
>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>> ---
>>>  tools/perf/util/synthetic-events.c | 49 +++++++++++++++++++++++++-----
>>>  1 file changed, 42 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
>>> index 9d4f5dacd154..e6978b2dee8f 100644
>>> --- a/tools/perf/util/synthetic-events.c
>>> +++ b/tools/perf/util/synthetic-events.c
>>> @@ -4,6 +4,7 @@
>>>  #include "util/data.h"
>>>  #include "util/debug.h"
>>>  #include "util/dso.h"
>>> +#include "util/dsos.h"
>>>  #include "util/event.h"
>>>  #include "util/evlist.h"
>>>  #include "util/machine.h"
>>> @@ -47,12 +48,25 @@
>>>
>>>  unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
>>>
>>> +static bool synth_init_done;
>>> +static struct dsos synth_dsos;
>>> +
>>>  void perf_event__synthesize_start(void)
>>>  {
>>> +     if (synth_init_done)
>>> +             return;
>>> +
>>> +     dsos__init(&synth_dsos);
>>> +
>>> +     synth_init_done = true;
>>>  }
>>>
>>>  void perf_event__synthesize_stop(void)
>>>  {
>>> +     if (!synth_init_done)
>>> +             return;
>>> +
>>> +     dsos__exit(&synth_dsos);
>>>  }
>>>
>>>  int perf_tool__process_synth_event(struct perf_tool *tool,
>>> @@ -374,26 +388,47 @@ static bool read_proc_maps_line(struct io *io, __u64 *start, __u64 *end,
>>>  static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
>>>                                            bool is_kernel)
>>>  {
>>> -     struct build_id bid;
>>> +     struct build_id _bid, *bid = &_bid;
>>> +     struct dso *dso = NULL;
>>> +     struct dso_id id;
>>>       int rc;
>>>
>>> -     if (is_kernel)
>>> -             rc = sysfs__read_build_id("/sys/kernel/notes", &bid);
>>> -     else
>>> -             rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
>>> +     if (is_kernel) {
>>> +             rc = sysfs__read_build_id("/sys/kernel/notes", bid);
>>> +             goto out;
>>> +     }
>>> +
>>> +     id.maj = event->maj;
>>> +     id.min = event->min;
>>> +     id.ino = event->ino;
>>> +     id.ino_generation = event->ino_generation;
>>>
>>
>> There might be some paths missing perf_event__synthesize_start()
>> e.g. perf trace
>> So it would probably be safer to use lazy initialization for
>> synth_dsos.
> 
> I thought about that too, but it'd need a cleanup routine at the end
> anyway.  So I ended up having a pair of start/stop routines.
> 
>>
>> Also, callers of perf_record_mmap2__read_build_id() have a struct
>> machine so I wonder about putting synth_dsos in struct machine ?
>> Or even just using machine->dsos ?
> 
> My intention was to use minimal info from struct dso - name, id and
> build-id.  But as it already uses dsos/dso routines, it's not much
> different from using the existing machine->dsos.
> 
> So yeah, I think it's good to reuse the existing one as it'd benefit
> the build-id processing at the end.  Will change.
> 
> Thanks,
> Namhyung
> 
> 
>>
>>> +     dso = dsos__findnew_id(&synth_dsos, event->filename, &id);
>>> +     if (dso && dso->has_build_id) {
>>> +             bid = &dso->bid;
>>> +             rc = 0;
>>> +             goto out;
>>> +     }

Also I guess the dsos optimization could be a separate patch ?

>>> +
>>> +     rc = filename__read_build_id(event->filename, bid) > 0 ? 0 : -1;
>>> +
>>> +out:
>>>       if (rc == 0) {
>>> -             memcpy(event->build_id, bid.data, sizeof(bid.data));
>>> -             event->build_id_size = (u8) bid.size;
>>> +             memcpy(event->build_id, bid->data, sizeof(bid->data));
>>> +             event->build_id_size = (u8) bid->size;
>>>               event->header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
>>>               event->__reserved_1 = 0;
>>>               event->__reserved_2 = 0;
>>> +
>>> +             if (dso && !dso->has_build_id)
>>> +                     dso__set_build_id(dso, bid);
>>>       } else {
>>>               if (event->filename[0] == '/') {
>>>                       pr_debug2("Failed to read build ID for %s\n",
>>>                                 event->filename);
>>>               }
>>>       }
>>> +     dso__put(dso);
>>>  }
>>>
>>>  int perf_event__synthesize_mmap_events(struct perf_tool *tool,
>>

Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing

[Adrian Hunter]

On 21/09/22 10:26, Adrian Hunter wrote:
> On 20/09/22 21:30, Namhyung Kim wrote:
>> On Tue, Sep 20, 2022 at 7:00 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 16/09/22 20:59, Namhyung Kim wrote:
>>>> When synthesizing MMAP2 with build-id, it'd read the same file repeatedly as
>>>> it has no idea if it's done already.  Maintain a dsos to check that and skip
>>>> the file access if possible.
>>>>
>>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>>> ---
>>>>  tools/perf/util/synthetic-events.c | 49 +++++++++++++++++++++++++-----
>>>>  1 file changed, 42 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
>>>> index 9d4f5dacd154..e6978b2dee8f 100644
>>>> --- a/tools/perf/util/synthetic-events.c
>>>> +++ b/tools/perf/util/synthetic-events.c
>>>> @@ -4,6 +4,7 @@
>>>>  #include "util/data.h"
>>>>  #include "util/debug.h"
>>>>  #include "util/dso.h"
>>>> +#include "util/dsos.h"
>>>>  #include "util/event.h"
>>>>  #include "util/evlist.h"
>>>>  #include "util/machine.h"
>>>> @@ -47,12 +48,25 @@
>>>>
>>>>  unsigned int proc_map_timeout = DEFAULT_PROC_MAP_PARSE_TIMEOUT;
>>>>
>>>> +static bool synth_init_done;
>>>> +static struct dsos synth_dsos;
>>>> +
>>>>  void perf_event__synthesize_start(void)
>>>>  {
>>>> +     if (synth_init_done)
>>>> +             return;
>>>> +
>>>> +     dsos__init(&synth_dsos);
>>>> +
>>>> +     synth_init_done = true;
>>>>  }
>>>>
>>>>  void perf_event__synthesize_stop(void)
>>>>  {
>>>> +     if (!synth_init_done)
>>>> +             return;
>>>> +
>>>> +     dsos__exit(&synth_dsos);
>>>>  }
>>>>
>>>>  int perf_tool__process_synth_event(struct perf_tool *tool,
>>>> @@ -374,26 +388,47 @@ static bool read_proc_maps_line(struct io *io, __u64 *start, __u64 *end,
>>>>  static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
>>>>                                            bool is_kernel)
>>>>  {
>>>> -     struct build_id bid;
>>>> +     struct build_id _bid, *bid = &_bid;
>>>> +     struct dso *dso = NULL;
>>>> +     struct dso_id id;
>>>>       int rc;
>>>>
>>>> -     if (is_kernel)
>>>> -             rc = sysfs__read_build_id("/sys/kernel/notes", &bid);
>>>> -     else
>>>> -             rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
>>>> +     if (is_kernel) {
>>>> +             rc = sysfs__read_build_id("/sys/kernel/notes", bid);
>>>> +             goto out;
>>>> +     }
>>>> +
>>>> +     id.maj = event->maj;
>>>> +     id.min = event->min;
>>>> +     id.ino = event->ino;
>>>> +     id.ino_generation = event->ino_generation;
>>>>
>>>
>>> There might be some paths missing perf_event__synthesize_start()
>>> e.g. perf trace
>>> So it would probably be safer to use lazy initialization for
>>> synth_dsos.
>>
>> I thought about that too, but it'd need a cleanup routine at the end
>> anyway.  So I ended up having a pair of start/stop routines.
>>
>>>
>>> Also, callers of perf_record_mmap2__read_build_id() have a struct
>>> machine so I wonder about putting synth_dsos in struct machine ?
>>> Or even just using machine->dsos ?
>>
>> My intention was to use minimal info from struct dso - name, id and
>> build-id.  But as it already uses dsos/dso routines, it's not much
>> different from using the existing machine->dsos.
>>
>> So yeah, I think it's good to reuse the existing one as it'd benefit
>> the build-id processing at the end.  Will change.
>>
>> Thanks,
>> Namhyung
>>
>>
>>>
>>>> +     dso = dsos__findnew_id(&synth_dsos, event->filename, &id);
>>>> +     if (dso && dso->has_build_id) {
>>>> +             bid = &dso->bid;
>>>> +             rc = 0;
>>>> +             goto out;
>>>> +     }
> 
> Also I guess the dsos optimization could be a separate patch ?

Disregard that - I thought there were other changes too.