On Tue, Aug 9, 2022 at 4:50 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>
> On 19/07/22 20:09, Ian Rogers wrote:
> > On Mon, Jul 11, 2022 at 2:32 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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
> >>
>
