Message-ID: <CAM9d7ci0OF3MLRoH3sZ=kYgfw0C7PBeYqgmun0mmas2U84NwkA@mail.gmail.com>
Date:   Sat, 26 Jun 2021 17:49:13 -0700
From:   Namhyung Kim <namhyung@...nel.org>
To:     Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Alexander Antonov <alexander.antonov@...ux.intel.com>,
        Alexei Budankov <abudankov@...wei.com>,
        Riccardo Mancini <rickyman7@...il.com>
Subject: Re: [PATCH v7 19/20] perf session: Load single file for analysis

On Tue, Jun 22, 2021 at 1:43 AM Alexey Bayduraev
<alexey.v.bayduraev@...ux.intel.com> wrote:
>
> Adding eof flag to reader state and moving the check to reader__mmap.
> Separating reading code of single event into reader__read_event function.
> Adding basic reader return codes to simplify the code and introducing
> reader remmap/read_event loop based on them.
>
> Design and implementation are based on the prototype [1], [2].
>
> [1] git clone https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git -b perf/record_threads
> [2] https://lore.kernel.org/lkml/20180913125450.21342-1-jolsa@kernel.org/
>
> Suggested-by: Jiri Olsa <jolsa@...nel.org>
> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>
> ---
>  tools/perf/util/session.c | 71 ++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 27 deletions(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 7d91205a6a47..fe25abf83b80 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -64,6 +64,12 @@ struct reader_state {
>         u64      file_offset;
>         u64      data_size;
>         u64      head;
> +       bool     eof;
> +};
> +
> +enum {
> +       READER_EOF      =  0,
> +       READER_OK       =  1,

Just a nitpick, it might be better to add READER_NODATA state
to differentiate it from the real end-of-file state.

Thanks,
Namhyung


>  };
>
>  struct reader {
> @@ -2245,6 +2251,11 @@ reader__mmap(struct reader *rd, struct perf_session *session)
>         char *buf, **mmaps = st->mmaps;
>         u64 page_offset;
>
> +       if (st->file_pos >= st->data_size) {
> +               st->eof = true;
> +               return READER_EOF;
> +       }
> +
>         mmap_prot  = PROT_READ;
>         mmap_flags = MAP_SHARED;
>
> @@ -2273,36 +2284,26 @@ reader__mmap(struct reader *rd, struct perf_session *session)
>         mmaps[st->mmap_idx] = st->mmap_cur = buf;
>         st->mmap_idx = (st->mmap_idx + 1) & (ARRAY_SIZE(st->mmaps) - 1);
>         st->file_pos = st->file_offset + st->head;
> -       return 0;
> +       return READER_OK;
>  }
>
>  static int
> -reader__process_events(struct reader *rd, struct perf_session *session,
> -                      struct ui_progress *prog)
> +reader__read_event(struct reader *rd, struct perf_session *session,
> +                  struct ui_progress *prog)
>  {
>         struct reader_state *st = &rd->state;
> -       u64 size;
> -       int err = 0;
> +       int err = READER_OK;
>         union perf_event *event;
> +       u64 size;
>         s64 skip;
>
> -remap:
> -       err = reader__mmap(rd, session);
> -       if (err)
> -               goto out;
> -       if (session->one_mmap) {
> -               session->one_mmap_addr   = rd->state.mmap_cur;
> -               session->one_mmap_offset = rd->state.file_offset;
> -       }
> -
> -more:
>         event = fetch_mmaped_event(st->head, st->mmap_size, st->mmap_cur,
>                                    session->header.needs_swap);
>         if (IS_ERR(event))
>                 return PTR_ERR(event);
>
>         if (!event)
> -               goto remap;
> +               return READER_EOF;
>
>         session->active_reader = rd;
>         size = event->header.size;
> @@ -2324,18 +2325,12 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>         st->head += size;
>         st->file_pos += size;
>
> -       err = __perf_session__process_decomp_events(session);
> -       if (err)
> -               goto out;
> +       skip = __perf_session__process_decomp_events(session);
> +       if (skip)
> +               err = skip;
>
>         ui_progress__update(prog, size);
>
> -       if (session_done())
> -               goto out;
> -
> -       if (st->file_pos < st->data_size)
> -               goto more;
> -
>  out:
>         session->active_reader = NULL;
>         return err;
> @@ -2379,9 +2374,31 @@ static int __perf_session__process_events(struct perf_session *session)
>         err = reader__init(rd, &session->one_mmap);
>         if (err)
>                 goto out_err;
> -       err = reader__process_events(rd, session, &prog);
> -       if (err)
> +       err = reader__mmap(rd, session);
> +       if (err != READER_OK) {
> +               if (err == READER_EOF)
> +                       err = -EINVAL;
>                 goto out_err;
> +       }
> +       if (session->one_mmap) {
> +               session->one_mmap_addr   = rd->state.mmap_cur;
> +               session->one_mmap_offset = rd->state.file_offset;
> +       }
> +
> +       while (true) {
> +               if (session_done())
> +                       break;
> +
> +               err = reader__read_event(rd, session, &prog);
> +               if (err < 0)
> +                       break;
> +               if (err == READER_EOF) {
> +                       err = reader__mmap(rd, session);
> +                       if (err <= 0)
> +                               break;
> +               }
> +       }
> +
>         /* do the final flush for ordered samples */
>         err = ordered_events__flush(oe, OE_FLUSH__FINAL);
>         if (err)
> --
> 2.19.0
>

Powered by blists - more mailing lists