linux-kernel - Re: [PATCH v2 03/10] perf record: Early auxtrace initialization before event parsing

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: <4473367b-38e1-7c15-3937-a077d68410b7@intel.com>
Date:   Sun, 5 Mar 2023 10:32:39 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        "Steinar H. Gunderson" <sesse@...gle.com>,
        Kim Phillips <kim.phillips@....com>,
        Florian Fischer <florian.fischer@...q.space>,
        James Clark <james.clark@....com>,
        Suzuki Poulouse <suzuki.poulose@....com>,
        Sean Christopherson <seanjc@...gle.com>,
        Leo Yan <leo.yan@...aro.org>,
        John Garry <john.g.garry@...cle.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v2 03/10] perf record: Early auxtrace initialization
 before event parsing

On 3/03/23 18:40, Liang, Kan wrote:
> 
> 
> On 2023-03-02 4:25 p.m., Ian Rogers wrote:
>> This allows event parsing to use the evsel__is_aux_event function,
>> which is important when determining event grouping.
>>
>> Signed-off-by: Ian Rogers <irogers@...gle.com>
>> ---
>>  tools/perf/arch/x86/util/auxtrace.c | 17 +++++++++++++----
>>  tools/perf/builtin-record.c         |  6 ++++++
>>  tools/perf/util/auxtrace.h          |  2 ++
>>  3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/auxtrace.c b/tools/perf/arch/x86/util/auxtrace.c
>> index 3da506e13f49..de1e4842ea2e 100644
>> --- a/tools/perf/arch/x86/util/auxtrace.c
>> +++ b/tools/perf/arch/x86/util/auxtrace.c
>> @@ -15,6 +15,19 @@
>>  #include "../../../util/intel-bts.h"
>>  #include "../../../util/evlist.h"
>>  
>> +void auxtrace__early_init(void)
>> +{
>> +	struct perf_pmu *intel_pt_pmu;
>> +	struct perf_pmu *intel_bts_pmu;
>> +
>> +	intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
>> +	if (intel_pt_pmu)
>> +		intel_pt_pmu->auxtrace = true;
>> +	intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
>> +	if (intel_bts_pmu)
>> +		intel_bts_pmu->auxtrace = true;
>> +}
>> +
>>  static
>>  struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
>>  						    int *err)
>> @@ -26,11 +39,7 @@ struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
>>  	bool found_bts = false;
>>  
>>  	intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
>> -	if (intel_pt_pmu)
>> -		intel_pt_pmu->auxtrace = true;
>>  	intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
>> -	if (intel_bts_pmu)
>> -		intel_bts_pmu->auxtrace = true;
>>  
>>  	evlist__for_each_entry(evlist, evsel) {
>>  		if (intel_pt_pmu && evsel->core.attr.type == intel_pt_pmu->type)
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index 8374117e66f6..a0870c076dc0 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -3940,6 +3940,10 @@ static int record__init_thread_masks(struct record *rec)
>>  	return ret;
>>  }
>>  
>> +__weak void auxtrace__early_init(void)
>> +{
>> +}
>> +
>>  int cmd_record(int argc, const char **argv)
>>  {
>>  	int err;
>> @@ -3985,6 +3989,8 @@ int cmd_record(int argc, const char **argv)
>>  	if (err)
>>  		return err;
>>  
>> +	auxtrace__early_init();
> 
> So the auxtrace__early_init() will be unconditionally invoked even there
> is no PT or BTS events, right?
> 
> Maybe we should move the auxtrace__early_init() to evsel__is_aux_event()
> and cache the value. The initialization will only be invoked when it's
> required.

Although perf_pmu__find() will be called unconditionally via
record__auxtrace_init() anyway.

> Something as below (not tested.)
> 
> +void auxtrace__init(void)
> +{
> +	struct perf_pmu *intel_pt_pmu;
> +	struct perf_pmu *intel_bts_pmu;
> +	static bool cached;
> +
> +	if (cached)
> +		return;
> +	intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
> +	if (intel_pt_pmu)
> +		intel_pt_pmu->auxtrace = true;
> +	intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
> +	if (intel_bts_pmu)
> +		intel_bts_pmu->auxtrace = true;
> +}
> 
> bool evsel__is_aux_event(struct evsel *evsel)
> {
> 	struct perf_pmu *pmu = evsel__find_pmu(evsel);
> +	auxtrace__init();
> 	return pmu && pmu->auxtrace;
> }
> 
> 
> 
> Thanks,
> Kan
> 
>> +
>>  	argc = parse_options(argc, argv, record_options, record_usage,
>>  			    PARSE_OPT_STOP_AT_NON_OPTION);
>>  	if (quiet)
>> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
>> index 29eb82dff574..49a86aa6ac94 100644
>> --- a/tools/perf/util/auxtrace.h
>> +++ b/tools/perf/util/auxtrace.h
>> @@ -457,6 +457,8 @@ struct addr_filters {
>>  
>>  struct auxtrace_cache;
>>  
>> +void auxtrace__early_init(void);
>> +
>>  #ifdef HAVE_AUXTRACE_SUPPORT
>>  
>>  u64 compat_auxtrace_mmap__read_head(struct auxtrace_mmap *mm);