Re: [PATCH 05/35] perf tools: Factor out evsel__id_hdr_size()

[Ian Rogers]

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
> >>
>