Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1417162 > unrolled thread

[PATCH perf/core v10 00/23] perf-probe --cache and SDT support

Back to article view | Back to linux.kernel

Contents

  [PATCH perf/core v10 00/23] perf-probe --cache and SDT support Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:30 +0200
    [PATCH perf/core v10 02/23] perf-probe: Fix to add NULL check for strndup Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:30 +0200
    [PATCH perf/core v10 05/23] perf probe: Recover and export synthesize_perf_probe_point() Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 14/23] perf probe: Accept %sdt and %cached event name Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 18/23] perf probe: Allow wildcard for cached events Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 22/23] perf build: Add sdt feature detection Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 15/23] perf-list: Show SDT and pre-cached events Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 10/23] perf probe: Remove caches when --cache is given Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
      Re: [PATCH perf/core v10 10/23] perf probe: Remove caches when  --cache is given Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-06-09 16:30 +0200
        Re: [PATCH perf/core v10 10/23] perf probe: Remove caches when  --cache is given Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-11 03:20 +0200
    [PATCH perf/core v10 09/23] perf probe: Show all cached probes Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
      Re: [PATCH perf/core v10 09/23] perf probe: Show all cached probes Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-06-09 16:30 +0200
        Re: [PATCH perf/core v10 09/23] perf probe: Show all cached probes Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-11 02:30 +0200
    [PATCH perf/core v10 08/23] perf probe: Use cache entry if possible Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 12/23] perf probe: Add group name support Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 11/23] perf/sdt: ELF support for SDT Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 04/23] perf probe: Add perf_probe_event__copy() Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 21/23] perf probe: Support a special SDT probe format Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 16/23] perf-list: Skip SDTs placed in invalid binaries Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 13/23] perf buildid-cache: Scan and import user SDT events to probe cache Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 17/23] perf: probe-cache: Add for_each_probe_cache_entry() wrapper Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 23/23] perf-test: Add a test case for SDT event Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 19/23] perf probe: Search SDT/cached event from all probe caches Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 20/23] perf probe: Support @BUILDID or @FILE suffix for SDT events Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
    [PATCH perf/core v10 07/23] perf probe: Add --cache option to cache the probe definitions Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
      Re: [PATCH perf/core v10 07/23] perf probe: Add --cache option to  cache the probe definitions Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-06-09 16:20 +0200
        Re: [PATCH perf/core v10 07/23] perf probe: Add --cache option to  cache the probe definitions Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-11 01:40 +0200
    [PATCH perf/core v10 06/23] perf probe-file: Introduce perf_cache interfaces Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 11:40 +0200
      Re: [PATCH perf/core v10 06/23] perf probe-file: Introduce  perf_cache interfaces Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-06-09 16:20 +0200
        Re: [PATCH perf/core v10 06/23] perf probe-file: Introduce  perf_cache interfaces Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-11 00:20 +0200
    [PATCH perf/core v10 23/23] perf-test: Add a test case for SDT event Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 13:20 +0200
    Re: [PATCH perf/core v10 00/23] perf-probe --cache and SDT support Masami Hiramatsu <masami.hiramatsu@gmail.com> - 2016-06-08 13:20 +0200
    [PATCH perf/core v10 22/23] perf build: Add sdt feature detection Masami Hiramatsu <mhiramat@kernel.org> - 2016-06-08 13:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1417184 — [PATCH perf/core v10 17/23] perf: probe-cache: Add for_each_probe_cache_entry() wrapper

Add for_each_probe_cache_entry() wrapper macro
for hiding list in probe_cache.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v10:
  - Splitted from "perf probe: Allow wildcard for cached events"
---
 tools/perf/util/probe-file.c |    8 ++++----
 tools/perf/util/probe-file.h |    2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 4d8ba72..7b60380 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -537,7 +537,7 @@ probe_cache__find(struct probe_cache *pcache, struct perf_probe_event *pev)
 	if (!cmd)
 		return NULL;
 
