lkml.org 
[lkml]   [2023]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 03/10] perf record: Early auxtrace initialization before event parsing
    From


    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);
    >>
    >

    \
     
     \ /
      Last update: 2023-03-27 00:45    [W:4.769 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site