[PATCH v5 0/7] perf/sdt: Directly record SDT events with 'perf record'

[PATCH v5 0/7] perf/sdt: Directly record SDT events with 'perf record'

[Ravi Bangoria]

All events from 'perf list', except SDT events, can be directly recorded
with 'perf record'. But, the flow is little different for SDT events.
Probe point for SDT event needs to be created using 'perf probe' before
recording it using 'perf record'.

As suggested by Ingo[1], it's better to make this process simple by
creating probe point automatically with 'perf record' for SDT events.

Features:
  - Allow both 'perf probe' and 'perf record' on sdt events without
    changing current functionality.

  - Event starting with 'sdt_' or '%' will be considered as SDT event.

  - Always prioritize events from uprobe_events by first checking if
    event exists with exact name. If not found and user has used
    pattern, again try to find pattern matching entries from
    uprobe_events. If found use them. If not, lookup into probe-cache.
    If events found from probe-cache, again check if any event exists
    in uprobe_events by matching filepath+address, as it might exists
    in uprobe_events but with different name. Reuse those events which
    exists in uprobe_events and create new entries for missing one.
    Also maintain list for new entries being created and at the end
    of the session, delete them.

  - Show various warnings/hints to help user understand _which_ events
    are being recorded and _why_. For ex,

    When multiple events of same name found and all are being recorded:

      $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
        Warning: Recording on 2 occurrences of sdt_libpthread:mutex_entry

    Events being reused from uprobe_events is listed as 'name addr@file'
    followed by hint on how to delete them:

      $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
        Matching event(s) from uprobe_events:
          sdt_libpthread:mutex_entry  0x9ddb@/usr/lib64/libpthread-2.24.so
        Use 'perf probe -d <event>' to delete event(s).

    If number of events found from cache is not equal to number of events
    being recorded:

      $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
        Warning: Found 2 events from probe-cache with name 'sdt_libpthread:mutex_entry'.
                 Since 1 probe point already exists, recording only it.
        Hint: Please use 'perf probe -d sdt_libpthread:mutex_entry' to allow record on all events.

  - If all events found from probe-cache are not present in uprobe_events,
    and user has used pattern to specify event, perf will record only
    those events which are present in uprobe_events. This is to make perf
    semantics consistent across normal and SDT events. And If user has
    not used pattern, perf will record all events found from probe-cache
    by reusing name for existing one and adding entries for missing one.
    For ex,

      $ sudo ./perf probe sdt_libpthread:mutex_release
        Added new events:
          sdt_libpthread:mutex_release (on %mutex_release in /usr/lib64/libpthread-2.24.so)
          sdt_libpthread:mutex_release_1 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
          sdt_libpthread:mutex_release_2 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
          sdt_libpthread:mutex_release_3 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
      $ sudo ./perf probe -d sdt_libpthread:mutex_release
      $ sudo ./perf probe -d sdt_libpthread:mutex_release_2

      $ sudo ./perf record -a -e sdt_libpthread:mutex_release*
        Warning: Recording on 2 occurrences of sdt_libpthread:mutex_release*

      $ sudo ./perf record -a -e sdt_libpthread:mutex_release
        Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release

Changes in v5:
  - Patch 2/7 is new. New option introduced in this patch helps to pass
    custome data from builtin-*.c to libperf.

  - All direct callbacks from libelf to builtin-record.c is removed.
 
  - Merged 2nd and 4th patch of v4 into patch 2 of v5.

  - Moved all functions from util/probe-file.c to util/probe-event.c
    which operates on perf_probe_event.

  - Made free_sdt_list() static as it's only used inside util/probe-event.c.

  - Couple of other changes as Masami has suggested in v4 review.

Note: Patchset is prepared on top of acme/perf/core.

v4 link: https://lkml.org/lkml/2017/3/6/565

[1] https://lkml.org/lkml/2017/2/7/59
[2] https://lkml.org/lkml/2016/5/3/810


Hemant Kumar (1):
  perf/sdt: Directly record SDT events with 'perf record'

Ravi Bangoria (6):
  perf/sdt: Introduce util func is_sdt_event()
  perf tool: Add option macro OPT_CALLBACK_ARG
  perf/sdt: Allow recording of existing events
  perf/sdt: Warn when number of events recorded are not equal to cached
    events
  perf/sdt: List events fetched from uprobe_events
  perf/sdt: List events fetched from uprobe_events

 tools/lib/api/fs/tracing_path.c  |  17 +-
 tools/lib/subcmd/parse-options.h |   4 +
 tools/perf/builtin-record.c      |  27 ++-
 tools/perf/util/parse-events.c   |  60 +++++++
 tools/perf/util/parse-events.h   |  23 +++
 tools/perf/util/probe-event.c    | 362 +++++++++++++++++++++++++++++++++++++--
 tools/perf/util/probe-event.h    |  16 ++
 tools/perf/util/probe-file.c     |  48 ++++++
 tools/perf/util/probe-file.h     |   1 +
 9 files changed, 527 insertions(+), 31 deletions(-)

-- 
2.9.3

[PATCH v5 1/7] perf/sdt: Introduce util func is_sdt_event()

[Ravi Bangoria]

Factor out the SDT event name checking routine as is_sdt_event().

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/parse-events.h | 20 ++++++++++++++++++++
 tools/perf/util/probe-event.c  |  9 +--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 1af6a26..8c72b0f 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -8,6 +8,7 @@
 #include <stdbool.h>
 #include <linux/types.h>
 #include <linux/perf_event.h>
+#include <string.h>
 
 struct list_head;
 struct perf_evsel;
@@ -196,4 +197,23 @@ int is_valid_tracepoint(const char *event_string);
 int valid_event_mount(const char *eventfs);
 char *parse_events_formats_error_string(char *additional_terms);
 
+#ifdef HAVE_LIBELF_SUPPORT
+/*
+ * If the probe point starts with '%',
+ * or starts with "sdt_" and has a ':' but no '=',
+ * then it should be a SDT/cached probe point.
+ */
+static inline bool is_sdt_event(char *str)
+{
+	return (str[0] == '%' ||
+		(!strncmp(str, "sdt_", 4) &&
+		 !!strchr(str, ':') && !strchr(str, '=')));
+}
+#else
+static inline bool is_sdt_event(char *str __maybe_unused)
+{
+	return false;
+}
+#endif /* HAVE_LIBELF_SUPPORT */
+
 #endif /* __PERF_PARSE_EVENTS_H */
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 28fb62c..2b1409f 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1339,14 +1339,7 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
 	if (!arg)
 		return -EINVAL;
 
-	/*
-	 * If the probe point starts with '%',
-	 * or starts with "sdt_" and has a ':' but no '=',
-	 * then it should be a SDT/cached probe point.
-	 */
-	if (arg[0] == '%' ||
-	    (!strncmp(arg, "sdt_", 4) &&
-	     !!strchr(arg, ':') && !strchr(arg, '='))) {
+	if (is_sdt_event(arg)) {
 		pev->sdt = true;
 		if (arg[0] == '%')
 			arg++;
-- 
2.9.3

[PATCH v5 2/7] perf tool: Add option macro OPT_CALLBACK_ARG

[Ravi Bangoria]

Add an option macro that is the same as OPT_CALLBACK_OPTARG except
that the argument is not optional.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/lib/subcmd/parse-options.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/lib/subcmd/parse-options.h b/tools/lib/subcmd/parse-options.h
index f054ca1..79472e0 100644
--- a/tools/lib/subcmd/parse-options.h
+++ b/tools/lib/subcmd/parse-options.h
@@ -160,6 +160,10 @@ struct option {
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), \
 	  .value = (v), .argh = (a), .help = (h), .callback = (f), \
 	  .flags = PARSE_OPT_OPTARG, .data = (d) }
+#define OPT_CALLBACK_ARG(s, l, v, d, a, h, f) \
+	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), \
+	  .value = (v), .argh = (a), .help = (h), .callback = (f), \
+	  .data = (d) }
 
 /* parse_options() will filter out the processed options and leave the
  * non-option argments in argv[].
-- 
2.9.3

[PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'

[Ravi Bangoria]

From: Hemant Kumar <hemant@linux.vnet.ibm.com>

Add basic support for directly recording SDT events which are present
in the probe cache. Without this patch, we could probe into SDT events
using 'perf probe' and 'perf record'. With this patch, we can probe the
SDT events directly using 'perf record'.

After invoking 'perf record', behind the scenes, it checks whether the
event specified is an SDT event using the string 'sdt_' or flag '%'.
After that, it does a lookup of the probe cache to find out the SDT
event. If its not present, it throws an error. Otherwise, it goes on
and writes the event into the uprobe_events file and starts recording.
It also maintains a list of the event names that were written to
uprobe_events file. At the end of the record session, it removes the
events from the uprobe_events file using the maintained name list.

For example:

  $ sudo ./perf list sdt
    sdt_libpthread:mutex_entry                         [SDT event]
    sdt_libc:setjmp                                    [SDT event]

  $ sudo ./perf record -a -e sdt_libc:setjmp

  $ sudo ./perf script
         bash   793 [002]   260.382957: sdt_libc:setjmp: (7ff85b6596a1)
        reset  1296 [000]   260.511983: sdt_libc:setjmp: (7f26862e06a1)

Recording on SDT events with same provider and marker names is also
supported:

  $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider
      Provider: libpthread
      Name: mutex_entry
      Location: 0x0000000000009ddb, Base: 0x00000000000139cc, ...
    --
      Provider: libpthread
      Name: mutex_entry
      Location: 0x000000000000bcbb, Base: 0x00000000000139cc, ...

  $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
    Warning : Recording on 2 occurences of sdt_libpthread:mutex_entry

  $ sudo ./perf evlist
    sdt_libpthread:mutex_entry_1
    sdt_libpthread:mutex_entry

Note that, it always fetch sdt events in probe cache and ignores
entries of uprobe_events file. Hence, it creates new probe points
for event even if it already exists in uprobe_events.

  $ sudo ./perf probe sdt_libpthread:mutex_entry
    Added new events:
      sdt_libpthread:mutex_entry (on %mutex_entry in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_entry_1 (on %mutex_entry in /usr/lib64/libpthread-2.24.so)

  $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
    Warning : Recording on 2 occurences of sdt_libpthread:mutex_entry

  $ sudo ./perf evlist
    sdt_libpthread:mutex_entry_3
    sdt_libpthread:mutex_entry_2

As it does not look at uprobe_events file, it can't record those events
whose probe points are created with different name. For ex,

  $ sudo ./perf record -a -e sdt_libpthread:mutex_entry_1
    Error: sdt_libpthread:mutex_entry_1 not found in the cache
    invalid or unsupported event: 'sdt_libpthread:mutex_entry_1'

Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/builtin-record.c    |  27 ++++++-
 tools/perf/util/parse-events.c |  60 +++++++++++++++
 tools/perf/util/parse-events.h |   3 +
 tools/perf/util/probe-event.c  | 165 ++++++++++++++++++++++++++++++++++++++++-
 tools/perf/util/probe-event.h  |  12 +++
 5 files changed, 261 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index bc84a37..e8e1f73 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -73,6 +73,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 +1504,23 @@ static struct record record = {
 	},
 };
 
+static int record__parse_events_option(const struct option *opt,
+				       const char *str,
+				       int unset)
+{
+	if (is_sdt_event((char *) str))
+		return parse_sdt_events_option(opt, str, unset);
+	else
+		return parse_events_option(opt, str, unset);
+}
+
+static void sdt_event_list__remove(void)
+{
+#ifdef HAVE_LIBELF_SUPPORT
+	remove_sdt_event_list(&record.sdt_event_list);
+#endif
+}
+
 const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
 	"\n\t\t\t\tDefault: fp";
 
@@ -1516,9 +1534,10 @@ static bool dry_run;
  * using pipes, etc.
  */
 static struct option __record_options[] = {
-	OPT_CALLBACK('e', "event", &record.evlist, "event",
-		     "event selector. use 'perf list' to list available events",
-		     parse_events_option),
+	OPT_CALLBACK_ARG('e', "event", &record.evlist,
+			 &record.sdt_event_list, "event",
+			 "event selector. use 'perf list' to list available events",
+			 record__parse_events_option),
 	OPT_CALLBACK(0, "filter", &record.evlist, "filter",
 		     "event filter", parse_filter),
 	OPT_CALLBACK_NOOPT(0, "exclude-perf", &record.evlist,
@@ -1671,6 +1690,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (rec->evlist == NULL)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&rec->sdt_event_list);
 	err = perf_config(perf_record_config, rec);
 	if (err)
 		return err;
@@ -1841,6 +1861,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	perf_evlist__delete(rec->evlist);
 	symbol__exit();
 	auxtrace_record__free(rec->itr);
+	sdt_event_list__remove();
 	return err;
 }
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 54355d3..252dac1 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1727,6 +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
+int parse_sdt_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, };
+	char *ptr = NULL;
+	int ret = 0;
+	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 */
+		list_splice(sdt_evlist, opt->data);
+	}
+
+ret:
+	if (ret) {
+		remove_sdt_event_list(opt->data);
+		parse_events_print_error(&err, str);
+	}
+	free(ptr);
+	return ret;
+}
+#else
+int parse_sdt_events_option(const struct option *opt __maybe_unused,
+			    const char *str __maybe_unused,
+			    int unset __maybe_unused)
+{
+	return -1;
+}
+#endif /* HAVE_LIBELF_SUPPORT */
+
 int parse_events_option(const struct option *opt, const char *str,
 			int unset __maybe_unused)
 {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 8c72b0f..8e29cb6 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -197,6 +197,9 @@ int is_valid_tracepoint(const char *event_string);
 int valid_event_mount(const char *eventfs);
 char *parse_events_formats_error_string(char *additional_terms);
 
+int parse_sdt_events_option(const struct option *opt, const char *str,
+				int unset);
+
 #ifdef HAVE_LIBELF_SUPPORT
 /*
  * If the probe point starts with '%',
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 2b1409f..f725953 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -49,6 +49,7 @@
 
 #define MAX_CMDLEN 256
 #define PERFPROBE_GROUP "probe"
+#define MAX_EVENT_LENGTH 512
 
 bool probe_event_dry_run;	/* Dry run flag */
 struct probe_conf probe_conf;
@@ -1293,7 +1294,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 +3126,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 +3477,161 @@ int copy_to_probe_trace_arg(struct probe_trace_arg *tvar,
 		tvar->name = NULL;
 	return 0;
 }
+
+static 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);
+	}
+}
+
+/*
+ * 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);
+}
+
+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;
+}
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index 5d4e940..6812230 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -113,6 +113,12 @@ struct variable_list {
 	struct strlist			*vars;	/* Available variables */
 };
 
