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

Ian Rogers
2022-07-19T17:09:24Z

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
>