Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231012062359.1616786-1-irogers@google.com> <20231012062359.1616786-13-irogers@google.com>
 <CAM9d7cgehgF0pXm_7VME0jUo=8dHwRH7_EruGqP7D-CVaj5sEw@mail.gmail.com>
In-Reply-To: <CAM9d7cgehgF0pXm_7VME0jUo=8dHwRH7_EruGqP7D-CVaj5sEw@mail.gmail.com>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 24 Oct 2023 12:21:23 -0700
Message-ID: <CAP-5=fULcy+9pY5U_S_19AvSQWKf-y28R9T6G4d6Xw7C3ENM8g@mail.gmail.com>
Subject: Re: [PATCH v2 12/13] perf mmap: Lazily initialize zstd streams
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Nick Terrell <terrelln@fb.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Song Liu <song@kernel.org>,
        Sandipan Das <sandipan.das@amd.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        James Clark <james.clark@arm.com>,
        Liam Howlett <liam.howlett@oracle.com>,
        Miguel Ojeda <ojeda@kernel.org>, Leo Yan <leo.yan@linaro.org>,
        German Gomez <german.gomez@arm.com>,
        Ravi Bangoria <ravi.bangoria@amd.com>,
        Artem Savkov <asavkov@redhat.com>,
        Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
        Andi Kleen <ak@linux.intel.com>, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org, bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 18, 2023 at 4:21=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> =
