Messages in this thread | | | Date | Mon, 6 Mar 2023 09:10:14 -0500 | Subject | Re: [PATCH v2 03/10] perf record: Early auxtrace initialization before event parsing | From | "Liang, Kan" <> |
| |
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@google.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); >> >
| |