[PATCH 1/3] perf inject: Add missing callbacks in perf_tool

[PATCH 1/3] perf inject: Add missing callbacks in perf_tool

[Namhyung Kim]

I found some events (like PERF_RECORD_CGROUP) are not copied by perf
inject due to the missing callbacks.  Let's add them.

While at it, I've changed the order of the callbacks to match with
struct perf_tool so that we can compare them easily.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-inject.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6d2f410d773a..59576877c67f 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -97,6 +97,13 @@ static int perf_event__repipe_op2_synth(struct perf_session *session,
 	return perf_event__repipe_synth(session->tool, event);
 }
 
+static int perf_event__repipe_op4_synth(struct perf_session *session,
+					union perf_event *event,
+					u64 data __maybe_unused)
+{
+	return perf_event__repipe_synth(session->tool, event);
+}
+
 static int perf_event__repipe_attr(struct perf_tool *tool,
 				   union perf_event *event,
 				   struct evlist **pevlist)
@@ -115,6 +122,13 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
 	return perf_event__repipe_synth(tool, event);
 }
 
+static int perf_event__repipe_event_update(struct perf_tool *tool,
+					   union perf_event *event,
+					   struct evlist **pevlist __maybe_unused)
+{
+	return perf_event__repipe_synth(tool, event);
+}
+
 #ifdef HAVE_AUXTRACE_SUPPORT
 
 static int copy_bytes(struct perf_inject *inject, int fd, off_t size)
