Hi Adrian, Thanks for your review!
On Wed, Sep 23, 2020 at 11:36 PM Adrian Hunter <adrian.hun...@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.hun...@intel.com> > > Signed-off-by: Namhyung Kim <namhy...@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 > > >