+/* List of sdt events */
+struct sdt_event_list {
+	struct list_head list;
+	char *name;		/* group:event */
+};
+
 struct map;
 int init_probe_symbol_maps(bool user_only);
 void exit_probe_symbol_maps(void);
@@ -182,4 +188,10 @@ 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);
+int add_sdt_event(char *event, struct list_head *sdt_event_list);
+void remove_sdt_event_list(struct list_head *sdt_event_list);
 #endif /*_PROBE_EVENT_H */
-- 
2.9.3

[PATCH v5 4/7] perf/sdt: Allow recording of existing events

[Ravi Bangoria]

Add functionality to fetch matching events from uprobe_events. If no
events are fourd from it, fetch matching events from probe-cache and
add them in uprobe_events. If all events are already present in
uprobe_events, reuse them. If few of them are present, add entries
for missing events and record them. At the end of the record session,
delete newly added entries. Below is detailed algorithm that describe
implementation of this patch:

    arr1 = fetch all sdt events from uprobe_events

    if (event with exact name in arr1)
        add that in sdt_event_list
        return

    if (user has used pattern)
        if (pattern matching entries found from arr1)
            add those events in sdt_event_list
            return

    arr2 = lookup probe-cache
    if (arr2 empty)
        return

    ctr = 0
    foreach (compare entries of arr1 and arr2 using filepath+address)
        if (match)
            add event from arr1 to sdt_event_list
            ctr++
            if (!pattern used)
                remove entry from arr2

    if (!pattern used || ctr == 0)
        add all entries of arr2 in sdt_event_list

Example: Consider sdt event sdt_libpthread:mutex_release found in
/usr/lib64/libpthread-2.24.so.

  $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider
      Provider: libpthread
      Name: mutex_release
      Location: 0x000000000000b126, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
    --
      Provider: libpthread
      Name: mutex_release
      Location: 0x000000000000b2f6, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
    --
      Provider: libpthread
      Name: mutex_release
      Location: 0x000000000000b498, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
    --
      Provider: libpthread
      Name: mutex_release
      Location: 0x000000000000b596, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000

When no probepoint exists,

  $ sudo ./perf record -a -e sdt_libpthread:mutex_*
    Warning: Recording on 15 occurrences of sdt_libpthread:mutex_*

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release
    Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release
  $ sudo ./perf evlist
    sdt_libpthread:mutex_release_3
    sdt_libpthread:mutex_release_2
    sdt_libpthread:mutex_release_1
    sdt_libpthread:mutex_release

When probepoints already exists for all matching events,

  $ sudo ./perf probe sdt_libpthread:mutex_release
    Added new events:
      sdt_libpthread:mutex_release (on %mutex_release in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_release_1 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_release_2 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_release_3 (on %mutex_release in /usr/lib64/libpthread-2.24.so)

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release_1
  $ sudo ./perf evlist
    sdt_libpthread:mutex_release_1

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release
  $ sudo ./perf evlist
    sdt_libpthread:mutex_release

  $ sudo ./perf record -a -e sdt_libpthread:mutex_*
    Warning: Recording on 4 occurrences of sdt_libpthread:mutex_*
  $ sudo ./perf evlist
    sdt_libpthread:mutex_release_3
    sdt_libpthread:mutex_release_2
    sdt_libpthread:mutex_release_1
    sdt_libpthread:mutex_release

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release_*
    Warning: Recording on 3 occurrences of sdt_libpthread:mutex_release_*

When probepoints are partially exists,

  $ sudo ./perf probe -d sdt_libpthread:mutex_release
  $ sudo ./perf probe -d sdt_libpthread:mutex_release_2

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release
    Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release
  $ sudo ./perf evlist
    sdt_libpthread:mutex_release
    sdt_libpthread:mutex_release_3
    sdt_libpthread:mutex_release_2
    sdt_libpthread:mutex_release_1

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release*
    Warning: Recording on 2 occurrences of sdt_libpthread:mutex_release*
  $ sudo ./perf evlist
    sdt_libpthread:mutex_release_3
    sdt_libpthread:mutex_release_1

  $ sudo ./perf record -a -e sdt_libpthread:*
    Warning: Recording on 2 occurrences of sdt_libpthread:*
  $ sudo ./perf evlist
    sdt_libpthread:mutex_release_3
    sdt_libpthread:mutex_release_1

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 186 ++++++++++++++++++++++++++++++++++++------
 tools/perf/util/probe-event.h |   4 +
 tools/perf/util/probe-file.c  |  48 +++++++++++
 tools/perf/util/probe-file.h  |   1 +
 4 files changed, 214 insertions(+), 25 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index f725953..94b9105 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -232,7 +232,7 @@ static void clear_perf_probe_point(struct perf_probe_point *pp)
 	free(pp->lazy_line);
 }
 
-static void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
+void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
 {
 	int i;
 
@@ -3044,9 +3044,8 @@ static void *memcat(void *a, size_t sz_a, void *b, size_t sz_b)
 	return ret;
 }
 
