Re: [PATCH 7/7] perf inject: Remove stale build-id processing

[Namhyung Kim <namhyung@kernel.org>]

Hi Adrian,

Thanks for your review!

On Wed, Sep 23, 2020 at 11:36 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 23/09/20 11:05 am, Namhyung Kim wrote:
> > I think we don't need to call build_id__mark_dso_hit() in the
> > perf_event__repipe_sample() as it's not used by -b option.  In case of
> > the -b option is used, it uses perf_event__inject_buildid() instead.
> > This can remove unnecessary overhead of finding thread/map for each
> > sample event.
> >
> > Also I suspect HEADER_BUILD_ID feature bit setting since we already
> > generated/injected BUILD_ID event into the output stream.  So this
> > header information seems redundant.  I'm not 100% sure about the
> > auxtrace usage, but it looks like not related to this directly.
> >
> > And we now have --buildid-all so users can get the same behavior if
> > they want.
>
> For a perf.data file, don't buildids get written to the HEADER_BUILD_ID
> feature section by perf_session__write_header() if the feature flag is set
> and if they are hit?

Right, that's what perf record does unless -B option is used.
But I'm more concerned about the pipe usage which doesn't use the header.

>
> So, unless -b is used, anything you don't hit you lose i.e. a buildid in the
> HEADER_BUILD_ID feature section of the input file, will not be written to
> the output file.

But perf inject generates PERF_RECORD_HEADER_BUILD_ID events
and puts them into the data stream when -b option is used.

Do you say perf inject should process build-id even when -b is not used?

> >
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-inject.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index 500428aaa576..0191d72be7c4 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -277,8 +277,6 @@ static int perf_event__repipe_sample(struct perf_tool *tool,
> >               return f(tool, event, sample, evsel, machine);
> >       }
> >
> > -     build_id__mark_dso_hit(tool, event, sample, evsel, machine);
> > -
>
> I guess that chunk would prevent losing a buildid in a perf.data file?
>
> >       if (inject->itrace_synth_opts.set && sample->aux_sample.size)
> >               event = perf_inject__cut_auxtrace_sample(inject, event, sample);
> >
> > @@ -767,16 +765,6 @@ static int __cmd_inject(struct perf_inject *inject)
> >               return ret;
> >
> >       if (!data_out->is_pipe) {
> > -             if (inject->build_ids)
> > -                     perf_header__set_feat(&session->header,
> > -                                           HEADER_BUILD_ID);
>
> That could be due to confusion with 'perf buildid-list' which will not show
> any buildids from synthesized buildid events unless "with hits" is selected,
> so then it looks like there are no buildids.

Yeah, it's confusing.. I think we should change the behavior to handle
the pipe case properly.

>
> It could be an advantage to have the buildids also in the HEADER_BUILD_ID
> feature section, because then then build-list can list them quickly.

I'm not sure it's good to have duplicate build-id info.
We may add an option just to update the header section and
not inject BUILD_ID events.

>
> > -             /*
> > -              * Keep all buildids when there is unprocessed AUX data because
> > -              * it is not known which ones the AUX trace hits.
> > -              */
> > -             if (perf_header__has_feat(&session->header, HEADER_BUILD_ID) &&
> > -                 inject->have_auxtrace && !inject->itrace_synth_opts.set)
> > -                     dsos__hit_all(session);
>
> I expect that is definitely needed.

OK.

Thanks
Namhyung

>
> >               /*
> >                * The AUX areas have been removed and replaced with
> >                * synthesized hardware events, so clear the feature flag and
> >
>