linux-kernel - Re: [PATCHSET 0/8] perf annotate: Make annotation

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=fWfKqgT60TFRALw8vTDQT7VFV+0+eo1rFSSH3eVrjzPmA@mail.gmail.com>
Date:   Tue, 28 Nov 2023 11:13:55 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCHSET 0/8] perf annotate: Make annotation_options global (v1)

On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Hello,
>
> It used to have annotation_options for each command separately (for
> example, perf report, annotate, and top), but we can make it global as
> they never used together (with different settings).  This would save
> some memory for each symbol when annotation is enabled.
>
> This code is available at 'perf/annotate-option-v1' branch in
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
> Thanks,
> Namhyung

Thanks for doing this and I think it is progress. I think there is a
common problem with having things be an option rather than say part of
session. Having a global variable seems unfortunate but I'm not sure
if in all locations we have easy access to the session. The rough
structure with annotations as I understand it is:

session has machines
a machine has dsos
a dso has symbols
a symbol has an annotation

Annotation is something of unfortunate abstraction as it covers things
like an IPC per symbol (why hard code to just IPC?) and things like
source files and line numbers.

A recent success story where we got rid of a configuration variable
was by switching to lazy allocation with sorting by name for symbols
within a dso. If we could have a lazy allocation model with
annotations then maybe we can do away with large hammers like global
options.

Thanks,
Ian




> Namhyung Kim (8):
>   perf annotate: Introduce global annotation_options
>   perf report: Convert to the global annotation_options
>   perf top: Convert to the global annotation_options
>   perf annotate: Use global annotation_options
>   perf ui/browser/annotate: Use global annotation_options
>   perf annotate: Ensure init/exit for global options
>   perf annotate: Remove remaining usages of local annotation options
>   perf annotate: Get rid of local annotation options
>
>  tools/perf/builtin-annotate.c     |  43 +++++----
>  tools/perf/builtin-report.c       |  37 ++++----
>  tools/perf/builtin-top.c          |  45 +++++-----
>  tools/perf/ui/browsers/annotate.c |  85 ++++++++----------
>  tools/perf/ui/browsers/hists.c    |  34 +++----
>  tools/perf/ui/browsers/hists.h    |   2 -
>  tools/perf/util/annotate.c        | 142 +++++++++++++++---------------
>  tools/perf/util/annotate.h        |  38 ++++----
>  tools/perf/util/block-info.c      |   6 +-
>  tools/perf/util/block-info.h      |   3 +-
>  tools/perf/util/hist.h            |  25 ++----
>  tools/perf/util/top.h             |   1 -
>  12 files changed, 206 insertions(+), 255 deletions(-)
>
>
> base-commit: 757489991f7c08603395b85037a981c31719c92c
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>