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

[Adrian Hunter <adrian.hunter@intel.com>]

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