-	list_for_each_entry(entry, &pcache->list, list) {
+	for_each_probe_cache_entry(entry, pcache) {
 		if (pev->sdt) {
 			if (entry->pev.event &&
 			    streql(entry->pev.event, pev->event) &&
@@ -567,7 +567,7 @@ probe_cache__find_by_name(struct probe_cache *pcache,
 {
 	struct probe_cache_entry *entry = NULL;
 
-	list_for_each_entry(entry, &pcache->list, list) {
+	for_each_probe_cache_entry(entry, pcache) {
 		/* Hit if same event name or same command-string */
 		if (streql(entry->pev.group, group) &&
 		    streql(entry->pev.event, event))
@@ -720,7 +720,7 @@ int probe_cache__commit(struct probe_cache *pcache)
 	if (ret < 0)
 		goto out;
 
-	list_for_each_entry(entry, &pcache->list, list) {
+	for_each_probe_cache_entry(entry, pcache) {
 		ret = probe_cache_entry__write(entry, pcache->fd);
 		pr_debug("Cache committed: %d\n", ret);
 		if (ret < 0)
@@ -761,7 +761,7 @@ static int probe_cache__show_entries(struct probe_cache *pcache,
 {
 	struct probe_cache_entry *entry;
 
-	list_for_each_entry(entry, &pcache->list, list) {
+	for_each_probe_cache_entry(entry, pcache) {
 		if (probe_cache_entry__compare(entry, filter))
 			printf("%s\n", entry->spev);
 	}
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index ce98162..6271ec6 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -21,6 +21,8 @@ struct probe_cache {
 
 #define PF_FL_UPROBE	1
 #define PF_FL_RW	2
+#define for_each_probe_cache_entry(entry, pcache) \
+	list_for_each_entry(entry, &pcache->list, list)
 
 /* probe-file.c depends on libelf */
 #ifdef HAVE_LIBELF_SUPPORT

[toc] | [prev] | [next] | [standalone]

#1417185 — [PATCH perf/core v10 23/23] perf-test: Add a test case for SDT event

Add a basic test case for SDT event support.
This test scans an SDT event in perftools and
check whether the SDT event is correctly stored
into the buildid cache.
---
 tools/perf/tests/Build          |    1 
 tools/perf/tests/builtin-test.c |    4 +
 tools/perf/tests/sdt.c          |  114 +++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |    1 
 4 files changed, 120 insertions(+)
 create mode 100644 tools/perf/tests/sdt.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 66a2898..4158422 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -39,6 +39,7 @@ perf-y += stat.o
 perf-y += event_update.o
 perf-y += event-times.o
 perf-y += backward-ring-buffer.o
+perf-y += sdt.o
 
 $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
 	$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 0e95c20..c0e0ccb 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -212,6 +212,10 @@ static struct test generic_tests[] = {
 		.func = test__backward_ring_buffer,
 	},
 	{
+		.desc = "Test SDT event probing",
+		.func = test__sdt_event,
+	},
+	{
 		.func = NULL,
 	},
 };
diff --git a/tools/perf/tests/sdt.c b/tools/perf/tests/sdt.c
new file mode 100644
index 0000000..d11065e
--- /dev/null
+++ b/tools/perf/tests/sdt.c
@@ -0,0 +1,114 @@
+#include <stdio.h>
+#include <sys/epoll.h>
+#include <util/util.h>
+#include <util/evlist.h>
+#include <linux/filter.h>
+#include "tests.h"
+#include "debug.h"
+#include "probe-file.h"
+#include "build-id.h"
+
+/* To test SDT event, we need libelf support to scan elf binary */
+#if defined(HAVE_SDT_EVENT) && defined(HAVE_LIBELF_SUPPORT)
+
+#include <sys/sdt.h>
+
+static int target_function(void)
+{
+	DTRACE_PROBE(perf, test_target);
+	return TEST_OK;
+}
+
+/* Copied from builtin-buildid-cache.c */
+static int build_id_cache__add_file(const char *filename)
+{
+	char sbuild_id[SBUILD_ID_SIZE];
+	u8 build_id[BUILD_ID_SIZE];
+	int err;
+
+	err = filename__read_build_id(filename, &build_id, sizeof(build_id));
+	if (err < 0) {
+		pr_debug("Failed to read build id of %s\n", filename);
+		return err;
+	}
+
+	build_id__sprintf(build_id, sizeof(build_id), sbuild_id);
+	err = build_id_cache__add_s(sbuild_id, filename, false, false);
+	if (err < 0)
+		pr_debug("Failed to add build id cache of %s\n", filename);
+	return err;
+}
+
+static char *get_self_path(void)
+{
+	char *buf = calloc(PATH_MAX, sizeof(char));
+	if (buf && readlink("/proc/self/exe", buf, PATH_MAX) < 0) {
+		pr_debug("Failed to get correct path of perf\n");
+		free(buf);
+		return NULL;
+	}
+	return buf;
+}
+
+static int search_cached_probe(const char *target,
+			       const char *group, const char *event)
+{
+	struct probe_cache *cache = probe_cache__new(target);
+	int ret = 0;
+
+	if (!cache) {
+		pr_debug("Failed to open probe cache of %s\n", target);
+		return -EINVAL;
+	}
+
+	if (!probe_cache__find_by_name(cache, group, event)) {
+		pr_debug("Failed to find %s:%s in the cache\n", group, event);
+		ret = -ENOENT;
+	}
+	probe_cache__delete(cache);
+
+	return ret;
+}
+
+int test__sdt_event(int subtests __maybe_unused)
+{
+	int ret = TEST_FAIL;
+	char __tempdir[] = "./test-buildid-XXXXXX";
+	char *tempdir = NULL, *myself = get_self_path();
+
+	if (myself == NULL || mkdtemp(__tempdir) == NULL) {
+		pr_debug("Failed to make a tempdir for build-id cache\n");
+		goto error;
+	}
+	/* Note that buildid_dir must be an absolute path */
+	tempdir = realpath(__tempdir, NULL);
+
+	/* At first, scan itself */
+	set_buildid_dir(tempdir);
+	if (build_id_cache__add_file(myself) < 0)
+		goto error_rmdir;
+
+	/* Open a cache and make sure the SDT is stored */
+	if (search_cached_probe(myself, "sdt_perf", "test_target") < 0)
+		goto error_rmdir;
+
+	/* TBD: probing on the SDT event and collect logs */
+
+	/* Call the target and get an event */
+	ret = target_function();
+
+error_rmdir:
+	/* Cleanup temporary buildid dir */
+	rm_rf(tempdir);
+error:
+	free(tempdir);
+	free(myself);
+	return ret;
+}
+#else
+int test__sdt_event(int subtests __maybe_unused)
+{
+        pr_debug("Skip SDT event test because SDT support is not compiled\n");
+        return TEST_SKIP;
+}
+#endif
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index c57e72c..93cfb0b 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -87,6 +87,7 @@ int test__synthesize_stat_round(int subtest);
 int test__event_update(int subtest);
 int test__event_times(int subtest);
 int test__backward_ring_buffer(int subtest);
+int test__sdt_event(int subtest);
 
 #if defined(__arm__) || defined(__aarch64__)
 #ifdef HAVE_DWARF_UNWIND_SUPPORT

[toc] | [prev] | [next] | [standalone]

#1417187 — [PATCH perf/core v10 19/23] perf probe: Search SDT/cached event from all probe caches

Search SDT/cached event from all probe caches if user doesn't
pass any binary. With this, we don't have to specify target
binary for SDT and named cached events (which start with %).

E.g. without this, a target binary must be passed with -x.

 # perf probe -x /usr/lib64/libc-2.20.so -a %sdt_libc:\*

With this change, we don't need it anymore.

 # perf probe -a %sdt_libc:\*

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes from v10:
  - Splitted from "perf probe: Allow wildcard for cached events"
---
 tools/perf/util/probe-event.c |  104 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 19 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index f692fd5..d30c307 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2557,41 +2557,60 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
 	return 0;
 }
 
-static int __add_probe_trace_events(struct perf_probe_event *pev,
-				     struct probe_trace_event *tevs,
-				     int ntevs, bool allow_suffix)
+static int __open_probe_file_and_namelist(bool uprobe,
+					  struct strlist **namelist)
 {
-	int i, fd, ret;
-	struct probe_trace_event *tev = NULL;
-	struct probe_cache *cache = NULL;
-	struct strlist *namelist;
+	int fd;
 
-	fd = probe_file__open(PF_FL_RW | (pev->uprobes ? PF_FL_UPROBE : 0));
+	fd = probe_file__open(PF_FL_RW | (uprobe ? PF_FL_UPROBE : 0));
 	if (fd < 0)
 		return fd;
 
 	/* Get current event names */
-	namelist = probe_file__get_namelist(fd);
-	if (!namelist) {
+	*namelist = probe_file__get_namelist(fd);
+	if (!(*namelist)) {
 		pr_debug("Failed to get current event list.\n");
-		ret = -ENOMEM;
-		goto close_out;
+		close(fd);
+		return -ENOMEM;
 	}
+	return fd;
+}
+
+static int __add_probe_trace_events(struct perf_probe_event *pev,
+				     struct probe_trace_event *tevs,
+				     int ntevs, bool allow_suffix)
+{
+	int i, fd[2] = {-1, -1}, up, ret;
+	struct probe_trace_event *tev = NULL;
+	struct probe_cache *cache = NULL;
+	struct strlist *namelist[2] = {NULL, NULL};
+
+	up = pev->uprobes ? 1 : 0;
+	fd[up] = __open_probe_file_and_namelist(up, &namelist[up]);
+	if (fd[up] < 0)
+		return fd[up];
 
 	ret = 0;
 	for (i = 0; i < ntevs; i++) {
 		tev = &tevs[i];
+		up = tev->uprobes ? 1 : 0;
+		if (fd[up] == -1) {	/* Open the kprobe/uprobe_events */
+			fd[up] = __open_probe_file_and_namelist(up,
+								&namelist[up]);
+			if (fd[up] < 0)
+				goto close_out;
+		}
 		/* Skip if the symbol is out of .text or blacklisted */
 		if (!tev->point.symbol && !pev->uprobes)
 			continue;
 
 		/* Set new name for tev (and update namelist) */
-		ret = probe_trace_event__set_name(tev, pev, namelist,
+		ret = probe_trace_event__set_name(tev, pev, namelist[up],
 						  allow_suffix);
 		if (ret < 0)
 			break;
 
-		ret = probe_file__add_event(fd, tev);
+		ret = probe_file__add_event(fd[up], tev);
 		if (ret < 0)
 			break;
 
@@ -2614,9 +2633,12 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 		}
 	}
 
-	strlist__delete(namelist);
 close_out:
-	close(fd);
+	for (up = 0; up < 2; up++) {
+		strlist__delete(namelist[up]);
+		if (fd[up] >= 0)
+			close(fd[up]);
+	}
 	return ret;
 }
 
@@ -2989,6 +3011,47 @@ static int find_cached_events(struct perf_probe_event *pev,
 	return ret;
 }
 
+/* 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)
+{
+	struct probe_trace_event *tmp_tevs = NULL;
+	struct strlist *bidlist;
+	struct str_node *nd;
+	char *pathname;
+	int ntevs = 0;
+	int ret;
+
+	/* Get the buildid list of all valid caches */
+	ret = build_id_cache__list_all(&bidlist, true);
+	if (ret < 0) {
+		pr_debug("Failed to get buildids: %d\n", ret);
+		return ret;
+	}
+
+	ret = 0;
+	strlist__for_each(nd, bidlist) {
+		pathname = build_id_cache__origname(nd->s);
+		ret = find_cached_events(pev, &tmp_tevs, pathname);
+		/* In the case of cnt == 0, we just skip it */
+		if (ret > 0)
+			ret = concat_probe_trace_events(tevs, &ntevs,
+							&tmp_tevs, ret);
+		free(pathname);
+		if (ret < 0)
+			break;
+	}
+	strlist__delete(bidlist);
+
+	if (ret < 0) {
+		clear_probe_trace_events(*tevs, ntevs);
+		zfree(tevs);
+	} else
+		ret = ntevs;
+
+	return ret;
+}
+
 static int find_probe_trace_events_from_cache(struct perf_probe_event *pev,
 					      struct probe_trace_event **tevs)
 {
@@ -2998,10 +3061,13 @@ static int find_probe_trace_events_from_cache(struct perf_probe_event *pev,
 	struct str_node *node;
 	int ret, i;
 
-	if (pev->sdt)
+	if (pev->sdt) {
 		/* For SDT/cached events, we use special search functions */
-		return find_cached_events(pev, tevs, pev->target);
-
+		if (!pev->target)
+			return find_cached_events_all(pev, tevs);
+		else
+			return find_cached_events(pev, tevs, pev->target);
+	}
 	cache = probe_cache__new(pev->target);
 	if (!cache)
 		return 0;

[toc] | [prev] | [next] | [standalone]

#1417188 — [PATCH perf/core v10 20/23] perf probe: Support @BUILDID or @FILE suffix for SDT events

Support @BUILDID or @FILE suffix for SDT events. This allows
perf to add probes on SDTs/pre-cached events on given FILE
or the file which has given BUILDID (also, this complements
BUILDID.)
For example, both gcc and libstdc++ has same SDTs as below.
If you would like to add a probe on sdt_libstdcxx:catch on gcc,
you can do as below.

  ----
  # perf list sdt | tail -n 6
    sdt_libstdcxx:catch@/usr/bin/gcc(0cc207fc4b27)     [SDT event]
    sdt_libstdcxx:catch@/usr/lib64/libstdc++.so.6.0.20(91c7a88fdf49)
    sdt_libstdcxx:rethrow@/usr/bin/gcc(0cc207fc4b27)   [SDT event]
    sdt_libstdcxx:rethrow@/usr/lib64/libstdc++.so.6.0.20(91c7a88fdf49)
    sdt_libstdcxx:throw@/usr/bin/gcc(0cc207fc4b27)     [SDT event]
    sdt_libstdcxx:throw@/usr/lib64/libstdc++.so.6.0.20(91c7a88fdf49)
  # perf probe -a %sdt_libstdcxx:catch@0cc
  Added new event:
    sdt_libstdcxx:catch  (on %catch in /usr/bin/gcc)

  You can now use it in all perf tools, such as:

  	perf record -e sdt_libstdcxx:catch -aR sleep 1
  ----

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/build-id.c    |   40 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/build-id.h    |    1 +
 tools/perf/util/probe-event.c |   16 ++++++++++++++--
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 424f4e2..c32931c 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -504,6 +504,46 @@ out:
 	return ret;
 }
 
+static bool str_is_build_id(const char *maybe_sbuild_id, size_t len)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++) {
+		if (!isxdigit(maybe_sbuild_id[i]))
+			return false;
+	}
+	return true;
+}
+
+/* Return the valid complete build-id */
+char *build_id_cache__complement(const char *incomplete_sbuild_id)
+{
+	struct strlist *list;
+	struct str_node *nd, *cand = NULL;
+	char *sbuild_id = NULL;
+	size_t len = strlen(incomplete_sbuild_id);
+
+	if (len >= SBUILD_ID_SIZE ||
+	    !str_is_build_id(incomplete_sbuild_id, len) ||
+	    build_id_cache__list_all(&list, true) < 0)
+		return NULL;
+
+	strlist__for_each(nd, list) {
+		if (strncmp(nd->s, incomplete_sbuild_id, len) != 0)
+			continue;
+		if (cand) {	/* Error: There are more than 2 candidates. */
+			cand = NULL;
+			break;
+		}
+		cand = nd;
+	}
+	if (cand)
+		sbuild_id = strdup(cand->s);
+	strlist__delete(list);
+
+	return sbuild_id;
+}
+
 char *build_id_cache__cachedir(const char *sbuild_id, const char *name,
 			       bool is_kallsyms, bool is_vdso)
 {
diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index 480600b..9bd6d0b 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -35,6 +35,7 @@ char *build_id_cache__linkname(const char *sbuild_id, char *bf, size_t size);
 char *build_id_cache__cachedir(const char *sbuild_id, const char *name,
 			       bool is_kallsyms, bool is_vdso);
 int build_id_cache__list_all(struct strlist **result, bool validonly);
+char *build_id_cache__complement(const char *incomplete_sbuild_id);
 int build_id_cache__list_build_ids(const char *pathname,
 				   struct strlist **result);
 bool build_id_cache__cached(const char *sbuild_id);
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index d30c307..6d3398b 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1253,8 +1253,20 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
 	ptr = strpbrk(arg, ";=@+%");
 	if (pev->sdt) {
 		if (ptr) {
-			semantic_error("%s must contain only an SDT event name.\n", arg);
-			return -EINVAL;
+			if (*ptr != '@') {
+				semantic_error("%s must contain only an SDT event name.\n", arg);
+				return -EINVAL;
+			}
+			/* This must be a target file name or build id */
+			tmp = build_id_cache__complement(ptr + 1);
+			if (tmp) {
+				pev->target = build_id_cache__origname(tmp);
+				free(tmp);
+			} else
+				pev->target = strdup(ptr + 1);
+			if (!pev->target)
+				return -ENOMEM;
+			*ptr = '\0';
 		}
 		ret = parse_perf_probe_event_name(&arg, pev);
 		if (ret == 0) {

[toc] | [prev] | [next] | [standalone]

#1417189 — [PATCH perf/core v10 07/23] perf probe: Add --cache option to cache the probe definitions

From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Add --cache option to cache the probe definitions. This
just saves the result of the dwarf analysis to probe cache.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v10:
  - Split into 5 subpatches.
 Changes in v6:
  - Remove unneeded O_APPEND from open(). (Thanks Namhyung!)
  - Fix to check the return value of probe_cache_entry__new and strdup.(ditto)
 Changes in v5:
  - Move probe_cache* definitions. (code cleanup)

 Changes in v4:
  - Remove cache saving failure message.
---
 tools/perf/Documentation/perf-probe.txt |    4 ++++
 tools/perf/builtin-probe.c              |    1 +
 tools/perf/util/probe-event.c           |    9 +++++++++
 tools/perf/util/probe-event.h           |    1 +
 4 files changed, 15 insertions(+)

diff --git a/tools/perf/Documentation/perf-probe.txt b/tools/perf/Documentation/perf-probe.txt
index 3a8a9ba..947db6f 100644
--- a/tools/perf/Documentation/perf-probe.txt
+++ b/tools/perf/Documentation/perf-probe.txt
@@ -109,6 +109,10 @@ OPTIONS
 	Dry run. With this option, --add and --del doesn't execute actual
 	adding and removal operations.
 
+--cache::
+	Cache the probes (with --add option). Any events which successfully added
+	are also stored in the cache file.
+
 --max-probes=NUM::
 	Set the maximum number of probe points for an event. Default is 128.
 
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 9af859b..6d7ab431 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -512,6 +512,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "Enable symbol demangling"),
 	OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
 		    "Enable kernel symbol demangling"),
+	OPT_BOOLEAN(0, "cache", &probe_conf.cache, "Manipulate probe cache"),
 	OPT_END()
 	};
 	int ret;
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index cbc8a8b..7cfd1fa 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2514,6 +2514,7 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 {
 	int i, fd, ret;
 	struct probe_trace_event *tev = NULL;
+	struct probe_cache *cache = NULL;
 	struct strlist *namelist;
 
 	fd = probe_file__open(PF_FL_RW | (pev->uprobes ? PF_FL_UPROBE : 0));
@@ -2555,6 +2556,14 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 	}
 	if (ret == -EINVAL && pev->uprobes)
 		warn_uprobe_event_compat(tev);
+	if (ret == 0 && probe_conf.cache) {
+		cache = probe_cache__new(pev->target);
+		if (cache) {
+			probe_cache__add_entry(cache, pev, tevs, ntevs);
+			probe_cache__commit(cache);
+			probe_cache__delete(cache);
+		}
+	}
 
 	strlist__delete(namelist);
 close_out:
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index 0b024ba..432b690 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -12,6 +12,7 @@ struct probe_conf {
 	bool	show_location_range;
 	bool	force_add;
 	bool	no_inlines;
+	bool	cache;
 	int	max_probes;
 };
 extern struct probe_conf probe_conf;

[toc] | [prev] | [next] | [standalone]

#1418369 — Re: [PATCH perf/core v10 07/23] perf probe: Add --cache option to cache the probe definitions

Em Wed, Jun 08, 2016 at 06:30:10PM +0900, Masami Hiramatsu escreveu:
> @@ -2555,6 +2556,14 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>  	}
>  	if (ret == -EINVAL && pev->uprobes)
>  		warn_uprobe_event_compat(tev);
> +	if (ret == 0 && probe_conf.cache) {
> +		cache = probe_cache__new(pev->target);
> +		if (cache) {
> +			probe_cache__add_entry(cache, pev, tevs, ntevs);
> +			probe_cache__commit(cache);

These two functions may fail, please check its return and forward errors
appropriately.

> +			probe_cache__delete(cache);

[toc] | [prev] | [next] | [standalone]

#1419837 — Re: [PATCH perf/core v10 07/23] perf probe: Add --cache option to cache the probe definitions

On Thu, 9 Jun 2016 11:18:50 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Wed, Jun 08, 2016 at 06:30:10PM +0900, Masami Hiramatsu escreveu:
> > @@ -2555,6 +2556,14 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
> >  	}
> >  	if (ret == -EINVAL && pev->uprobes)
> >  		warn_uprobe_event_compat(tev);
> > +	if (ret == 0 && probe_conf.cache) {
> > +		cache = probe_cache__new(pev->target);
> > +		if (cache) {
> > +			probe_cache__add_entry(cache, pev, tevs, ntevs);
> > +			probe_cache__commit(cache);
> 
> These two functions may fail, please check its return and forward errors
> appropriately.

OK, will check and warn that.

Thank you!
-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1417191 — [PATCH perf/core v10 06/23] perf probe-file: Introduce perf_cache interfaces

Introduce perf_cache object and interfaces to create,
add entry, commit, and delete the object.
perf_cache represents a file for the cached perf-probe
definitions on one binary file or vmlinux which has its
own build id. The probe cache file is located under the
build-id cache directory of the target binary, as below;

 <perf-debug-dir>/.build-id/<BU>/<ILDID>/probe

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v10:
  - Splited from "Add --cache option to cache the probe definitions"
---
 tools/perf/util/probe-file.c |  307 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/probe-file.h |   19 +++
 2 files changed, 326 insertions(+)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 3fe6214..689d874 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -14,6 +14,7 @@
  * GNU General Public License for more details.
  *
  */
+#include <sys/uio.h>
 #include "util.h"
 #include "event.h"
 #include "strlist.h"
@@ -324,3 +325,309 @@ int probe_file__del_events(int fd, struct strfilter *filter)
 
 	return ret;
 }
+
+static void probe_cache_entry__delete(struct probe_cache_entry *node)
+{
+	if (!list_empty(&node->list))
+		list_del(&node->list);
+	if (node->tevlist)
+		strlist__delete(node->tevlist);
+	clear_perf_probe_event(&node->pev);
+	free(node->spev);
+	free(node);
+}
+
+static struct probe_cache_entry *
+probe_cache_entry__new(struct perf_probe_event *pev)
+{
+	struct probe_cache_entry *ret = zalloc(sizeof(*ret));
+
+	if (ret) {
+		INIT_LIST_HEAD(&ret->list);
+		ret->tevlist = strlist__new(NULL, NULL);
+		if (!ret->tevlist)
+			zfree(&ret);
+		if (ret && pev) {
+			ret->spev = synthesize_perf_probe_command(pev);
+			if (!ret->spev ||
+			    perf_probe_event__copy(&ret->pev, pev) < 0) {
+				probe_cache_entry__delete(ret);
+				return NULL;
+			}
+		}
+	}
+
+	return ret;
+}
+
+/* For the kernel probe caches, pass target = NULL */
+static int probe_cache__open(struct probe_cache *pcache, const char *target)
+{
+	char cpath[PATH_MAX];
+	char sbuildid[SBUILD_ID_SIZE];
+	char *dir_name;
+	bool is_kallsyms = !target;
+	int ret, fd;
+
+	if (target)
+		ret = filename__sprintf_build_id(target, sbuildid);
+	else {
+		target = DSO__NAME_KALLSYMS;
+		ret = sysfs__sprintf_build_id("/", sbuildid);
+	}
+	if (ret < 0) {
+		pr_debug("Failed to get build-id from %s.\n", target);
+		return ret;
+	}
+
+	/* If we have no buildid cache, make it */
+	if (!build_id_cache__cached(sbuildid)) {
+		ret = build_id_cache__add_s(sbuildid, target,
+					    is_kallsyms, NULL);
+		if (ret < 0) {
+			pr_debug("Failed to add build-id cache: %s\n", target);
+			return ret;
+		}
+	}
+
+	dir_name = build_id_cache__cachedir(sbuildid, target, is_kallsyms,
+					    false);
+	if (!dir_name)
+		return -ENOMEM;
+
+	snprintf(cpath, PATH_MAX, "%s/probes", dir_name);
+	fd = open(cpath, O_CREAT | O_RDWR, 0644);
+	if (fd < 0)
+		pr_debug("Failed to open cache(%d): %s\n", fd, cpath);
+	free(dir_name);
+	pcache->fd = fd;
+
+	return fd;
+}
+
+static int probe_cache__load(struct probe_cache *pcache)
+{
+	struct probe_cache_entry *entry = NULL;
+	char buf[MAX_CMDLEN], *p;
+	int ret = 0;
+	FILE *fp;
+
+	fp = fdopen(dup(pcache->fd), "r");
+	while (!feof(fp)) {
+		if (!fgets(buf, MAX_CMDLEN, fp))
+			break;
+		p = strchr(buf, '\n');
+		if (p)
+			*p = '\0';
+		if (buf[0] == '#') {	/* #perf_probe_event */
+			entry = probe_cache_entry__new(NULL);
+			if (!entry) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			entry->spev = strdup(buf + 1);
+			if (entry->spev)
+				ret = parse_perf_probe_command(buf + 1,
+								&entry->pev);
+			else
+				ret = -ENOMEM;
+			if (ret < 0) {
+				probe_cache_entry__delete(entry);
+				goto out;
+			}
+			list_add_tail(&entry->list, &pcache->list);
+		} else {	/* trace_probe_event */
+			if (!entry) {
+				ret = -EINVAL;
+				goto out;
+			}
+			strlist__add(entry->tevlist, buf);
+		}
+	}
+out:
+	fclose(fp);
+	return ret;
+}
+
+static struct probe_cache *probe_cache__alloc(void)
+{
+	struct probe_cache *ret = zalloc(sizeof(*ret));
+
+	if (ret) {
+		INIT_LIST_HEAD(&ret->list);
+		ret->fd = -EINVAL;
+	}
+	return ret;
+}
+
+void probe_cache__delete(struct probe_cache *pcache)
+{
+	struct probe_cache_entry *entry;
+
+	if (!pcache)
+		return;
+
+	while (!list_empty(&pcache->list)) {
+		entry = list_first_entry(&pcache->list, typeof(*entry), list);
+		probe_cache_entry__delete(entry);
+	}
+	if (pcache->fd > 0)
+		close(pcache->fd);
+	free(pcache);
+}
+
+struct probe_cache *probe_cache__new(const char *target)
+{
+	struct probe_cache *pcache = probe_cache__alloc();
+	int ret;
+
+	if (!pcache)
+		return NULL;
+
+	ret = probe_cache__open(pcache, target);
+	if (ret < 0) {
+		pr_debug("Cache open error: %d\n", ret);
+		goto out_err;
+	}
+
+	ret = probe_cache__load(pcache);
+	if (ret < 0) {
+		pr_debug("Cache read error: %d\n", ret);
+		goto out_err;
+	}
+
+	return pcache;
+
+out_err:
+	probe_cache__delete(pcache);
+	return NULL;
+}
+
+static bool streql(const char *a, const char *b)
+{
+	if (a == b)
+		return true;
+
+	if (!a || !b)
+		return false;
+
+	return !strcmp(a, b);
+}
+
+static struct probe_cache_entry *
+probe_cache__find(struct probe_cache *pcache, struct perf_probe_event *pev)
+{
+	struct probe_cache_entry *entry = NULL;
+	char *cmd = NULL;
+
+	cmd = synthesize_perf_probe_command(pev);
+	if (!cmd)
+		return NULL;
+
+	list_for_each_entry(entry, &pcache->list, list) {
+		/* Hit if same event name or same command-string */
+		if ((pev->event &&
+		     (streql(entry->pev.group, pev->group) &&
+		      streql(entry->pev.event, pev->event))) ||
+		    (!strcmp(entry->spev, cmd)))
+			goto found;
+	}
+	entry = NULL;
+
+found:
+	free(cmd);
+	return entry;
+}
+
+int probe_cache__add_entry(struct probe_cache *pcache,
+			   struct perf_probe_event *pev,
+			   struct probe_trace_event *tevs, int ntevs)
+{
+	struct probe_cache_entry *entry = NULL;
+	char *command;
+	int i, ret = 0;
+
+	if (!pcache || !pev || !tevs || ntevs <= 0) {
+		ret = -EINVAL;
+		goto out_err;
+	}
+
+	/* Remove old cache entry */
+	entry = probe_cache__find(pcache, pev);
+	if (entry)
+		probe_cache_entry__delete(entry);
+
+	ret = -ENOMEM;
+	entry = probe_cache_entry__new(pev);
+	if (!entry)
+		goto out_err;
+
+	for (i = 0; i < ntevs; i++) {
+		if (!tevs[i].point.symbol)
+			continue;
+
+		command = synthesize_probe_trace_command(&tevs[i]);
+		if (!command)
+			goto out_err;
+		strlist__add(entry->tevlist, command);
+		free(command);
+	}
+	list_add_tail(&entry->list, &pcache->list);
+	pr_debug("Added probe cache: %d\n", ntevs);
+	return 0;
+
+out_err:
+	pr_debug("Failed to add probe caches\n");
+	if (entry)
+		probe_cache_entry__delete(entry);
+	return ret;
+}
+
+static int probe_cache_entry__write(struct probe_cache_entry *entry, int fd)
+{
+	struct str_node *snode;
+	struct iovec iov[3];
+	int ret;
+
+	pr_debug("Writing cache: #%s\n", entry->spev);
+	iov[0].iov_base = (void *)"#"; iov[0].iov_len = 1;
+	iov[1].iov_base = entry->spev; iov[1].iov_len = strlen(entry->spev);
+	iov[2].iov_base = (void *)"\n"; iov[2].iov_len = 1;
+	ret = writev(fd, iov, 3);
+	if (ret < 0)
+		return ret;
+
+	strlist__for_each(snode, entry->tevlist) {
+		iov[0].iov_base = (void *)snode->s;
+		iov[0].iov_len = strlen(snode->s);
+		iov[1].iov_base = (void *)"\n"; iov[1].iov_len = 1;
+		ret = writev(fd, iov, 2);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
+int probe_cache__commit(struct probe_cache *pcache)
+{
+	struct probe_cache_entry *entry;
+	int ret = 0;
+
+	/* TBD: if we do not update existing entries, skip it */
+	ret = lseek(pcache->fd, 0, SEEK_SET);
+	if (ret < 0)
+		goto out;
+
+	ret = ftruncate(pcache->fd, 0);
+	if (ret < 0)
+		goto out;
+
+	list_for_each_entry(entry, &pcache->list, list) {
+		ret = probe_cache_entry__write(entry, pcache->fd);
+		pr_debug("Cache committed: %d\n", ret);
+		if (ret < 0)
+			break;
+	}
+out:
+	return ret;
+}
diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
index 18ac9cf..d2b8791d 100644
--- a/tools/perf/util/probe-file.h
+++ b/tools/perf/util/probe-file.h
@@ -5,6 +5,19 @@
 #include "strfilter.h"
 #include "probe-event.h"
 
+/* Cache of probe definitions */
+struct probe_cache_entry {
+	struct list_head	list;
+	struct perf_probe_event pev;
+	char			*spev;
+	struct strlist		*tevlist;
+};
+
+struct probe_cache {
+	int	fd;
+	struct list_head list;
+};
+
 #define PF_FL_UPROBE	1
 #define PF_FL_RW	2
 
@@ -18,5 +31,11 @@ int probe_file__get_events(int fd, struct strfilter *filter,
 				  struct strlist *plist);
 int probe_file__del_strlist(int fd, struct strlist *namelist);
 
+struct probe_cache *probe_cache__new(const char *target);
+int probe_cache__add_entry(struct probe_cache *pcache,
+			   struct perf_probe_event *pev,
+			   struct probe_trace_event *tevs, int ntevs);
+int probe_cache__commit(struct probe_cache *pcache);
+void probe_cache__delete(struct probe_cache *pcache);
 
 #endif

[toc] | [prev] | [next] | [standalone]

#1418375 — Re: [PATCH perf/core v10 06/23] perf probe-file: Introduce perf_cache interfaces

Em Wed, Jun 08, 2016 at 06:29:59PM +0900, Masami Hiramatsu escreveu:
> Introduce perf_cache object and interfaces to create,
> add entry, commit, and delete the object.
> perf_cache represents a file for the cached perf-probe
> definitions on one binary file or vmlinux which has its
> own build id. The probe cache file is located under the
> build-id cache directory of the target binary, as below;
> 
>  <perf-debug-dir>/.build-id/<BU>/<ILDID>/probe
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  Changes in v10:
>   - Splited from "Add --cache option to cache the probe definitions"
> ---
>  tools/perf/util/probe-file.c |  307 ++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/probe-file.h |   19 +++
>  2 files changed, 326 insertions(+)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 3fe6214..689d874 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -14,6 +14,7 @@
>   * GNU General Public License for more details.
>   *
>   */
> +#include <sys/uio.h>
>  #include "util.h"
>  #include "event.h"
>  #include "strlist.h"
> @@ -324,3 +325,309 @@ int probe_file__del_events(int fd, struct strfilter *filter)
>  
>  	return ret;
>  }
> +
> +static void probe_cache_entry__delete(struct probe_cache_entry *node)
> +{
> +	if (!list_empty(&node->list))
> +		list_del(&node->list);

Humm, shouldn't this be something like:

BUG_ON(!list_empty(&node->list)

?

I.e. whoever inserted this on a list should take care of removing it
before deleting it, taking locks, whatever is needed to keep the
integrity of such list.

You may not be using this stuff in a multithreaded app now, but lets not
make it difficult to :-)

I noticed why you do it this way and have a suggestion to use a more
usual model, see below.

> +	if (node->tevlist)
> +		strlist__delete(node->tevlist);

No checking, destructors generally follows the free() model, i.e. they
eat NULL for breakfast. Lemme see if strlist does that... Yes, they do.

> +	clear_perf_probe_event(&node->pev);
> +	free(node->spev);

Here you may want to use:

	zfree(&node->spev);

To free and set node->spev to NULL, to help in debugging when this node
may be still referenced even having being deleted.

> +	free(node);
> +}
> +
> +static struct probe_cache_entry *
> +probe_cache_entry__new(struct perf_probe_event *pev)
> +{
> +	struct probe_cache_entry *ret = zalloc(sizeof(*ret));
> +
> +	if (ret) {
> +		INIT_LIST_HEAD(&ret->list);
> +		ret->tevlist = strlist__new(NULL, NULL);
> +		if (!ret->tevlist)
> +			zfree(&ret);
> +		if (ret && pev) {
> +			ret->spev = synthesize_perf_probe_command(pev);
> +			if (!ret->spev ||
> +			    perf_probe_event__copy(&ret->pev, pev) < 0) {
> +				probe_cache_entry__delete(ret);
> +				return NULL;
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/* For the kernel probe caches, pass target = NULL */
> +static int probe_cache__open(struct probe_cache *pcache, const char *target)
> +{
> +	char cpath[PATH_MAX];
> +	char sbuildid[SBUILD_ID_SIZE];
> +	char *dir_name;
> +	bool is_kallsyms = !target;
> +	int ret, fd;
> +
> +	if (target)
> +		ret = filename__sprintf_build_id(target, sbuildid);
> +	else {
> +		target = DSO__NAME_KALLSYMS;
> +		ret = sysfs__sprintf_build_id("/", sbuildid);
> +	}
> +	if (ret < 0) {
> +		pr_debug("Failed to get build-id from %s.\n", target);
> +		return ret;
> +	}
> +
> +	/* If we have no buildid cache, make it */
> +	if (!build_id_cache__cached(sbuildid)) {
> +		ret = build_id_cache__add_s(sbuildid, target,
> +					    is_kallsyms, NULL);
> +		if (ret < 0) {
> +			pr_debug("Failed to add build-id cache: %s\n", target);
> +			return ret;
> +		}
> +	}
> +
> +	dir_name = build_id_cache__cachedir(sbuildid, target, is_kallsyms,
> +					    false);
> +	if (!dir_name)
> +		return -ENOMEM;
> +
> +	snprintf(cpath, PATH_MAX, "%s/probes", dir_name);
> +	fd = open(cpath, O_CREAT | O_RDWR, 0644);
> +	if (fd < 0)
> +		pr_debug("Failed to open cache(%d): %s\n", fd, cpath);
> +	free(dir_name);
> +	pcache->fd = fd;
> +
> +	return fd;
> +}
> +
> +static int probe_cache__load(struct probe_cache *pcache)
> +{
> +	struct probe_cache_entry *entry = NULL;
> +	char buf[MAX_CMDLEN], *p;
> +	int ret = 0;
> +	FILE *fp;
> +
> +	fp = fdopen(dup(pcache->fd), "r");

fdopen may return NULL, a check is needed.

> +	while (!feof(fp)) {
> +		if (!fgets(buf, MAX_CMDLEN, fp))
> +			break;
> +		p = strchr(buf, '\n');
> +		if (p)
> +			*p = '\0';
> +		if (buf[0] == '#') {	/* #perf_probe_event */
> +			entry = probe_cache_entry__new(NULL);
> +			if (!entry) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +			entry->spev = strdup(buf + 1);
> +			if (entry->spev)
> +				ret = parse_perf_probe_command(buf + 1,
> +								&entry->pev);
> +			else
> +				ret = -ENOMEM;
> +			if (ret < 0) {
> +				probe_cache_entry__delete(entry);
> +				goto out;
> +			}
> +			list_add_tail(&entry->list, &pcache->list);
> +		} else {	/* trace_probe_event */
> +			if (!entry) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			strlist__add(entry->tevlist, buf);
> +		}
> +	}
> +out:
> +	fclose(fp);
> +	return ret;
> +}
> +
> +static struct probe_cache *probe_cache__alloc(void)
> +{
> +	struct probe_cache *ret = zalloc(sizeof(*ret));
> +
> +	if (ret) {
> +		INIT_LIST_HEAD(&ret->list);
> +		ret->fd = -EINVAL;
> +	}
> +	return ret;
> +}
> +
> +void probe_cache__delete(struct probe_cache *pcache)
> +{
> +	struct probe_cache_entry *entry;
> +
> +	if (!pcache)
> +		return;

see, a good destructor, accepts NULL, does nothing with it.

> +
> +	while (!list_empty(&pcache->list)) {
> +		entry = list_first_entry(&pcache->list, typeof(*entry), list);
> +		probe_cache_entry__delete(entry);
> +	}

the above while is the definition of a "purge()" operation, that may be
useful outside of a delete operation, please consider adding it, like:

void probe_cache__purge(struct probe_cache *pcache)
{
	struct probe_cache_entry *entry, *n;
	list_for_each_entry_safe(entry, n, &pcache->list, node)
		probe_cache_entry__delete(entry);
}

And please use 'list' for lists and 'node' for entries in a list, i.e.
you have, at the end of this patch:

> struct probe_cache_entry {
>	struct list_head	list;

This one should be 'node', not list, this way we know that this is an
entry in a list, not a list of some other structs.

>	struct perf_probe_event pev;
>	char			*spev;
>	struct strlist		*tevlist;
> };

> struct probe_cache {
> 	int	fd;
>	struct list_head list;

This one is ok, but then if you rename it from the generic name 'list'
to something more informative, for instance 'cache_entries', I think it
will help people reading your code to grasp what it is doing more
quickly.

> };

> +	if (pcache->fd > 0)
> +		close(pcache->fd);
> +	free(pcache);
> +}
> +
> +struct probe_cache *probe_cache__new(const char *target)
> +{
> +	struct probe_cache *pcache = probe_cache__alloc();
> +	int ret;

This is odd, what you call "probe_cache__alloc() looks like what a
"new()" does, if you think that the constructor for 'probe_cache' has to
always open and load it, then why not just do the zalloc() here and then
call probe_cache__init() on it?

> +
> +	if (!pcache)
> +		return NULL;
> +
> +	ret = probe_cache__open(pcache, target);
> +	if (ret < 0) {
> +		pr_debug("Cache open error: %d\n", ret);
> +		goto out_err;
> +	}
> +
> +	ret = probe_cache__load(pcache);
> +	if (ret < 0) {
> +		pr_debug("Cache read error: %d\n", ret);
> +		goto out_err;
> +	}
> +
> +	return pcache;
> +
> +out_err:
> +	probe_cache__delete(pcache);
> +	return NULL;
> +}
> +
> +static bool streql(const char *a, const char *b)
> +{
> +	if (a == b)
> +		return true;
> +
> +	if (!a || !b)
> +		return false;
> +
> +	return !strcmp(a, b);
> +}
> +
> +static struct probe_cache_entry *
> +probe_cache__find(struct probe_cache *pcache, struct perf_probe_event *pev)
> +{
> +	struct probe_cache_entry *entry = NULL;
> +	char *cmd = NULL;
> +
> +	cmd = synthesize_perf_probe_command(pev);

Why init it to NULL only to immediately init it again to something else?
Perhaps:

	char *cmd = synthesize_perf_probe_command(pev);

instead?

> +	if (!cmd)
> +		return NULL;
> +
> +	list_for_each_entry(entry, &pcache->list, list) {
> +		/* Hit if same event name or same command-string */
> +		if ((pev->event &&
> +		     (streql(entry->pev.group, pev->group) &&
> +		      streql(entry->pev.event, pev->event))) ||
> +		    (!strcmp(entry->spev, cmd)))
> +			goto found;
> +	}
> +	entry = NULL;
> +
> +found:
> +	free(cmd);
> +	return entry;
> +}
> +
> +int probe_cache__add_entry(struct probe_cache *pcache,
> +			   struct perf_probe_event *pev,
> +			   struct probe_trace_event *tevs, int ntevs)
> +{
> +	struct probe_cache_entry *entry = NULL;
> +	char *command;
> +	int i, ret = 0;
> +
> +	if (!pcache || !pev || !tevs || ntevs <= 0) {
> +		ret = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	/* Remove old cache entry */
> +	entry = probe_cache__find(pcache, pev);
> +	if (entry)
> +		probe_cache_entry__delete(entry);

Here you could be more compact with:

	probe_cache_entry__delete(probe_cache__find(pcache, pev));

Because delete() functions accept NULL?

> +
> +	ret = -ENOMEM;
> +	entry = probe_cache_entry__new(pev);
> +	if (!entry)
> +		goto out_err;
> +
> +	for (i = 0; i < ntevs; i++) {
> +		if (!tevs[i].point.symbol)
> +			continue;
> +
> +		command = synthesize_probe_trace_command(&tevs[i]);
> +		if (!command)
> +			goto out_err;
> +		strlist__add(entry->tevlist, command);
> +		free(command);
> +	}
> +	list_add_tail(&entry->list, &pcache->list);
> +	pr_debug("Added probe cache: %d\n", ntevs);
> +	return 0;
> +
> +out_err:
> +	pr_debug("Failed to add probe caches\n");
> +	if (entry)
> +		probe_cache_entry__delete(entry);

No need to check for NULL, call the destructor directly.

> +	return ret;
> +}
> +
> +static int probe_cache_entry__write(struct probe_cache_entry *entry, int fd)
> +{
> +	struct str_node *snode;
> +	struct iovec iov[3];
> +	int ret;
> +
> +	pr_debug("Writing cache: #%s\n", entry->spev);
> +	iov[0].iov_base = (void *)"#"; iov[0].iov_len = 1;
> +	iov[1].iov_base = entry->spev; iov[1].iov_len = strlen(entry->spev);
> +	iov[2].iov_base = (void *)"\n"; iov[2].iov_len = 1;
> +	ret = writev(fd, iov, 3);
> +	if (ret < 0)
> +		return ret;

Shouldn't we check short writes? writev() returns the number of bytes
written, isn't it possible to return less than what you asked for?

> +
> +	strlist__for_each(snode, entry->tevlist) {
> +		iov[0].iov_base = (void *)snode->s;
> +		iov[0].iov_len = strlen(snode->s);
> +		iov[1].iov_base = (void *)"\n"; iov[1].iov_len = 1;
> +		ret = writev(fd, iov, 2);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +int probe_cache__commit(struct probe_cache *pcache)
> +{
> +	struct probe_cache_entry *entry;
> +	int ret = 0;
> +
> +	/* TBD: if we do not update existing entries, skip it */
> +	ret = lseek(pcache->fd, 0, SEEK_SET);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = ftruncate(pcache->fd, 0);
> +	if (ret < 0)
> +		goto out;
> +
> +	list_for_each_entry(entry, &pcache->list, list) {
> +		ret = probe_cache_entry__write(entry, pcache->fd);
> +		pr_debug("Cache committed: %d\n", ret);
> +		if (ret < 0)
> +			break;
> +	}
> +out:
> +	return ret;
> +}
> diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> index 18ac9cf..d2b8791d 100644
> --- a/tools/perf/util/probe-file.h
> +++ b/tools/perf/util/probe-file.h
> @@ -5,6 +5,19 @@
>  #include "strfilter.h"
>  #include "probe-event.h"
>  
> +/* Cache of probe definitions */
> +struct probe_cache_entry {
> +	struct list_head	list;
> +	struct perf_probe_event pev;
> +	char			*spev;
> +	struct strlist		*tevlist;
> +};
> +
> +struct probe_cache {
> +	int	fd;
> +	struct list_head list;
> +};
> +
>  #define PF_FL_UPROBE	1
>  #define PF_FL_RW	2
>  
> @@ -18,5 +31,11 @@ int probe_file__get_events(int fd, struct strfilter *filter,
>  				  struct strlist *plist);
>  int probe_file__del_strlist(int fd, struct strlist *namelist);
>  
> +struct probe_cache *probe_cache__new(const char *target);
> +int probe_cache__add_entry(struct probe_cache *pcache,
> +			   struct perf_probe_event *pev,
> +			   struct probe_trace_event *tevs, int ntevs);
> +int probe_cache__commit(struct probe_cache *pcache);
> +void probe_cache__delete(struct probe_cache *pcache);
>  
>  #endif

[toc] | [prev] | [next] | [standalone]

#1419810 — Re: [PATCH perf/core v10 06/23] perf probe-file: Introduce perf_cache interfaces

On Thu, 9 Jun 2016 11:16:31 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Wed, Jun 08, 2016 at 06:29:59PM +0900, Masami Hiramatsu escreveu:
> > Introduce perf_cache object and interfaces to create,
> > add entry, commit, and delete the object.
> > perf_cache represents a file for the cached perf-probe
> > definitions on one binary file or vmlinux which has its
> > own build id. The probe cache file is located under the
> > build-id cache directory of the target binary, as below;
> > 
> >  <perf-debug-dir>/.build-id/<BU>/<ILDID>/probe
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  Changes in v10:
> >   - Splited from "Add --cache option to cache the probe definitions"
> > ---
> >  tools/perf/util/probe-file.c |  307 ++++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/probe-file.h |   19 +++
> >  2 files changed, 326 insertions(+)
> > 
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 3fe6214..689d874 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -14,6 +14,7 @@
> >   * GNU General Public License for more details.
> >   *
> >   */
> > +#include <sys/uio.h>
> >  #include "util.h"
> >  #include "event.h"
> >  #include "strlist.h"
> > @@ -324,3 +325,309 @@ int probe_file__del_events(int fd, struct strfilter *filter)
> >  
> >  	return ret;
> >  }
> > +
> > +static void probe_cache_entry__delete(struct probe_cache_entry *node)
> > +{
> > +	if (!list_empty(&node->list))
> > +		list_del(&node->list);
> 
> Humm, shouldn't this be something like:
> 
> BUG_ON(!list_empty(&node->list)
> 
> ?

Would you mean assert? (it seems similart to die()...)

> 
> I.e. whoever inserted this on a list should take care of removing it
> before deleting it, taking locks, whatever is needed to keep the
> integrity of such list.

Ah, I see.

> 
> You may not be using this stuff in a multithreaded app now, but lets not
> make it difficult to :-)
> 
> I noticed why you do it this way and have a suggestion to use a more
> usual model, see below.
> 
> > +	if (node->tevlist)
> > +		strlist__delete(node->tevlist);
> 
> No checking, destructors generally follows the free() model, i.e. they
> eat NULL for breakfast. Lemme see if strlist does that... Yes, they do.

For the strlist, I should clear the pointer. OK.

> 
> > +	clear_perf_probe_event(&node->pev);
> > +	free(node->spev);
> 
> Here you may want to use:
> 
> 	zfree(&node->spev);
> 
> To free and set node->spev to NULL, to help in debugging when this node
> may be still referenced even having being deleted.

OK, I'll clear that.

> 
> > +	free(node);
> > +}
> > +
> > +static struct probe_cache_entry *
> > +probe_cache_entry__new(struct perf_probe_event *pev)
> > +{
> > +	struct probe_cache_entry *ret = zalloc(sizeof(*ret));
> > +
> > +	if (ret) {
> > +		INIT_LIST_HEAD(&ret->list);
> > +		ret->tevlist = strlist__new(NULL, NULL);
> > +		if (!ret->tevlist)
> > +			zfree(&ret);
> > +		if (ret && pev) {
> > +			ret->spev = synthesize_perf_probe_command(pev);
> > +			if (!ret->spev ||
> > +			    perf_probe_event__copy(&ret->pev, pev) < 0) {
> > +				probe_cache_entry__delete(ret);
> > +				return NULL;
> > +			}
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/* For the kernel probe caches, pass target = NULL */
> > +static int probe_cache__open(struct probe_cache *pcache, const char *target)
> > +{
> > +	char cpath[PATH_MAX];
> > +	char sbuildid[SBUILD_ID_SIZE];
> > +	char *dir_name;
> > +	bool is_kallsyms = !target;
> > +	int ret, fd;
> > +
> > +	if (target)
> > +		ret = filename__sprintf_build_id(target, sbuildid);
> > +	else {
> > +		target = DSO__NAME_KALLSYMS;
> > +		ret = sysfs__sprintf_build_id("/", sbuildid);
> > +	}
> > +	if (ret < 0) {
> > +		pr_debug("Failed to get build-id from %s.\n", target);
> > +		return ret;
> > +	}
> > +
> > +	/* If we have no buildid cache, make it */
> > +	if (!build_id_cache__cached(sbuildid)) {
> > +		ret = build_id_cache__add_s(sbuildid, target,
> > +					    is_kallsyms, NULL);
> > +		if (ret < 0) {
> > +			pr_debug("Failed to add build-id cache: %s\n", target);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	dir_name = build_id_cache__cachedir(sbuildid, target, is_kallsyms,
> > +					    false);
> > +	if (!dir_name)
> > +		return -ENOMEM;
> > +
> > +	snprintf(cpath, PATH_MAX, "%s/probes", dir_name);
> > +	fd = open(cpath, O_CREAT | O_RDWR, 0644);
> > +	if (fd < 0)
> > +		pr_debug("Failed to open cache(%d): %s\n", fd, cpath);
> > +	free(dir_name);
> > +	pcache->fd = fd;
> > +
> > +	return fd;
> > +}
> > +
> > +static int probe_cache__load(struct probe_cache *pcache)
> > +{
> > +	struct probe_cache_entry *entry = NULL;
> > +	char buf[MAX_CMDLEN], *p;
> > +	int ret = 0;
> > +	FILE *fp;
> > +
> > +	fp = fdopen(dup(pcache->fd), "r");
> 
> fdopen may return NULL, a check is needed.

Oops, right. I'll add a check.

> 
> > +	while (!feof(fp)) {
> > +		if (!fgets(buf, MAX_CMDLEN, fp))
> > +			break;
> > +		p = strchr(buf, '\n');
> > +		if (p)
> > +			*p = '\0';
> > +		if (buf[0] == '#') {	/* #perf_probe_event */
> > +			entry = probe_cache_entry__new(NULL);
> > +			if (!entry) {
> > +				ret = -ENOMEM;
> > +				goto out;
> > +			}
> > +			entry->spev = strdup(buf + 1);
> > +			if (entry->spev)
> > +				ret = parse_perf_probe_command(buf + 1,
> > +								&entry->pev);
> > +			else
> > +				ret = -ENOMEM;
> > +			if (ret < 0) {
> > +				probe_cache_entry__delete(entry);
> > +				goto out;
> > +			}
> > +			list_add_tail(&entry->list, &pcache->list);
> > +		} else {	/* trace_probe_event */
> > +			if (!entry) {
> > +				ret = -EINVAL;
> > +				goto out;
> > +			}
> > +			strlist__add(entry->tevlist, buf);
> > +		}
> > +	}
> > +out:
> > +	fclose(fp);
> > +	return ret;
> > +}
> > +
> > +static struct probe_cache *probe_cache__alloc(void)
> > +{
> > +	struct probe_cache *ret = zalloc(sizeof(*ret));
> > +
> > +	if (ret) {
> > +		INIT_LIST_HEAD(&ret->list);
> > +		ret->fd = -EINVAL;
> > +	}
> > +	return ret;
> > +}
> > +
> > +void probe_cache__delete(struct probe_cache *pcache)
> > +{
> > +	struct probe_cache_entry *entry;
> > +
> > +	if (!pcache)
> > +		return;
> 
> see, a good destructor, accepts NULL, does nothing with it.
> 
> > +
> > +	while (!list_empty(&pcache->list)) {
> > +		entry = list_first_entry(&pcache->list, typeof(*entry), list);
> > +		probe_cache_entry__delete(entry);
> > +	}
> 
> the above while is the definition of a "purge()" operation, that may be
> useful outside of a delete operation, please consider adding it, like:
> 
> void probe_cache__purge(struct probe_cache *pcache)
> {
> 	struct probe_cache_entry *entry, *n;
> 	list_for_each_entry_safe(entry, n, &pcache->list, node)
> 		probe_cache_entry__delete(entry);
> }

Hm, I see.

> 
> And please use 'list' for lists and 'node' for entries in a list, i.e.
> you have, at the end of this patch:
> 
> > struct probe_cache_entry {
> >	struct list_head	list;
> 
> This one should be 'node', not list, this way we know that this is an
> entry in a list, not a list of some other structs.

OK.

> 
> >	struct perf_probe_event pev;
> >	char			*spev;
> >	struct strlist		*tevlist;
> > };
> 
> > struct probe_cache {
> > 	int	fd;
> >	struct list_head list;
> 
> This one is ok, but then if you rename it from the generic name 'list'
> to something more informative, for instance 'cache_entries', I think it
> will help people reading your code to grasp what it is doing more
> quickly.

OK.

> 
> > };
> 
> > +	if (pcache->fd > 0)
> > +		close(pcache->fd);
> > +	free(pcache);
> > +}
> > +
> > +struct probe_cache *probe_cache__new(const char *target)
> > +{
> > +	struct probe_cache *pcache = probe_cache__alloc();
> > +	int ret;
> 
> This is odd, what you call "probe_cache__alloc() looks like what a
> "new()" does, if you think that the constructor for 'probe_cache' has to
> always open and load it, then why not just do the zalloc() here and then
> call probe_cache__init() on it?

I just wanted to keep probe_cache always initialized.
But yeah, add probe_cache__init() is OK for me too.

> > +
> > +	if (!pcache)
> > +		return NULL;
> > +
> > +	ret = probe_cache__open(pcache, target);
> > +	if (ret < 0) {
> > +		pr_debug("Cache open error: %d\n", ret);
> > +		goto out_err;
> > +	}
> > +
> > +	ret = probe_cache__load(pcache);
> > +	if (ret < 0) {
> > +		pr_debug("Cache read error: %d\n", ret);
> > +		goto out_err;
> > +	}
> > +
> > +	return pcache;
> > +
> > +out_err:
> > +	probe_cache__delete(pcache);
> > +	return NULL;
> > +}
> > +
> > +static bool streql(const char *a, const char *b)
> > +{
> > +	if (a == b)
> > +		return true;
> > +
> > +	if (!a || !b)
> > +		return false;
> > +
> > +	return !strcmp(a, b);
> > +}
> > +
> > +static struct probe_cache_entry *
> > +probe_cache__find(struct probe_cache *pcache, struct perf_probe_event *pev)
> > +{
> > +	struct probe_cache_entry *entry = NULL;
> > +	char *cmd = NULL;
> > +
> > +	cmd = synthesize_perf_probe_command(pev);
> 
> Why init it to NULL only to immediately init it again to something else?
> Perhaps:
> 
> 	char *cmd = synthesize_perf_probe_command(pev);
> 
> instead?

OK, I'll do.

> 
> > +	if (!cmd)
> > +		return NULL;
> > +
> > +	list_for_each_entry(entry, &pcache->list, list) {
> > +		/* Hit if same event name or same command-string */
> > +		if ((pev->event &&
> > +		     (streql(entry->pev.group, pev->group) &&
> > +		      streql(entry->pev.event, pev->event))) ||
> > +		    (!strcmp(entry->spev, cmd)))
> > +			goto found;
> > +	}
> > +	entry = NULL;
> > +
> > +found:
> > +	free(cmd);
> > +	return entry;
> > +}
> > +
> > +int probe_cache__add_entry(struct probe_cache *pcache,
> > +			   struct perf_probe_event *pev,
> > +			   struct probe_trace_event *tevs, int ntevs)
> > +{
> > +	struct probe_cache_entry *entry = NULL;
> > +	char *command;
> > +	int i, ret = 0;
> > +
> > +	if (!pcache || !pev || !tevs || ntevs <= 0) {
> > +		ret = -EINVAL;
> > +		goto out_err;
> > +	}
> > +
> > +	/* Remove old cache entry */
> > +	entry = probe_cache__find(pcache, pev);
> > +	if (entry)
> > +		probe_cache_entry__delete(entry);
> 
> Here you could be more compact with:
> 
> 	probe_cache_entry__delete(probe_cache__find(pcache, pev));
> 
> Because delete() functions accept NULL?

Ah, right! maybe I forgot that :(

> 
> > +
> > +	ret = -ENOMEM;
> > +	entry = probe_cache_entry__new(pev);
> > +	if (!entry)
> > +		goto out_err;
> > +
> > +	for (i = 0; i < ntevs; i++) {
> > +		if (!tevs[i].point.symbol)
> > +			continue;
> > +
> > +		command = synthesize_probe_trace_command(&tevs[i]);
> > +		if (!command)
> > +			goto out_err;
> > +		strlist__add(entry->tevlist, command);
> > +		free(command);
> > +	}
> > +	list_add_tail(&entry->list, &pcache->list);
> > +	pr_debug("Added probe cache: %d\n", ntevs);
> > +	return 0;
> > +
> > +out_err:
> > +	pr_debug("Failed to add probe caches\n");
> > +	if (entry)
> > +		probe_cache_entry__delete(entry);
> 
> No need to check for NULL, call the destructor directly.

OK.

> 
> > +	return ret;
> > +}
> > +
> > +static int probe_cache_entry__write(struct probe_cache_entry *entry, int fd)
> > +{
> > +	struct str_node *snode;
> > +	struct iovec iov[3];
> > +	int ret;
> > +
> > +	pr_debug("Writing cache: #%s\n", entry->spev);
> > +	iov[0].iov_base = (void *)"#"; iov[0].iov_len = 1;
> > +	iov[1].iov_base = entry->spev; iov[1].iov_len = strlen(entry->spev);
> > +	iov[2].iov_base = (void *)"\n"; iov[2].iov_len = 1;
> > +	ret = writev(fd, iov, 3);
> > +	if (ret < 0)
> > +		return ret;
> 
> Shouldn't we check short writes? writev() returns the number of bytes
> written, isn't it possible to return less than what you asked for?

Yeah, I found this in write(2):

       If a write() is interrupted by a signal handler before  any  bytes  are
       written, then the call fails with the error EINTR; if it is interrupted
       after at least one byte  has  been  written,  the  call  succeeds,  and
       returns the number of bytes written.

Since writev can also be interrupted intermediate of writes,
it can return a smaller number. Let me add a check.

Thanks!!

> 
> > +
> > +	strlist__for_each(snode, entry->tevlist) {
> > +		iov[0].iov_base = (void *)snode->s;
> > +		iov[0].iov_len = strlen(snode->s);
> > +		iov[1].iov_base = (void *)"\n"; iov[1].iov_len = 1;
> > +		ret = writev(fd, iov, 2);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +int probe_cache__commit(struct probe_cache *pcache)
> > +{
> > +	struct probe_cache_entry *entry;
> > +	int ret = 0;
> > +
> > +	/* TBD: if we do not update existing entries, skip it */
> > +	ret = lseek(pcache->fd, 0, SEEK_SET);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = ftruncate(pcache->fd, 0);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	list_for_each_entry(entry, &pcache->list, list) {
> > +		ret = probe_cache_entry__write(entry, pcache->fd);
> > +		pr_debug("Cache committed: %d\n", ret);
> > +		if (ret < 0)
> > +			break;
> > +	}
> > +out:
> > +	return ret;
> > +}
> > diff --git a/tools/perf/util/probe-file.h b/tools/perf/util/probe-file.h
> > index 18ac9cf..d2b8791d 100644
> > --- a/tools/perf/util/probe-file.h
> > +++ b/tools/perf/util/probe-file.h
> > @@ -5,6 +5,19 @@
> >  #include "strfilter.h"
> >  #include "probe-event.h"
> >  
> > +/* Cache of probe definitions */
> > +struct probe_cache_entry {
> > +	struct list_head	list;
> > +	struct perf_probe_event pev;
> > +	char			*spev;
> > +	struct strlist		*tevlist;
> > +};
> > +
> > +struct probe_cache {
> > +	int	fd;
> > +	struct list_head list;
> > +};
> > +
> >  #define PF_FL_UPROBE	1
> >  #define PF_FL_RW	2
> >  
> > @@ -18,5 +31,11 @@ int probe_file__get_events(int fd, struct strfilter *filter,
> >  				  struct strlist *plist);
> >  int probe_file__del_strlist(int fd, struct strlist *namelist);
> >  
> > +struct probe_cache *probe_cache__new(const char *target);
> > +int probe_cache__add_entry(struct probe_cache *pcache,
> > +			   struct perf_probe_event *pev,
> > +			   struct probe_trace_event *tevs, int ntevs);
> > +int probe_cache__commit(struct probe_cache *pcache);
> > +void probe_cache__delete(struct probe_cache *pcache);
> >  
> >  #endif


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1417301 — [PATCH perf/core v10 23/23] perf-test: Add a test case for SDT event

Add a basic test case for SDT event support.
This test scans an SDT event in perftools and
check whether the SDT event is correctly stored
into the buildid cache.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/tests/Build          |    1 
 tools/perf/tests/builtin-test.c |    4 +
 tools/perf/tests/sdt.c          |  114 +++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |    1 
 4 files changed, 120 insertions(+)
 create mode 100644 tools/perf/tests/sdt.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 66a2898..4158422 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -39,6 +39,7 @@ perf-y += stat.o
 perf-y += event_update.o
 perf-y += event-times.o
 perf-y += backward-ring-buffer.o
+perf-y += sdt.o
 
 $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
 	$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 0e95c20..c0e0ccb 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -212,6 +212,10 @@ static struct test generic_tests[] = {
 		.func = test__backward_ring_buffer,
 	},
 	{
+		.desc = "Test SDT event probing",
+		.func = test__sdt_event,
+	},
+	{
 		.func = NULL,
 	},
 };
diff --git a/tools/perf/tests/sdt.c b/tools/perf/tests/sdt.c
new file mode 100644
index 0000000..d11065e
--- /dev/null
+++ b/tools/perf/tests/sdt.c
@@ -0,0 +1,114 @@
+#include <stdio.h>
+#include <sys/epoll.h>
+#include <util/util.h>
+#include <util/evlist.h>
+#include <linux/filter.h>
+#include "tests.h"
+#include "debug.h"
+#include "probe-file.h"
+#include "build-id.h"
+
+/* To test SDT event, we need libelf support to scan elf binary */
+#if defined(HAVE_SDT_EVENT) && defined(HAVE_LIBELF_SUPPORT)
+
+#include <sys/sdt.h>
+
+static int target_function(void)
+{
+	DTRACE_PROBE(perf, test_target);
+	return TEST_OK;
+}
+
+/* Copied from builtin-buildid-cache.c */
+static int build_id_cache__add_file(const char *filename)
+{
+	char sbuild_id[SBUILD_ID_SIZE];
+	u8 build_id[BUILD_ID_SIZE];
+	int err;
+
+	err = filename__read_build_id(filename, &build_id, sizeof(build_id));
+	if (err < 0) {
+		pr_debug("Failed to read build id of %s\n", filename);
+		return err;
+	}
+
+	build_id__sprintf(build_id, sizeof(build_id), sbuild_id);
+	err = build_id_cache__add_s(sbuild_id, filename, false, false);
+	if (err < 0)
+		pr_debug("Failed to add build id cache of %s\n", filename);
+	return err;
+}
+
+static char *get_self_path(void)
+{
+	char *buf = calloc(PATH_MAX, sizeof(char));
+	if (buf && readlink("/proc/self/exe", buf, PATH_MAX) < 0) {
+		pr_debug("Failed to get correct path of perf\n");
+		free(buf);
+		return NULL;
+	}
+	return buf;
+}
+
+static int search_cached_probe(const char *target,
+			       const char *group, const char *event)
+{
+	struct probe_cache *cache = probe_cache__new(target);
+	int ret = 0;
+
+	if (!cache) {
+		pr_debug("Failed to open probe cache of %s\n", target);
+		return -EINVAL;
+	}
+
+	if (!probe_cache__find_by_name(cache, group, event)) {
+		pr_debug("Failed to find %s:%s in the cache\n", group, event);
+		ret = -ENOENT;
+	}
+	probe_cache__delete(cache);
+
+	return ret;
+}
+
+int test__sdt_event(int subtests __maybe_unused)
+{
+	int ret = TEST_FAIL;
+	char __tempdir[] = "./test-buildid-XXXXXX";
+	char *tempdir = NULL, *myself = get_self_path();
+
+	if (myself == NULL || mkdtemp(__tempdir) == NULL) {
+		pr_debug("Failed to make a tempdir for build-id cache\n");
+		goto error;
+	}
+	/* Note that buildid_dir must be an absolute path */
+	tempdir = realpath(__tempdir, NULL);
+
+	/* At first, scan itself */
+	set_buildid_dir(tempdir);
+	if (build_id_cache__add_file(myself) < 0)
+		goto error_rmdir;
+
+	/* Open a cache and make sure the SDT is stored */
+	if (search_cached_probe(myself, "sdt_perf", "test_target") < 0)
+		goto error_rmdir;
+
+	/* TBD: probing on the SDT event and collect logs */
+
+	/* Call the target and get an event */
+	ret = target_function();
+
+error_rmdir:
+	/* Cleanup temporary buildid dir */
+	rm_rf(tempdir);
+error:
+	free(tempdir);
+	free(myself);
+	return ret;
+}
+#else
+int test__sdt_event(int subtests __maybe_unused)
+{
+        pr_debug("Skip SDT event test because SDT support is not compiled\n");
+        return TEST_SKIP;
+}
+#endif
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index c57e72c..93cfb0b 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -87,6 +87,7 @@ int test__synthesize_stat_round(int subtest);
 int test__event_update(int subtest);
 int test__event_times(int subtest);
 int test__backward_ring_buffer(int subtest);
+int test__sdt_event(int subtest);
 
 #if defined(__arm__) || defined(__aarch64__)
 #ifdef HAVE_DWARF_UNWIND_SUPPORT

[toc] | [prev] | [next] | [standalone]

#1417307

On Wed,  8 Jun 2016 18:29:00 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> Here is the 10th version of the patchset for probe-cache and 
> initial SDT support.
> 
> This version fixes a bug and split big patches into small subsets
> according to Namhyung's comment.
> Also I've added a testcase for SDT. In this version, the number of
> patches are increased, but basically, only 3 patches (bugfix, and
> adding test) are new.
> 

Oops, the last 2 patches has no signed-off, I resend it.

> Changes in v10:
>   - [1/23] Fix a bug in rm_rf().
>   - [2/23,] Separate a big patch into 3 sub patches.
>   - [3/23 - 7/23] separate a big patch into 5 sub patches.
>   - [13/23, 14/23] Update documents for SDT explanations.
>   - [22/23, 23/23] Add a test case for SDT and update build config
>     to detect sys/sdt.h.
> 
> Thank you,
> 
> ---
> 
> Hemant Kumar (1):
>       perf/sdt: ELF support for SDT
> 
> Masami Hiramatsu (22):
>       perf: util: Fix rm_rf() to handle non-regular files correctly
>       perf-probe: Fix to add NULL check for strndup
>       perf-buildid: Rename and export build_id_cache__cachedir()
>       perf probe: Add perf_probe_event__copy()
>       perf probe: Recover and export synthesize_perf_probe_point()
>       perf probe-file: Introduce perf_cache interfaces
>       perf probe: Add --cache option to cache the probe definitions
>       perf probe: Use cache entry if possible
>       perf probe: Show all cached probes
>       perf probe: Remove caches when --cache is given
>       perf probe: Add group name support
>       perf buildid-cache: Scan and import user SDT events to probe cache
>       perf probe: Accept %sdt and %cached event name
>       perf-list: Show SDT and pre-cached events
>       perf-list: Skip SDTs placed in invalid binaries
>       perf: probe-cache: Add for_each_probe_cache_entry() wrapper
>       perf probe: Allow wildcard for cached events
>       perf probe: Search SDT/cached event from all probe caches
>       perf probe: Support @BUILDID or @FILE suffix for SDT events
>       perf probe: Support a special SDT probe format
>       perf build: Add sdt feature detection
>       perf-test: Add a test case for SDT event
> 
> 
>  tools/perf/Documentation/perf-buildid-cache.txt |    3 
>  tools/perf/Documentation/perf-probe.txt         |   32 +
>  tools/perf/Makefile.perf                        |    3 
>  tools/perf/builtin-list.c                       |    6 
>  tools/perf/builtin-probe.c                      |   30 +
>  tools/perf/config/Makefile                      |   10 
>  tools/perf/tests/Build                          |    1 
>  tools/perf/tests/builtin-test.c                 |    4 
>  tools/perf/tests/make                           |    3 
>  tools/perf/tests/sdt.c                          |  114 +++++
>  tools/perf/tests/tests.h                        |    1 
>  tools/perf/util/build-id.c                      |  196 ++++++++-
>  tools/perf/util/build-id.h                      |    6 
>  tools/perf/util/parse-events.c                  |   83 ++++
>  tools/perf/util/parse-events.h                  |    2 
>  tools/perf/util/probe-event.c                   |  490 ++++++++++++++++++++--
>  tools/perf/util/probe-event.h                   |    6 
>  tools/perf/util/probe-file.c                    |  504 +++++++++++++++++++++++
>  tools/perf/util/probe-file.h                    |   41 ++
>  tools/perf/util/symbol-elf.c                    |  252 ++++++++++++
>  tools/perf/util/symbol.h                        |   22 +
>  tools/perf/util/util.c                          |   13 -
>  22 files changed, 1736 insertions(+), 86 deletions(-)
>  create mode 100644 tools/perf/tests/sdt.c
> 
> --
> Masami Hiramatsu


-- 
Masami Hiramatsu

[toc] | [prev] | [next] | [standalone]

#1417308 — [PATCH perf/core v10 22/23] perf build: Add sdt feature detection

Will be used to define SDT events in perf test code.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/Makefile.perf   |    3 +++
 tools/perf/config/Makefile |   10 ++++++++++
 tools/perf/tests/make      |    3 ++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 57c8f98..6bc7767 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -60,7 +60,8 @@ FEATURE_TESTS_BASIC :=			\
 	zlib				\
 	lzma				\
 	get_cpuid			\
-	bpf
+	bpf				\
+	sdt
 
 # FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list
 # of all feature tests
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index 3d88f09..82d357f 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -43,7 +43,8 @@ FILES=					\
 	test-zlib.bin			\
 	test-lzma.bin			\
 	test-bpf.bin			\
-	test-get_cpuid.bin
+	test-get_cpuid.bin		\
+	test-sdt.bin
 
 FILES := $(addprefix $(OUTPUT),$(FILES))
 
@@ -205,6 +206,9 @@ $(OUTPUT)test-get_cpuid.bin:
 $(OUTPUT)test-bpf.bin:
 	$(BUILD)
 
+$(OUTPUT)test-sdt.bin:
+	$(BUILD)
+
 -include $(OUTPUT)*.d
 
 ###############################
diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index a282e8c..f538c5f 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -137,6 +137,10 @@
 # include "test-libcrypto.c"
 #undef main
 
+#define main main_test_sdt
+# include "test-sdt.c"
+#undef main
+
 int main(int argc, char *argv[])
 {
 	main_test_libpython();
@@ -168,6 +172,7 @@ int main(int argc, char *argv[])
 	main_test_get_cpuid();
 	main_test_bpf();
 	main_test_libcrypto();
+	main_test_sdt();
 
 	return 0;
 }
diff --git a/tools/build/feature/test-sdt.c b/tools/build/feature/test-sdt.c
new file mode 100644
index 0000000..e4531a6
--- /dev/null
+++ b/tools/build/feature/test-sdt.c
@@ -0,0 +1,7 @@
+#include <sys/sdt.h>
+
+int main(void)
+{
+	DTRACE_PROBE(provider, name);
+	return 0;
+}
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index bde8cba..a818388 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -81,6 +81,9 @@ include ../scripts/utilities.mak
 #
 # Define NO_LIBBPF if you do not want BPF support
 #
+# Define NO_SDT if you do not want to define SDT event in perf tools,
+# note that it doesn't disable SDT scanning support.
+#
 # Define FEATURES_DUMP to provide features detection dump file
 # and bypass the feature detection
 
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index 1e46277..641a82a 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -331,6 +331,16 @@ ifndef NO_LIBELF
   endif # NO_LIBBPF
 endif # NO_LIBELF
 
+ifndef NO_SDT
+  ifneq ($(feature-sdt), 1)
+    msg := $(warning No sys/sdt.h found, no SDT events are defined, please install systemtap-sdt-devel or systemtap-sdt-dev);
+    NO_SDT := 1;
+  else
+    CFLAGS += -DHAVE_SDT_EVENT
+    $(call detected,CONFIG_SDT_EVENT)
+  endif
+endif
+
 ifdef PERF_HAVE_JITDUMP
   ifndef NO_DWARF
     $(call detected,CONFIG_JITDUMP)
diff --git a/tools/perf/tests/make b/tools/perf/tests/make
index cac15d9..ac801be 100644
--- a/tools/perf/tests/make
+++ b/tools/perf/tests/make
@@ -81,6 +81,7 @@ make_no_libbionic   := NO_LIBBIONIC=1
 make_no_auxtrace    := NO_AUXTRACE=1
 make_no_libbpf	    := NO_LIBBPF=1
 make_no_libcrypto   := NO_LIBCRYPTO=1
+make_no_sdt	    := NO_SDT=1
 make_tags           := tags
 make_cscope         := cscope
 make_help           := help
@@ -104,7 +105,7 @@ make_minimal        := NO_LIBPERL=1 NO_LIBPYTHON=1 NO_NEWT=1 NO_GTK2=1
 make_minimal        += NO_DEMANGLE=1 NO_LIBELF=1 NO_LIBUNWIND=1 NO_BACKTRACE=1
 make_minimal        += NO_LIBNUMA=1 NO_LIBAUDIT=1 NO_LIBBIONIC=1
 make_minimal        += NO_LIBDW_DWARF_UNWIND=1 NO_AUXTRACE=1 NO_LIBBPF=1
-make_minimal        += NO_LIBCRYPTO=1
+make_minimal        += NO_LIBCRYPTO=1 NO_SDT=1
 
 # $(run) contains all available tests
 run := make_pure

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web