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-googOn 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,
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,
>
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,
>>
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.
© 2016 - 2026 Red Hat, Inc.