linux-kernel - Re: [PATCH 2/4] tools lib perf: Handle read format in perf_evsel_

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] [day] [month] [year] [list]
Message-ID: <CAM9d7cjE+EFoUpxgj0WOn70W9cZwchtuqEJtY_BoOy3rrE6zrA@mail.gmail.com>
Date:   Tue, 16 Aug 2022 14:54:40 -0700
From:   Namhyung Kim <namhyung@...nel.org>
To:     Jiri Olsa <olsajiri@...il.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ian Rogers <irogers@...gle.com>,
        linux-perf-users <linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH 2/4] tools lib perf: Handle read format in perf_evsel__read()

Hi Jiri,

On Tue, Aug 16, 2022 at 6:19 AM Jiri Olsa <olsajiri@...il.com> wrote:
>
> On Mon, Aug 15, 2022 at 12:01:04PM -0700, Namhyung Kim wrote:
> > The perf_counts_values should be increased to read the new lost data.
> > Also adjust values after read according the read format.
> >
> > This supports PERF_FORMAT_GROUP which has a different data format but
> > it's only available for leader events.  Currently it doesn't have an API
> > to read sibling (member) events in the group.  But users may read the
> > sibling event directly.
> >
> > Also reading from mmap would be disabled when the read format has ID or
> > LOST bit as it's not exposed via mmap.
> >
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> >  tools/lib/perf/evsel.c              | 72 +++++++++++++++++++++++++++++
> >  tools/lib/perf/include/perf/event.h |  3 +-
> >  tools/lib/perf/include/perf/evsel.h |  4 +-
> >  3 files changed, 77 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
> > index 952f3520d5c2..fc23670231cb 100644
> > --- a/tools/lib/perf/evsel.c
> > +++ b/tools/lib/perf/evsel.c
> > @@ -305,6 +305,9 @@ int perf_evsel__read_size(struct perf_evsel *evsel)
> >       if (read_format & PERF_FORMAT_ID)
> >               entry += sizeof(u64);
> >
> > +     if (read_format & PERF_FORMAT_LOST)
> > +             entry += sizeof(u64);
> > +
> >       if (read_format & PERF_FORMAT_GROUP) {
> >               nr = evsel->nr_members;
> >               size += sizeof(u64);
> > @@ -314,24 +317,93 @@ int perf_evsel__read_size(struct perf_evsel *evsel)
> >       return size;
> >  }
> >
> > +/* This only reads values for the leader */
> > +static int perf_evsel__read_group(struct perf_evsel *evsel, int cpu_map_idx,
> > +                               int thread, struct perf_counts_values *count)
> > +{
> > +     size_t size = perf_evsel__read_size(evsel);
> > +     int *fd = FD(evsel, cpu_map_idx, thread);
> > +     u64 read_format = evsel->attr.read_format;
> > +     u64 *data;
> > +     int idx = 1;
> > +
> > +     if (fd == NULL || *fd < 0)
> > +             return -EINVAL;
> > +
> > +     data = calloc(1, size);
> > +     if (data == NULL)
> > +             return -ENOMEM;
> > +
> > +     if (readn(*fd, data, size) <= 0) {
> > +             free(data);
> > +             return -errno;
> > +     }
>
> could you please put in here some comment that this is intentionaly
> reading only the leader or better yet rename the function? I was lost
> before I got to read the changelog ;-)

Sure, actually it has a comment above the function signature.
Maybe we can add an API to read whole group counters
but it could be a follow up.

>
> > +
> > +     if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
> > +             count->ena = data[idx++];
> > +     if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
> > +             count->run = data[idx++];
> > +
> > +     /* value is always available */
> > +     count->val = data[idx++];
> > +     if (read_format & PERF_FORMAT_ID)
> > +             count->id = data[idx++];
> > +     if (read_format & PERF_FORMAT_LOST)
> > +             count->lost = data[idx++];
> > +
> > +     free(data);
> > +     return 0;
> > +}
> > +
> > +/*
> > + * The perf read format is very flexible.  It needs to set the proper
> > + * values according to the read format.
> > + */
> > +static void perf_evsel__adjust_values(struct perf_evsel *evsel,
> > +                                   struct perf_counts_values *count)
> > +{
> > +     u64 read_format = evsel->attr.read_format;
> > +
> > +     if (!(read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)) {
> > +             memmove(&count->values[2], &count->values[1], 24);
> > +             count->ena = 0;
> > +     }
> > +
> > +     if (!(read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)) {
> > +             memmove(&count->values[3], &count->values[2], 16);
> > +             count->run = 0;
> > +     }
> > +
> > +     if (!(read_format & PERF_FORMAT_ID)) {
> > +             memmove(&count->values[4], &count->values[3], 8);
> > +             count->id = 0;
> > +     }
> > +}
>
>
> could we do this the same way we read group counters.. like make read
> into local buffer and initialize perf_counts_values values based on
> format, something like:
>
>         readn(fd, data ...
>
>         if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
>                 count->ena = data[idx++];
>         if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
>                 count->run = data[idx++];
>
>         /* value is always available */
>         count->val = data[idx++];
>         if (read_format & PERF_FORMAT_ID)
>                 count->id = data[idx++];
>         if (read_format & PERF_FORMAT_LOST)
>                 count->lost = data[idx++];

Sure.

>
>
> and perhaps we should cancel that perf_counts_values's union and keep
> only 'val/ena/run...' fields?

I'd like to have them and use it for reading the lost count soon.

Thanks,
Namhyung