wrote:
>
> On Wed, Oct 11, 2023 at 11:24=E2=80=AFPM Ian Rogers <irogers@google.com> =
wrote:
> >
> > Zstd streams create dictionaries that can require significant RAM,
> > especially when there is one per-CPU. Tools like perf record won't use
> > the streams without the -z option, and so the creation of the streams
> > is pure overhead. Switch to creating the streams on first use.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/compress.h |  1 +
> >  tools/perf/util/mmap.c     |  5 ++--
> >  tools/perf/util/mmap.h     |  1 -
> >  tools/perf/util/zstd.c     | 61 ++++++++++++++++++++------------------
> >  4 files changed, 35 insertions(+), 33 deletions(-)
> >
> > diff --git a/tools/perf/util/compress.h b/tools/perf/util/compress.h
> > index 0cd3369af2a4..9391850f1a7e 100644
> > --- a/tools/perf/util/compress.h
> > +++ b/tools/perf/util/compress.h
> > @@ -21,6 +21,7 @@ struct zstd_data {
> >  #ifdef HAVE_ZSTD_SUPPORT
> >         ZSTD_CStream    *cstream;
> >         ZSTD_DStream    *dstream;
> > +       int comp_level;
> >  #endif
> >  };
> >
> > diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> > index 49093b21ee2d..122ee198a86e 100644
> > --- a/tools/perf/util/mmap.c
> > +++ b/tools/perf/util/mmap.c
> > @@ -295,15 +295,14 @@ int mmap__mmap(struct mmap *map, struct mmap_para=
ms *mp, int fd, struct perf_cpu
> >
> >         map->core.flush =3D mp->flush;
> >
> > -       map->comp_level =3D mp->comp_level;
> >  #ifndef PYTHON_PERF
> > -       if (zstd_init(&map->zstd_data, map->comp_level)) {
> > +       if (zstd_init(&map->zstd_data, mp->comp_level)) {
> >                 pr_debug2("failed to init mmap compressor, error %d\n",=
 errno);
> >                 return -1;
> >         }
> >  #endif
> >
> > -       if (map->comp_level && !perf_mmap__aio_enabled(map)) {
> > +       if (mp->comp_level && !perf_mmap__aio_enabled(map)) {
> >                 map->data =3D mmap(NULL, mmap__mmap_len(map), PROT_READ=
|PROT_WRITE,
> >                                  MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
> >                 if (map->data =3D=3D MAP_FAILED) {
> > diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h
> > index f944c3cd5efa..0df6e1621c7e 100644
> > --- a/tools/perf/util/mmap.h
> > +++ b/tools/perf/util/mmap.h
> > @@ -39,7 +39,6 @@ struct mmap {
> >  #endif
> >         struct mmap_cpu_mask    affinity_mask;
> >         void            *data;
> > -       int             comp_level;
> >         struct perf_data_file *file;
> >         struct zstd_data      zstd_data;
> >  };
> > diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> > index 48dd2b018c47..60f2d749b1c0 100644
> > --- a/tools/perf/util/zstd.c
> > +++ b/tools/perf/util/zstd.c
> > @@ -7,35 +7,9 @@
> >
> >  int zstd_init(struct zstd_data *data, int level)
> >  {
> > -       size_t ret;
> > -
> > -       data->dstream =3D ZSTD_createDStream();
> > -       if (data->dstream =3D=3D NULL) {
> > -               pr_err("Couldn't create decompression stream.\n");
> > -               return -1;
> > -       }
> > -
> > -       ret =3D ZSTD_initDStream(data->dstream);
> > -       if (ZSTD_isError(ret)) {
> > -               pr_err("Failed to initialize decompression stream: %s\n=
", ZSTD_getErrorName(ret));
> > -               return -1;
> > -       }
> > -
> > -       if (!level)
> > -               return 0;
> > -
> > -       data->cstream =3D ZSTD_createCStream();
> > -       if (data->cstream =3D=3D NULL) {
> > -               pr_err("Couldn't create compression stream.\n");
> > -               return -1;
> > -       }
> > -
> > -       ret =3D ZSTD_initCStream(data->cstream, level);
> > -       if (ZSTD_isError(ret)) {
> > -               pr_err("Failed to initialize compression stream: %s\n",=
 ZSTD_getErrorName(ret));
> > -               return -1;
> > -       }
> > -
> > +       data->comp_level =3D level;
> > +       data->dstream =3D NULL;
> > +       data->cstream =3D NULL;
> >         return 0;
> >  }
> >
> > @@ -63,6 +37,21 @@ size_t zstd_compress_stream_to_records(struct zstd_d=
ata *data, void *dst, size_t
> >         ZSTD_outBuffer output;
> >         void *record;
> >
> > +       if (!data->cstream) {
> > +               data->cstream =3D ZSTD_createCStream();
> > +               if (data->cstream =3D=3D NULL) {
> > +                       pr_err("Couldn't create compression stream.\n")=
;
> > +                       return -1;
> > +               }
> > +
> > +               ret =3D ZSTD_initCStream(data->cstream, data->comp_leve=
l);
> > +               if (ZSTD_isError(ret)) {
> > +                       pr_err("Failed to initialize compression stream=
: %s\n",
> > +                               ZSTD_getErrorName(ret));
> > +                       return -1;
>
> I'm not sure if the callers are ready to handle the failure.

Thanks, fixed in v3.

Ian

> Thanks,
> Namhyung
>
>
> > +               }
> > +       }
> > +
> >         while (input.pos < input.size) {
> >                 record =3D dst;
> >                 size =3D process_header(record, 0);
> > @@ -96,6 +85,20 @@ size_t zstd_decompress_stream(struct zstd_data *data=
, void *src, size_t src_size
> >         ZSTD_inBuffer input =3D { src, src_size, 0 };
> >         ZSTD_outBuffer output =3D { dst, dst_size, 0 };
> >
> > +       if (!data->dstream) {
> > +               data->dstream =3D ZSTD_createDStream();
> > +               if (data->dstream =3D=3D NULL) {
> > +                       pr_err("Couldn't create decompression stream.\n=
");
> > +                       return -1;
> > +               }
> > +
> > +               ret =3D ZSTD_initDStream(data->dstream);
> > +               if (ZSTD_isError(ret)) {
> > +                       pr_err("Failed to initialize decompression stre=
am: %s\n",
> > +                               ZSTD_getErrorName(ret));
> > +                       return -1;
> > +               }
> > +       }
> >         while (input.pos < input.size) {
> >                 ret =3D ZSTD_decompressStream(data->dstream, &output, &=
input);
> >                 if (ZSTD_isError(ret)) {
> > --
> > 2.42.0.609.gbb76f46606-goog
> >