Message-Id: <20170308124134.af4f89057ad9b21b5a883bf7@kernel.org>
Date:   Wed, 8 Mar 2017 12:41:34 +0100
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Cc:     mingo@...hat.com, acme@...nel.org, brendan.d.gregg@...il.com,
        peterz@...radead.org, alexander.shishkin@...ux.intel.com,
        wangnan0@...wei.com, jolsa@...nel.org, ak@...ux.intel.com,
        treeze.taeung@...il.com, mathieu.poirier@...aro.org,
        hekuang@...wei.com, sukadev@...ux.vnet.ibm.com, ananth@...ibm.com,
        naveen.n.rao@...ux.vnet.ibm.com, adrian.hunter@...el.com,
        linux-kernel@...r.kernel.org, hemant@...ux.vnet.ibm.com
Subject: Re: [PATCH v4 2/7] perf/sdt: Directly record SDT events with 'perf
 record'

Hi Ravi,

On Mon,  6 Mar 2017 18:42:59 +0530
Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com> wrote:

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index bc84a37..e87b19b 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -39,6 +39,7 @@
>  #include "util/trigger.h"
>  #include "util/perf-hooks.h"
>  #include "asm/bug.h"
> +#include "util/probe-file.h"
>  
>  #include <unistd.h>
>  #include <sched.h>
> @@ -73,6 +74,7 @@ struct record {
>  	bool			timestamp_filename;
>  	struct switch_output	switch_output;
>  	unsigned long long	samples;
> +	struct list_head	sdt_event_list;
>  };
>  
>  static volatile int auxtrace_record__snapshot_started;
> @@ -1503,6 +1505,26 @@ static struct record record = {
>  	},
>  };
>  
> +void sdt_event_list__add(struct list_head *sdt_event_list)
> +{
> +	if (list_empty(sdt_event_list))
> +		return;
> +	list_splice(sdt_event_list, &record.sdt_event_list);
> +}
> +
> +bool is_cmd_record(void)
> +{
> +	return (record.evlist != NULL);
> +}

Hmm, exporting this from builtin-record.c makes inverted reference from
builtin-record.o to libperf. I think it is not good way to do.

