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

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: <91de4a5d-2646-0522-92a6-6c0de69ed007@intel.com>
Date:   Wed, 23 Sep 2020 17:36:07 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Namhyung Kim <namhyung@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>,
        Ian Rogers <irogers@...gle.com>
Subject: Re: [PATCH 7/7] perf inject: Remove stale build-id processing

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?

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.

> 
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Signed-off-by: Namhyung Kim <namhyung@...nel.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.

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.

> -		/*
> -		 * 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.

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