-static int
-concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
-			  struct probe_trace_event **tevs2, int ntevs2)
+int concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
+			      struct probe_trace_event **tevs2, int ntevs2)
 {
 	struct probe_trace_event *new_tevs;
 	int ret = 0;
@@ -3505,6 +3504,9 @@ void remove_sdt_event_list(struct list_head *sdt_events)
 		return;
 
 	list_for_each_entry(sdt_event, sdt_events, list) {
+		if (sdt_event->exst)
+			continue;
+
 		if (!filter) {
 			filter = strfilter__new(sdt_event->name, &err);
 			if (!filter)
@@ -3514,7 +3516,8 @@ void remove_sdt_event_list(struct list_head *sdt_events)
 		}
 	}
 
-	del_perf_probe_events(filter);
+	if (filter)
+		del_perf_probe_events(filter);
 
 free_list:
 	free_sdt_list(sdt_events);
@@ -3533,16 +3536,14 @@ static int get_sdt_events_from_cache(struct perf_probe_event *pev)
 		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 list_head *sdt_evlist,
+				   bool exst)
 {
 	struct sdt_event_list *tmp;
 
@@ -3557,6 +3558,7 @@ static int add_event_to_sdt_evlist(struct probe_trace_event *tev,
 
 	snprintf(tmp->name, strlen(tev->group) + strlen(tev->event) + 2,
 		 "%s:%s", tev->group, tev->event);
+	tmp->exst = exst;
 	list_add(&tmp->list, sdt_evlist);
 
 	return 0;
@@ -3568,7 +3570,7 @@ static int add_events_to_sdt_evlist(struct perf_probe_event *pev,
 	int i, ret;
 
 	for (i = 0; i < pev->ntevs; i++) {
-		ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist);
+		ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist, false);
 
 		if (ret < 0)
 			return ret;
@@ -3576,14 +3578,133 @@ static int add_events_to_sdt_evlist(struct perf_probe_event *pev,
 	return 0;
 }
 
-/*
- * Find the SDT event from the cache and if found add it/them
- * to the uprobe_events file
- */
+static bool sdt_is_ptrn_used(struct perf_probe_event *pev)
+{
+	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);
+}
+
+static bool sdt_name_match(struct perf_probe_event *pev,
+			   struct probe_trace_event *tev)
+{
+	if (sdt_is_ptrn_used(pev))
+		return strglobmatch(tev->group, pev->group) &&
+			strglobmatch(tev->event, pev->event);
+
+	return !strcmp(tev->group, pev->group) &&
+		!strcmp(tev->event, pev->event);
+}
+
+static void sdt_warn_multi_events(int ctr, struct perf_probe_event *pev)
+{
+	pr_warning("Warning: Recording on %d occurrences of %s:%s\n",
+		   ctr, pev->group, pev->event);
+}
+
+static int sdt_event_probepoint_exists(struct perf_probe_event *pev,
+				       struct probe_trace_event *tevs,
+				       int ntevs,
+				       struct list_head *sdt_evlist)
+{
+	int i = 0, ret = 0, ctr = 0;
+
+	for (i = 0; i < ntevs; i++) {
+		if (sdt_name_match(pev, &tevs[i])) {
+			ret = add_event_to_sdt_evlist(&tevs[i],
+						sdt_evlist, true);
+			if (ret < 0)
+				return ret;
+
+			ctr++;
+		}
+	}
+
+	if (ctr > 1)
+		sdt_warn_multi_events(ctr, pev);
+
+	return ctr;
+}
+
+static bool sdt_file_addr_match(struct probe_trace_event *tev1,
+				struct probe_trace_event *tev2)
+{
+	return (tev1->point.address == tev2->point.address &&
+		!(strcmp(tev1->point.module, tev2->point.module)));
+}
+
+static void shift_sdt_events(struct perf_probe_event *pev, int i)
+{
+	int j = 0;
+
+	clear_probe_trace_event(&pev->tevs[i]);
+
+	if (i == pev->ntevs - 1)
+		goto out;
+
+	for (j = i; j < pev->ntevs - 1; j++)
+		memcpy(&pev->tevs[j], &pev->tevs[j + 1],
+		       sizeof(struct probe_trace_event));
+
+out:
+	pev->ntevs--;
+}
+
+static int sdt_merge_events(struct perf_probe_event *pev,
+			    struct probe_trace_event *exst_tevs,
+			    int exst_ntevs,
+			    struct list_head *sdt_evlist)
+{
+	int i, j, ret = 0, ctr = 0;
+	bool ptrn_used = sdt_is_ptrn_used(pev);
+
+	for (i = 0; i < pev->ntevs; i++) {
+		for (j = 0; j < exst_ntevs; j++) {
+			if (sdt_file_addr_match(&pev->tevs[i],
+						&exst_tevs[j])) {
+				ret = add_event_to_sdt_evlist(&exst_tevs[j],
+							  sdt_evlist, true);
+				if (ret < 0)
+					return ret;
+
+				if (!ptrn_used)
+					shift_sdt_events(pev, i);
+				ctr++;
+			}
+		}
+	}
+
+	if (!ptrn_used || ctr == 0) {
+		/*
+		 * Create probe point for all probe-cached events by
+		 * adding them in uprobe_events file.
+		 */
+		ret = apply_perf_probe_events(pev, 1);
+		if (ret < 0) {
+			pr_err("Error in adding SDT event: %s:%s\n",
+				pev->group, pev->event);
+			goto out;
+		}
+
+		/* Add events to sdt_evlist. */
+		ret = add_events_to_sdt_evlist(pev, sdt_evlist);
+		if (ret < 0) {
+			pr_err("Error while updating event list\n");
+			goto out;
+		}
+
+		ctr += pev->ntevs;
+		if (ctr > 1)
+			sdt_warn_multi_events(ctr, pev);
+	}
+
+out:
+	return ret;
+}
+
 int add_sdt_event(char *event, struct list_head *sdt_evlist)
 {
 	struct perf_probe_event *pev;
-	int ret;
+	int ret, exst_ntevs;
+	struct probe_trace_event *exst_tevs = NULL;
 
 	pev = zalloc(sizeof(*pev));
 	if (!pev)
@@ -3606,23 +3727,37 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
 	probe_conf.max_probes = MAX_PROBES;
 	probe_conf.force_add = 1;
 
+	/* Fetch all sdt events from uprobe_events */
+	exst_ntevs = probe_file__get_sdt_events(&exst_tevs);
+	if (exst_ntevs < 0) {
+		ret = exst_ntevs;
+		goto free_pev;
+	}
+
+	/* Check if events with same name already exists in uprobe_events. */
+	ret = sdt_event_probepoint_exists(pev, exst_tevs,
+					 exst_ntevs, sdt_evlist);
+	if (ret) {
+		ret = ret > 0 ? 0 : ret;
+		goto free_pev;
+	}
+
 	/* 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
+	 * Merge events found from uprobe_events with events found
+	 * from cache. Reuse events whose probepoint already exists
+	 * in uprobe_events, while add new entries for other events
+	 * in uprobe_events.
+	 *
+	 * This always tries to give first priority to events from
+	 * uprobe_events. By doing so, it ensures the existing
+	 * behaviour of perf remains same for sdt events too.
 	 */
-	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);
+	ret = sdt_merge_events(pev, exst_tevs, exst_ntevs, sdt_evlist);
 	if (ret < 0)
 		goto free_pev;
 
@@ -3632,6 +3767,7 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
 	if (ret < 0)
 		free_sdt_list(sdt_evlist);
 	cleanup_perf_probe_events(pev, 1);
+	clear_probe_trace_events(exst_tevs, exst_ntevs);
 	free(pev);
 	return ret;
 }
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index 6812230..fd8ec36 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -117,6 +117,7 @@ struct variable_list {
 struct sdt_event_list {
 	struct list_head list;
 	char *name;		/* group:event */
+	bool exst;		/* Even already exists in uprobe_events? */
 };
 
 struct map;
@@ -144,6 +145,7 @@ bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
 /* Release event contents */
 void clear_perf_probe_event(struct perf_probe_event *pev);
 void clear_probe_trace_event(struct probe_trace_event *tev);
+void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs);
 
 /* Command string to line-range */
 int parse_line_range_desc(const char *cmd, struct line_range *lr);
@@ -190,6 +192,8 @@ void arch__post_process_probe_trace_events(struct perf_probe_event *pev,
 
 int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev);
 
+int concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
+			      struct probe_trace_event **tevs2, int ntevs2);
 int find_cached_events_all(struct perf_probe_event *pev,
 			   struct probe_trace_event **tevs);
 int add_sdt_event(char *event, struct list_head *sdt_event_list);
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 1a62dac..9fb0a1f 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -310,6 +310,54 @@ int probe_file__get_events(int fd, struct strfilter *filter,
 	return ret;
 }
 