[...]
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 54355d3..1fcc9d13 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1727,12 +1727,66 @@ static void parse_events_print_error(struct parse_events_error *err,
>  
>  #undef MAX_WIDTH
>  
> +/* SDT event needs LIBELF support for creating a probe point */
> +#ifdef HAVE_LIBELF_SUPPORT
> +static int parse_sdt_event(struct perf_evlist *evlist, const char *str,
> +			   struct parse_events_error *err)
> +{
> +	char *ptr = NULL;
> +	int ret;
> +	struct list_head *sdt_evlist;
> +	struct sdt_event_list *sdt_event;
> +
> +	if (str[0] == '%')
> +		str++;
> +
> +	ptr = strdup(str);
> +	if (ptr == NULL)
> +		return -ENOMEM;
> +
> +	sdt_evlist = zalloc(sizeof(*sdt_evlist));
> +	if (!sdt_evlist) {
> +		free(ptr);
> +		pr_debug("Error in sdt_evlist memory allocation\n");
> +		return -ENOMEM;
> +	}
> +	INIT_LIST_HEAD(sdt_evlist);
> +
> +	/*
> +	 * If there is an error in this call, no need to free
> +	 * up sdt_evlist, its already free'ed up in the previous
> +	 * call. Free up 'ptr' though.
> +	 */
> +	ret = add_sdt_event(ptr, sdt_evlist);
> +	if (!ret) {
> +		list_for_each_entry(sdt_event, sdt_evlist, list) {
> +			ret = parse_events(evlist, sdt_event->name, err);
> +			if (ret < 0)
> +				goto ret;
> +		}
> +		/* Add it to the record struct */
> +		sdt_event_list__add(sdt_evlist);
> +	}
> +
> +ret:
> +	free(ptr);
> +	return ret;
> +}
> +#endif /* HAVE_LIBELF_SUPPORT */
> +
>  int parse_events_option(const struct option *opt, const char *str,
>  			int unset __maybe_unused)
>  {
>  	struct perf_evlist *evlist = *(struct perf_evlist **)opt->value;
>  	struct parse_events_error err = { .idx = 0, };
> -	int ret = parse_events(evlist, str, &err);
> +	int ret = 0;
> +
> +#ifdef HAVE_LIBELF_SUPPORT
> +	if (is_sdt_event((char *)str) && is_cmd_record())
> +		ret = parse_sdt_event(evlist, str, &err);
> +	else
> +#endif

To avoid these ifdefs, could you add a dummy (error return) function
of add_sdt_event() for !HAVE_LIBELF_SUPPORT case in probe-event.h?



> +		ret = parse_events(evlist, str, &err);
>  
>  	if (ret)
>  		parse_events_print_error(&err, str);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index c6172cd..0887269 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -208,4 +208,6 @@ static inline bool is_sdt_event(char *str)
>  		(!strncmp(str, "sdt_", 4) &&
>  		 !!strchr(str, ':') && !strchr(str, '=')));
>  }
> +
> +void sdt_event_list__add(struct list_head *sdt_event_list);
>  #endif /* __PERF_PARSE_EVENTS_H */
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 2b1409f..b879076 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1293,7 +1293,7 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
>  	return err;
>  }
>  
> -static int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev)
> +int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev)
>  {
>  	char *ptr;
>  
> @@ -3125,8 +3125,8 @@ static int find_cached_events(struct perf_probe_event *pev,
>  }
>  
>  /* Try to find probe_trace_event from all probe caches */
> -static int find_cached_events_all(struct perf_probe_event *pev,
> -				   struct probe_trace_event **tevs)
> +int find_cached_events_all(struct perf_probe_event *pev,
> +			   struct probe_trace_event **tevs)
>  {
>  	struct probe_trace_event *tmp_tevs = NULL;
>  	struct strlist *bidlist;
> @@ -3476,3 +3476,32 @@ int copy_to_probe_trace_arg(struct probe_trace_arg *tvar,
>  		tvar->name = NULL;
>  	return 0;
>  }
> +
> +/*
> + * Record session for SDT events has ended. Delete the SDT events
> + * from uprobe_events file that were created initially.
> + */
> +void remove_sdt_event_list(struct list_head *sdt_events)
> +{
> +	struct sdt_event_list *sdt_event;
> +	struct strfilter *filter = NULL;
> +	const char *err = NULL;
> +
> +	if (list_empty(sdt_events))
> +		return;
> +
> +	list_for_each_entry(sdt_event, sdt_events, list) {
> +		if (!filter) {
> +			filter = strfilter__new(sdt_event->name, &err);
> +			if (!filter)
> +				goto free_list;
> +		} else {
> +			strfilter__or(filter, sdt_event->name, &err);
> +		}
> +	}
> +
> +	del_perf_probe_events(filter);
> +
> +free_list:
> +	free_sdt_list(sdt_events);
> +}
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 5d4e940..91e277e 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -182,4 +182,8 @@ struct map *get_target_map(const char *target, bool user);
>  void arch__post_process_probe_trace_events(struct perf_probe_event *pev,
>  					   int ntevs);
>  
> +int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev);
> +
> +int find_cached_events_all(struct perf_probe_event *pev,
> +			   struct probe_trace_event **tevs);
>  #endif /*_PROBE_EVENT_H */
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 1a62dac..c1cf67f 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -27,8 +27,10 @@
>  #include "probe-event.h"
>  #include "probe-file.h"
>  #include "session.h"
> +#include "probe-finder.h"
>  
>  #define MAX_CMDLEN 256
> +#define MAX_EVENT_LENGTH 512
>  
>  static void print_open_warning(int err, bool uprobe)
>  {
> @@ -935,3 +937,132 @@ bool probe_type_is_available(enum probe_type type)
>  
>  	return ret;
>  }
> +
> +void free_sdt_list(struct list_head *sdt_evlist)
> +{
> +	struct sdt_event_list *tmp, *ptr;
> +
> +	if (list_empty(sdt_evlist))
> +		return;
> +	list_for_each_entry_safe(tmp, ptr, sdt_evlist, list) {
> +		list_del(&tmp->list);
> +		free(tmp->name);
> +		free(tmp);
> +	}
> +}
> +
> +static int get_sdt_events_from_cache(struct perf_probe_event *pev)
> +{
> +	int ret = 0;
> +
> +	pev->ntevs = find_cached_events_all(pev, &pev->tevs);
> +
> +	if (pev->ntevs < 0) {
> +		pr_err("Error: Cache lookup failed (code: %d)\n", pev->ntevs);
> +		ret = pev->ntevs;
> +	} else if (!pev->ntevs) {
> +		pr_err("Error: %s:%s not found in the cache\n",
> +			pev->group, pev->event);
> +		ret = -EINVAL;
> +	} else if (pev->ntevs > 1) {
> +		pr_warning("Warning : Recording on %d occurences of %s:%s\n",
> +			   pev->ntevs, pev->group, pev->event);
> +	}
> +
> +	return ret;
> +}
> +
> +static int add_event_to_sdt_evlist(struct probe_trace_event *tev,
> +				   struct list_head *sdt_evlist)
> +{
> +	struct sdt_event_list *tmp;
> +
> +	tmp = zalloc(sizeof(*tmp));
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&tmp->list);
> +	tmp->name = zalloc(MAX_EVENT_LENGTH * sizeof(char));
> +	if (!tmp->name)
> +		return -ENOMEM;
> +
> +	snprintf(tmp->name, strlen(tev->group) + strlen(tev->event) + 2,
> +		 "%s:%s", tev->group, tev->event);
> +	list_add(&tmp->list, sdt_evlist);
> +
> +	return 0;
> +}
> +
> +static int add_events_to_sdt_evlist(struct perf_probe_event *pev,
> +				    struct list_head *sdt_evlist)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < pev->ntevs; i++) {
> +		ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist);
> +
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Find the SDT event from the cache and if found add it/them
> + * to the uprobe_events file
> + */
> +int add_sdt_event(char *event, struct list_head *sdt_evlist)
> +{
> +	struct perf_probe_event *pev;
> +	int ret;
> +
> +	pev = zalloc(sizeof(*pev));
> +	if (!pev)
> +		return -ENOMEM;
> +
> +	pev->sdt = true;
> +	pev->uprobes = true;
> +
> +	/*
> +	 * Parse event to find the group name and event name of
> +	 * the sdt event.
> +	 */
> +	ret = parse_perf_probe_event_name(&event, pev);
> +	if (ret) {
> +		pr_err("Error in parsing sdt event %s\n", event);
> +		free(pev);
> +		return ret;
> +	}
> +
> +	probe_conf.max_probes = MAX_PROBES;
> +	probe_conf.force_add = 1;
> +
> +	/* Fetch all matching events from cache. */
> +	ret = get_sdt_events_from_cache(pev);
> +	if (ret < 0)
> +		goto free_pev;
> +
> +	/*
> +	 * Create probe point for all events by adding them in
> +	 * uprobe_events file
> +	 */
> +	ret = apply_perf_probe_events(pev, 1);
> +	if (ret) {
> +		pr_err("Error in adding SDT event : %s\n", event);
> +		goto free_pev;
> +	}
> +
> +	/* Add events to sdt_evlist */
> +	ret = add_events_to_sdt_evlist(pev, sdt_evlist);
> +	if (ret < 0)
> +		goto free_pev;
> +
> +	ret = 0;
> +
> +free_pev:
> +	if (ret < 0)
> +		free_sdt_list(sdt_evlist);
> +	cleanup_perf_probe_events(pev, 1);
> +	free(pev);
> +	return ret;
> +}

Hmm, why would you put this function in probe-file.c? I would like to
ask you move this kind of functions which operates perf_probe_event
in probe-event.c.

Thanks,

> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index a17a82e..6d2d3e5 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -19,6 +19,11 @@ struct probe_cache {
>  	struct list_head entries;
>  };
>  
> +struct sdt_event_list {
> +	char *name;		/* group:event */
> +	struct list_head list;
> +};
> +
>  enum probe_type {
>  	PROBE_TYPE_U = 0,
>  	PROBE_TYPE_S,
> @@ -65,6 +70,9 @@ struct probe_cache_entry *probe_cache__find_by_name(struct probe_cache *pcache,
>  					const char *group, const char *event);
>  int probe_cache__show_all_caches(struct strfilter *filter);
>  bool probe_type_is_available(enum probe_type type);
> +int add_sdt_event(char *event, struct list_head *sdt_event_list);
> +void remove_sdt_event_list(struct list_head *sdt_event_list);
> +void free_sdt_list(struct list_head *sdt_events);
>  #else	/* ! HAVE_LIBELF_SUPPORT */
>  static inline struct probe_cache *probe_cache__new(const char *tgt __maybe_unused)
>  {
> -- 
> 2.9.3
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists