| Message ID | 20220711093218.10967-6-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf intel-pt: Add support for tracing virtual machine user space on the host | expand |
On Mon, Jul 11, 2022 at 2:32 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > Factor out evsel__id_hdr_size() so it can be reused. > > This is needed by perf inject. When injecting events from a guest perf.data > file, there is a possibility that the sample ID numbers conflict. To > re-write an ID sample, the old one needs to be removed first, which means > determining how big it is with evsel__id_hdr_size() and then subtracting > that from the event size. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > tools/perf/util/evlist.c | 28 +--------------------------- > tools/perf/util/evsel.c | 26 ++++++++++++++++++++++++++ > tools/perf/util/evsel.h | 2 ++ > 3 files changed, 29 insertions(+), 27 deletions(-) > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > index 48af7d379d82..03fbe151b0c4 100644 > --- a/tools/perf/util/evlist.c > +++ b/tools/perf/util/evlist.c > @@ -1244,34 +1244,8 @@ bool evlist__valid_read_format(struct evlist *evlist) > u16 evlist__id_hdr_size(struct evlist *evlist) > { > struct evsel *first = evlist__first(evlist); > - struct perf_sample *data; > - u64 sample_type; > - u16 size = 0; > > - if (!first->core.attr.sample_id_all) > - goto out; > - > - sample_type = first->core.attr.sample_type; > - > - if (sample_type & PERF_SAMPLE_TID) > - size += sizeof(data->tid) * 2; > - > - if (sample_type & PERF_SAMPLE_TIME) > - size += sizeof(data->time); > - > - if (sample_type & PERF_SAMPLE_ID) > - size += sizeof(data->id); > - > - if (sample_type & PERF_SAMPLE_STREAM_ID) > - size += sizeof(data->stream_id); > - > - if (sample_type & PERF_SAMPLE_CPU) > - size += sizeof(data->cpu) * 2; > - > - if (sample_type & PERF_SAMPLE_IDENTIFIER) > - size += sizeof(data->id); > -out: > - return size; > + return first->core.attr.sample_id_all ? evsel__id_hdr_size(first) : 0; > } > > bool evlist__valid_sample_id_all(struct evlist *evlist) > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index a67cc3f2fa74..9a30ccb7b104 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -2724,6 +2724,32 @@ int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, > return 0; > } > > +u16 evsel__id_hdr_size(struct evsel *evsel) > +{ > + u64 sample_type = evsel->core.attr.sample_type; As this just uses core, would it be more appropriate to put it in libperf? > + u16 size = 0; Perhaps size_t or int? u16 seems odd. > + > + if (sample_type & PERF_SAMPLE_TID) > + size += sizeof(u64); > + > + if (sample_type & PERF_SAMPLE_TIME) > + size += sizeof(u64); > + > + if (sample_type & PERF_SAMPLE_ID) > + size += sizeof(u64); > + > + if (sample_type & PERF_SAMPLE_STREAM_ID) > + size += sizeof(u64); > + > + if (sample_type & PERF_SAMPLE_CPU) > + size += sizeof(u64); > + > + if (sample_type & PERF_SAMPLE_IDENTIFIER) > + size += sizeof(u64); > + > + return size; > +} > + > struct tep_format_field *evsel__field(struct evsel *evsel, const char *name) > { > return tep_find_field(evsel->tp_format, name); > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 92bed8e2f7d8..699448f2bc2b 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -381,6 +381,8 @@ int evsel__parse_sample(struct evsel *evsel, union perf_event *event, > int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, > u64 *timestamp); > > +u16 evsel__id_hdr_size(struct evsel *evsel); > + A comment would be nice, I know this is just moving code about but this is a new function. Thanks, Ian > static inline struct evsel *evsel__next(struct evsel *evsel) > { > return list_entry(evsel->core.node.next, struct evsel, core.node); > -- > 2.25.1 >
On 19/07/22 20:09, Ian Rogers wrote: > On Mon, Jul 11, 2022 at 2:32 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> Factor out evsel__id_hdr_size() so it can be reused. >> >> This is needed by perf inject. When injecting events from a guest perf.data >> file, there is a possibility that the sample ID numbers conflict. To >> re-write an ID sample, the old one needs to be removed first, which means >> determining how big it is with evsel__id_hdr_size() and then subtracting >> that from the event size. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> tools/perf/util/evlist.c | 28 +--------------------------- >> tools/perf/util/evsel.c | 26 ++++++++++++++++++++++++++ >> tools/perf/util/evsel.h | 2 ++ >> 3 files changed, 29 insertions(+), 27 deletions(-) >> >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >> index 48af7d379d82..03fbe151b0c4 100644 >> --- a/tools/perf/util/evlist.c >> +++ b/tools/perf/util/evlist.c >> @@ -1244,34 +1244,8 @@ bool evlist__valid_read_format(struct evlist *evlist) >> u16 evlist__id_hdr_size(struct evlist *evlist) >> { >> struct evsel *first = evlist__first(evlist); >> - struct perf_sample *data; >> - u64 sample_type; >> - u16 size = 0; >> >> - if (!first->core.attr.sample_id_all) >> - goto out; >> - >> - sample_type = first->core.attr.sample_type; >> - >> - if (sample_type & PERF_SAMPLE_TID) >> - size += sizeof(data->tid) * 2; >> - >> - if (sample_type & PERF_SAMPLE_TIME) >> - size += sizeof(data->time); >> - >> - if (sample_type & PERF_SAMPLE_ID) >> - size += sizeof(data->id); >> - >> - if (sample_type & PERF_SAMPLE_STREAM_ID) >> - size += sizeof(data->stream_id); >> - >> - if (sample_type & PERF_SAMPLE_CPU) >> - size += sizeof(data->cpu) * 2; >> - >> - if (sample_type & PERF_SAMPLE_IDENTIFIER) >> - size += sizeof(data->id); >> -out: >> - return size; >> + return first->core.attr.sample_id_all ? evsel__id_hdr_size(first) : 0; >> } >> >> bool evlist__valid_sample_id_all(struct evlist *evlist) >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >> index a67cc3f2fa74..9a30ccb7b104 100644 >> --- a/tools/perf/util/evsel.c >> +++ b/tools/perf/util/evsel.c >> @@ -2724,6 +2724,32 @@ int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, >> return 0; >> } >> >> +u16 evsel__id_hdr_size(struct evsel *evsel) >> +{ >> + u64 sample_type = evsel->core.attr.sample_type; > > As this just uses core, would it be more appropriate to put it in libperf? AFAIK we move to libperf only as needed. > >> + u16 size = 0; > > Perhaps size_t or int? u16 seems odd. Event header size member is 16-bit > >> + >> + if (sample_type & PERF_SAMPLE_TID) >> + size += sizeof(u64); >> + >> + if (sample_type & PERF_SAMPLE_TIME) >> + size += sizeof(u64); >> + >> + if (sample_type & PERF_SAMPLE_ID) >> + size += sizeof(u64); >> + >> + if (sample_type & PERF_SAMPLE_STREAM_ID) >> + size += sizeof(u64); >> + >> + if (sample_type & PERF_SAMPLE_CPU) >> + size += sizeof(u64); >> + >> + if (sample_type & PERF_SAMPLE_IDENTIFIER) >> + size += sizeof(u64); >> + >> + return size; >> +} >> + >> struct tep_format_field *evsel__field(struct evsel *evsel, const char *name) >> { >> return tep_find_field(evsel->tp_format, name); >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h >> index 92bed8e2f7d8..699448f2bc2b 100644 >> --- a/tools/perf/util/evsel.h >> +++ b/tools/perf/util/evsel.h >> @@ -381,6 +381,8 @@ int evsel__parse_sample(struct evsel *evsel, union perf_event *event, >> int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, >> u64 *timestamp); >> >> +u16 evsel__id_hdr_size(struct evsel *evsel); >> + > > A comment would be nice, I know this is just moving code about but > this is a new function. > > Thanks, > Ian > >> static inline struct evsel *evsel__next(struct evsel *evsel) >> { >> return list_entry(evsel->core.node.next, struct evsel, core.node); >> -- >> 2.25.1 >>
On Tue, Aug 9, 2022 at 4:50 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 19/07/22 20:09, Ian Rogers wrote: > > On Mon, Jul 11, 2022 at 2:32 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > >> > >> Factor out evsel__id_hdr_size() so it can be reused. > >> > >> This is needed by perf inject. When injecting events from a guest perf.data > >> file, there is a possibility that the sample ID numbers conflict. To > >> re-write an ID sample, the old one needs to be removed first, which means > >> determining how big it is with evsel__id_hdr_size() and then subtracting > >> that from the event size. > >> > >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > >> --- > >> tools/perf/util/evlist.c | 28 +--------------------------- > >> tools/perf/util/evsel.c | 26 ++++++++++++++++++++++++++ > >> tools/perf/util/evsel.h | 2 ++ > >> 3 files changed, 29 insertions(+), 27 deletions(-) > >> > >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c > >> index 48af7d379d82..03fbe151b0c4 100644 > >> --- a/tools/perf/util/evlist.c > >> +++ b/tools/perf/util/evlist.c > >> @@ -1244,34 +1244,8 @@ bool evlist__valid_read_format(struct evlist *evlist) > >> u16 evlist__id_hdr_size(struct evlist *evlist) > >> { > >> struct evsel *first = evlist__first(evlist); > >> - struct perf_sample *data; > >> - u64 sample_type; > >> - u16 size = 0; > >> > >> - if (!first->core.attr.sample_id_all) > >> - goto out; > >> - > >> - sample_type = first->core.attr.sample_type; > >> - > >> - if (sample_type & PERF_SAMPLE_TID) > >> - size += sizeof(data->tid) * 2; > >> - > >> - if (sample_type & PERF_SAMPLE_TIME) > >> - size += sizeof(data->time); > >> - > >> - if (sample_type & PERF_SAMPLE_ID) > >> - size += sizeof(data->id); > >> - > >> - if (sample_type & PERF_SAMPLE_STREAM_ID) > >> - size += sizeof(data->stream_id); > >> - > >> - if (sample_type & PERF_SAMPLE_CPU) > >> - size += sizeof(data->cpu) * 2; > >> - > >> - if (sample_type & PERF_SAMPLE_IDENTIFIER) > >> - size += sizeof(data->id); > >> -out: > >> - return size; > >> + return first->core.attr.sample_id_all ? evsel__id_hdr_size(first) : 0; > >> } > >> > >> bool evlist__valid_sample_id_all(struct evlist *evlist) > >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > >> index a67cc3f2fa74..9a30ccb7b104 100644 > >> --- a/tools/perf/util/evsel.c > >> +++ b/tools/perf/util/evsel.c > >> @@ -2724,6 +2724,32 @@ int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, > >> return 0; > >> } > >> > >> +u16 evsel__id_hdr_size(struct evsel *evsel) > >> +{ > >> + u64 sample_type = evsel->core.attr.sample_type; > > > > As this just uses core, would it be more appropriate to put it in libperf? > > AFAIK we move to libperf only as needed. I don't think there is an expectation yet that libperf is stable - I hope not as I need to nuke the CPU map empty function. So, the cost of putting something there rather than perf is minimal, and perf can be just a consumer of libperf as any other tool - which builds confidence the API in libperf is complete. Jiri has posted patches in the past migrating parse-events, there's no "need" for that but the point is to improve the library API. I think this is the same case and minimal cost given only core is being used. Given we're actively migrating util APIs to libperf I think it is better to introduce simple APIs like this in libperf rather than creating something that someone will later have to migrate. > > > >> + u16 size = 0; > > > > Perhaps size_t or int? u16 seems odd. > > Event header size member is 16-bit sizeof is generally considered size_t so the code as-is has implicit truncation - again I'll stand by it looking odd. Thanks, Ian > > > >> + > >> + if (sample_type & PERF_SAMPLE_TID) > >> + size += sizeof(u64); > >> + > >> + if (sample_type & PERF_SAMPLE_TIME) > >> + size += sizeof(u64); > >> + > >> + if (sample_type & PERF_SAMPLE_ID) > >> + size += sizeof(u64); > >> + > >> + if (sample_type & PERF_SAMPLE_STREAM_ID) > >> + size += sizeof(u64); > >> + > >> + if (sample_type & PERF_SAMPLE_CPU) > >> + size += sizeof(u64); > >> + > >> + if (sample_type & PERF_SAMPLE_IDENTIFIER) > >> + size += sizeof(u64); > >> + > >> + return size; > >> +} > >> + > >> struct tep_format_field *evsel__field(struct evsel *evsel, const char *name) > >> { > >> return tep_find_field(evsel->tp_format, name); > >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > >> index 92bed8e2f7d8..699448f2bc2b 100644 > >> --- a/tools/perf/util/evsel.h > >> +++ b/tools/perf/util/evsel.h > >> @@ -381,6 +381,8 @@ int evsel__parse_sample(struct evsel *evsel, union perf_event *event, > >> int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, > >> u64 *timestamp); > >> > >> +u16 evsel__id_hdr_size(struct evsel *evsel); > >> + > > > > A comment would be nice, I know this is just moving code about but > > this is a new function. > > > > Thanks, > > Ian > > > >> static inline struct evsel *evsel__next(struct evsel *evsel) > >> { > >> return list_entry(evsel->core.node.next, struct evsel, core.node); > >> -- > >> 2.25.1 > >> >
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index 48af7d379d82..03fbe151b0c4 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -1244,34 +1244,8 @@ bool evlist__valid_read_format(struct evlist *evlist) u16 evlist__id_hdr_size(struct evlist *evlist) { struct evsel *first = evlist__first(evlist); - struct perf_sample *data; - u64 sample_type; - u16 size = 0; - if (!first->core.attr.sample_id_all) - goto out; - - sample_type = first->core.attr.sample_type; - - if (sample_type & PERF_SAMPLE_TID) - size += sizeof(data->tid) * 2; - - if (sample_type & PERF_SAMPLE_TIME) - size += sizeof(data->time); - - if (sample_type & PERF_SAMPLE_ID) - size += sizeof(data->id); - - if (sample_type & PERF_SAMPLE_STREAM_ID) - size += sizeof(data->stream_id); - - if (sample_type & PERF_SAMPLE_CPU) - size += sizeof(data->cpu) * 2; - - if (sample_type & PERF_SAMPLE_IDENTIFIER) - size += sizeof(data->id); -out: - return size; + return first->core.attr.sample_id_all ? evsel__id_hdr_size(first) : 0; } bool evlist__valid_sample_id_all(struct evlist *evlist) diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index a67cc3f2fa74..9a30ccb7b104 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -2724,6 +2724,32 @@ int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, return 0; } +u16 evsel__id_hdr_size(struct evsel *evsel) +{ + u64 sample_type = evsel->core.attr.sample_type; + u16 size = 0; + + if (sample_type & PERF_SAMPLE_TID) + size += sizeof(u64); + + if (sample_type & PERF_SAMPLE_TIME) + size += sizeof(u64); + + if (sample_type & PERF_SAMPLE_ID) + size += sizeof(u64); + + if (sample_type & PERF_SAMPLE_STREAM_ID) + size += sizeof(u64); + + if (sample_type & PERF_SAMPLE_CPU) + size += sizeof(u64); + + if (sample_type & PERF_SAMPLE_IDENTIFIER) + size += sizeof(u64); + + return size; +} + struct tep_format_field *evsel__field(struct evsel *evsel, const char *name) { return tep_find_field(evsel->tp_format, name); diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 92bed8e2f7d8..699448f2bc2b 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -381,6 +381,8 @@ int evsel__parse_sample(struct evsel *evsel, union perf_event *event, int evsel__parse_sample_timestamp(struct evsel *evsel, union perf_event *event, u64 *timestamp); +u16 evsel__id_hdr_size(struct evsel *evsel); + static inline struct evsel *evsel__next(struct evsel *evsel) { return list_entry(evsel->core.node.next, struct evsel, core.node);
Factor out evsel__id_hdr_size() so it can be reused. This is needed by perf inject. When injecting events from a guest perf.data file, there is a possibility that the sample ID numbers conflict. To re-write an ID sample, the old one needs to be removed first, which means determining how big it is with evsel__id_hdr_size() and then subtracting that from the event size. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- tools/perf/util/evlist.c | 28 +--------------------------- tools/perf/util/evsel.c | 26 ++++++++++++++++++++++++++ tools/perf/util/evsel.h | 2 ++ 3 files changed, 29 insertions(+), 27 deletions(-)