@@ -708,9 +722,12 @@ int cmd_inject(int argc, const char **argv)
 	struct perf_inject inject = {
 		.tool = {
 			.sample		= perf_event__repipe_sample,
+			.read		= perf_event__repipe_sample,
 			.mmap		= perf_event__repipe,
 			.mmap2		= perf_event__repipe,
 			.comm		= perf_event__repipe,
+			.namespaces	= perf_event__repipe,
+			.cgroup		= perf_event__repipe,
 			.fork		= perf_event__repipe,
 			.exit		= perf_event__repipe,
 			.lost		= perf_event__repipe,
@@ -718,19 +735,28 @@ int cmd_inject(int argc, const char **argv)
 			.aux		= perf_event__repipe,
 			.itrace_start	= perf_event__repipe,
 			.context_switch	= perf_event__repipe,
-			.read		= perf_event__repipe_sample,
 			.throttle	= perf_event__repipe,
 			.unthrottle	= perf_event__repipe,
+			.ksymbol	= perf_event__repipe,
+			.bpf		= perf_event__repipe,
+			.text_poke	= perf_event__repipe,
 			.attr		= perf_event__repipe_attr,
+			.event_update	= perf_event__repipe_event_update,
 			.tracing_data	= perf_event__repipe_op2_synth,
-			.auxtrace_info	= perf_event__repipe_op2_synth,
-			.auxtrace	= perf_event__repipe_auxtrace,
-			.auxtrace_error	= perf_event__repipe_op2_synth,
-			.time_conv	= perf_event__repipe_op2_synth,
 			.finished_round	= perf_event__repipe_oe_synth,
 			.build_id	= perf_event__repipe_op2_synth,
 			.id_index	= perf_event__repipe_op2_synth,
+			.auxtrace_info	= perf_event__repipe_op2_synth,
+			.auxtrace_error	= perf_event__repipe_op2_synth,
+			.time_conv	= perf_event__repipe_op2_synth,
+			.thread_map	= perf_event__repipe_op2_synth,
+			.cpu_map	= perf_event__repipe_op2_synth,
+			.stat_config	= perf_event__repipe_op2_synth,
+			.stat		= perf_event__repipe_op2_synth,
+			.stat_round	= perf_event__repipe_op2_synth,
 			.feature	= perf_event__repipe_op2_synth,
+			.compressed	= perf_event__repipe_op4_synth,
+			.auxtrace	= perf_event__repipe_auxtrace,
 		},
 		.input_name  = "-",
 		.samples = LIST_HEAD_INIT(inject.samples),
-- 
2.28.0.618.gf4bc123cb7-goog

[PATCH 2/3] perf inject: Enter namespace when reading build-id

[Namhyung Kim]

It should be in a proper mnt namespace when accessing the file.

I think this had no problem since the build-id was actually read from
map__load() -> dso__load() already.  But I'd like to change it in the
following commit.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-inject.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 59576877c67f..a2804d906d2a 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -23,6 +23,7 @@
 #include "util/symbol.h"
 #include "util/synthetic-events.h"
 #include "util/thread.h"
+#include "util/namespaces.h"
 #include <linux/err.h>
 
 #include <subcmd/parse-options.h>
@@ -419,16 +420,19 @@ static int perf_event__repipe_tracing_data(struct perf_session *session,
 
 static int dso__read_build_id(struct dso *dso)
 {
+	struct nscookie nsc;
+
 	if (dso->has_build_id)
 		return 0;
 
+	nsinfo__mountns_enter(dso->nsinfo, &nsc);
 	if (filename__read_build_id(dso->long_name, dso->build_id,
 				    sizeof(dso->build_id)) > 0) {
 		dso->has_build_id = true;
-		return 0;
 	}
+	nsinfo__mountns_exit(&nsc);
 
-	return -1;
+	return dso->has_build_id ? 0 : -1;
 }
 
 static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
-- 
2.28.0.618.gf4bc123cb7-goog

[PATCH 3/3] perf inject: Do not load map/dso when injecting build-id

[Namhyung Kim]

No need to load symbols in a DSO when injecting build-id.  I guess the
reason was to check the DSO is a special file like anon files.  Use
some helper functions in map.c to check them before reading build-id.
Also pass sample event's cpumode to a new build-id event.

Original-patch-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-inject.c | 30 ++++++++++--------------------
 tools/perf/util/map.c       | 17 +----------------
 tools/perf/util/map.h       | 14 ++++++++++++++
 3 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index a2804d906d2a..6d4e6833efed 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -436,21 +436,22 @@ static int dso__read_build_id(struct dso *dso)
 }
 
 static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
-				struct machine *machine)
+				struct machine *machine, u8 cpumode)
 {
-	u16 misc = PERF_RECORD_MISC_USER;
 	int err;
 
+	if (is_anon_memory(dso->long_name))
+		return 0;
+	if (is_no_dso_memory(dso->long_name))
+		return 0;
+
 	if (dso__read_build_id(dso) < 0) {
 		pr_debug("no build_id found for %s\n", dso->long_name);
 		return -1;
 	}
 
-	if (dso->kernel)
-		misc = PERF_RECORD_MISC_KERNEL;
-
-	err = perf_event__synthesize_build_id(tool, dso, misc, perf_event__repipe,
-					      machine);
+	err = perf_event__synthesize_build_id(tool, dso, cpumode,
+					      perf_event__repipe, machine);
 	if (err) {
 		pr_err("Can't synthesize build_id event for %s\n", dso->long_name);
 		return -1;
@@ -478,19 +479,8 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
 	if (thread__find_map(thread, sample->cpumode, sample->ip, &al)) {
 		if (!al.map->dso->hit) {
 			al.map->dso->hit = 1;
-			if (map__load(al.map) >= 0) {
-				dso__inject_build_id(al.map->dso, tool, machine);
-				/*
-				 * If this fails, too bad, let the other side
-				 * account this as unresolved.
-				 */
-			} else {
-#ifdef HAVE_LIBELF_SUPPORT
-				pr_warning("no symbols found in %s, maybe "
-					   "install a debug package?\n",
-					   al.map->dso->long_name);
-#endif
-			}
+			dso__inject_build_id(al.map->dso, tool, machine,
+					     sample->cpumode);
 		}
 	}
 
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index cc0faf8f1321..8b305e624124 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -27,21 +27,6 @@
 
 static void __maps__insert(struct maps *maps, struct map *map);
 
-static inline int is_anon_memory(const char *filename, u32 flags)
-{
-	return flags & MAP_HUGETLB ||
-	       !strcmp(filename, "//anon") ||
-	       !strncmp(filename, "/dev/zero", sizeof("/dev/zero") - 1) ||
-	       !strncmp(filename, "/anon_hugepage", sizeof("/anon_hugepage") - 1);
-}
-
-static inline int is_no_dso_memory(const char *filename)
-{
-	return !strncmp(filename, "[stack", 6) ||
-	       !strncmp(filename, "/SYSV",5)   ||
-	       !strcmp(filename, "[heap]");
-}
-
 static inline int is_android_lib(const char *filename)
 {
 	return strstarts(filename, "/data/app-lib/") ||
@@ -158,7 +143,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
 		int anon, no_dso, vdso, android;
 
 		android = is_android_lib(filename);
-		anon = is_anon_memory(filename, flags);
+		anon = is_anon_memory(filename) || flags & MAP_HUGETLB;
 		vdso = is_vdso_map(filename);
 		no_dso = is_no_dso_memory(filename);
 		map->prot = prot;
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index c2f5d28fe73a..b1c0686db1b7 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -171,4 +171,18 @@ static inline bool is_bpf_image(const char *name)
 	return strncmp(name, "bpf_trampoline_", sizeof("bpf_trampoline_") - 1) == 0 ||
 	       strncmp(name, "bpf_dispatcher_", sizeof("bpf_dispatcher_") - 1) == 0;
 }
+
+static inline int is_anon_memory(const char *filename)
+{
+	return !strcmp(filename, "//anon") ||
+	       !strncmp(filename, "/dev/zero", sizeof("/dev/zero") - 1) ||
+	       !strncmp(filename, "/anon_hugepage", sizeof("/anon_hugepage") - 1);
+}
+
+static inline int is_no_dso_memory(const char *filename)
+{
+	return !strncmp(filename, "[stack", 6) ||
+	       !strncmp(filename, "/SYSV", 5)  ||
+	       !strcmp(filename, "[heap]");
+}
 #endif /* __PERF_MAP_H */
-- 
2.28.0.618.gf4bc123cb7-goog

Re: [PATCH 3/3] perf inject: Do not load map/dso when injecting build-id

[Jiri Olsa]

On Mon, Sep 14, 2020 at 11:18:59PM +0900, Namhyung Kim wrote:
> No need to load symbols in a DSO when injecting build-id.  I guess the
> reason was to check the DSO is a special file like anon files.  Use
> some helper functions in map.c to check them before reading build-id.
> Also pass sample event's cpumode to a new build-id event.
> 
> Original-patch-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-inject.c | 30 ++++++++++--------------------
>  tools/perf/util/map.c       | 17 +----------------
>  tools/perf/util/map.h       | 14 ++++++++++++++
>  3 files changed, 25 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index a2804d906d2a..6d4e6833efed 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -436,21 +436,22 @@ static int dso__read_build_id(struct dso *dso)
>  }
>  
>  static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
> -				struct machine *machine)
> +				struct machine *machine, u8 cpumode)
>  {
> -	u16 misc = PERF_RECORD_MISC_USER;
>  	int err;
>  
> +	if (is_anon_memory(dso->long_name))
> +		return 0;
> +	if (is_no_dso_memory(dso->long_name))
> +		return 0;

should we check for vdso as well? I don't think it has build id

> +
>  	if (dso__read_build_id(dso) < 0) {
>  		pr_debug("no build_id found for %s\n", dso->long_name);
>  		return -1;
>  	}
>  
> -	if (dso->kernel)
> -		misc = PERF_RECORD_MISC_KERNEL;
> -
> -	err = perf_event__synthesize_build_id(tool, dso, misc, perf_event__repipe,
> -					      machine);
> +	err = perf_event__synthesize_build_id(tool, dso, cpumode,
> +					      perf_event__repipe, machine);
>  	if (err) {
>  		pr_err("Can't synthesize build_id event for %s\n", dso->long_name);
>  		return -1;
> @@ -478,19 +479,8 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
>  	if (thread__find_map(thread, sample->cpumode, sample->ip, &al)) {
>  		if (!al.map->dso->hit) {
>  			al.map->dso->hit = 1;
> -			if (map__load(al.map) >= 0) {

nice, that might do some nice speedup, did you see any?

jirka

> -				dso__inject_build_id(al.map->dso, tool, machine);
> -				/*
> -				 * If this fails, too bad, let the other side
> -				 * account this as unresolved.
> -				 */
> -			} else {
> -#ifdef HAVE_LIBELF_SUPPORT
> -				pr_warning("no symbols found in %s, maybe "
> -					   "install a debug package?\n",
> -					   al.map->dso->long_name);
> -#endif
> -			}
> +			dso__inject_build_id(al.map->dso, tool, machine,
> +					     sample->cpumode);
>  		}

SNIP

Re: [PATCH 3/3] perf inject: Do not load map/dso when injecting build-id

[Namhyung Kim]

On Tue, Sep 15, 2020 at 7:05 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Sep 14, 2020 at 11:18:59PM +0900, Namhyung Kim wrote:
> > No need to load symbols in a DSO when injecting build-id.  I guess the
> > reason was to check the DSO is a special file like anon files.  Use
> > some helper functions in map.c to check them before reading build-id.
> > Also pass sample event's cpumode to a new build-id event.
> >
> > Original-patch-by: Stephane Eranian <eranian@google.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-inject.c | 30 ++++++++++--------------------
> >  tools/perf/util/map.c       | 17 +----------------
> >  tools/perf/util/map.h       | 14 ++++++++++++++
> >  3 files changed, 25 insertions(+), 36 deletions(-)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index a2804d906d2a..6d4e6833efed 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -436,21 +436,22 @@ static int dso__read_build_id(struct dso *dso)
> >  }
> >
> >  static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
> > -                             struct machine *machine)
> > +                             struct machine *machine, u8 cpumode)
> >  {
> > -     u16 misc = PERF_RECORD_MISC_USER;
> >       int err;
> >
> > +     if (is_anon_memory(dso->long_name))
> > +             return 0;
> > +     if (is_no_dso_memory(dso->long_name))
> > +             return 0;
>
> should we check for vdso as well? I don't think it has build id

I don't know.. But I guess there's no reason it shouldn't?


>
> > +
> >       if (dso__read_build_id(dso) < 0) {
> >               pr_debug("no build_id found for %s\n", dso->long_name);
> >               return -1;
> >       }
> >
> > -     if (dso->kernel)
> > -             misc = PERF_RECORD_MISC_KERNEL;
> > -
> > -     err = perf_event__synthesize_build_id(tool, dso, misc, perf_event__repipe,
> > -                                           machine);
> > +     err = perf_event__synthesize_build_id(tool, dso, cpumode,
> > +                                           perf_event__repipe, machine);
> >       if (err) {
> >               pr_err("Can't synthesize build_id event for %s\n", dso->long_name);
> >               return -1;
> > @@ -478,19 +479,8 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
> >       if (thread__find_map(thread, sample->cpumode, sample->ip, &al)) {
> >               if (!al.map->dso->hit) {
> >                       al.map->dso->hit = 1;
> > -                     if (map__load(al.map) >= 0) {
>
> nice, that might do some nice speedup, did you see any?

Yes, I believe so.  But my quick test on a laptop was too short.
I will test it again and share the speedup.

Thanks
Namhyung


>
> jirka
>
> > -                             dso__inject_build_id(al.map->dso, tool, machine);
> > -                             /*
> > -                              * If this fails, too bad, let the other side
> > -                              * account this as unresolved.
> > -                              */
> > -                     } else {
> > -#ifdef HAVE_LIBELF_SUPPORT
> > -                             pr_warning("no symbols found in %s, maybe "
> > -                                        "install a debug package?\n",
> > -                                        al.map->dso->long_name);
> > -#endif
> > -                     }
> > +                     dso__inject_build_id(al.map->dso, tool, machine,
> > +                                          sample->cpumode);
> >               }
>
> SNIP
>

Re: [PATCH 3/3] perf inject: Do not load map/dso when injecting build-id

[Jiri Olsa]

On Tue, Sep 15, 2020 at 11:55:34PM +0900, Namhyung Kim wrote:
> On Tue, Sep 15, 2020 at 7:05 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Sep 14, 2020 at 11:18:59PM +0900, Namhyung Kim wrote:
> > > No need to load symbols in a DSO when injecting build-id.  I guess the
> > > reason was to check the DSO is a special file like anon files.  Use
> > > some helper functions in map.c to check them before reading build-id.
> > > Also pass sample event's cpumode to a new build-id event.
> > >
> > > Original-patch-by: Stephane Eranian <eranian@google.com>
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/builtin-inject.c | 30 ++++++++++--------------------
> > >  tools/perf/util/map.c       | 17 +----------------
> > >  tools/perf/util/map.h       | 14 ++++++++++++++
> > >  3 files changed, 25 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > > index a2804d906d2a..6d4e6833efed 100644
> > > --- a/tools/perf/builtin-inject.c
> > > +++ b/tools/perf/builtin-inject.c
> > > @@ -436,21 +436,22 @@ static int dso__read_build_id(struct dso *dso)
> > >  }
> > >
> > >  static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
> > > -                             struct machine *machine)
> > > +                             struct machine *machine, u8 cpumode)
> > >  {
> > > -     u16 misc = PERF_RECORD_MISC_USER;
> > >       int err;
> > >
> > > +     if (is_anon_memory(dso->long_name))
> > > +             return 0;
> > > +     if (is_no_dso_memory(dso->long_name))
> > > +             return 0;
> >
> > should we check for vdso as well? I don't think it has build id
> 
> I don't know.. But I guess there's no reason it shouldn't?

I haven't checked, it's just I always saw only zeros for vdso build ids

jirka
> 
> 
> >
> > > +
> > >       if (dso__read_build_id(dso) < 0) {
> > >               pr_debug("no build_id found for %s\n", dso->long_name);
> > >               return -1;
> > >       }
> > >
> > > -     if (dso->kernel)
> > > -             misc = PERF_RECORD_MISC_KERNEL;
> > > -
> > > -     err = perf_event__synthesize_build_id(tool, dso, misc, perf_event__repipe,
> > > -                                           machine);
> > > +     err = perf_event__synthesize_build_id(tool, dso, cpumode,
> > > +                                           perf_event__repipe, machine);
> > >       if (err) {
> > >               pr_err("Can't synthesize build_id event for %s\n", dso->long_name);
> > >               return -1;
> > > @@ -478,19 +479,8 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
> > >       if (thread__find_map(thread, sample->cpumode, sample->ip, &al)) {
> > >               if (!al.map->dso->hit) {
> > >                       al.map->dso->hit = 1;
> > > -                     if (map__load(al.map) >= 0) {
> >
> > nice, that might do some nice speedup, did you see any?
> 
> Yes, I believe so.  But my quick test on a laptop was too short.
> I will test it again and share the speedup.
> 
> Thanks
> Namhyung
> 
> 
> >
> > jirka
> >
> > > -                             dso__inject_build_id(al.map->dso, tool, machine);
> > > -                             /*
> > > -                              * If this fails, too bad, let the other side
> > > -                              * account this as unresolved.
> > > -                              */
> > > -                     } else {
> > > -#ifdef HAVE_LIBELF_SUPPORT
> > > -                             pr_warning("no symbols found in %s, maybe "
> > > -                                        "install a debug package?\n",
> > > -                                        al.map->dso->long_name);
> > > -#endif
> > > -                     }
> > > +                     dso__inject_build_id(al.map->dso, tool, machine,
> > > +                                          sample->cpumode);
> > >               }
> >
> > SNIP
> >
> 

Re: [PATCH 3/3] perf inject: Do not load map/dso when injecting build-id

[Namhyung Kim]

On Wed, Sep 16, 2020 at 5:24 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Sep 15, 2020 at 11:55:34PM +0900, Namhyung Kim wrote:
> > On Tue, Sep 15, 2020 at 7:05 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, Sep 14, 2020 at 11:18:59PM +0900, Namhyung Kim wrote:
> > > > No need to load symbols in a DSO when injecting build-id.  I guess the
> > > > reason was to check the DSO is a special file like anon files.  Use
> > > > some helper functions in map.c to check them before reading build-id.
> > > > Also pass sample event's cpumode to a new build-id event.
> > > >
> > > > Original-patch-by: Stephane Eranian <eranian@google.com>
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/builtin-inject.c | 30 ++++++++++--------------------
> > > >  tools/perf/util/map.c       | 17 +----------------
> > > >  tools/perf/util/map.h       | 14 ++++++++++++++
> > > >  3 files changed, 25 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > > > index a2804d906d2a..6d4e6833efed 100644
> > > > --- a/tools/perf/builtin-inject.c
> > > > +++ b/tools/perf/builtin-inject.c
> > > > @@ -436,21 +436,22 @@ static int dso__read_build_id(struct dso *dso)
> > > >  }
> > > >
> > > >  static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
> > > > -                             struct machine *machine)
> > > > +                             struct machine *machine, u8 cpumode)
> > > >  {
> > > > -     u16 misc = PERF_RECORD_MISC_USER;
> > > >       int err;
> > > >
> > > > +     if (is_anon_memory(dso->long_name))
> > > > +             return 0;
> > > > +     if (is_no_dso_memory(dso->long_name))
> > > > +             return 0;
> > >
> > > should we check for vdso as well? I don't think it has build id
> >
> > I don't know.. But I guess there's no reason it shouldn't?
>
> I haven't checked, it's just I always saw only zeros for vdso build ids

I found this in arch/x86/entry/vdso/Makefile.  It seems to have one..

quiet_cmd_vdso = VDSO    $@
      cmd_vdso = $(LD) -nostdlib -o $@ \
               $(VDSO_LDFLAGS) $(VDSO_LDFLAGS_$(filter %.lds,$(^F))) \
               -T $(filter %.lds,$^) $(filter %.o,$^) && \
         sh $(srctree)/$(src)/checkundef.sh '$(NM)' '$@'

VDSO_LDFLAGS = -shared --hash-style=both --build-id \
    $(call ld-option, --eh-frame-hdr) -Bsymbolic

Thanks
Namhyung

Re: [PATCH 3/3] perf inject: Do not load map/dso when injecting build-id

[Jiri Olsa]

On Thu, Sep 17, 2020 at 10:27:41AM +0900, Namhyung Kim wrote:
> On Wed, Sep 16, 2020 at 5:24 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Tue, Sep 15, 2020 at 11:55:34PM +0900, Namhyung Kim wrote:
> > > On Tue, Sep 15, 2020 at 7:05 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > >
> > > > On Mon, Sep 14, 2020 at 11:18:59PM +0900, Namhyung Kim wrote:
> > > > > No need to load symbols in a DSO when injecting build-id.  I guess the
> > > > > reason was to check the DSO is a special file like anon files.  Use
> > > > > some helper functions in map.c to check them before reading build-id.
> > > > > Also pass sample event's cpumode to a new build-id event.
> > > > >
> > > > > Original-patch-by: Stephane Eranian <eranian@google.com>
> > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > > ---
> > > > >  tools/perf/builtin-inject.c | 30 ++++++++++--------------------
> > > > >  tools/perf/util/map.c       | 17 +----------------
> > > > >  tools/perf/util/map.h       | 14 ++++++++++++++
> > > > >  3 files changed, 25 insertions(+), 36 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > > > > index a2804d906d2a..6d4e6833efed 100644
> > > > > --- a/tools/perf/builtin-inject.c
> > > > > +++ b/tools/perf/builtin-inject.c
> > > > > @@ -436,21 +436,22 @@ static int dso__read_build_id(struct dso *dso)
> > > > >  }
> > > > >
> > > > >  static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
> > > > > -                             struct machine *machine)
> > > > > +                             struct machine *machine, u8 cpumode)
> > > > >  {
> > > > > -     u16 misc = PERF_RECORD_MISC_USER;
> > > > >       int err;
> > > > >
> > > > > +     if (is_anon_memory(dso->long_name))
> > > > > +             return 0;
> > > > > +     if (is_no_dso_memory(dso->long_name))
> > > > > +             return 0;
> > > >
> > > > should we check for vdso as well? I don't think it has build id
> > >
> > > I don't know.. But I guess there's no reason it shouldn't?
> >
> > I haven't checked, it's just I always saw only zeros for vdso build ids
> 
> I found this in arch/x86/entry/vdso/Makefile.  It seems to have one..
> 
> quiet_cmd_vdso = VDSO    $@
>       cmd_vdso = $(LD) -nostdlib -o $@ \
>                $(VDSO_LDFLAGS) $(VDSO_LDFLAGS_$(filter %.lds,$(^F))) \
>                -T $(filter %.lds,$^) $(filter %.o,$^) && \
>          sh $(srctree)/$(src)/checkundef.sh '$(NM)' '$@'
> 
> VDSO_LDFLAGS = -shared --hash-style=both --build-id \
>     $(call ld-option, --eh-frame-hdr) -Bsymbolic

I see, I'll check why I'm getting zeros

thanks,
jirka

Re: [PATCH 3/3] perf inject: Do not load map/dso when injecting build-id

[Namhyung Kim]

On Tue, Sep 15, 2020 at 7:05 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Sep 14, 2020 at 11:18:59PM +0900, Namhyung Kim wrote:
> > @@ -478,19 +479,8 @@ static int perf_event__inject_buildid(struct perf_tool *tool,
> >       if (thread__find_map(thread, sample->cpumode, sample->ip, &al)) {
> >               if (!al.map->dso->hit) {
> >                       al.map->dso->hit = 1;
> > -                     if (map__load(al.map) >= 0) {
>
> nice, that might do some nice speedup, did you see any?

I've checked it but the speed up was tiny on my machine.
I guess it's because the machine was idle and had everything
in memory.  But I also found some more points to improve.
Will share the result in the next spin.

Thanks
Namhyung