diff mbox series

[v1,03/10] perf record: Early auxtrace initialization before event parsing

Message ID 20230302041211.852330-4-irogers@google.com
State New
Headers
Series Better fixes for grouping of events |

Commit Message

Ian Rogers March 2, 2023, 4:12 a.m. UTC 
  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(-)

Comments

Arnaldo Carvalho de Melo March 2, 2023, 2:32 p.m. UTC | #1 
Em Wed, Mar 01, 2023 at 08:12:04PM -0800, Ian Rogers escreveu:
> 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;

In this case, can't we do it as:

	if (intel_pt_pmu == NULL && intel_bts_pmu == NULL)
		auxtrace__early_init();

To avoid possibly doing the finds again?

- Arnaldo

>  	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();
> +
>  	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);
> -- 
> 2.39.2.722.g9855ee24e9-goog
>
Ian Rogers March 2, 2023, 4:05 p.m. UTC | #2 
On Thu, Mar 2, 2023 at 6:32 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Wed, Mar 01, 2023 at 08:12:04PM -0800, Ian Rogers escreveu:
> > 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;
>
> In this case, can't we do it as:
>
>         if (intel_pt_pmu == NULL && intel_bts_pmu == NULL)
>                 auxtrace__early_init();
>
> To avoid possibly doing the finds again?
>
> - Arnaldo

So this code is called after parse events and has the evlist. The
early init code is called before parse events so that the aux trace
PMUs are flagged for the sake of the is_aux call in evsel__pmu_name,
as aux events must be grouped with their different PMU type leader. It
won't be possible to combine the calls but I plan to look at
restructuring how PMUs are accessed so that searches are less
necessary.

Thanks,
Ian

> >       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();
> > +
> >       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);
> > --
> > 2.39.2.722.g9855ee24e9-goog
> >
>
> --
>
> - Arnaldo
diff mbox series

Patch

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