+/*
+ * Look into uprobe_events file and prepare list of sdt events
+ * whose probepoint is already registered.
+ */
+int probe_file__get_sdt_events(struct probe_trace_event **tevs)
+{
+	int fd, ret, ntevs = 0;
+	struct strlist *rawlist;
+	struct str_node *ent;
+	struct probe_trace_event *tev;
+
+	fd = probe_file__open(PF_FL_RW | PF_FL_UPROBE);
+	if (fd < 0)
+		return fd;
+
+	rawlist = probe_file__get_rawlist(fd);
+	strlist__for_each_entry(ent, rawlist) {
+		tev = zalloc(sizeof(struct probe_trace_event));
+		if (!tev) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		ret = parse_probe_trace_command(ent->s, tev);
+		if (ret < 0)
+			goto err;
+
+		if (strncmp(tev->group, "sdt_", 4)) {
+			/* Interested in SDT events only. */
+			free(tev);
+			continue;
+		}
+
+		ret = concat_probe_trace_events(tevs, &ntevs, &tev, 1);
+		if (ret < 0)
+			goto err;
+	}
+
+	close(fd);
+	return ntevs;
+
+err:
+	close(fd);
+	clear_probe_trace_events(*tevs, ntevs);
+	zfree(tevs);
+	return ret;
+}
+
 int probe_file__del_strlist(int fd, struct strlist *namelist)
 {
 	int ret = 0;
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index a17a82e..f696e65 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -44,6 +44,7 @@ int probe_file__add_event(int fd, struct probe_trace_event *tev);
 int probe_file__del_events(int fd, struct strfilter *filter);
 int probe_file__get_events(int fd, struct strfilter *filter,
 				  struct strlist *plist);
+int probe_file__get_sdt_events(struct probe_trace_event **tevs);
 int probe_file__del_strlist(int fd, struct strlist *namelist);
 
 int probe_cache_entry__get_event(struct probe_cache_entry *entry,
-- 
2.9.3

[PATCH v5 5/7] perf/sdt: Warn when number of events recorded are not equal to cached events

[Ravi Bangoria]

If number of events found from probe-cache is not equal to number of
existing events(fetched from uprobe_events), and somehow we decides
to record only existing events, we warn user about the same. For ex,

  $ sudo ./perf probe sdt_libpthread:mutex_release
    Added new events:
      sdt_libpthread:mutex_release (on %mutex_release in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_release_1 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_release_2 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
      sdt_libpthread:mutex_release_3 (on %mutex_release in /usr/lib64/libpthread-2.24.so)

  $ sudo ./perf record -a -e sdt_libpthread:*
    Warning: Recording on 4 occurrences of sdt_libpthread:*
    Warning: Found 35 events from probe-cache with name 'sdt_libpthread:*'.
             Since 4 probe points already exists, recording only them.
    Hint: Please use 'perf probe -d sdt_libpthread:*' to allow record on all events.

  $ sudo ./perf evlist
    sdt_libpthread:mutex_release_3
    sdt_libpthread:mutex_release_2
    sdt_libpthread:mutex_release_1
    sdt_libpthread:mutex_release

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 53 ++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 94b9105..7bf8783 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -3523,22 +3523,15 @@ void remove_sdt_event_list(struct list_head *sdt_events)
 	free_sdt_list(sdt_events);
 }
 
-static int get_sdt_events_from_cache(struct perf_probe_event *pev)
+static void sdt_warn_abt_exist_events(struct perf_probe_event *pev, int ctr)
 {
-	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;
-	}
-
-	return ret;
+	pr_warning("Warning: Found %d events from probe-cache with name '%s:%s'.\n"
+		"\t Since %d probe point%c already exists, recording only %s.\n"
+		"Hint: Please use 'perf probe -d %s:%s' to allow record on all events.\n\n",
+		pev->ntevs, pev->group, pev->event, ctr,
+		ctr > 1 ? 's' : '\0',
+		ctr > 1 ? "them" : "it",
+		pev->group, pev->event);
 }
 
 static int add_event_to_sdt_evlist(struct probe_trace_event *tev,
@@ -3727,6 +3720,15 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
 	probe_conf.max_probes = MAX_PROBES;
 	probe_conf.force_add = 1;
 
+	/*
+	 * This call is intentionally placed before fetching events
+	 * from uprobe_events file. If number of events found from probe-
+	 * cache is not equal to number of existing events, and somehow
+	 * we decides to record only existing events, we warn user about
+	 * the same (sdt_warn_abt_exist_events()).
+	 */
+	pev->ntevs = find_cached_events_all(pev, &pev->tevs);
+
 	/* Fetch all sdt events from uprobe_events */
 	exst_ntevs = probe_file__get_sdt_events(&exst_tevs);
 	if (exst_ntevs < 0) {
@@ -3738,14 +3740,29 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
 	ret = sdt_event_probepoint_exists(pev, exst_tevs,
 					 exst_ntevs, sdt_evlist);
 	if (ret) {
+		if (ret > 0 && pev->ntevs > 0 && ret != pev->ntevs)
+			sdt_warn_abt_exist_events(pev, ret);
 		ret = ret > 0 ? 0 : ret;
 		goto free_pev;
 	}
 
-	/* Fetch all matching events from cache. */
-	ret = get_sdt_events_from_cache(pev);
-	if (ret < 0)
+	/*
+	 * Check if find_cached_events_all() failed.
+	 * We deliberately check failure of this function after checking
+	 * entries in uprobe_events. Because, even if this function fails,
+	 * we may find matching entry from uprobe_events and in that case
+	 * we should continue recording that event.
+	 */
+	if (pev->ntevs < 0) {
+		pr_err("Error: Cache lookup failed (code: %d)\n", pev->ntevs);
+		ret = pev->ntevs;
 		goto free_pev;
+	} else if (!pev->ntevs) {
+		pr_err("Error: %s:%s not found in the cache\n",
+			pev->group, pev->event);
+		ret = -EINVAL;
+		goto free_pev;
+	}
 
 	/*
 	 * Merge events found from uprobe_events with events found
-- 
2.9.3

[PATCH v5 6/7] perf/sdt: List events fetched from uprobe_events

[Ravi Bangoria]

List those events which are fetched from uprobe_events as 'event addr@file'
followed by hint on how these events can be deleted with 'perf probe -d'
command.

For example:
  $ sudo cat /sys/kernel/debug/tracing/uprobe_events
    p:sdt_libpthread/mutex_release /usr/lib64/libpthread-2.24.so:0x000000000000b126

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release
    Matching event(s) from uprobe_events:
       sdt_libpthread:mutex_release  0xb126@/usr/lib64/libpthread-2.24.so
    Use 'perf probe -d <event>' to delete event(s).

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 7bf8783..e150de0 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -3593,6 +3593,24 @@ static void sdt_warn_multi_events(int ctr, struct perf_probe_event *pev)
 		   ctr, pev->group, pev->event);
 }
 
+static void print_exst_sdt_events(struct probe_trace_event *tev)
+{
+	static bool msg_head;
+
+	if (!msg_head) {
+		pr_info("Matching event(s) from uprobe_events:\n");
+		msg_head = true;
+	}
+
+	pr_info("   %s:%s  0x%" PRIx64 "@%s\n", tev->group,
+		tev->event, tev->point.address, tev->point.module);
+}
+
+static void print_exst_sdt_event_footer(void)
+{
+	pr_info("Use 'perf probe -d <event>' to delete event(s).\n\n");
+}
+
 static int sdt_event_probepoint_exists(struct perf_probe_event *pev,
 				       struct probe_trace_event *tevs,
 				       int ntevs,
@@ -3607,10 +3625,14 @@ static int sdt_event_probepoint_exists(struct perf_probe_event *pev,
 			if (ret < 0)
 				return ret;
 
+			print_exst_sdt_events(&tevs[i]);
 			ctr++;
 		}
 	}
 
+	if (ctr > 0)
+		print_exst_sdt_event_footer();
+
 	if (ctr > 1)
 		sdt_warn_multi_events(ctr, pev);
 
@@ -3660,11 +3682,16 @@ static int sdt_merge_events(struct perf_probe_event *pev,
 
 				if (!ptrn_used)
 					shift_sdt_events(pev, i);
+
+				print_exst_sdt_events(&exst_tevs[j]);
 				ctr++;
 			}
 		}
 	}
 
+	if (ctr > 0)
+		print_exst_sdt_event_footer();
+
 	if (!ptrn_used || ctr == 0) {
 		/*
 		 * Create probe point for all probe-cached events by
-- 
2.9.3

[PATCH v5 7/7] perf/sdt: List events fetched from uprobe_events

[Ravi Bangoria]

Perf was showing warning if user tries to record sdt event without
creating a probepoint. Now we are allowing direct record on sdt
events, remove this stale warning/hint.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/lib/api/fs/tracing_path.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/tools/lib/api/fs/tracing_path.c b/tools/lib/api/fs/tracing_path.c
index 3e606b9..fa52e67 100644
--- a/tools/lib/api/fs/tracing_path.c
+++ b/tools/lib/api/fs/tracing_path.c
@@ -103,19 +103,10 @@ int tracing_path__strerror_open_tp(int err, char *buf, size_t size,
 		 * - jirka
 		 */
 		if (debugfs__configured() || tracefs__configured()) {
-			/* sdt markers */
-			if (!strncmp(filename, "sdt_", 4)) {
-				snprintf(buf, size,
-					"Error:\tFile %s/%s not found.\n"
-					"Hint:\tSDT event cannot be directly recorded on.\n"
-					"\tPlease first use 'perf probe %s:%s' before recording it.\n",
-					tracing_events_path, filename, sys, name);
-			} else {
-				snprintf(buf, size,
-					 "Error:\tFile %s/%s not found.\n"
-					 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
-					 tracing_events_path, filename);
-			}
+			snprintf(buf, size,
+				 "Error:\tFile %s/%s not found.\n"
+				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
+				 tracing_events_path, filename);
 			break;
 		}
 		snprintf(buf, size, "%s",
-- 
2.9.3

Re: [PATCH v5 2/7] perf tool: Add option macro OPT_CALLBACK_ARG

[Arnaldo Carvalho de Melo]

Em Tue, Mar 14, 2017 at 08:36:53PM +0530, Ravi Bangoria escreveu:
> Add an option macro that is the same as OPT_CALLBACK_OPTARG except
> that the argument is not optional.

Not 'perf tool:', adjusted to 'tools lib subcmd:' as this is not perf
specific at all, tools/lib/subcmd/ is by other tools, such as objtool.

- Arnaldo
 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/lib/subcmd/parse-options.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/lib/subcmd/parse-options.h b/tools/lib/subcmd/parse-options.h
> index f054ca1..79472e0 100644
> --- a/tools/lib/subcmd/parse-options.h
> +++ b/tools/lib/subcmd/parse-options.h
> @@ -160,6 +160,10 @@ struct option {
>  	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), \
>  	  .value = (v), .argh = (a), .help = (h), .callback = (f), \
>  	  .flags = PARSE_OPT_OPTARG, .data = (d) }
> +#define OPT_CALLBACK_ARG(s, l, v, d, a, h, f) \
> +	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), \
> +	  .value = (v), .argh = (a), .help = (h), .callback = (f), \
> +	  .data = (d) }
>  
>  /* parse_options() will filter out the processed options and leave the
>   * non-option argments in argv[].
> -- 
> 2.9.3

Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'

[Jiri Olsa]

On Tue, Mar 14, 2017 at 08:36:54PM +0530, Ravi Bangoria wrote:

SNIP

> ---
>  tools/perf/builtin-record.c    |  27 ++++++-
>  tools/perf/util/parse-events.c |  60 +++++++++++++++
>  tools/perf/util/parse-events.h |   3 +
>  tools/perf/util/probe-event.c  | 165 ++++++++++++++++++++++++++++++++++++++++-
>  tools/perf/util/probe-event.h  |  12 +++
>  5 files changed, 261 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index bc84a37..e8e1f73 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -73,6 +73,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 +1504,23 @@ static struct record record = {
>  	},
>  };
>  
> +static int record__parse_events_option(const struct option *opt,
> +				       const char *str,
> +				       int unset)
> +{
> +	if (is_sdt_event((char *) str))
> +		return parse_sdt_events_option(opt, str, unset);
> +	else
> +		return parse_events_option(opt, str, unset);

so what happens if there're more than one event in 'str' like:
  -e cycles,std_...

would it be better to plug this directly into parse-events.y
parser.. and handle it like any other event type?

thanks,
jirka

Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'

[Arnaldo Carvalho de Melo]

Em Wed, Mar 15, 2017 at 01:03:31PM +0100, Jiri Olsa escreveu:
> On Tue, Mar 14, 2017 at 08:36:54PM +0530, Ravi Bangoria wrote:
> > +++ b/tools/perf/builtin-record.c
> > +static int record__parse_events_option(const struct option *opt,
> > +				       const char *str,
> > +				       int unset)
> > +{
> > +	if (is_sdt_event((char *) str))
> > +		return parse_sdt_events_option(opt, str, unset);
> > +	else
> > +		return parse_events_option(opt, str, unset);
> 
> so what happens if there're more than one event in 'str' like:
>   -e cycles,std_...
> 
> would it be better to plug this directly into parse-events.y
> parser.. and handle it like any other event type?

Yeah, I went to bed thinking about this :-) 

Ravi,

	Please test something like:

   perf record -e cycles,sdt_glib:mem__alloc,sched:*switch -a sleep 5s

- Arnaldo

- Arnaldo

Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'

[Ravi Bangoria]

On Wednesday 15 March 2017 06:46 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 15, 2017 at 01:03:31PM +0100, Jiri Olsa escreveu:
>> On Tue, Mar 14, 2017 at 08:36:54PM +0530, Ravi Bangoria wrote:
>>> +++ b/tools/perf/builtin-record.c
>>> +static int record__parse_events_option(const struct option *opt,
>>> +				       const char *str,
>>> +				       int unset)
>>> +{
>>> +	if (is_sdt_event((char *) str))
>>> +		return parse_sdt_events_option(opt, str, unset);
>>> +	else
>>> +		return parse_events_option(opt, str, unset);
>> so what happens if there're more than one event in 'str' like:
>>   -e cycles,std_...
>>
>> would it be better to plug this directly into parse-events.y
>> parser.. and handle it like any other event type?
> Yeah, I went to bed thinking about this :-) 
>
> Ravi,
>
> 	Please test something like:
>
>    perf record -e cycles,sdt_glib:mem__alloc,sched:*switch -a sleep 5s

Thanks Jiri, Arnaldo,

Yeah, this is failing :)

  $ sudo ./perf record -a -e sdt_libpthread:mutex_release,sced:*switch
    Error: sdt_libpthread:mutex_release,sched:*switch not found in the cache
    invalid or unsupported event: 'sdt_libpthread:mutex_release,sched:*switch'

Actually I tested with 'perf record -e ev1 -e ev2', but in this case
record__parse_events_option will get called separately for each
individual event.

Anyway, I'll fix this in next version.

Thanks for the review,
Ravi

Re: [PATCH v5 0/7] perf/sdt: Directly record SDT events with 'perf record'

[Masami Hiramatsu]

On Tue, 14 Mar 2017 20:36:51 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> All events from 'perf list', except SDT events, can be directly recorded
> with 'perf record'. But, the flow is little different for SDT events.
> Probe point for SDT event needs to be created using 'perf probe' before
> recording it using 'perf record'.
> 
> As suggested by Ingo[1], it's better to make this process simple by
> creating probe point automatically with 'perf record' for SDT events.
> 
> Features:
>   - Allow both 'perf probe' and 'perf record' on sdt events without
>     changing current functionality.
> 
>   - Event starting with 'sdt_' or '%' will be considered as SDT event.
> 
>   - Always prioritize events from uprobe_events by first checking if
>     event exists with exact name. If not found and user has used
>     pattern, again try to find pattern matching entries from
>     uprobe_events. If found use them. If not, lookup into probe-cache.
>     If events found from probe-cache, again check if any event exists
>     in uprobe_events by matching filepath+address, as it might exists
>     in uprobe_events but with different name. Reuse those events which
>     exists in uprobe_events and create new entries for missing one.
>     Also maintain list for new entries being created and at the end
>     of the session, delete them.
> 
>   - Show various warnings/hints to help user understand _which_ events
>     are being recorded and _why_. For ex,
> 
>     When multiple events of same name found and all are being recorded:
> 
>       $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
>         Warning: Recording on 2 occurrences of sdt_libpthread:mutex_entry
> 
>     Events being reused from uprobe_events is listed as 'name addr@file'
>     followed by hint on how to delete them:
> 
>       $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
>         Matching event(s) from uprobe_events:
>           sdt_libpthread:mutex_entry  0x9ddb@/usr/lib64/libpthread-2.24.so
>         Use 'perf probe -d <event>' to delete event(s).
> 
>     If number of events found from cache is not equal to number of events
>     being recorded:
> 
>       $ sudo ./perf record -a -e sdt_libpthread:mutex_entry
>         Warning: Found 2 events from probe-cache with name 'sdt_libpthread:mutex_entry'.
>                  Since 1 probe point already exists, recording only it.
>         Hint: Please use 'perf probe -d sdt_libpthread:mutex_entry' to allow record on all events.
> 
>   - If all events found from probe-cache are not present in uprobe_events,
>     and user has used pattern to specify event, perf will record only
>     those events which are present in uprobe_events. This is to make perf
>     semantics consistent across normal and SDT events. And If user has
>     not used pattern, perf will record all events found from probe-cache
>     by reusing name for existing one and adding entries for missing one.
>     For ex,
> 
>       $ sudo ./perf probe sdt_libpthread:mutex_release
>         Added new events:
>           sdt_libpthread:mutex_release (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>           sdt_libpthread:mutex_release_1 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>           sdt_libpthread:mutex_release_2 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>           sdt_libpthread:mutex_release_3 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>       $ sudo ./perf probe -d sdt_libpthread:mutex_release
>       $ sudo ./perf probe -d sdt_libpthread:mutex_release_2
> 
>       $ sudo ./perf record -a -e sdt_libpthread:mutex_release*
>         Warning: Recording on 2 occurrences of sdt_libpthread:mutex_release*
> 
>       $ sudo ./perf record -a -e sdt_libpthread:mutex_release
>         Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release
> 
> Changes in v5:
>   - Patch 2/7 is new. New option introduced in this patch helps to pass
>     custome data from builtin-*.c to libperf.
> 
>   - All direct callbacks from libelf to builtin-record.c is removed.
>  
>   - Merged 2nd and 4th patch of v4 into patch 2 of v5.
> 
>   - Moved all functions from util/probe-file.c to util/probe-event.c
>     which operates on perf_probe_event.
> 
>   - Made free_sdt_list() static as it's only used inside util/probe-event.c.
> 
>   - Couple of other changes as Masami has suggested in v4 review.

Hi Ravi,
Could you also describe which patches are updated? It seems 1/7 is not
modified, correct?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 0/7] perf/sdt: Directly record SDT events with 'perf record'

[Ravi Bangoria]

Hi Masami,

On Thursday 16 March 2017 03:21 PM, Masami Hiramatsu wrote:
> On Tue, 14 Mar 2017 20:36:51 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> Changes in v5:
>>   - Patch 2/7 is new. New option introduced in this patch helps to pass
>>     custome data from builtin-*.c to libperf.
>>
>>   - All direct callbacks from libelf to builtin-record.c is removed.
>>  

Minor correction.. s/libelf/libperf/

>>   - Merged 2nd and 4th patch of v4 into patch 2 of v5.

s/patch 2 of v5/patch 3 of v5/

>>
>>   - Moved all functions from util/probe-file.c to util/probe-event.c
>>     which operates on perf_probe_event.
>>
>>   - Made free_sdt_list() static as it's only used inside util/probe-event.c.
>>
>>   - Couple of other changes as Masami has suggested in v4 review.
> Hi Ravi,
> Could you also describe which patches are updated? It seems 1/7 is not
> modified, correct?

Let me list a patch-wise brief changelog.

patch 1/7:- Introduced dummy version of is_sdt_event() which always return false
             if !HAVE_LIBELF_SUPPORT.

patch 2/7: - is new. A new option introduced in this patch helps to passcustom
             data from builtin-*.c to libperf.

patch 3/7: - Removed direct calls from libperf to builtin-record.c which was used
             to prepare record.sdt_event_list. Instead pass list to libperf and let
             libperf manage it.

           - Introduce new wrapper func record__parse_events_option() that can
             differentiate between sdt and other events while parsing them in
             perf record.

           - Moved all functions from util/probe-file.c to util/probe-event.c
             which operates on perf_probe_event.

           - Merged 2nd and 4th patch of v4 into this patch.

           - Made free_sdt_list() static as it's only used inside util/probe-event.c.

patch 4/7:- Removed direct calls from libperf to builtin-record.c which was used
             to prepare record.sdt_event_list. Instead pass list to libperf and let
             libperf manage it.

           - Moved all functions from util/probe-file.c to util/probe-event.c
             which operates on perf_probe_event.

patch 5/7: No changes

patch 6/7: No changes

patch 7/7: No changes

Let me know if you need more details.

Thanks,
Ravi

[tip:perf/core] perf probe: Introduce util func is_sdt_event()

[tip-bot for Ravi Bangoria]

Commit-ID:  af9100ad149cf31a1ab1160f71bb4025443dbdb6
Gitweb:     http://git.kernel.org/tip/af9100ad149cf31a1ab1160f71bb4025443dbdb6
Author:     Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
AuthorDate: Tue, 14 Mar 2017 20:36:52 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 15 Mar 2017 17:48:37 -0300

perf probe: Introduce util func is_sdt_event()

Factor out the SDT event name checking routine as is_sdt_event().

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
Cc: He Kuang <hekuang@huawei.com>
Cc: Hemant Kumar <hemant@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Cc: Taeung Song <treeze.taeung@gmail.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170314150658.7065-2-ravi.bangoria@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.h | 20 ++++++++++++++++++++
 tools/perf/util/probe-event.c  |  9 +--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 1af6a26..8c72b0f 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -8,6 +8,7 @@
 #include <stdbool.h>
 #include <linux/types.h>
 #include <linux/perf_event.h>
+#include <string.h>
 
 struct list_head;
 struct perf_evsel;
@@ -196,4 +197,23 @@ int is_valid_tracepoint(const char *event_string);
 int valid_event_mount(const char *eventfs);
 char *parse_events_formats_error_string(char *additional_terms);
 
+#ifdef HAVE_LIBELF_SUPPORT
+/*
+ * If the probe point starts with '%',
+ * or starts with "sdt_" and has a ':' but no '=',
+ * then it should be a SDT/cached probe point.
+ */
+static inline bool is_sdt_event(char *str)
+{
+	return (str[0] == '%' ||
+		(!strncmp(str, "sdt_", 4) &&
+		 !!strchr(str, ':') && !strchr(str, '=')));
+}
+#else
+static inline bool is_sdt_event(char *str __maybe_unused)
+{
+	return false;
+}
+#endif /* HAVE_LIBELF_SUPPORT */
+
 #endif /* __PERF_PARSE_EVENTS_H */
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index c9bdc9d..b19d178 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1341,14 +1341,7 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
 	if (!arg)
 		return -EINVAL;
 
-	/*
-	 * If the probe point starts with '%',
-	 * or starts with "sdt_" and has a ':' but no '=',
-	 * then it should be a SDT/cached probe point.
-	 */
-	if (arg[0] == '%' ||
-	    (!strncmp(arg, "sdt_", 4) &&
-	     !!strchr(arg, ':') && !strchr(arg, '='))) {
+	if (is_sdt_event(arg)) {
 		pev->sdt = true;
 		if (arg[0] == '%')
 			arg++;

Re: [PATCH v5 0/7] perf/sdt: Directly record SDT events with 'perf record'

[Masami Hiramatsu]

Hi Ravi,

On Thu, 16 Mar 2017 16:57:52 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Hi Masami,
> 
> On Thursday 16 March 2017 03:21 PM, Masami Hiramatsu wrote:
> > On Tue, 14 Mar 2017 20:36:51 +0530
> > Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> >
> >> Changes in v5:
> >>   - Patch 2/7 is new. New option introduced in this patch helps to pass
> >>     custome data from builtin-*.c to libperf.
> >>
> >>   - All direct callbacks from libelf to builtin-record.c is removed.
> >>  
> 
> Minor correction.. s/libelf/libperf/
> 
> >>   - Merged 2nd and 4th patch of v4 into patch 2 of v5.
> 
> s/patch 2 of v5/patch 3 of v5/
> 
> >>
> >>   - Moved all functions from util/probe-file.c to util/probe-event.c
> >>     which operates on perf_probe_event.
> >>
> >>   - Made free_sdt_list() static as it's only used inside util/probe-event.c.
> >>
> >>   - Couple of other changes as Masami has suggested in v4 review.
> > Hi Ravi,
> > Could you also describe which patches are updated? It seems 1/7 is not
> > modified, correct?
> 
> Let me list a patch-wise brief changelog.
> 
> patch 1/7:- Introduced dummy version of is_sdt_event() which always return false
>              if !HAVE_LIBELF_SUPPORT.
> 
> patch 2/7: - is new. A new option introduced in this patch helps to passcustom
>              data from builtin-*.c to libperf.
> 
> patch 3/7: - Removed direct calls from libperf to builtin-record.c which was used
>              to prepare record.sdt_event_list. Instead pass list to libperf and let
>              libperf manage it.
> 
>            - Introduce new wrapper func record__parse_events_option() that can
>              differentiate between sdt and other events while parsing them in
>              perf record.
> 
>            - Moved all functions from util/probe-file.c to util/probe-event.c
>              which operates on perf_probe_event.
> 
>            - Merged 2nd and 4th patch of v4 into this patch.
> 
>            - Made free_sdt_list() static as it's only used inside util/probe-event.c.
> 
> patch 4/7:- Removed direct calls from libperf to builtin-record.c which was used
>              to prepare record.sdt_event_list. Instead pass list to libperf and let
>              libperf manage it.
> 
>            - Moved all functions from util/probe-file.c to util/probe-event.c
>              which operates on perf_probe_event.
> 
> patch 5/7: No changes
> 
> patch 6/7: No changes
> 
> patch 7/7: No changes
> 
> Let me know if you need more details.

Thanks! that's very helpful for me to review it.


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'

[Masami Hiramatsu]

Hi Ravi,

(I avoided to review parser part since it may go to yacc in next version) 

On Tue, 14 Mar 2017 20:36:54 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

[SNIP]
> @@ -1516,9 +1534,10 @@ static bool dry_run;
>   * using pipes, etc.
>   */
>  static struct option __record_options[] = {
> -	OPT_CALLBACK('e', "event", &record.evlist, "event",
> -		     "event selector. use 'perf list' to list available events",
> -		     parse_events_option),
> +	OPT_CALLBACK_ARG('e', "event", &record.evlist,
> +			 &record.sdt_event_list, "event",
> +			 "event selector. use 'perf list' to list available events",
> +			 record__parse_events_option),

Does --event option NOT requires argument without this patch?
If it should be changed to use OPT_CALLBACK_ARG(), would it be
better merge this part to previous patch?

[SNIP]
> +/*
> + * 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;

Don't we need to return error code for this case?

> +		} else {
> +			strfilter__or(filter, sdt_event->name, &err);

strfilter__or() can fail here.

> +		}
> +	}
> +
> +	del_perf_probe_events(filter);

Here too, if it is ignored silently by design, please comment it here.

> +
> +free_list:
> +	free_sdt_list(sdt_events);
> +}
> +
> +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;

Well, strbuf can make this simpler as below ;-)

	struct strbuf buf = STRBUF_INIT;

> +
> +	tmp = zalloc(sizeof(*tmp));
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&tmp->list);

	if (strbuf_addf(&buf, "%s:%s", tev->group, tev->event))
		goto error;

	tmp->name = strbuf_detach(&buf);

> +	list_add(&tmp->list, sdt_evlist);
> +
> +	return 0;

error:
	free(tmp);

	return -ENOMEM;
> +}
> +
> +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;
> +}


Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events

[Masami Hiramatsu]

On Tue, 14 Mar 2017 20:36:55 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Add functionality to fetch matching events from uprobe_events. If no
> events are fourd from it, fetch matching events from probe-cache and
> add them in uprobe_events. If all events are already present in
> uprobe_events, reuse them. If few of them are present, add entries
> for missing events and record them. At the end of the record session,
> delete newly added entries. Below is detailed algorithm that describe
> implementation of this patch:
> 
>     arr1 = fetch all sdt events from uprobe_events
> 
>     if (event with exact name in arr1)
>         add that in sdt_event_list
>         return
> 
>     if (user has used pattern)
>         if (pattern matching entries found from arr1)
>             add those events in sdt_event_list
>             return
> 
>     arr2 = lookup probe-cache
>     if (arr2 empty)
>         return
> 
>     ctr = 0
>     foreach (compare entries of arr1 and arr2 using filepath+address)
>         if (match)
>             add event from arr1 to sdt_event_list
>             ctr++
>             if (!pattern used)
>                 remove entry from arr2
> 
>     if (!pattern used || ctr == 0)
>         add all entries of arr2 in sdt_event_list
> 
> Example: Consider sdt event sdt_libpthread:mutex_release found in
> /usr/lib64/libpthread-2.24.so.
> 
>   $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b126, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
>     --
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b2f6, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
>     --
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b498, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
>     --
>       Provider: libpthread
>       Name: mutex_release
>       Location: 0x000000000000b596, Base: 0x00000000000139cc, Semaphore: 0x0000000000000000
> 
> When no probepoint exists,
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_*
>     Warning: Recording on 15 occurrences of sdt_libpthread:mutex_*
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release
>     Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_2
>     sdt_libpthread:mutex_release_1
>     sdt_libpthread:mutex_release
> 
> When probepoints already exists for all matching events,
> 
>   $ sudo ./perf probe sdt_libpthread:mutex_release
>     Added new events:
>       sdt_libpthread:mutex_release (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>       sdt_libpthread:mutex_release_1 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>       sdt_libpthread:mutex_release_2 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
>       sdt_libpthread:mutex_release_3 (on %mutex_release in /usr/lib64/libpthread-2.24.so)
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release_1
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_1
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_*
>     Warning: Recording on 4 occurrences of sdt_libpthread:mutex_*
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_2
>     sdt_libpthread:mutex_release_1
>     sdt_libpthread:mutex_release
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release_*
>     Warning: Recording on 3 occurrences of sdt_libpthread:mutex_release_*
> 
> When probepoints are partially exists,
> 
>   $ sudo ./perf probe -d sdt_libpthread:mutex_release
>   $ sudo ./perf probe -d sdt_libpthread:mutex_release_2
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release
>     Warning: Recording on 4 occurrences of sdt_libpthread:mutex_release
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_2
>     sdt_libpthread:mutex_release_1
> 
>   $ sudo ./perf record -a -e sdt_libpthread:mutex_release*
>     Warning: Recording on 2 occurrences of sdt_libpthread:mutex_release*
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_1
> 
>   $ sudo ./perf record -a -e sdt_libpthread:*
>     Warning: Recording on 2 occurrences of sdt_libpthread:*
>   $ sudo ./perf evlist
>     sdt_libpthread:mutex_release_3
>     sdt_libpthread:mutex_release_1
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 186 ++++++++++++++++++++++++++++++++++++------
>  tools/perf/util/probe-event.h |   4 +
>  tools/perf/util/probe-file.c  |  48 +++++++++++
>  tools/perf/util/probe-file.h  |   1 +
>  4 files changed, 214 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index f725953..94b9105 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -232,7 +232,7 @@ static void clear_perf_probe_point(struct perf_probe_point *pp)
>  	free(pp->lazy_line);
>  }
>  
> -static void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
> +void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs)
>  {
>  	int i;
>  
> @@ -3044,9 +3044,8 @@ static void *memcat(void *a, size_t sz_a, void *b, size_t sz_b)
>  	return ret;
>  }
>  
> -static int
> -concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
> -			  struct probe_trace_event **tevs2, int ntevs2)
> +int concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
> +			      struct probe_trace_event **tevs2, int ntevs2)
>  {
>  	struct probe_trace_event *new_tevs;
>  	int ret = 0;
> @@ -3505,6 +3504,9 @@ void remove_sdt_event_list(struct list_head *sdt_events)
>  		return;
>  
>  	list_for_each_entry(sdt_event, sdt_events, list) {
> +		if (sdt_event->exst)
> +			continue;
> +
>  		if (!filter) {
>  			filter = strfilter__new(sdt_event->name, &err);
>  			if (!filter)
> @@ -3514,7 +3516,8 @@ void remove_sdt_event_list(struct list_head *sdt_events)
>  		}
>  	}
>  
> -	del_perf_probe_events(filter);
> +	if (filter)
> +		del_perf_probe_events(filter);

Hmm, I found strfilter__string(NULL) (which will happen if 
del_perf_probe_events(NULL) invoked) causes SEGV. It should be safe
for NULL instead of checking here.

>  
>  free_list:
>  	free_sdt_list(sdt_events);
> @@ -3533,16 +3536,14 @@ static int get_sdt_events_from_cache(struct perf_probe_event *pev)
>  		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 list_head *sdt_evlist,
> +				   bool exst)
>  {
>  	struct sdt_event_list *tmp;
>  
> @@ -3557,6 +3558,7 @@ static int add_event_to_sdt_evlist(struct probe_trace_event *tev,
>  
>  	snprintf(tmp->name, strlen(tev->group) + strlen(tev->event) + 2,
>  		 "%s:%s", tev->group, tev->event);
> +	tmp->exst = exst;
>  	list_add(&tmp->list, sdt_evlist);
>  
>  	return 0;
> @@ -3568,7 +3570,7 @@ static int add_events_to_sdt_evlist(struct perf_probe_event *pev,
>  	int i, ret;
>  
>  	for (i = 0; i < pev->ntevs; i++) {
> -		ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist);
> +		ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist, false);
>  
>  		if (ret < 0)
>  			return ret;
> @@ -3576,14 +3578,133 @@ static int add_events_to_sdt_evlist(struct perf_probe_event *pev,
>  	return 0;
>  }
>  
> -/*
> - * Find the SDT event from the cache and if found add it/them
> - * to the uprobe_events file
> - */
> +static bool sdt_is_ptrn_used(struct perf_probe_event *pev)

This might be sdt_name_is_glob().

> +{
> +	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);

Would you mean strisglob()@util.h ? :)

> +}
> +
> +static bool sdt_name_match(struct perf_probe_event *pev,
> +			   struct probe_trace_event *tev)
> +{
> +	if (sdt_is_ptrn_used(pev))
> +		return strglobmatch(tev->group, pev->group) &&
> +			strglobmatch(tev->event, pev->event);
> +
> +	return !strcmp(tev->group, pev->group) &&
> +		!strcmp(tev->event, pev->event);

Would we really need these two? Since strglobmatch() accepts a string
without wildcard, you don't need strcmp() version...

> +}
> +
> +static void sdt_warn_multi_events(int ctr, struct perf_probe_event *pev)
> +{
> +	pr_warning("Warning: Recording on %d occurrences of %s:%s\n",
> +		   ctr, pev->group, pev->event);

Could you show what event will be recorded instead of just showing 
the number of events?

> +}
> +
> +static int sdt_event_probepoint_exists(struct perf_probe_event *pev,
> +				       struct probe_trace_event *tevs,
> +				       int ntevs,
> +				       struct list_head *sdt_evlist)
> +{
> +	int i = 0, ret = 0, ctr = 0;
> +
> +	for (i = 0; i < ntevs; i++) {
> +		if (sdt_name_match(pev, &tevs[i])) {
> +			ret = add_event_to_sdt_evlist(&tevs[i],
> +						sdt_evlist, true);
> +			if (ret < 0)
> +				return ret;
> +
> +			ctr++;
> +		}
> +	}
> +
> +	if (ctr > 1)
> +		sdt_warn_multi_events(ctr, pev);
> +
> +	return ctr;
> +}
> +
> +static bool sdt_file_addr_match(struct probe_trace_event *tev1,
> +				struct probe_trace_event *tev2)
> +{
> +	return (tev1->point.address == tev2->point.address &&
> +		!(strcmp(tev1->point.module, tev2->point.module)));
> +}
> +
> +static void shift_sdt_events(struct perf_probe_event *pev, int i)
> +{
> +	int j = 0;
> +
> +	clear_probe_trace_event(&pev->tevs[i]);
> +
> +	if (i == pev->ntevs - 1)
> +		goto out;
> +
> +	for (j = i; j < pev->ntevs - 1; j++)
> +		memcpy(&pev->tevs[j], &pev->tevs[j + 1],
> +		       sizeof(struct probe_trace_event));
> +
> +out:
> +	pev->ntevs--;
> +}
> +
> +static int sdt_merge_events(struct perf_probe_event *pev,
> +			    struct probe_trace_event *exst_tevs,
> +			    int exst_ntevs,
> +			    struct list_head *sdt_evlist)
> +{
> +	int i, j, ret = 0, ctr = 0;
> +	bool ptrn_used = sdt_is_ptrn_used(pev);
> +
> +	for (i = 0; i < pev->ntevs; i++) {
> +		for (j = 0; j < exst_ntevs; j++) {
> +			if (sdt_file_addr_match(&pev->tevs[i],
> +						&exst_tevs[j])) {
> +				ret = add_event_to_sdt_evlist(&exst_tevs[j],
> +							  sdt_evlist, true);
> +				if (ret < 0)
> +					return ret;
> +
> +				if (!ptrn_used)
> +					shift_sdt_events(pev, i);
> +				ctr++;
> +			}
> +		}
> +	}
> +
> +	if (!ptrn_used || ctr == 0) {
> +		/*
> +		 * Create probe point for all probe-cached events by
> +		 * adding them in uprobe_events file.
> +		 */
> +		ret = apply_perf_probe_events(pev, 1);
> +		if (ret < 0) {
> +			pr_err("Error in adding SDT event: %s:%s\n",
> +				pev->group, pev->event);
> +			goto out;
> +		}
> +
> +		/* Add events to sdt_evlist. */
> +		ret = add_events_to_sdt_evlist(pev, sdt_evlist);
> +		if (ret < 0) {
> +			pr_err("Error while updating event list\n");
> +			goto out;
> +		}
> +
> +		ctr += pev->ntevs;
> +		if (ctr > 1)
> +			sdt_warn_multi_events(ctr, pev);
> +	}
> +
> +out:
> +	return ret;
> +}
> +
>  int add_sdt_event(char *event, struct list_head *sdt_evlist)
>  {
>  	struct perf_probe_event *pev;
> -	int ret;
> +	int ret, exst_ntevs;
> +	struct probe_trace_event *exst_tevs = NULL;
>  
>  	pev = zalloc(sizeof(*pev));
>  	if (!pev)
> @@ -3606,23 +3727,37 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
>  	probe_conf.max_probes = MAX_PROBES;
>  	probe_conf.force_add = 1;
>  
> +	/* Fetch all sdt events from uprobe_events */
> +	exst_ntevs = probe_file__get_sdt_events(&exst_tevs);
> +	if (exst_ntevs < 0) {
> +		ret = exst_ntevs;
> +		goto free_pev;
> +	}
> +
> +	/* Check if events with same name already exists in uprobe_events. */
> +	ret = sdt_event_probepoint_exists(pev, exst_tevs,
> +					 exst_ntevs, sdt_evlist);
> +	if (ret) {
> +		ret = ret > 0 ? 0 : ret;
> +		goto free_pev;
> +	}
> +
>  	/* Fetch all matching events from cache. */
>  	ret = get_sdt_events_from_cache(pev);
>  	if (ret < 0)
>  		goto free_pev;

Hmm, IMHO, you'd better call get_sdt_events_from_cache() first, and
check the existence of those events afterwards. Then you may not
need the "shift" function.

Thank you,

>  
>  	/*
> -	 * Create probe point for all events by adding them in
> -	 * uprobe_events file
> +	 * Merge events found from uprobe_events with events found
> +	 * from cache. Reuse events whose probepoint already exists
> +	 * in uprobe_events, while add new entries for other events
> +	 * in uprobe_events.
> +	 *
> +	 * This always tries to give first priority to events from
> +	 * uprobe_events. By doing so, it ensures the existing
> +	 * behaviour of perf remains same for sdt events too.
>  	 */
> -	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);
> +	ret = sdt_merge_events(pev, exst_tevs, exst_ntevs, sdt_evlist);
>  	if (ret < 0)
>  		goto free_pev;
>  
> @@ -3632,6 +3767,7 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
>  	if (ret < 0)
>  		free_sdt_list(sdt_evlist);
>  	cleanup_perf_probe_events(pev, 1);
> +	clear_probe_trace_events(exst_tevs, exst_ntevs);
>  	free(pev);
>  	return ret;
>  }
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 6812230..fd8ec36 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -117,6 +117,7 @@ struct variable_list {
>  struct sdt_event_list {
>  	struct list_head list;
>  	char *name;		/* group:event */
> +	bool exst;		/* Even already exists in uprobe_events? */
>  };
>  
>  struct map;
> @@ -144,6 +145,7 @@ bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
>  /* Release event contents */
>  void clear_perf_probe_event(struct perf_probe_event *pev);
>  void clear_probe_trace_event(struct probe_trace_event *tev);
> +void clear_probe_trace_events(struct probe_trace_event *tevs, int ntevs);
>  
>  /* Command string to line-range */
>  int parse_line_range_desc(const char *cmd, struct line_range *lr);
> @@ -190,6 +192,8 @@ void arch__post_process_probe_trace_events(struct perf_probe_event *pev,
>  
>  int parse_perf_probe_event_name(char **arg, struct perf_probe_event *pev);
>  
> +int concat_probe_trace_events(struct probe_trace_event **tevs, int *ntevs,
> +			      struct probe_trace_event **tevs2, int ntevs2);
>  int find_cached_events_all(struct perf_probe_event *pev,
>  			   struct probe_trace_event **tevs);
>  int add_sdt_event(char *event, struct list_head *sdt_event_list);
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 1a62dac..9fb0a1f 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -310,6 +310,54 @@ int probe_file__get_events(int fd, struct strfilter *filter,
>  	return ret;
>  }
>  
> +/*
> + * Look into uprobe_events file and prepare list of sdt events
> + * whose probepoint is already registered.
> + */
> +int probe_file__get_sdt_events(struct probe_trace_event **tevs)
> +{
> +	int fd, ret, ntevs = 0;
> +	struct strlist *rawlist;
> +	struct str_node *ent;
> +	struct probe_trace_event *tev;
> +
> +	fd = probe_file__open(PF_FL_RW | PF_FL_UPROBE);
> +	if (fd < 0)
> +		return fd;
> +
> +	rawlist = probe_file__get_rawlist(fd);
> +	strlist__for_each_entry(ent, rawlist) {
> +		tev = zalloc(sizeof(struct probe_trace_event));
> +		if (!tev) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +
> +		ret = parse_probe_trace_command(ent->s, tev);
> +		if (ret < 0)
> +			goto err;
> +
> +		if (strncmp(tev->group, "sdt_", 4)) {
> +			/* Interested in SDT events only. */
> +			free(tev);
> +			continue;
> +		}
> +
> +		ret = concat_probe_trace_events(tevs, &ntevs, &tev, 1);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
> +	close(fd);
> +	return ntevs;
> +
> +err:
> +	close(fd);
> +	clear_probe_trace_events(*tevs, ntevs);
> +	zfree(tevs);
> +	return ret;
> +}
> +
>  int probe_file__del_strlist(int fd, struct strlist *namelist)
>  {
>  	int ret = 0;
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index a17a82e..f696e65 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -44,6 +44,7 @@ int probe_file__add_event(int fd, struct probe_trace_event *tev);
>  int probe_file__del_events(int fd, struct strfilter *filter);
>  int probe_file__get_events(int fd, struct strfilter *filter,
>  				  struct strlist *plist);
> +int probe_file__get_sdt_events(struct probe_trace_event **tevs);
>  int probe_file__del_strlist(int fd, struct strlist *namelist);
>  
>  int probe_cache_entry__get_event(struct probe_cache_entry *entry,
> -- 
> 2.9.3
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 7/7] perf/sdt: List events fetched from uprobe_events

[Masami Hiramatsu]

The title of this patch seems not correct.

On Tue, 14 Mar 2017 20:36:58 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Perf was showing warning if user tries to record sdt event without
> creating a probepoint. Now we are allowing direct record on sdt
> events, remove this stale warning/hint.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/lib/api/fs/tracing_path.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/lib/api/fs/tracing_path.c b/tools/lib/api/fs/tracing_path.c
> index 3e606b9..fa52e67 100644
> --- a/tools/lib/api/fs/tracing_path.c
> +++ b/tools/lib/api/fs/tracing_path.c
> @@ -103,19 +103,10 @@ int tracing_path__strerror_open_tp(int err, char *buf, size_t size,
>  		 * - jirka
>  		 */
>  		if (debugfs__configured() || tracefs__configured()) {
> -			/* sdt markers */
> -			if (!strncmp(filename, "sdt_", 4)) {
> -				snprintf(buf, size,
> -					"Error:\tFile %s/%s not found.\n"
> -					"Hint:\tSDT event cannot be directly recorded on.\n"
> -					"\tPlease first use 'perf probe %s:%s' before recording it.\n",
> -					tracing_events_path, filename, sys, name);
> -			} else {
> -				snprintf(buf, size,
> -					 "Error:\tFile %s/%s not found.\n"
> -					 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
> -					 tracing_events_path, filename);
> -			}
> +			snprintf(buf, size,
> +				 "Error:\tFile %s/%s not found.\n"
> +				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
> +				 tracing_events_path, filename);
>  			break;
>  		}
>  		snprintf(buf, size, "%s",
> -- 
> 2.9.3
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'

[Ravi Bangoria]

Thanks Masami for detailed review.

Please see my comments below.

On Friday 17 March 2017 02:35 PM, Masami Hiramatsu wrote:
> Hi Ravi,
>
> (I avoided to review parser part since it may go to yacc in next version) 
>
> On Tue, 14 Mar 2017 20:36:54 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
> [SNIP]
>> @@ -1516,9 +1534,10 @@ static bool dry_run;
>>   * using pipes, etc.
>>   */
>>  static struct option __record_options[] = {
>> -	OPT_CALLBACK('e', "event", &record.evlist, "event",
>> -		     "event selector. use 'perf list' to list available events",
>> -		     parse_events_option),
>> +	OPT_CALLBACK_ARG('e', "event", &record.evlist,
>> +			 &record.sdt_event_list, "event",
>> +			 "event selector. use 'perf list' to list available events",
>> +			 record__parse_events_option),
> Does --event option NOT requires argument without this patch?
> If it should be changed to use OPT_CALLBACK_ARG(), would it be
> better merge this part to previous patch?

Ok. Yes, it does. I think macro name is confusing.

This new macro allows passing of extra data from builtin-*.c to libelf.
One such macro already exists (OPT_CALLBACK_OPTARG), but the
argument is optional for it and thus it ignores the argument. I need
a macro in which argument is necessary and it also allows to pass
extra data. Hence, I introduced this macro.

Will change macro to OPT_CALLBACK_ARGDATA. Please suggest if
you have better name.

> [SNIP]
>> +/*
>> + * 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;
> Don't we need to return error code for this case?
>
>> +		} else {
>> +			strfilter__or(filter, sdt_event->name, &err);
> strfilter__or() can fail here.
>
>> +		}
>> +	}
>> +
>> +	del_perf_probe_events(filter);
> Here too, if it is ignored silently by design, please comment it here.

Sure. Will think about handling errors in this function.

>
>> +
>> +free_list:
>> +	free_sdt_list(sdt_events);
>> +}
>> +
>> +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;
> Well, strbuf can make this simpler as below ;-)
>
> 	struct strbuf buf = STRBUF_INIT;

Sure, will do it.

Thanks :)
Ravi

>> +
>> +	tmp = zalloc(sizeof(*tmp));
>> +	if (!tmp)
>> +		return -ENOMEM;
>> +
>> +	INIT_LIST_HEAD(&tmp->list);
> 	if (strbuf_addf(&buf, "%s:%s", tev->group, tev->event))
> 		goto error;
>
> 	tmp->name = strbuf_detach(&buf);
>
>> +	list_add(&tmp->list, sdt_evlist);
>> +
>> +	return 0;
> error:
> 	free(tmp);
>
> 	return -ENOMEM;
>> +}
>> +
>> +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;
>> +}
>
> Thanks,
>

Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events

[Ravi Bangoria]

Thanks Masami for detailed review.

Please see my comments below.

On Saturday 18 March 2017 04:43 AM, Masami Hiramatsu wrote:
> On Tue, 14 Mar 2017 20:36:55 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>>  		}
>>  	}
>>  
>> -	del_perf_probe_events(filter);
>> +	if (filter)
>> +		del_perf_probe_events(filter);
> Hmm, I found strfilter__string(NULL) (which will happen if 
> del_perf_probe_events(NULL) invoked) causes SEGV. It should be safe
> for NULL instead of checking here.

Sure will change accordingly.

>>  }
>>  
>> -/*
>> - * Find the SDT event from the cache and if found add it/them
>> - * to the uprobe_events file
>> - */
>> +static bool sdt_is_ptrn_used(struct perf_probe_event *pev)
> This might be sdt_name_is_glob().

Hmm looks good. Will change it.

>> +{
>> +	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);
> Would you mean strisglob()@util.h ? :)

Yes, BUT this needs to be changed now.

I found perf probe accepts glob wildcards(*, ?) and character classes like
[a-z]. But perf record only allow wildcards. So I can't use strisglob()
function.

Please let me know if that is wrong.

>> +}
>> +
>> +static bool sdt_name_match(struct perf_probe_event *pev,
>> +			   struct probe_trace_event *tev)
>> +{
>> +	if (sdt_is_ptrn_used(pev))
>> +		return strglobmatch(tev->group, pev->group) &&
>> +			strglobmatch(tev->event, pev->event);
>> +
>> +	return !strcmp(tev->group, pev->group) &&
>> +		!strcmp(tev->event, pev->event);
> Would we really need these two? Since strglobmatch() accepts a string
> without wildcard, you don't need strcmp() version...

Hmm. yes, second part looks unnecessary.

>> +}
>> +
>> +static void sdt_warn_multi_events(int ctr, struct perf_probe_event *pev)
>> +{
>> +	pr_warning("Warning: Recording on %d occurrences of %s:%s\n",
>> +		   ctr, pev->group, pev->event);
> Could you show what event will be recorded instead of just showing 
> the number of events?

Sure. I'll remove this warning and show all events as 'event addr@file'.
Please let me know if you want to list it differently.

Actually, I do that, but partially. Please see patch 6/7. It list all those
*existing* events which are being recorded.

>> +}
>> +
>> +static int sdt_event_probepoint_exists(struct perf_probe_event *pev,
>> +				       struct probe_trace_event *tevs,
>> +				       int ntevs,
>> +				       struct list_head *sdt_evlist)
>> +{
>> +	int i = 0, ret = 0, ctr = 0;
>> +
>> +	for (i = 0; i < ntevs; i++) {
>> +		if (sdt_name_match(pev, &tevs[i])) {
>> +			ret = add_event_to_sdt_evlist(&tevs[i],
>> +						sdt_evlist, true);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			ctr++;
>> +		}
>> +	}
>> +
>> +	if (ctr > 1)
>> +		sdt_warn_multi_events(ctr, pev);
>> +
>> +	return ctr;
>> +}
>> +
>> +static bool sdt_file_addr_match(struct probe_trace_event *tev1,
>> +				struct probe_trace_event *tev2)
>> +{
>> +	return (tev1->point.address == tev2->point.address &&
>> +		!(strcmp(tev1->point.module, tev2->point.module)));
>> +}
>> +
>> +static void shift_sdt_events(struct perf_probe_event *pev, int i)
>> +{
>> +	int j = 0;
>> +
>> +	clear_probe_trace_event(&pev->tevs[i]);
>> +
>> +	if (i == pev->ntevs - 1)
>> +		goto out;
>> +
>> +	for (j = i; j < pev->ntevs - 1; j++)
>> +		memcpy(&pev->tevs[j], &pev->tevs[j + 1],
>> +		       sizeof(struct probe_trace_event));
>> +
>> +out:
>> +	pev->ntevs--;
>> +}
>> +
>> +static int sdt_merge_events(struct perf_probe_event *pev,
>> +			    struct probe_trace_event *exst_tevs,
>> +			    int exst_ntevs,
>> +			    struct list_head *sdt_evlist)
>> +{
>> +	int i, j, ret = 0, ctr = 0;
>> +	bool ptrn_used = sdt_is_ptrn_used(pev);
>> +
>> +	for (i = 0; i < pev->ntevs; i++) {
>> +		for (j = 0; j < exst_ntevs; j++) {
>> +			if (sdt_file_addr_match(&pev->tevs[i],
>> +						&exst_tevs[j])) {
>> +				ret = add_event_to_sdt_evlist(&exst_tevs[j],
>> +							  sdt_evlist, true);
>> +				if (ret < 0)
>> +					return ret;
>> +
>> +				if (!ptrn_used)
>> +					shift_sdt_events(pev, i);
>> +				ctr++;
>> +			}
>> +		}
>> +	}
>> +
>> +	if (!ptrn_used || ctr == 0) {
>> +		/*
>> +		 * Create probe point for all probe-cached events by
>> +		 * adding them in uprobe_events file.
>> +		 */
>> +		ret = apply_perf_probe_events(pev, 1);
>> +		if (ret < 0) {
>> +			pr_err("Error in adding SDT event: %s:%s\n",
>> +				pev->group, pev->event);
>> +			goto out;
>> +		}
>> +
>> +		/* Add events to sdt_evlist. */
>> +		ret = add_events_to_sdt_evlist(pev, sdt_evlist);
>> +		if (ret < 0) {
>> +			pr_err("Error while updating event list\n");
>> +			goto out;
>> +		}
>> +
>> +		ctr += pev->ntevs;
>> +		if (ctr > 1)
>> +			sdt_warn_multi_events(ctr, pev);
>> +	}
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>>  int add_sdt_event(char *event, struct list_head *sdt_evlist)
>>  {
>>  	struct perf_probe_event *pev;
>> -	int ret;
>> +	int ret, exst_ntevs;
>> +	struct probe_trace_event *exst_tevs = NULL;
>>  
>>  	pev = zalloc(sizeof(*pev));
>>  	if (!pev)
>> @@ -3606,23 +3727,37 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
>>  	probe_conf.max_probes = MAX_PROBES;
>>  	probe_conf.force_add = 1;
>>  
>> +	/* Fetch all sdt events from uprobe_events */
>> +	exst_ntevs = probe_file__get_sdt_events(&exst_tevs);
>> +	if (exst_ntevs < 0) {
>> +		ret = exst_ntevs;
>> +		goto free_pev;
>> +	}
>> +
>> +	/* Check if events with same name already exists in uprobe_events. */
>> +	ret = sdt_event_probepoint_exists(pev, exst_tevs,
>> +					 exst_ntevs, sdt_evlist);
>> +	if (ret) {
>> +		ret = ret > 0 ? 0 : ret;
>> +		goto free_pev;
>> +	}
>> +
>>  	/* Fetch all matching events from cache. */
>>  	ret = get_sdt_events_from_cache(pev);
>>  	if (ret < 0)
>>  		goto free_pev;
> Hmm, IMHO, you'd better call get_sdt_events_from_cache() first, and
> check the existence of those events afterwards. Then you may not
> need the "shift" function.

"shift" function is needed. let me explain.

As we are allowing both, 'perf probe' as well as 'perf record' on SDT
events, there are two sources of events for 'perf record'
  1. uprobe_events file
  2. probe-cache

When perf fetches events from cache, it fetch all matching events,
no matter if any event already exists in uprobe_events or not.

Consider an example, There are 3 events with same name sdt_a:b
exists in 'test' binary.

  $ readelf -n test
    sdt_a:b @ addr 0x123
    sdt_a:b @ addr 0x456
    sdt_a:b @ addr 0x789

  $ perf probe -x test sdt_a:b
     /* Add all 3 events sdt_a:b  sdt_a:b_1 and sdt_a:b_2 */
  $ perf probe -d sdt_a:b
  $ perf probe -d sdt_a:b_2

  $ perf record sdt_a:b
    /*
      arr1 = probe_file__get_sdt_events();
                 {sdt_a:b_1 addr 0x456}

      arr2 = get_sdt_events_from_cache();
                 {sdt_a:b addr 0x123, sdt_a:b addr 0x456, sdt_a:b addr 0x789}

      Now, as sdt event of addr 0x456 already exists in arr1, it needs to
      be reused and thus it needs to be removed from arr2. Removing
      2nd element needs shifting of third element to 2nd position.
    */

I don't think it's easy to remove "shift" function. I can remove it but
that needs changes in existing infrastructure like fetch only those
events from cache which are not present in uprobe_events. But,
IMHO, it's not a good way to go.

Let me know if I misunderstood your point.

Thanks,
Ravi

Re: [PATCH v5 7/7] perf/sdt: List events fetched from uprobe_events

[Ravi Bangoria]

On Saturday 18 March 2017 04:44 AM, Masami Hiramatsu wrote:
> The title of this patch seems not correct.

Oops. Sorry about that. Actual title is:

    "perf/sdt: Remove stale warning"

Thanks for reporting,
Ravi

> On Tue, 14 Mar 2017 20:36:58 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> Perf was showing warning if user tries to record sdt event without
>> creating a probepoint. Now we are allowing direct record on sdt
>> events, remove this stale warning/hint.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>

Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events

[Masami Hiramatsu]

On Mon, 20 Mar 2017 14:42:22 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

[SNIP]
> >>  }
> >>  
> >> -/*
> >> - * Find the SDT event from the cache and if found add it/them
> >> - * to the uprobe_events file
> >> - */
> >> +static bool sdt_is_ptrn_used(struct perf_probe_event *pev)
> > This might be sdt_name_is_glob().
> 
> Hmm looks good. Will change it.
> 
> >> +{
> >> +	return !is_c_func_name(pev->group) || !is_c_func_name(pev->event);
> > Would you mean strisglob()@util.h ? :)
> 
> Yes, BUT this needs to be changed now.
> 
> I found perf probe accepts glob wildcards(*, ?) and character classes like
> [a-z]. But perf record only allow wildcards. So I can't use strisglob()
> function.

Oops, right. Hmm, Jiri, can we support full glob matching for events?
(or what happen if we use it?)

> 
> Please let me know if that is wrong.
> 
> >> +}
> >> +
> >> +static bool sdt_name_match(struct perf_probe_event *pev,
> >> +			   struct probe_trace_event *tev)
> >> +{
> >> +	if (sdt_is_ptrn_used(pev))
> >> +		return strglobmatch(tev->group, pev->group) &&
> >> +			strglobmatch(tev->event, pev->event);
> >> +
> >> +	return !strcmp(tev->group, pev->group) &&
> >> +		!strcmp(tev->event, pev->event);
> > Would we really need these two? Since strglobmatch() accepts a string
> > without wildcard, you don't need strcmp() version...
> 
> Hmm. yes, second part looks unnecessary.
> 
> >> +}
> >> +
> >> +static void sdt_warn_multi_events(int ctr, struct perf_probe_event *pev)
> >> +{
> >> +	pr_warning("Warning: Recording on %d occurrences of %s:%s\n",
> >> +		   ctr, pev->group, pev->event);
> > Could you show what event will be recorded instead of just showing 
> > the number of events?
> 
> Sure. I'll remove this warning and show all events as 'event addr@file'.
> Please let me know if you want to list it differently.
> 
> Actually, I do that, but partially. Please see patch 6/7. It list all those
> *existing* events which are being recorded.

OK, that will be enough for users to warn.

[SNIP]
> >> @@ -3606,23 +3727,37 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist)
> >>  	probe_conf.max_probes = MAX_PROBES;
> >>  	probe_conf.force_add = 1;
> >>  
> >> +	/* Fetch all sdt events from uprobe_events */
> >> +	exst_ntevs = probe_file__get_sdt_events(&exst_tevs);
> >> +	if (exst_ntevs < 0) {
> >> +		ret = exst_ntevs;
> >> +		goto free_pev;
> >> +	}
> >> +
> >> +	/* Check if events with same name already exists in uprobe_events. */
> >> +	ret = sdt_event_probepoint_exists(pev, exst_tevs,
> >> +					 exst_ntevs, sdt_evlist);
> >> +	if (ret) {
> >> +		ret = ret > 0 ? 0 : ret;
> >> +		goto free_pev;
> >> +	}
> >> +
> >>  	/* Fetch all matching events from cache. */
> >>  	ret = get_sdt_events_from_cache(pev);
> >>  	if (ret < 0)
> >>  		goto free_pev;
> > Hmm, IMHO, you'd better call get_sdt_events_from_cache() first, and
> > check the existence of those events afterwards. Then you may not
> > need the "shift" function.
> 
> "shift" function is needed. let me explain.
> 
> As we are allowing both, 'perf probe' as well as 'perf record' on SDT
> events, there are two sources of events for 'perf record'
>   1. uprobe_events file
>   2. probe-cache
> 
> When perf fetches events from cache, it fetch all matching events,
> no matter if any event already exists in uprobe_events or not.
> 
> Consider an example, There are 3 events with same name sdt_a:b
> exists in 'test' binary.
> 
>   $ readelf -n test
>     sdt_a:b @ addr 0x123
>     sdt_a:b @ addr 0x456
>     sdt_a:b @ addr 0x789
> 
>   $ perf probe -x test sdt_a:b
>      /* Add all 3 events sdt_a:b  sdt_a:b_1 and sdt_a:b_2 */
>   $ perf probe -d sdt_a:b
>   $ perf probe -d sdt_a:b_2
> 
>   $ perf record sdt_a:b
>     /*
>       arr1 = probe_file__get_sdt_events();
>                  {sdt_a:b_1 addr 0x456}
> 
>       arr2 = get_sdt_events_from_cache();
>                  {sdt_a:b addr 0x123, sdt_a:b addr 0x456, sdt_a:b addr 0x789}
> 
>       Now, as sdt event of addr 0x456 already exists in arr1, it needs to
>       be reused and thus it needs to be removed from arr2. Removing
>       2nd element needs shifting of third element to 2nd position.
>     */

Ah, I see. In that case, you can just set the tev->point.symbol = NULL,
then it is just skipped to apply in __add_probe_trace_events().

So what you need to do is, 1) get_sdt_events_from_cache(pev); and
2) set tev[x].point.symbol = NULL in sdt_event_probepoint_exists().
(I also recommend to release tev[x].event and group, then you can
 easilly find what events should be removed afterwards)

Thank you,



> 
> I don't think it's easy to remove "shift" function. I can remove it but
> that needs changes in existing infrastructure like fetch only those
> events from cache which are not present in uprobe_events. But,
> IMHO, it's not a good way to go.
> 
> Let me know if I misunderstood your point.
> 
> Thanks,
> Ravi
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>