linux-kernel - Re: [PATCH 05/35] perf tools: Factor out evsel__id_hdr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fUChJLqfJ__joVhtKwgjLTtBtMm8M0vt9eaOfLMmck85g@mail.gmail.com>
Date:   Tue, 19 Jul 2022 10:09:04 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Adrian Hunter <adrian.hunter@...el.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [PATCH 05/35] perf tools: Factor out evsel__id_hdr_size()

On Mon, Jul 11, 2022 at 2:32 AM Adrian Hunter <adrian.hunter@...el.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@...el.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
>