| 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
| ||
|
Date: Mon, 6 Mar 2023 09:10:14 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Adrian Hunter <adrian.hunter@...el.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 2023-03-06 4:31 a.m., Adrian Hunter wrote:
> On 5/03/23 10:32, Adrian Hunter wrote:
>> 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.
>
> However auxtrace__early_init() is before parsing 'verbose' so
> debug prints don't work anymore.
>
> How about this instead:
Yes, I think it should be a better place to initialize them.
Thanks,
Kan
>
> diff --git a/tools/perf/arch/x86/util/auxtrace.c
> b/tools/perf/arch/x86/util/auxtrace.c
> index 3da506e13f49d..330d03216b0e6 100644
> --- a/tools/perf/arch/x86/util/auxtrace.c
> +++ b/tools/perf/arch/x86/util/auxtrace.c
> @@ -26,11 +26,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/arch/x86/util/pmu.c b/tools/perf/arch/x86/util/pmu.c
> index 358340b342431..f73b80dcd8bdc 100644
> --- a/tools/perf/arch/x86/util/pmu.c
> +++ b/tools/perf/arch/x86/util/pmu.c
> @@ -27,10 +27,14 @@ static bool cached_list;
> struct perf_event_attr *perf_pmu__get_default_config(struct perf_pmu
> *pmu __maybe_unused)
> {
> #ifdef HAVE_AUXTRACE_SUPPORT
> - if (!strcmp(pmu->name, INTEL_PT_PMU_NAME))
> + if (!strcmp(pmu->name, INTEL_PT_PMU_NAME)) {
> + pmu->auxtrace = true;
> return intel_pt_pmu_default_config(pmu);
> - if (!strcmp(pmu->name, INTEL_BTS_PMU_NAME))
> + }
> + if (!strcmp(pmu->name, INTEL_BTS_PMU_NAME)) {
> + pmu->auxtrace = true;
> pmu->selectable = true;
> + }
> #endif
> return NULL;
> }
>
>
>
>>
>>> 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);
>>
>
Powered by blists - more mailing lists