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]

Date:   Mon, 4 Dec 2023 14:46:37 -0800
From:   Namhyung Kim <namhyung@...nel.org>
To:     Ian Rogers <irogers@...gle.com>
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 Thu, Nov 30, 2023 at 10:37 AM Ian Rogers <irogers@...gle.com> wrote:
>
> On Wed, Nov 29, 2023 at 3:56 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > Hi Ian,
> >
> > On Tue, Nov 28, 2023 at 11:14 AM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > 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.
> >
> > That's not the case when you deal with hist entry or TUI browser.
> > I think that's the reason we have the option in the annotation.
> >
> >
> > > 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
> >
> > That's true.  But the annotation struct is used only if
> > symbol__annotation_init() is called.
>
> Right. I find this approach likely to lead to errors, such as a symbol
> created globally before symbol__annotation_init() was called breaking
> the container_of assumptions.

Sure, but that's a different issue.  Maybe we can add a hash table
to map a symbol to annotation or annotated_source directly.  But
I don't think we need annotation_option for each symbol/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.
> >
> > Right, that's why I splitted the struct annotated_branch.
> >
> > >
> > > 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.
> >
> > Maybe I can move the pointer to option into the annotated_source
> > which is allocated lazily.  But I don't think it needs to keep the option
> > for each symbol or annotation.  It's usually to control some display
> > behaviors in the disasm output globally.  So I think it's better to have
> > a global variable.
>
> Sgtm. My point wasn't to criticize, I think this is a good change, I
> was just trying to imagine doing things in a way that could overall
> reduce complexity

Yep, thanks for your review.  Can I get your ACKs? :)
Namhyung