Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231128175441.721579-1-namhyung@kernel.org> <CAP-5=fWfKqgT60TFRALw8vTDQT7VFV+0+eo1rFSSH3eVrjzPmA@mail.gmail.com>
 <CAM9d7chKmDETK6Ea2wyR8L21jyHWcPHbKavarnq-JmNA-AoUnQ@mail.gmail.com>
 <CAP-5=fUf6R=bsfg7i8atFApJBY-=zWUBMq7inFsCPZhB+w2==Q@mail.gmail.com> <CAM9d7cjDiu=dksnhboJFT4uPQJcvGMB-vBt96v3i7Kqy5LKRMw@mail.gmail.com>
In-Reply-To: <CAM9d7cjDiu=dksnhboJFT4uPQJcvGMB-vBt96v3i7Kqy5LKRMw@mail.gmail.com>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 5 Dec 2023 09:59:02 -0800
Message-ID: <CAP-5=fXKbi3DYoOKrJvNKLNU=fJEY9aDAOQhH+Vh+XWxHzGjwA@mail.gmail.com>
Subject: Re: [PATCHSET 0/8] perf annotate: Make annotation_options global (v1)
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Dec 4, 2023 at 2:46=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> w=
rote:
>
> On Thu, Nov 30, 2023 at 10:37=E2=80=AFAM Ian Rogers <irogers@google.com> =
wrote:
> >
> > On Wed, Nov 29, 2023 at 3:56=E2=80=AFPM Namhyung Kim <namhyung@kernel.o=
rg> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Tue, Nov 28, 2023 at 11:14=E2=80=AFAM Ian Rogers <irogers@google.c=
om> wrote:
> > > >
> > > > On Tue, Nov 28, 2023 at 9:54=E2=80=AFAM Namhyung Kim <namhyung@kern=
el.org> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > It used to have annotation_options for each command separately (f=
or
> > > > > example, perf report, annotate, and top), but we can make it glob=
al as
> > > > > they never used together (with different settings).  This would s=
ave
> > > > > 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-pe=
rf.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 sur=
e
> > > > 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.

Agreed.

> >
> > > >
> > > > Annotation is something of unfortunate abstraction as it covers thi=
ngs
> > > > 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 symbol=
s
> > > > within a dso. If we could have a lazy allocation model with
> > > > annotations then maybe we can do away with large hammers like globa=
l
> > > > 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 op=
tion
> > > 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 h=
ave
> > > 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? :)

For the series:
Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Namhyung