* [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c
2022-09-16 17:58 [PATCH 0/4] perf tools: Fix MMAP2 build-ID synthesis in namespaces (v1) Namhyung Kim
@ 2022-09-16 17:58 ` Namhyung Kim
2022-09-20 13:51 ` Adrian Hunter
2022-09-16 17:59 ` [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}() Namhyung Kim
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2022-09-16 17:58 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
linux-perf-users
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c
2022-09-16 17:58 ` [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c Namhyung Kim
@ 2022-09-20 13:51 ` Adrian Hunter
2022-09-20 17:49 ` Namhyung Kim
0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2022-09-20 13:51 UTC (permalink / raw)
To: Namhyung Kim, Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, linux-perf-users
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;
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c
2022-09-20 13:51 ` Adrian Hunter
@ 2022-09-20 17:49 ` Namhyung Kim
0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2022-09-20 17:49 UTC (permalink / raw)
To: Adrian Hunter
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Peter Zijlstra,
LKML, Ian Rogers, linux-perf-users
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}()
2022-09-16 17:58 [PATCH 0/4] perf tools: Fix MMAP2 build-ID synthesis in namespaces (v1) Namhyung Kim
2022-09-16 17:58 ` [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c Namhyung Kim
@ 2022-09-16 17:59 ` Namhyung Kim
2022-09-20 13:50 ` Adrian Hunter
2022-09-16 17:59 ` [PATCH 3/4] perf record: Save DSO build-ID for synthesizing Namhyung Kim
2022-09-16 17:59 ` [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id Namhyung Kim
3 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2022-09-16 17:59 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
linux-perf-users
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}()
2022-09-16 17:59 ` [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}() Namhyung Kim
@ 2022-09-20 13:50 ` Adrian Hunter
2022-09-20 18:17 ` Namhyung Kim
0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2022-09-20 13:50 UTC (permalink / raw)
To: Namhyung Kim, Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, linux-perf-users
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);
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}()
2022-09-20 13:50 ` Adrian Hunter
@ 2022-09-20 18:17 ` Namhyung Kim
0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2022-09-20 18:17 UTC (permalink / raw)
To: Adrian Hunter
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Peter Zijlstra,
LKML, Ian Rogers, linux-perf-users
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);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] perf record: Save DSO build-ID for synthesizing
2022-09-16 17:58 [PATCH 0/4] perf tools: Fix MMAP2 build-ID synthesis in namespaces (v1) Namhyung Kim
2022-09-16 17:58 ` [PATCH 1/4] perf tools: Move dsos functions to util/dsos.c Namhyung Kim
2022-09-16 17:59 ` [PATCH 2/4] perf tools: Add perf_event__synthesize_{start,stop}() Namhyung Kim
@ 2022-09-16 17:59 ` Namhyung Kim
2022-09-20 13:59 ` Adrian Hunter
2022-09-16 17:59 ` [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id Namhyung Kim
3 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2022-09-16 17:59 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
linux-perf-users
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing
2022-09-16 17:59 ` [PATCH 3/4] perf record: Save DSO build-ID for synthesizing Namhyung Kim
@ 2022-09-20 13:59 ` Adrian Hunter
2022-09-20 18:30 ` Namhyung Kim
0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2022-09-20 13:59 UTC (permalink / raw)
To: Namhyung Kim, Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, linux-perf-users
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,
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing
2022-09-20 13:59 ` Adrian Hunter
@ 2022-09-20 18:30 ` Namhyung Kim
2022-09-21 7:26 ` Adrian Hunter
0 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2022-09-20 18:30 UTC (permalink / raw)
To: Adrian Hunter
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Peter Zijlstra,
LKML, Ian Rogers, linux-perf-users
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,
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing
2022-09-20 18:30 ` Namhyung Kim
@ 2022-09-21 7:26 ` Adrian Hunter
2022-09-21 10:11 ` Adrian Hunter
0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2022-09-21 7:26 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Peter Zijlstra,
LKML, Ian Rogers, linux-perf-users
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,
>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] perf record: Save DSO build-ID for synthesizing
2022-09-21 7:26 ` Adrian Hunter
@ 2022-09-21 10:11 ` Adrian Hunter
0 siblings, 0 replies; 15+ messages in thread
From: Adrian Hunter @ 2022-09-21 10:11 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Peter Zijlstra,
LKML, Ian Rogers, linux-perf-users
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id
2022-09-16 17:58 [PATCH 0/4] perf tools: Fix MMAP2 build-ID synthesis in namespaces (v1) Namhyung Kim
` (2 preceding siblings ...)
2022-09-16 17:59 ` [PATCH 3/4] perf record: Save DSO build-ID for synthesizing Namhyung Kim
@ 2022-09-16 17:59 ` Namhyung Kim
2022-09-20 13:36 ` Adrian Hunter
3 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2022-09-16 17:59 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
linux-perf-users
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id
2022-09-16 17:59 ` [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id Namhyung Kim
@ 2022-09-20 13:36 ` Adrian Hunter
2022-09-20 18:31 ` Namhyung Kim
0 siblings, 1 reply; 15+ messages in thread
From: Adrian Hunter @ 2022-09-20 13:36 UTC (permalink / raw)
To: Namhyung Kim, Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, linux-perf-users
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));
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] perf tools: Honor namespace when synthesizing build-id
2022-09-20 13:36 ` Adrian Hunter
@ 2022-09-20 18:31 ` Namhyung Kim
0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2022-09-20 18:31 UTC (permalink / raw)
To: Adrian Hunter
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ingo Molnar, Peter Zijlstra,
LKML, Ian Rogers, linux-perf-users
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));
>
^ permalink raw reply [flat|nested] 15+ messages in thread