Re: [PATCH v2 5/6] perf evsel: Allow evsel__newtp without libtraceevent

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Sat, Nov 02, 2024 at 09:53:59AM -0700, Ian Rogers wrote:
> Switch from reading the tracepoint format to reading the id directly
> for the evsel config. This avoids the need to initialize
> libtraceevent, plugins, etc. It is sufficient for many tracepoint
> commands to work like:
> $ perf stat -e sched:sched_switch true

root@x1:~# perf check feature libtraceevent
         libtraceevent: [ OFF ]  # HAVE_LIBTRACEEVENT
root@x1:~# perf stat -e sched:sched_switch -I 1000
#           time             counts unit events
     1.001071607              4,631      sched:sched_switch                                                    
     2.004003548              5,138      sched:sched_switch                                                    
     3.006847313              4,967      sched:sched_switch                                                    
^C     3.944095456              5,553      sched:sched_switch                                                    

root@x1:~#

Works:

Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
 
> To populate evsel->tp_format, do lazy initialization using
> libtraceevent in the evsel__tp_format function (the sys and name are
> saved in evsel__newtp_idx for this purpose). Reading the id should be
> indicative of the format failing to load, but if not an error is
> reported in evsel__tp_format. This could happen for a tracepoint with
> a format that fails to parse.
> 
> As tracepoints can be parsed without libtraceevent with this, remove
> the associated #ifdefs in parse-events.c.
> 
> By only lazily parsing the tracepoint format information it is hoped
> this will help improve the performance of code using tracepoints but
> not the format information. It also cuts down on the build and ifdef
> logic.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c        | 103 ++++++++++++++++++++++++---------
>  tools/perf/util/evsel.h        |  14 ++---
>  tools/perf/util/parse-events.c |  16 +----
>  3 files changed, 82 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a95db7e900d5..56e19e32a852 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -543,54 +543,101 @@ struct evsel *evsel__clone(struct evsel *orig)
>  	return NULL;
>  }
>  
> +static int trace_event__id(const char *sys, const char *name)
> +{
> +	char *tp_dir = get_events_file(sys);
> +	char path[PATH_MAX];
> +	int id, err;
> +
> +	if (!tp_dir)
> +		return -1;
> +
> +	scnprintf(path, PATH_MAX, "%s/%s/id", tp_dir, name);
> +	put_events_file(tp_dir);
> +	err = filename__read_int(path, &id);
> +	if (err)
> +		return err;
> +
> +	return id;
> +}
> +
>  /*
>   * Returns pointer with encoded error via <linux/err.h> interface.
>   */
> -#ifdef HAVE_LIBTRACEEVENT
>  struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx, bool format)
>  {
> +	struct perf_event_attr attr = {
> +		.type	       = PERF_TYPE_TRACEPOINT,
> +		.sample_type   = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |
> +				PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD),
> +	};
>  	struct evsel *evsel = zalloc(perf_evsel__object.size);
> -	int err = -ENOMEM;
> +	int err = -ENOMEM, id = -1;
>  
> -	if (evsel == NULL) {
> +	if (evsel == NULL)
>  		goto out_err;
> -	} else {
> -		struct perf_event_attr attr = {
> -			.type	       = PERF_TYPE_TRACEPOINT,
> -			.sample_type   = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |
> -					  PERF_SAMPLE_CPU | PERF_SAMPLE_PERIOD),
> -		};
>  
> -		if (asprintf(&evsel->name, "%s:%s", sys, name) < 0)
> -			goto out_free;
>  
> -		event_attr_init(&attr);
> +	if (asprintf(&evsel->name, "%s:%s", sys, name) < 0)
> +		goto out_free;
>  
> -		if (format) {
> -			evsel->tp_format = trace_event__tp_format(sys, name);
> -			if (IS_ERR(evsel->tp_format)) {
> -				err = PTR_ERR(evsel->tp_format);
> -				evsel->tp_format = NULL;
> -				goto out_free;
> -			}
> -			attr.config = evsel->tp_format->id;
> -		} else {
> -			attr.config = (__u64) -1;
> -		}
> +#ifdef HAVE_LIBTRACEEVENT
> +	evsel->tp_sys = strdup(sys);
> +	if (!evsel->tp_sys)
> +		goto out_free;
>  
> +	evsel->tp_name = strdup(name);
> +	if (!evsel->tp_name)
> +		goto out_free;
> +#endif
>  
> -		attr.sample_period = 1;
> -		evsel__init(evsel, &attr, idx);
> -	}
> +	event_attr_init(&attr);
>  
> +	if (format) {
> +		id = trace_event__id(sys, name);
> +		if (id < 0) {
> +			err = id;
> +			goto out_free;
> +		}
> +	}
> +	attr.config = (__u64)id;
> +	attr.sample_period = 1;
> +	evsel__init(evsel, &attr, idx);
>  	return evsel;
>  
>  out_free:
>  	zfree(&evsel->name);
> +#ifdef HAVE_LIBTRACEEVENT
> +	zfree(&evsel->tp_sys);
> +	zfree(&evsel->tp_name);
> +#endif
>  	free(evsel);
>  out_err:
>  	return ERR_PTR(err);
>  }
> +
> +#ifdef HAVE_LIBTRACEEVENT
> +struct tep_event *evsel__tp_format(struct evsel *evsel)
> +{
> +	struct tep_event *tp_format = evsel->tp_format;
> +
> +	if (tp_format)
> +		return tp_format;
> +
> +	if (evsel->core.attr.type != PERF_TYPE_TRACEPOINT)
> +		return NULL;
> +
> +	tp_format = trace_event__tp_format(evsel->tp_sys, evsel->tp_name);
> +	if (IS_ERR(tp_format)) {
> +		int err = -PTR_ERR(evsel->tp_format);
> +
> +		pr_err("Error getting tracepoint format '%s' '%s'(%d)\n",
> +			evsel__name(evsel), strerror(err), err);
> +		return NULL;
> +	}
> +	evsel->tp_format = tp_format;
> +	return evsel->tp_format;
> +}
>  #endif
>  
>  const char *const evsel__hw_names[PERF_COUNT_HW_MAX] = {
> @@ -1587,6 +1634,10 @@ void evsel__exit(struct evsel *evsel)
>  	perf_thread_map__put(evsel->core.threads);
>  	zfree(&evsel->group_name);
>  	zfree(&evsel->name);
> +#ifdef HAVE_LIBTRACEEVENT
> +	zfree(&evsel->tp_sys);
> +	zfree(&evsel->tp_name);
> +#endif
>  	zfree(&evsel->filter);
>  	zfree(&evsel->group_pmu_name);
>  	zfree(&evsel->unit);
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index c3e53d320bf5..93b6244ec302 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -59,6 +59,8 @@ struct evsel {
>  		char			*group_name;
>  		const char		*group_pmu_name;
>  #ifdef HAVE_LIBTRACEEVENT
> +		char			*tp_sys;
> +		char			*tp_name;
>  		struct tep_event	*tp_format;
>  #endif
>  		char			*filter;
> @@ -247,25 +249,17 @@ int copy_config_terms(struct list_head *dst, struct list_head *src);
>  void free_config_terms(struct list_head *config_terms);
>  
>  
> -#ifdef HAVE_LIBTRACEEVENT
> -struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx, bool format);
> -
>  /*
>   * Returns pointer with encoded error via <linux/err.h> interface.
>   */
> +struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx, bool format);
>  static inline struct evsel *evsel__newtp(const char *sys, const char *name)
>  {
>  	return evsel__newtp_idx(sys, name, 0, true);
>  }
>  
> -static inline struct tep_event *evsel__tp_format(struct evsel *evsel)
> -{
> -	return evsel->tp_format;
> -}
> -#endif
> -
>  #ifdef HAVE_LIBTRACEEVENT
> -struct tep_event *event_format__new(const char *sys, const char *name);
> +struct tep_event *evsel__tp_format(struct evsel *evsel);
>  #endif
>  
>  void evsel__init(struct evsel *evsel, struct perf_event_attr *attr, int idx);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index afeb8d815bbf..7fc1c36ef2a4 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -489,7 +489,6 @@ int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
>  	return found_supported ? 0 : -EINVAL;
>  }
>  
> -#ifdef HAVE_LIBTRACEEVENT
>  static void tracepoint_error(struct parse_events_error *e, int err,
>  			     const char *sys, const char *name, int column)
>  {
> @@ -644,7 +643,6 @@ static int add_tracepoint_multi_sys(struct parse_events_state *parse_state,
>  	closedir(events_dir);
>  	return ret;
>  }
> -#endif /* HAVE_LIBTRACEEVENT */
>  
>  size_t default_breakpoint_len(void)
>  {
> @@ -1066,7 +1064,6 @@ static int config_term_pmu(struct perf_event_attr *attr,
>  	return config_term_common(attr, term, err);
>  }
>  
> -#ifdef HAVE_LIBTRACEEVENT
>  static int config_term_tracepoint(struct perf_event_attr *attr,
>  				  struct parse_events_term *term,
>  				  struct parse_events_error *err)
> @@ -1111,7 +1108,6 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
>  
>  	return 0;
>  }
> -#endif
>  
>  static int config_attr(struct perf_event_attr *attr,
>  		       const struct parse_events_terms *head,
> @@ -1303,7 +1299,7 @@ int parse_events_add_tracepoint(struct parse_events_state *parse_state,
>  				struct parse_events_terms *head_config, void *loc_)
>  {
>  	YYLTYPE *loc = loc_;
> -#ifdef HAVE_LIBTRACEEVENT
> +
>  	if (head_config) {
>  		struct perf_event_attr attr;
>  
> @@ -1318,16 +1314,6 @@ int parse_events_add_tracepoint(struct parse_events_state *parse_state,
>  	else
>  		return add_tracepoint_event(parse_state, list, sys, event,
>  					    err, head_config, loc);
> -#else
> -	(void)parse_state;
> -	(void)list;
> -	(void)sys;
> -	(void)event;
> -	(void)head_config;
> -	parse_events_error__handle(err, loc->first_column, strdup("unsupported tracepoint"),
> -				strdup("libtraceevent is necessary for tracepoint support"));
> -	return -1;
> -#endif
>  }
>  
>  static int __parse_events_add_numeric(struct parse_events_state *parse_state,
> -- 
> 2.47.0.199.ga7371fff76-goog
>