From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753081AbdCHMhJ (ORCPT ); Wed, 8 Mar 2017 07:37:09 -0500 Received: from mail.kernel.org ([198.145.29.136]:36058 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751720AbdCHMhC (ORCPT ); Wed, 8 Mar 2017 07:37:02 -0500 Date: Wed, 8 Mar 2017 12:41:34 +0100 From: Masami Hiramatsu To: Ravi Bangoria Cc: mingo@redhat.com, acme@kernel.org, brendan.d.gregg@gmail.com, peterz@infradead.org, alexander.shishkin@linux.intel.com, wangnan0@huawei.com, jolsa@kernel.org, ak@linux.intel.com, treeze.taeung@gmail.com, mathieu.poirier@linaro.org, hekuang@huawei.com, sukadev@linux.vnet.ibm.com, ananth@in.ibm.com, naveen.n.rao@linux.vnet.ibm.com, adrian.hunter@intel.com, linux-kernel@vger.kernel.org, hemant@linux.vnet.ibm.com Subject: Re: [PATCH v4 2/7] perf/sdt: Directly record SDT events with 'perf record' Message-Id: <20170308124134.af4f89057ad9b21b5a883bf7@kernel.org> In-Reply-To: <20170306131304.1625-3-ravi.bangoria@linux.vnet.ibm.com> References: <20170306131304.1625-1-ravi.bangoria@linux.vnet.ibm.com> <20170306131304.1625-3-ravi.bangoria@linux.vnet.ibm.com> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ravi, On Mon, 6 Mar 2017 18:42:59 +0530 Ravi Bangoria 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 > #include > @@ -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